Re: [PATCH v9 15/16] xen/arm: vpci: check guest range

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <stewart.hildebrand@xxxxxxx>
> >>>>>>>>
> >>>>>>>> Skip mapping the BAR if it is not in a valid range.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> >>>>>>>> ---
> >>>>>>>>  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.