Re: [PATCH v4 11/11] xen/arm: translate virtual PCI bus topology for guests

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

On Mon, Nov 08, 2021 at 11:16:42AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 08.11.21 13:10, Jan Beulich wrote:
> > On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
> >> --- a/xen/arch/arm/vpci.c
> >> +++ b/xen/arch/arm/vpci.c
> >> @@ -41,6 +41,15 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t 
> >> *info,
> >>       /* data is needed to prevent a pointer cast on 32bit */
> >>       unsigned long data;
> >>   
> >> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> >> +    /*
> >> +     * For the passed through devices we need to map their virtual SBDF
> >> +     * to the physical PCI device being passed through.
> >> +     */
> >> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) )
> >> +            return 1;
> > Nit: Indentation.
> Ouch, sure
> >
> >> @@ -59,6 +68,15 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t 
> >> *info,
> >>       struct pci_host_bridge *bridge = p;
> >>       pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> >>   
> >> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> >> +    /*
> >> +     * For the passed through devices we need to map their virtual SBDF
> >> +     * to the physical PCI device being passed through.
> >> +     */
> >> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) )
> >> +            return 1;
> > Again.
> Will fix
> >
> >> @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, 
> >> VPCI_PRIORITY_MIDDLE);
> >>   static void vpci_remove_virtual_device(struct domain *d,
> >>                                          const struct pci_dev *pdev)
> >>   {
> >> +    ASSERT(pcidevs_locked());
> >> +
> >>       clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map);
> >>       pdev->vpci->guest_sbdf.sbdf = ~0;
> >>   }
> >>   
> >> +/*
> >> + * Find the physical device which is mapped to the virtual device
> >> + * and translate virtual SBDF to the physical one.
> >> + */
> >> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf)
> > const struct domain *d ?
> Will change
> >
> >> +{
> >> +    const struct pci_dev *pdev;
> >> +    bool found = false;
> >> +
> >> +    pcidevs_lock();
> >> +    for_each_pdev( d, pdev )
> >> +    {
> >> +        if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf )
> >> +        {
> >> +            /* Replace virtual SBDF with the physical one. */
> >> +            *sbdf = pdev->sbdf;
> >> +            found = true;
> >> +            break;
> >> +        }
> >> +    }
> >> +    pcidevs_unlock();
> > I think the description wants to at least mention that in principle
> > this is too coarse grained a lock, providing justification for why
> > it is deemed good enough nevertheless. (Personally, as expressed
> > before, I don't think the lock should be used here, but as long as
> > Roger agrees with you, you're fine.)
> Yes, makes sense

Seeing as we don't take the lock in vpci_{read,write} I'm not sure we
need it here either then.

Since on Arm you will add devices to the guest at runtime (ie: while
there could already be PCI accesses), have you seen issues with not
taking the lock here?

I think the whole pcidevs locking needs to be clarified, as it's
currently a mess. If you want to take it here that's fine, but overall
there are issues in other places that would make removing a device at
runtime not reliable.

Thanks, Roger.