[for-4.13,v3] passthrough: simplify locking and logging

Commit Message

Igor Druzhinin Nov. 15, 2019, 6:59 p.m. UTC

From: Paul Durrant <[e-mail]>

Dropping the pcidevs lock between calling device_assigned() and
assign_device() means that the latter has to do the same check as the
former for no obvious gain. Also, since long running operations under
pcidevs lock already drop the lock and return -ERESTART periodically there
is little point in immediately failing an assignment operation with
-ERESTART just because the pcidevs lock could not be acquired (for the
second time, having already blocked on acquiring the lock in
device_assigned()).

This patch instead acquires the lock once for assignment (or test assign)
operations directly in iommu_do_pci_domctl() and thus can remove the
duplicate domain ownership check in assign_device(). Whilst in the
neighbourhood, the patch also removes some debug logging from
assign_device() and deassign_device() and replaces it with proper error
logging, which allows error logging in iommu_do_pci_domctl() to be
removed.

Signed-off-by: Paul Durrant <[e-mail]>
Signed-off-by: Igor Druzhinin <[e-mail]>
---
Since Paul doesn't mind and kindly agreed - I'm taking ownership of this patch
review process from now on.

Changes in v3:
- Dropped controversial hunk with error code processing of device_assigned().
  Readability is worse with it and I don't think we can safely stop converting
  the error code to avoid userspace breakage.
- Addressed other minor comments.
- Fixed Paul's email again to reflect that the code was made in Citrix.
---
 xen/drivers/passthrough/pci.c | 78 ++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 56 deletions(-)

Comments

Jan Beulich Nov. 18, 2019, 11:21 a.m. UTC | #1

On 15.11.2019 19:59, Igor Druzhinin wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -932,30 +932,26 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>              break;
>          ret = hd->platform_ops->reassign_device(d, target, devfn,
>                                                  pci_to_dev(pdev));
> -        if ( !ret )
> -            continue;
> -
> -        printk(XENLOG_G_ERR "%pd: deassign %04x:%02x:%02x.%u failed (%d)\n",
> -               d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
> -        return ret;
> +        if ( ret )
> +            goto out;
>      }
>  
>      devfn = pdev->devfn;
>      ret = hd->platform_ops->reassign_device(d, target, devfn,
>                                              pci_to_dev(pdev));
>      if ( ret )
> -    {
> -        dprintk(XENLOG_G_ERR,
> -                "%pd: deassign device (%04x:%02x:%02x.%u) failed\n",
> -                d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> -        return ret;
> -    }
> +        goto out;
>  
>      if ( pdev->domain == hardware_domain  )
>          pdev->quarantine = false;
>  
>      pdev->fault.count = 0;
>  
> +out:
> +    if ( ret )
> +        printk(XENLOG_G_ERR "%pd: deassign (%04x:%02x:%02x.%u) failed (%d)\n",
> +               d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
> +

Since, despite my suggestion to the contrary, you've kept the
introduction of goto here, the label should have got indented
(as pointed out for v2). With this adjusted (which could be done
while committing)
Acked-by: Jan Beulich <[e-mail]>

Jan

Igor Druzhinin Nov. 18, 2019, 12:45 p.m. UTC | #2

On 18/11/2019 11:21, Jan Beulich wrote:
> On 15.11.2019 19:59, Igor Druzhinin wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -932,30 +932,26 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>>              break;
>>          ret = hd->platform_ops->reassign_device(d, target, devfn,
>>                                                  pci_to_dev(pdev));
>> -        if ( !ret )
>> -            continue;
>> -
>> -        printk(XENLOG_G_ERR "%pd: deassign %04x:%02x:%02x.%u failed (%d)\n",
>> -               d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
>> -        return ret;
>> +        if ( ret )
>> +            goto out;
>>      }
>>  
>>      devfn = pdev->devfn;
>>      ret = hd->platform_ops->reassign_device(d, target, devfn,
>>                                              pci_to_dev(pdev));
>>      if ( ret )
>> -    {
>> -        dprintk(XENLOG_G_ERR,
>> -                "%pd: deassign device (%04x:%02x:%02x.%u) failed\n",
>> -                d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>> -        return ret;
>> -    }
>> +        goto out;
>>  
>>      if ( pdev->domain == hardware_domain  )
>>          pdev->quarantine = false;
>>  
>>      pdev->fault.count = 0;
>>  
>> +out:
>> +    if ( ret )
>> +        printk(XENLOG_G_ERR "%pd: deassign (%04x:%02x:%02x.%u) failed (%d)\n",
>> +               d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
>> +
> 
> Since, despite my suggestion to the contrary, you've kept the
> introduction of goto here, the label should have got indented
> (as pointed out for v2). With this adjusted (which could be done
> while committing)
> Acked-by: Jan Beulich <[e-mail]>

Sorry, thought you meant the other thing. Andrew clarified the rationale
behind your request.

Igor

Jürgen Groß Nov. 20, 2019, 3:49 p.m. UTC | #3

On 15.11.19 19:59, Igor Druzhinin wrote:
> From: Paul Durrant <[e-mail]>
> 
> Dropping the pcidevs lock between calling device_assigned() and
> assign_device() means that the latter has to do the same check as the
> former for no obvious gain. Also, since long running operations under
> pcidevs lock already drop the lock and return -ERESTART periodically there
> is little point in immediately failing an assignment operation with
> -ERESTART just because the pcidevs lock could not be acquired (for the
> second time, having already blocked on acquiring the lock in
> device_assigned()).
> 
> This patch instead acquires the lock once for assignment (or test assign)
> operations directly in iommu_do_pci_domctl() and thus can remove the
> duplicate domain ownership check in assign_device(). Whilst in the
> neighbourhood, the patch also removes some debug logging from
> assign_device() and deassign_device() and replaces it with proper error
> logging, which allows error logging in iommu_do_pci_domctl() to be
> removed.
> 
> Signed-off-by: Paul Durrant <[e-mail]>
> Signed-off-by: Igor Druzhinin <[e-mail]>

As the release is coming nearer I don't want to take "cosmetic" patches
for 4.13 anymore.


Juergen

Igor Druzhinin Nov. 20, 2019, 4 p.m. UTC | #4

On 20/11/2019 15:49, Jürgen Groß wrote:
> On 15.11.19 19:59, Igor Druzhinin wrote:
>> From: Paul Durrant <[e-mail]>
>>
>> Dropping the pcidevs lock between calling device_assigned() and
>> assign_device() means that the latter has to do the same check as the
>> former for no obvious gain. Also, since long running operations under
>> pcidevs lock already drop the lock and return -ERESTART periodically
>> there
>> is little point in immediately failing an assignment operation with
>> -ERESTART just because the pcidevs lock could not be acquired (for the
>> second time, having already blocked on acquiring the lock in
>> device_assigned()).
>>
>> This patch instead acquires the lock once for assignment (or test assign)
>> operations directly in iommu_do_pci_domctl() and thus can remove the
>> duplicate domain ownership check in assign_device(). Whilst in the
>> neighbourhood, the patch also removes some debug logging from
>> assign_device() and deassign_device() and replaces it with proper error
>> logging, which allows error logging in iommu_do_pci_domctl() to be
>> removed.
>>
>> Signed-off-by: Paul Durrant <[e-mail]>
>> Signed-off-by: Igor Druzhinin <[e-mail]>
> 
> As the release is coming nearer I don't want to take "cosmetic" patches
> for 4.13 anymore.
> 

Understood, we will carry it with our local patches then.

Igor

Patch

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 18a7dc7..8a25d4f 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -932,30 +932,26 @@  static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
             break;
         ret = hd->platform_ops->reassign_device(d, target, devfn,
                                                 pci_to_dev(pdev));
-        if ( !ret )
-            continue;
-
-        printk(XENLOG_G_ERR "%pd: deassign %04x:%02x:%02x.%u failed (%d)\n",
-               d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
-        return ret;
+        if ( ret )
+            goto out;
     }
 
     devfn = pdev->devfn;
     ret = hd->platform_ops->reassign_device(d, target, devfn,
                                             pci_to_dev(pdev));
     if ( ret )
-    {
-        dprintk(XENLOG_G_ERR,
-                "%pd: deassign device (%04x:%02x:%02x.%u) failed\n",
-                d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
-        return ret;
-    }
+        goto out;
 
     if ( pdev->domain == hardware_domain  )
         pdev->quarantine = false;
 
     pdev->fault.count = 0;
 
+out:
+    if ( ret )
+        printk(XENLOG_G_ERR "%pd: deassign (%04x:%02x:%02x.%u) failed (%d)\n",
+               d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
+
     return ret;
 }
 
@@ -976,10 +972,7 @@  int pci_release_devices(struct domain *d)
     {
         bus = pdev->bus;
         devfn = pdev->devfn;
-        if ( deassign_device(d, pdev->seg, bus, devfn) )
-            printk("domain %d: deassign device (%04x:%02x:%02x.%u) failed!\n",
-                   d->domain_id, pdev->seg, bus,
-                   PCI_SLOT(devfn), PCI_FUNC(devfn));
+        deassign_device(d, pdev->seg, bus, devfn);
     }
     pcidevs_unlock();
 
@@ -1475,8 +1468,7 @@  static int device_assigned(u16 seg, u8 bus, u8 devfn)
     struct pci_dev *pdev;
     int rc = 0;
 
-    pcidevs_lock();
-
+    ASSERT(pcidevs_locked());
     pdev = pci_get_pdev(seg, bus, devfn);
 
     if ( !pdev )
@@ -1490,11 +1482,10 @@  static int device_assigned(u16 seg, u8 bus, u8 devfn)
               pdev->domain != dom_io )
         rc = -EBUSY;
 
-    pcidevs_unlock();
-
     return rc;
 }
 
+/* Caller should hold the pcidevs_lock */
 static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
 {
     const struct domain_iommu *hd = dom_iommu(d);
@@ -1513,23 +1504,11 @@  static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
                   p2m_get_hostp2m(d)->global_logdirty) )
         return -EXDEV;
 
-    if ( !pcidevs_trylock() )
-        return -ERESTART;
-
+    /* device_assigned() should already have cleared the device for assignment */
+    ASSERT(pcidevs_locked());
     pdev = pci_get_pdev(seg, bus, devfn);
-
-    rc = -ENODEV;
-    if ( !pdev )
-        goto done;
-
-    rc = 0;
-    if ( d == pdev->domain )
-        goto done;
-
-    rc = -EBUSY;
-    if ( pdev->domain != hardware_domain &&
-         pdev->domain != dom_io )
-        goto done;
+    ASSERT(pdev && (pdev->domain == hardware_domain ||
+                    pdev->domain == dom_io));
 
     if ( pdev->msix )
     {
@@ -1550,19 +1529,16 @@  static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
         if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
             break;
         rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag);
-        if ( rc )
-            printk(XENLOG_G_WARNING "d%d: assign %04x:%02x:%02x.%u failed (%d)\n",
-                   d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                   rc);
     }
 
  done:
+    if ( rc )
+        printk(XENLOG_G_WARNING "%pd: assign (%04x:%02x:%02x.%u) failed (%d)\n",
+               d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), rc);
     /* The device is assigned to dom_io so mark it as quarantined */
-    if ( !rc && d == dom_io )
+    else if ( d == dom_io )
         pdev->quarantine = true;
 
-    pcidevs_unlock();
-
     return rc;
 }
 
@@ -1718,6 +1694,7 @@  int iommu_do_pci_domctl(
         bus = PCI_BUS(machine_sbdf);
         devfn = PCI_DEVFN2(machine_sbdf);
 
+        pcidevs_lock();
         ret = device_assigned(seg, bus, devfn);
         if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
         {
@@ -1730,17 +1707,12 @@  int iommu_do_pci_domctl(
             }
             break;
         }
-        if ( !ret )
+        else if ( !ret )
             ret = assign_device(d, seg, bus, devfn, flags);
+        pcidevs_unlock();
         if ( ret == -ERESTART )
             ret = hypercall_create_continuation(__HYPERVISOR_domctl,
                                                 "h", u_domctl);
-        else if ( ret )
-            printk(XENLOG_G_ERR
-                   "assign %04x:%02x:%02x.%u to dom%d failed (%d)\n",
-                   seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                   d->domain_id, ret);
-
         break;
 
     case XEN_DOMCTL_deassign_device:
@@ -1772,12 +1744,6 @@  int iommu_do_pci_domctl(
         pcidevs_lock();
         ret = deassign_device(d, seg, bus, devfn);
         pcidevs_unlock();
-        if ( ret )
-            printk(XENLOG_G_ERR
-                   "deassign %04x:%02x:%02x.%u from dom%d failed (%d)\n",
-                   seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                   d->domain_id, ret);
-
         break;
 
     default: