On Thu, Sep 28, 2023 at 02:28:11PM -0400, Stewart Hildebrand wrote: > > > On 9/28/23 04:28, Roger Pau Monné wrote: > > Caution: This message originated from an External Source. Use proper > > caution when opening attachments, clicking links, or responding. > > > > > > On Wed, Sep 27, 2023 at 02:03:30PM -0400, Stewart Hildebrand wrote: > >> On 9/26/23 11:48, Roger Pau Monné wrote: > >>> On Tue, Sep 26, 2023 at 11:27:48AM -0400, Stewart Hildebrand wrote: > >>>> On 9/26/23 04:07, Roger Pau Monné wrote: > >>>>> On Mon, Sep 25, 2023 at 05:49:00PM -0400, Stewart Hildebrand wrote: > >>>>>> On 9/22/23 04:44, Roger Pau Monné wrote: > >>>>>>> On Tue, Aug 29, 2023 at 11:19:47PM +0000, Volodymyr Babchuk wrote: > >>>>>>>> From: Stewart Hildebrand <[email protected]> > >>>>>>>> > >>>>>>>> Skip mapping the BAR if it is not in a valid range. > >>>>>>>> > >>>>>>>> Signed-off-by: Stewart Hildebrand <[email protected]> > >>>>>>>> --- > >>>>>>>> xen/drivers/vpci/header.c | 9 +++++++++ > >>>>>>>> 1 file changed, 9 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > >>>>>>>> index 1d243eeaf9..dbabdcbed2 100644 > >>>>>>>> --- a/xen/drivers/vpci/header.c > >>>>>>>> +++ b/xen/drivers/vpci/header.c > >>>>>>>> @@ -345,6 +345,15 @@ static int modify_bars(const struct pci_dev > >>>>>>>> *pdev, uint16_t cmd, bool rom_only) > >>>>>>>> bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) ) > >>>>>>>> continue; > >>>>>>>> > >>>>>>>> +#ifdef CONFIG_ARM > >>>>>>>> + if ( !is_hardware_domain(pdev->domain) ) > >>>>>>>> + { > >>>>>>>> + if ( (start_guest < PFN_DOWN(GUEST_VPCI_MEM_ADDR)) || > >>>>>>>> + (end_guest >= PFN_DOWN(GUEST_VPCI_MEM_ADDR + > >>>>>>>> GUEST_VPCI_MEM_SIZE)) ) > >>>>>>>> + continue; > >>>>>>>> + } > >>>>>>>> +#endif > >>>>>>> > >>>>>>> Hm, I think this should be in a hook similar to pci_check_bar() that > >>>>>>> can be implemented per-arch. > >>>>>>> > >>>>>>> IIRC at least on x86 we allow the guest to place the BARs whenever it > >>>>>>> wants, would such placement cause issues to the hypervisor on Arm? > >>>>>> > >>>>>> Hm. I wrote this patch in a hurry to make v9 of this series work on > >>>>>> ARM. In my haste I also forgot about the prefetchable range starting > >>>>>> at GUEST_VPCI_PREFETCH_MEM_ADDR, but that won't matter as we can > >>>>>> probably throw this patch out. > >>>>>> > >>>>>> Now that I've had some more time to investigate, I believe the check > >>>>>> in this patch is more or less redundant to the existing check in > >>>>>> map_range() added in baa6ea700386 ("vpci: add permission checks to > >>>>>> map_range()"). > >>>>>> > >>>>>> The issue is that during initialization bar->guest_addr is zeroed, and > >>>>>> this initial value of bar->guest_addr will fail the permissions check > >>>>>> in map_range() and crash the domain. When the guest writes a new valid > >>>>>> BAR, the old invalid address remains in the rangeset to be mapped. If > >>>>>> we simply remove the old invalid BAR from the rangeset, that seems to > >>>>>> fix the issue. So something like this: > >>>>> > >>>>> It does seem to me we are missing a proper cleanup of the rangeset > >>>>> contents in some paths then. In the above paragraph you mention "the > >>>>> old invalid address remains in the rangeset to be mapped", how does it > >>>>> get in there in the first place, and why is the rangeset not emptied > >>>>> if the mapping failed? > >>>> > >>>> Back in ("vpci/header: handle p2m range sets per BAR") I added a > >>>> v->domain == pdev->domain check near the top of vpci_process_pending() > >>>> as you appropriately suggested. > >>>> > >>>> + if ( v->domain != pdev->domain ) > >>>> + { > >>>> + read_unlock(&v->domain->pci_lock); > >>>> + return false; > >>>> + } > >>>> > >>>> I have also reverted this patch ("xen/arm: vpci: check guest range"). > >>>> > >>>> The sequence of events leading to the old value remaining in the > >>>> rangeset are: > >>>> > >>>> # xl pci-assignable-add 01:00.0 > >>>> drivers/vpci/vpci.c:vpci_deassign_device() > >>>> deassign 0000:01:00.0 from d0 > >>>> # grep pci domu.cfg > >>>> pci = [ "01:00.0" ] > >>>> # xl create domu.cfg > >>>> drivers/vpci/vpci.c:vpci_deassign_device() > >>>> deassign 0000:01:00.0 from d[IO] > >>>> drivers/vpci/vpci.c:vpci_assign_device() > >>>> assign 0000:01:00.0 to d1 > >>>> bar->guest_addr is initialized to zero because of the line: > >>>> pdev->vpci = xzalloc(struct vpci); > >>>> drivers/vpci/header.c:init_bars() > >>>> drivers/vpci/header.c:modify_bars() > >>> > >>> I think I've commented this on another patch, but why is the device > >>> added with memory decoding enabled? I would expect the FLR performed > >>> before assigning would leave the device with memory decoding disabled? > >> > >> It seems the device is indeed being assigned to the domU with memory > >> decoding enabled, but I'm not entirely sure why. The device I'm testing > >> with doesn't support FLR, but it does support pm bus reset: > >> # cat /sys/bus/pci/devices/0000\:01\:00.0/reset_method > >> pm bus > >> > >> As I understand it, libxl__device_pci_reset() should still be able to > >> issue a reset in this case. > > > > Maybe pciback is somehow restoring part of the previous state? I > > have no insight in what state we expect the device to be handled by > > pciback, but this needs investigation in order to know what to expect. > > Yep, during "xl pci-assignable-add ..." pciback resets the device and > restores the state, including whether memory decoding is enabled. > > drivers/xen/xen-pciback/pci_stub.c:pcistub_init_device(): > > /* We need the device active to save the state. */ > dev_dbg(&dev->dev, "save state of device\n"); > pci_save_state(dev); > dev_data->pci_saved_state = pci_store_saved_state(dev); > if (!dev_data->pci_saved_state) > dev_err(&dev->dev, "Could not store PCI conf saved state!\n"); > else { > dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n"); > __pci_reset_function_locked(dev); > pci_restore_state(dev); > } > /* Now disable the device (this also ensures some private device > * data is setup before we export) > */ > dev_dbg(&dev->dev, "reset device\n"); > xen_pcibk_reset_device(dev); > > That last function, xen_pcibk_reset_device(), clears the bus master enable > bit in the command register for devices with PCI_HEADER_TYPE_NORMAL (not a > reset contrary to the function name). > > xl create should reset the device again, but, similarly, this also seems to > restore the state. > > > Can you paste the full contents of the command register for this > > device? > Start of day (PCIe controller and bridge initialized, no device BARs or > anything have been programmed yet): 0x0000 > After dom0 boot, device is in use: 0x0006 > After pci-assignable-add: 0x0002 > After echo 1 > /sys/bus/pci/devices/0000\:01\:00.0/reset: 0x0002 > After xl create, domU booted: 0x0006 > > Should mapping bars should be conditional on PCI_COMMAND_MASTER, not > PCI_COMMAND_MEMORY? E.g.:
NO, I don't think so, as then Xen state would get out of sync with the hardware state. I think just disabling memory and IO decoding at init_bars() for devices assigned to domUs should be fine for the time being. Thanks, Roger.
