[PATCH 0/6] hw/accel: Use RCU_READ macros

[PATCH 0/6] hw/accel: Use RCU_READ macros

[Philippe Mathieu-Daudé]

Use RCU_READ_LOCK_GUARD and WITH_RCU_READ_LOCK_GUARD
to replace the manual rcu_read_(un)lock calls.

Philippe Mathieu-Daudé (6):
  accel/tcg/cpu-exec: Use RCU_READ macro
  hw/9pfs/9p-synth: Use RCU_READ macro
  hw/display/virtio-gpu-udmabuf: Use RCU_READ macro
  hw/scsi/virtio-scsi: Use RCU_READ macro
  hw/vfio/common: Use RCU_READ macros
  hw/virtio/vhost: Use RCU_READ macro

 accel/tcg/cpu-exec.c            | 24 +++++++++++------------
 hw/9pfs/9p-synth.c              | 24 +++++++++++------------
 hw/display/virtio-gpu-udmabuf.c |  6 +++---
 hw/scsi/virtio-scsi.c           | 12 ++++++------
 hw/vfio/common.c                | 34 ++++++++++++++++-----------------
 hw/virtio/vhost.c               |  6 +++---
 6 files changed, 52 insertions(+), 54 deletions(-)

-- 
2.41.0

[PATCH 1/6] accel/tcg/cpu-exec: Use RCU_READ macro

[Philippe Mathieu-Daudé]

Replace the manual rcu_read_(un)lock calls by the
WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
"docs/style: call out the use of GUARD macros").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/cpu-exec.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 67eda9865e..6b3f66930e 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -1070,21 +1070,21 @@ int cpu_exec(CPUState *cpu)
         return EXCP_HALTED;
     }
 
-    rcu_read_lock();
-    cpu_exec_enter(cpu);
+    WITH_RCU_READ_LOCK_GUARD() {
+        cpu_exec_enter(cpu);
 
-    /*
-     * Calculate difference between guest clock and host clock.
-     * This delay includes the delay of the last cycle, so
-     * what we have to do is sleep until it is 0. As for the
-     * advance/delay we gain here, we try to fix it next time.
-     */
-    init_delay_params(&sc, cpu);
+        /*
+         * Calculate difference between guest clock and host clock.
+         * This delay includes the delay of the last cycle, so
+         * what we have to do is sleep until it is 0. As for the
+         * advance/delay we gain here, we try to fix it next time.
+         */
+        init_delay_params(&sc, cpu);
 
-    ret = cpu_exec_setjmp(cpu, &sc);
+        ret = cpu_exec_setjmp(cpu, &sc);
 
-    cpu_exec_exit(cpu);
-    rcu_read_unlock();
+        cpu_exec_exit(cpu);
+    };
 
     return ret;
 }
-- 
2.41.0

Re: [PATCH 1/6] accel/tcg/cpu-exec: Use RCU_READ macro

[Manos Pitsidianakis]

On Wed, 24 Jan 2024 09:41, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Replace the manual rcu_read_(un)lock calls by the
>WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
>"docs/style: call out the use of GUARD macros").
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> accel/tcg/cpu-exec.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
>diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>index 67eda9865e..6b3f66930e 100644
>--- a/accel/tcg/cpu-exec.c
>+++ b/accel/tcg/cpu-exec.c
>@@ -1070,21 +1070,21 @@ int cpu_exec(CPUState *cpu)
>         return EXCP_HALTED;
>     }
> 
>-    rcu_read_lock();
>-    cpu_exec_enter(cpu);
>+    WITH_RCU_READ_LOCK_GUARD() {
>+        cpu_exec_enter(cpu);
> 
>-    /*
>-     * Calculate difference between guest clock and host clock.
>-     * This delay includes the delay of the last cycle, so
>-     * what we have to do is sleep until it is 0. As for the
>-     * advance/delay we gain here, we try to fix it next time.
>-     */
>-    init_delay_params(&sc, cpu);
>+        /*
>+         * Calculate difference between guest clock and host clock.
>+         * This delay includes the delay of the last cycle, so
>+         * what we have to do is sleep until it is 0. As for the
>+         * advance/delay we gain here, we try to fix it next time.
>+         */
>+        init_delay_params(&sc, cpu);
> 
>-    ret = cpu_exec_setjmp(cpu, &sc);
>+        ret = cpu_exec_setjmp(cpu, &sc);
> 
>-    cpu_exec_exit(cpu);
>-    rcu_read_unlock();
>+        cpu_exec_exit(cpu);
>+    };
> 
>     return ret;
> }
>-- 
>2.41.0
>

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

Re: [PATCH 1/6] accel/tcg/cpu-exec: Use RCU_READ macro

[Richard Henderson]

On 1/24/24 17:41, Philippe Mathieu-Daudé wrote:
> Replace the manual rcu_read_(un)lock calls by the
> WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
> "docs/style: call out the use of GUARD macros").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/tcg/cpu-exec.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> +    };

Stray ;


r~

Re: [PATCH 1/6] accel/tcg/cpu-exec: Use RCU_READ macro

[Richard Henderson]

On 1/24/24 17:41, Philippe Mathieu-Daudé wrote:
> Replace the manual rcu_read_(un)lock calls by the
> WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
> "docs/style: call out the use of GUARD macros").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/tcg/cpu-exec.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 67eda9865e..6b3f66930e 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -1070,21 +1070,21 @@ int cpu_exec(CPUState *cpu)
>           return EXCP_HALTED;
>       }
>   
> -    rcu_read_lock();
> -    cpu_exec_enter(cpu);
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        cpu_exec_enter(cpu);
>   
> -    /*
> -     * Calculate difference between guest clock and host clock.
> -     * This delay includes the delay of the last cycle, so
> -     * what we have to do is sleep until it is 0. As for the
> -     * advance/delay we gain here, we try to fix it next time.
> -     */
> -    init_delay_params(&sc, cpu);
> +        /*
> +         * Calculate difference between guest clock and host clock.
> +         * This delay includes the delay of the last cycle, so
> +         * what we have to do is sleep until it is 0. As for the
> +         * advance/delay we gain here, we try to fix it next time.
> +         */
> +        init_delay_params(&sc, cpu);
>   
> -    ret = cpu_exec_setjmp(cpu, &sc);
> +        ret = cpu_exec_setjmp(cpu, &sc);
>   
> -    cpu_exec_exit(cpu);
> -    rcu_read_unlock();
> +        cpu_exec_exit(cpu);
> +    };
>   
>       return ret;
>   }

I've tweaked this to use RCU_READ_LOCK_GUARD instead -- there's little point
in excluding the return from the block, and thus avoid the re-indent -- and queued this 
one patch to tcg-next.


r~

[PATCH 2/6] hw/9pfs/9p-synth: Use RCU_READ macro

[Philippe Mathieu-Daudé]

Replace the manual rcu_read_(un)lock calls by the
WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
"docs/style: call out the use of GUARD macros").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/9pfs/9p-synth.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 0ac79a500b..419ea69e3a 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -241,15 +241,15 @@ static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
     int i = 0;
     V9fsSynthNode *node;
 
-    rcu_read_lock();
-    QLIST_FOREACH(node, &dir->child, sibling) {
-        /* This is the off child of the directory */
-        if (i == off) {
-            break;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QLIST_FOREACH(node, &dir->child, sibling) {
+            /* This is the off child of the directory */
+            if (i == off) {
+                break;
+            }
+            i++;
         }
-        i++;
     }
-    rcu_read_unlock();
     if (!node) {
         /* end of directory */
         return NULL;
@@ -494,13 +494,13 @@ static int synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
         goto out;
     }
     /* search for the name in the children */
-    rcu_read_lock();
-    QLIST_FOREACH(node, &dir_node->child, sibling) {
-        if (!strcmp(node->name, name)) {
-            break;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QLIST_FOREACH(node, &dir_node->child, sibling) {
+            if (!strcmp(node->name, name)) {
+                break;
+            }
         }
     }
-    rcu_read_unlock();
 
     if (!node) {
         errno = ENOENT;
-- 
2.41.0

Re: [PATCH 2/6] hw/9pfs/9p-synth: Use RCU_READ macro

[Greg Kurz]

On Wed, 24 Jan 2024 08:41:57 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Replace the manual rcu_read_(un)lock calls by the
> WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
> "docs/style: call out the use of GUARD macros").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/9pfs/9p-synth.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 

Acked-by: Greg Kurz <groug@kaod.org>

> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index 0ac79a500b..419ea69e3a 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -241,15 +241,15 @@ static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
>      int i = 0;
>      V9fsSynthNode *node;
>  
> -    rcu_read_lock();
> -    QLIST_FOREACH(node, &dir->child, sibling) {
> -        /* This is the off child of the directory */
> -        if (i == off) {
> -            break;
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QLIST_FOREACH(node, &dir->child, sibling) {
> +            /* This is the off child of the directory */
> +            if (i == off) {
> +                break;
> +            }
> +            i++;
>          }
> -        i++;
>      }
> -    rcu_read_unlock();
>      if (!node) {
>          /* end of directory */
>          return NULL;
> @@ -494,13 +494,13 @@ static int synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>          goto out;
>      }
>      /* search for the name in the children */
> -    rcu_read_lock();
> -    QLIST_FOREACH(node, &dir_node->child, sibling) {
> -        if (!strcmp(node->name, name)) {
> -            break;
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QLIST_FOREACH(node, &dir_node->child, sibling) {
> +            if (!strcmp(node->name, name)) {
> +                break;
> +            }
>          }
>      }
> -    rcu_read_unlock();
>  
>      if (!node) {
>          errno = ENOENT;



-- 
Greg

Re: [PATCH 2/6] hw/9pfs/9p-synth: Use RCU_READ macro

[Christian Schoenebeck]

On Wednesday, January 24, 2024 8:41:57 AM CET Philippe Mathieu-Daudé wrote:
> Replace the manual rcu_read_(un)lock calls by the
> WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
> "docs/style: call out the use of GUARD macros").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

>  hw/9pfs/9p-synth.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index 0ac79a500b..419ea69e3a 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -241,15 +241,15 @@ static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
>      int i = 0;
>      V9fsSynthNode *node;
>  
> -    rcu_read_lock();
> -    QLIST_FOREACH(node, &dir->child, sibling) {
> -        /* This is the off child of the directory */
> -        if (i == off) {
> -            break;
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QLIST_FOREACH(node, &dir->child, sibling) {
> +            /* This is the off child of the directory */
> +            if (i == off) {
> +                break;
> +            }
> +            i++;
>          }
> -        i++;
>      }
> -    rcu_read_unlock();
>      if (!node) {
>          /* end of directory */
>          return NULL;
> @@ -494,13 +494,13 @@ static int synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>          goto out;
>      }
>      /* search for the name in the children */
> -    rcu_read_lock();
> -    QLIST_FOREACH(node, &dir_node->child, sibling) {
> -        if (!strcmp(node->name, name)) {
> -            break;
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QLIST_FOREACH(node, &dir_node->child, sibling) {
> +            if (!strcmp(node->name, name)) {
> +                break;
> +            }
>          }
>      }
> -    rcu_read_unlock();
>  
>      if (!node) {
>          errno = ENOENT;
> 

[PATCH 3/6] hw/display/virtio-gpu-udmabuf: Use RCU_READ macro

[Philippe Mathieu-Daudé]

Replace the manual rcu_read_(un)lock calls by the
WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
"docs/style: call out the use of GUARD macros").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/virtio-gpu-udmabuf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index d51184d658..0ee6685803 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -42,9 +42,9 @@ static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res)
                      sizeof(struct udmabuf_create_item) * res->iov_cnt);
 
     for (i = 0; i < res->iov_cnt; i++) {
-        rcu_read_lock();
-        rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset);
-        rcu_read_unlock();
+        WITH_RCU_READ_LOCK_GUARD() {
+            rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset);
+        }
 
         if (!rb || rb->fd < 0) {
             g_free(list);
-- 
2.41.0

Re: [PATCH 3/6] hw/display/virtio-gpu-udmabuf: Use RCU_READ macro

[Manos Pitsidianakis]

On Wed, 24 Jan 2024 09:41, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Replace the manual rcu_read_(un)lock calls by the
>WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
>"docs/style: call out the use of GUARD macros").
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/display/virtio-gpu-udmabuf.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
>index d51184d658..0ee6685803 100644
>--- a/hw/display/virtio-gpu-udmabuf.c
>+++ b/hw/display/virtio-gpu-udmabuf.c
>@@ -42,9 +42,9 @@ static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res)
>                      sizeof(struct udmabuf_create_item) * res->iov_cnt);
> 
>     for (i = 0; i < res->iov_cnt; i++) {
>-        rcu_read_lock();
>-        rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset);
>-        rcu_read_unlock();
>+        WITH_RCU_READ_LOCK_GUARD() {
>+            rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset);
>+        }
> 
>         if (!rb || rb->fd < 0) {
>             g_free(list);
>-- 
>2.41.0

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

[PATCH 4/6] hw/scsi/virtio-scsi: Use RCU_READ macro

[Philippe Mathieu-Daudé]

Replace the manual rcu_read_(un)lock calls by the
WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
"docs/style: call out the use of GUARD macros").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/virtio-scsi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 690aceec45..998227404a 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -343,14 +343,14 @@ static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req)
         target = req->req.tmf.lun[1];
         qatomic_inc(&s->resetting);
 
-        rcu_read_lock();
-        QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
-            SCSIDevice *d1 = SCSI_DEVICE(kid->child);
-            if (d1->channel == 0 && d1->id == target) {
-                device_cold_reset(&d1->qdev);
+        WITH_RCU_READ_LOCK_GUARD() {
+            QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
+                SCSIDevice *d1 = SCSI_DEVICE(kid->child);
+                if (d1->channel == 0 && d1->id == target) {
+                    device_cold_reset(&d1->qdev);
+                }
             }
         }
-        rcu_read_unlock();
 
         qatomic_dec(&s->resetting);
         break;
-- 
2.41.0

Re: [PATCH 4/6] hw/scsi/virtio-scsi: Use RCU_READ macro

[Manos Pitsidianakis]

On Wed, 24 Jan 2024 09:41, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Replace the manual rcu_read_(un)lock calls by the
>WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
>"docs/style: call out the use of GUARD macros").
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/scsi/virtio-scsi.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>index 690aceec45..998227404a 100644
>--- a/hw/scsi/virtio-scsi.c
>+++ b/hw/scsi/virtio-scsi.c
>@@ -343,14 +343,14 @@ static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req)
>         target = req->req.tmf.lun[1];
>         qatomic_inc(&s->resetting);
> 
>-        rcu_read_lock();
>-        QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
>-            SCSIDevice *d1 = SCSI_DEVICE(kid->child);
>-            if (d1->channel == 0 && d1->id == target) {
>-                device_cold_reset(&d1->qdev);
>+        WITH_RCU_READ_LOCK_GUARD() {
>+            QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
>+                SCSIDevice *d1 = SCSI_DEVICE(kid->child);
>+                if (d1->channel == 0 && d1->id == target) {
>+                    device_cold_reset(&d1->qdev);
>+                }
>             }
>         }
>-        rcu_read_unlock();
> 
>         qatomic_dec(&s->resetting);
>         break;
>-- 
>2.41.0
>

Unrelated to your patch: I just noticed in hw/scsi/virtio-scsi.c, 
s->resetting is used to flag whether the bus is resetting; but there's 
no check if a resetting is taking place before starting another. Is this 
single threaded code so it's not necessary?

As for the patch:

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

Re: [PATCH 4/6] hw/scsi/virtio-scsi: Use RCU_READ macro

[Philippe Mathieu-Daudé]

On 24/1/24 10:17, Manos Pitsidianakis wrote:
> On Wed, 24 Jan 2024 09:41, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
>> Replace the manual rcu_read_(un)lock calls by the
>> WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
>> "docs/style: call out the use of GUARD macros").
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/scsi/virtio-scsi.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)

> Unrelated to your patch: I just noticed in hw/scsi/virtio-scsi.c, 
> s->resetting is used to flag whether the bus is resetting; but there's 
> no check if a resetting is taking place before starting another. Is this 
> single threaded code so it's not necessary?

No clue about this device, I'll let the maintainers answer you :)

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

[Philippe Mathieu-Daudé]

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;
         }
         /*
          * 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

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

[Manos Pitsidianakis]

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>

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

[Philippe Mathieu-Daudé]

On 24/1/24 10:25, Manos Pitsidianakis wrote:
> 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.

If the maintainer agrees with the style & code churn, I don't
mind respining.

> 
>>         }
>>         /*
>>          * 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();
>> }

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

Thanks!

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

[Philippe Mathieu-Daudé]

On 24/1/24 15:09, Philippe Mathieu-Daudé wrote:
> On 24/1/24 10:25, Manos Pitsidianakis wrote:
>> 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.
> 
> If the maintainer agrees with the style & code churn, I don't
> mind respining.

Alex, Cédric, any preference?

> 
>>
>>>         }
>>>         /*
>>>          * 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();
>>> }
> 
>> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> 
> Thanks!

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

[Cédric Le Goater]

On 2/16/24 09:49, Philippe Mathieu-Daudé wrote:
> On 24/1/24 15:09, Philippe Mathieu-Daudé wrote:
>> On 24/1/24 10:25, Manos Pitsidianakis wrote:
>>> 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.
>>
>> If the maintainer agrees with the style & code churn, I don't
>> mind respining.
> 
> Alex, Cédric, any preference?

my choice would be to keep the 'goto' statement and protect
the vfio_get_xlat_addr() call with :

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



Thanks,

C.

[PATCH 6/6] hw/virtio/vhost: Use RCU_READ macro

[Philippe Mathieu-Daudé]

Replace the manual rcu_read_(un)lock calls by the
WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
"docs/style: call out the use of GUARD macros").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/virtio/vhost.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac79468..1f5ecb843e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -186,12 +186,12 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
             hwaddr phys, s, offset;
 
             while (used_size) {
-                rcu_read_lock();
-                iotlb = address_space_get_iotlb_entry(dev->vdev->dma_as,
+                WITH_RCU_READ_LOCK_GUARD() {
+                    iotlb = address_space_get_iotlb_entry(dev->vdev->dma_as,
                                                       used_phys,
                                                       true,
                                                       MEMTXATTRS_UNSPECIFIED);
-                rcu_read_unlock();
+                }
 
                 if (!iotlb.target_as) {
                     qemu_log_mask(LOG_GUEST_ERROR, "translation "
-- 
2.41.0

Re: [PATCH 6/6] hw/virtio/vhost: Use RCU_READ macro

[Manos Pitsidianakis]

On Wed, 24 Jan 2024 09:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Replace the manual rcu_read_(un)lock calls by the
>WITH_RCU_READ_LOCK_GUARD macro (See commit ef46ae67ba
>"docs/style: call out the use of GUARD macros").
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/virtio/vhost.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>index 2c9ac79468..1f5ecb843e 100644
>--- a/hw/virtio/vhost.c
>+++ b/hw/virtio/vhost.c
>@@ -186,12 +186,12 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
>             hwaddr phys, s, offset;
> 
>             while (used_size) {
>-                rcu_read_lock();
>-                iotlb = address_space_get_iotlb_entry(dev->vdev->dma_as,
>+                WITH_RCU_READ_LOCK_GUARD() {
>+                    iotlb = address_space_get_iotlb_entry(dev->vdev->dma_as,
>                                                       used_phys,
>                                                       true,
>                                                       MEMTXATTRS_UNSPECIFIED);
>-                rcu_read_unlock();
>+                }
> 
>                 if (!iotlb.target_as) {
>                     qemu_log_mask(LOG_GUEST_ERROR, "translation "
>-- 
>2.41.0
>

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