Re: [PATCH v7 10/12] vpci: add initial support for virtual PCI bus topology

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 21.06.23 15:06, Jan Beulich wrote:

Hello all


> On 13.06.2023 12:32, Volodymyr Babchuk wrote:
> > @@ -121,6 +124,62 @@ int vpci_add_handlers(struct pci_dev *pdev)
> >   }
> >   
> >   #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> > +static int add_virtual_device(struct pci_dev *pdev)
> > +{
> > +    struct domain *d = pdev->domain;
> > +    pci_sbdf_t sbdf = { 0 };
> > +    unsigned long new_dev_number;
> > +
> > +    if ( is_hardware_domain(d) )
> > +        return 0;
> > +
> > +    ASSERT(pcidevs_locked());
> > +
> > +    /*
> > +     * Each PCI bus supports 32 devices/slots at max or up to 256 when
> > +     * there are multi-function ones which are not yet supported.
> > +     */
> > +    if ( pdev->info.is_extfn )
> > +    {
> > +        gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n",
> > +                 &pdev->sbdf);
> > +        return -EOPNOTSUPP;
> > +    }
> > +
> > +    new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map,
> > +                                         VPCI_MAX_VIRT_DEV);
> > +    if ( new_dev_number >= VPCI_MAX_VIRT_DEV )
> > +        return -ENOSPC;
> > +
> > +    __set_bit(new_dev_number, &d->vpci_dev_assigned_map);
> Since the find-and-set can't easily be atomic, the lock used here (
> asserted to be held above) needs to be the same as ...
>
> > +    /*
> > +     * Both segment and bus number are 0:
> > +     *  - we emulate a single host bridge for the guest, e.g. segment 0
> > +     *  - with bus 0 the virtual devices are seen as embedded
> > +     *    endpoints behind the root complex
> > +     *
> > +     * TODO: add support for multi-function devices.
> > +     */
> > +    sbdf.devfn = PCI_DEVFN(new_dev_number, 0);
> > +    pdev->vpci->guest_sbdf = sbdf;
> > +
> > +    return 0;
> > +
> > +}
> > +
> > +static void vpci_remove_virtual_device(const struct pci_dev *pdev)
> > +{
> > +    write_lock(&pdev->domain->vpci_rwlock);
> > +    if ( pdev->vpci )
> > +    {
> > +        __clear_bit(pdev->vpci->guest_sbdf.dev,
> > +                    &pdev->domain->vpci_dev_assigned_map);
> > +        pdev->vpci->guest_sbdf.sbdf = ~0;
> > +    }
> > +    write_unlock(&pdev->domain->vpci_rwlock);
> ... the one used here.

I think, it makes sense, yes.

***

There is one more thing. As far as I remember, there were some requests 
provided for the previous version (also v7) [1]. At least one of them, I 
assume, is still applicable here. I am speaking about a request to 
consider moving "cleaning up guest_sbdf / vpci_dev_assigned_map" into 
vpci_remove_device() here and aliasing of vpci_deassign_device() to 
vpci_remove_device() in commit #03/12.
The diff below (to be applied on top of current patch) is my 
understanding (not even build tested):
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index a61282cc5b..c3e6c153bc 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -51,6 +51,15 @@ void vpci_remove_device(struct pci_dev *pdev)
         return;
     }

+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+    if ( pdev->vpci->guest_sbdf.sbdf != ~0 )
+    {
+        __clear_bit(pdev->vpci->guest_sbdf.dev,
+                    &pdev->domain->vpci_dev_assigned_map);
+        pdev->vpci->guest_sbdf.sbdf = ~0;
+    }
+#endif
+
     vpci = pdev->vpci;
     pdev->vpci = NULL;
     write_unlock(&pdev->domain->vpci_rwlock);
@@ -152,10 +161,14 @@ static int add_virtual_device(struct pci_dev *pdev)
         return -EOPNOTSUPP;
     }

+    write_lock(&pdev->domain->vpci_rwlock);
     new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map,
                                          VPCI_MAX_VIRT_DEV);
     if ( new_dev_number >= VPCI_MAX_VIRT_DEV )
+    {
+        write_unlock(&pdev->domain->vpci_rwlock);
         return -ENOSPC;
+    }

     __set_bit(new_dev_number, &d->vpci_dev_assigned_map);

@@ -169,23 +182,12 @@ static int add_virtual_device(struct pci_dev *pdev)
      */
     sbdf.devfn = PCI_DEVFN(new_dev_number, 0);
     pdev->vpci->guest_sbdf = sbdf;
+    write_unlock(&pdev->domain->vpci_rwlock);

     return 0;

 }

-static void vpci_remove_virtual_device(const struct pci_dev *pdev)
-{
-    write_lock(&pdev->domain->vpci_rwlock);
-    if ( pdev->vpci )
-    {
-        __clear_bit(pdev->vpci->guest_sbdf.dev,
-                    &pdev->domain->vpci_dev_assigned_map);
-        pdev->vpci->guest_sbdf.sbdf = ~0;
-    }
-    write_unlock(&pdev->domain->vpci_rwlock);
-}
-
 /* Notify vPCI that device is assigned to guest. */
 int vpci_assign_device(struct pci_dev *pdev)
 {
@@ -215,7 +217,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
     if ( !has_vpci(pdev->domain) )
         return;

-    vpci_remove_virtual_device(pdev);
     vpci_remove_device(pdev);
 }
 #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
(END)



[1] 
https://lore.kernel.org/xen-devel/20220719174253.541965-10-olekstysh@xxxxxxxxx/
https://lore.kernel.org/xen-devel/20220719174253.541965-3-olekstysh@xxxxxxxxx/

> Jan