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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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?

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);
      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
          goto done;
+    }

      for ( ; pdev->phantom_stride; rc = 0 )
      {

I think we don't care about pci_remove_device because:

int pci_remove_device(u16 seg, u8 bus, u8 devfn)
{
[snip]
     pcidevs_lock();
     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
         if ( pdev->bus == bus && pdev->devfn == devfn )
         {
             vpci_remove_device(pdev);

as vpci_remove_device will take care there are no readers and
will safely remove vpci.

Thank you,
Oleksandr