Re: [PATCH 5/6] hw/vfio/common: Use RCU_READ macros

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Wed, 24 Jan 2024 09:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Replace the manual rcu_read_(un)lock calls by the
>*RCU_READ_LOCK_GUARD macros (See commit ef46ae67ba
>"docs/style: call out the use of GUARD macros").
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/vfio/common.c | 34 ++++++++++++++++------------------
> 1 file changed, 16 insertions(+), 18 deletions(-)
>
>diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>index 4aa86f563c..09878a3603 100644
>--- a/hw/vfio/common.c
>+++ b/hw/vfio/common.c
>@@ -308,13 +308,13 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>         return;
>     }
> 
>-    rcu_read_lock();
>+    RCU_READ_LOCK_GUARD();
> 
>     if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
>         bool read_only;
> 
>         if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
>-            goto out;
>+            return;

Since this is the only early return, we could alternatively do:

-         if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
+         if (vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {

remove the goto/return, and wrap the rest of the codeflow in this if's 
brackets. And then we could use WITH_RCU_READ_LOCK_GUARD instead. That'd 
increase the code indentation however.


>         }
>         /*
>          * vaddr is only valid until rcu_read_unlock(). But after
>@@ -343,8 +343,6 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>             vfio_set_migration_error(ret);
>         }
>     }
>-out:
>-    rcu_read_unlock();
> }
> 
> static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>@@ -1223,23 +1221,23 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>     if (iotlb->target_as != &address_space_memory) {
>         error_report("Wrong target AS \"%s\", only system memory is allowed",
>                      iotlb->target_as->name ? iotlb->target_as->name : "none");
>-        goto out;
>-    }
>-
>-    rcu_read_lock();
>-    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
>-        ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>-                                    translated_addr);
>-        if (ret) {
>-            error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>-                         "0x%"HWADDR_PRIx") = %d (%s)",
>-                         bcontainer, iova, iotlb->addr_mask + 1, ret,
>-                         strerror(-ret));
>+    } else {
>+        WITH_RCU_READ_LOCK_GUARD() {
>+            if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
>+                ret = vfio_get_dirty_bitmap(bcontainer, iova,
>+                                            iotlb->addr_mask + 1,
>+                                            translated_addr);
>+                if (ret) {
>+                    error_report("vfio_iommu_map_dirty_notify(%p,"
>+                                 " 0x%"HWADDR_PRIx
>+                                 ", 0x%"HWADDR_PRIx") = %d (%s)",
>+                                 bcontainer, iova, iotlb->addr_mask + 1, ret,
>+                                 strerror(-ret));
>+                }
>+            }
>         }
>     }
>-    rcu_read_unlock();
> 
>-out:
>     if (ret) {
>         vfio_set_migration_error(ret);
>     }
>-- 
>2.41.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>