Re: [PATCH] vpci: introduce per-domain lock to protect vpci structure

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

On Fri, Feb 11, 2022 at 08:46:59AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 10.02.22 18:16, Roger Pau Monné wrote:
> > On Wed, Feb 09, 2022 at 03:36:27PM +0200, Oleksandr Andrushchenko wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >>
> >> Introduce a per-domain read/write lock to check whether vpci is present,
> >> so we are sure there are no accesses to the contents of the vpci struct
> >> if not. This lock can be used (and in a few cases is used right away)
> >> so that vpci removal can be performed while holding the lock in write
> >> mode. Previously such removal could race with vpci_read for example.
> > Sadly there's still a race in the usage of pci_get_pdev_by_domain wrt
> > pci_remove_device, and likely when vPCI gets also used in
> > {de}assign_device I think.
> >
> How about the below? It seems to guarantee that we can access pdev
> without issues and without requiring pcidevs_lock to be used?

Hm, I'm unsure this is correct. It's in general a bad idea to use a
per-domain lock approach to protect the consistency of elements moving
between domains.

In order for this to be safe you will likely need to hold both the
source and the destination per-domain locks, and then you could also
get into ABBA lock issues unless you always take the lock in the same
order.

I think it's safer to use a global lock in this case (pcidevs_lock).

> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index e8b09d77d880..fd464a58b3b3 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -937,8 +937,14 @@ static int deassign_device(struct domain *d, uint16_t 
> seg, uint8_t bus,
>       }
> 
>       devfn = pdev->devfn;
> +#ifdef CONFIG_HAS_VPCI
> +    write_lock(&d->vpci_rwlock);
> +#endif
>       ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
>                        pci_to_dev(pdev));
> +#ifdef CONFIG_HAS_VPCI
> +    write_unlock(&d->vpci_rwlock);
> +#endif
>       if ( ret )
>           goto out;
> 
> @@ -1474,6 +1480,9 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>       const struct domain_iommu *hd = dom_iommu(d);
>       struct pci_dev *pdev;
>       int rc = 0;
> +#ifdef CONFIG_HAS_VPCI
> +    struct domain *old_d;
> +#endif
> 
>       if ( !is_iommu_enabled(d) )
>           return 0;
> @@ -1487,15 +1496,34 @@ static int assign_device(struct domain *d, u16 seg, 
> u8 bus, u8 devfn, u32 flag)
>       ASSERT(pdev && (pdev->domain == hardware_domain ||
>                       pdev->domain == dom_io));
> 
> +#ifdef CONFIG_HAS_VPCI
> +    /* pdev->domain is either hwdom or dom_io. We do not want the later. */
> +    old_d = pdev->domain == hardware_domain ? pdev->domain : NULL;
> +    if ( old_d )
> +        write_lock(&old_d->vpci_rwlock);
> +#endif
> +
>       rc = pdev_msix_assign(d, pdev);

I don't think you need the vpci lock for this operation.

>       if ( rc )
> +    {
> +#ifdef CONFIG_HAS_VPCI
> +        if ( old_d )
> +            write_unlock(&old_d->vpci_rwlock);
> +#endif
>           goto done;
> +    }
> 
>       pdev->fault.count = 0;
> 
>       if ( (rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
>                             pci_to_dev(pdev), flag)) )
> +    {
> +#ifdef CONFIG_HAS_VPCI
> +        if ( old_d )
> +            write_unlock(&old_d->vpci_rwlock);
> +#endif

Like I've mentioned above, I'm unsure this is correct. You are holding
the lock of the previous domain, but at some point the device will be
assigned to a new domain, so that change won't be protected by the
lock of the new domain.

Thanks, Roger.