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

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

On Mon, Oct 18, 2021 at 09:47:06AM +0200, Jan Beulich wrote:
> On 15.10.2021 18:51, Bertrand Marquis wrote:
> > --- /dev/null
> > +++ b/xen/arch/arm/vpci.c
> > @@ -0,0 +1,77 @@
> > +/*
> > + * xen/arch/arm/vpci.c
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +#include <xen/sched.h>
> > +#include <xen/vpci.h>
> > +
> > +#include <asm/mmio.h>
> > +
> > +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> > +                          register_t *r, void *p)
> > +{
> > +    pci_sbdf_t sbdf;
> > +    /* data is needed to prevent a pointer cast on 32bit */
> > +    unsigned long data;
> > +
> > +    /* We ignore segment part and always handle segment 0 */
> > +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
> > +
> > +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
> > +                        1U << info->dabt.size, &data) )
> > +    {
> 
> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
> the virtual one. The function then calls vpci_read(), which in turn
> will call vpci_read_hw() in a number of situations (first and foremost
> whenever pci_get_pdev_by_domain() returns NULL). That function as well
> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
> physical one; I'm unable to spot any translation. Yet I do recall
> seeing assignment of a virtual device and function number somewhere
> (perhaps another of the related series), so the model also doesn't
> look to assume 1:1 mapping of SBDF.
> 
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> >      if ( !pdev->domain )
> >      {
> >          pdev->domain = hardware_domain;
> > +#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);
> > +            pdev->domain = NULL;
> > +            goto out;
> > +        }
> > +#endif
> >          ret = iommu_add_device(pdev);
> >          if ( ret )
> >          {
> 
> Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
> What about iommu_add_device() failure? The device will have ->domain
> zapped, but all vPCI handlers still in place. This aspect of insufficient
> error cleanup was pointed out already in review of v1.
> 
> Furthermore already in v1 I questioned why this would be Arm-specific: On
> x86 this code path is going to be taken for all devices Xen wasn't able
> to discover at boot (anything on segments we wouldn't consider config
> space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
> very least).

My original plans for SR-IOV VFs on PVH dom0 involved trapping
accesses to the SR-IOV capability and detecting the creation of VFs
without the need for dom0 to notify them to Xen. This would avoid dom0
having to call PHYSDEVOP_pci_device_add for that case.

I might be confused, but I think we also spoke about other (non SR-IOV
related) cases where PCI devices might appear after certain actions by
dom0, so I think we need to keep the PHYSDEVOP_pci_device_add for PVH
dom0.

> Hence it is my understanding that something along these
> lines is actually also needed for PVH Dom0. I've just checked, and
> according to my mailbox that comment was actually left unresponded to.
> 
> Roger, am I missing anything here as to PVH Dom0 getting handlers put in
> place?

No, I think we will need those, likewise for run-time reported MCFG
regions.

Thanks, Roger.