Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM.

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

On Fri, Oct 15, 2021 at 09:52:28AM +0000, Bertrand Marquis wrote:
> Hi Roger,
> 
> > On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> > 
> > On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote:
> >> From: Rahul Singh <rahul.singh@xxxxxxx>
> >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> >> index 3aa8c3175f..8cc529ecec 100644
> >> --- a/xen/drivers/passthrough/pci.c
> >> +++ b/xen/drivers/passthrough/pci.c
> >> @@ -658,7 +658,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> >>                    const struct pci_dev_info *info, nodeid_t node)
> >> {
> >>     struct pci_seg *pseg;
> >> -    struct pci_dev *pdev;
> >> +    struct pci_dev *pdev = NULL;
> >>     unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
> >>     const char *pdev_type;
> >>     int ret;
> >> @@ -752,6 +752,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> >> 
> >>     check_pdev(pdev);
> >> 
> >> +#ifdef CONFIG_ARM
> >> +    /*
> >> +     * On ARM PCI devices discovery will be done by Dom0. Add vpci 
> >> handler when
> >> +     * Dom0 inform XEN to add the PCI devices in XEN.
> >> +     */
> >> +    ret = vpci_add_handlers(pdev);
> >> +    if ( ret )
> >> +    {
> >> +        printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> >> +        goto out;
> >> +    }
> >> +#endif
> > 
> > I think vpci_add_handlers should be called after checking that
> > pdev->domain is != NULL, so I would move this chunk a bit below.
> 
> On arm this would prevent the dom0less use case or to have the PCI
> bus enumerated from an other domain.

vpci_add_handlers will try to access pdev->domain, so passing a pdev
without domain being set is wrong.

> @oleksandr: can you comment on this one, you might have a better
> answer than me on this ?
> 
> > 
> >> +
> >>     ret = 0;
> >>     if ( !pdev->domain )
> >>     {
> >> @@ -784,6 +797,9 @@ out:
> >>                    &PCI_SBDF(seg, bus, slot, func));
> >>         }
> >>     }
> >> +    else if ( pdev )
> >> +        pci_cleanup_msi(pdev);
> > 
> > I'm slightly lost at why you add this chunk, is this strictly related
> > to the patch?
> 
> This was discussed a lot in previous version of the patch and
> requested by Stefano. The idea here is that as soon as handlers
> are added some bits might be modified in the PCI config space
> leading possibly to msi interrupts. So it is safer to cleanup on the
> error path. For references please see discussion on v4 and v5 where
> this was actually added (to much references as the discussion was
> long so here [1] and [2] are the patchwork thread).

pci_add_device being solely used by trusted domains, I think it would
be fine to require that the domain doesn't poke the PCI config space
until the call to pci_add_device has finished. Else it's likely to get
inconsistent results, or mess up with the device state.

In any case, such addition needs some kind of reasoning added to the
commit message if it's really required.

Thanks, Roger.