Re: [PATCH v7 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Julien Grall <julien@xxxxxxx>]

Hi Bertrand,

On 15/10/2021 16:06, Bertrand Marquis wrote:
> > On 15 Oct 2021, at 15:30, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Fri, Oct 15, 2021 at 02:59:19PM +0100, Bertrand Marquis wrote:
> > > From: Rahul Singh <rahul.singh@xxxxxxx>
> > >
> > > The existing VPCI support available for X86 is adapted for Arm.
> > > When the device is added to XEN via the hyper call
> > > “PHYSDEVOP_pci_device_add”, VPCI handler for the config space
> > > access is added to the Xen to emulate the PCI devices config space.
> > >
> > > A MMIO trap handler for the PCI ECAM space is registered in XEN
> > > so that when guest is trying to access the PCI config space,XEN
> > > will trap the access and emulate read/write using the VPCI and
> > > not the real PCI hardware.
> > >
> > > For Dom0less systems scan_pci_devices() would be used to discover the
> > > PCI device in XEN and VPCI handler will be added during XEN boots.
> > >
> > > This patch is also doing some small fixes to fix compilation errors on
> > > arm32 of vpci and prevent 64bit accesses on 32bit:
> > > - use %zu instead of lu in header.c for print
> > > - prevent 64bit accesses in vpci_access_allowed
> > > - ifdef out using CONFIG_64BIT handling of len 8 in
> > > vpci_ecam_{read/write}
> > >
> > > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > > Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > The vpci bits looks fine to me, so:
> >
> > Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Thanks
>
> > I have one question however related to the placement of the vpci setup
> > call in pci_add_device.
> >
> > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> > > index 3aa8c3175f..082892c8a2 100644
> > > --- a/xen/drivers/passthrough/pci.c
> > > +++ b/xen/drivers/passthrough/pci.c
> > > @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> > >          list_add(&pdev->domain_list, &hardware_domain->pdev_list);
> > >      }
> > >      else
> > > +    {
> > > +#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;
> > > +        }
> > I'm likely lost here, but shouldn't this also be done for devices that
> > belong to the hardware domain and are assigned to it in the first
> > branch of this conditional?
> >
> > Or else you will end up with devices assigned to the hardware domain
> > that don't have vPCI setup for them.
> I might be wrong but when the hardware domain is declaring the devices they are 
> added to him.
> Then later when those device are assigned to a guest, they are removed from the 
> hardware domain.
From my understanding, when the device is initially registered we would 
go through the first branch because pdev->domain is not yet set.
The else would be taken only with subsequent call of 
PHYSDEVOP_manage_pci_add & co.
For the device assignment, a different path would be taken. This would 
go through the domctl XEN_DOMCTL_assign_device.
Therefore, I think Roger is right and the call belongs to the first 
branch. Otherwise, we would miss out the vpci handlers in some cases.
Cheers,

--
Julien Grall