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.
