1. Patchew
  2. QEMU
  3. [PATCH 0/6] hw/accel: Use RCU_READ macros
  4. View patch

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

Philippe Mathieu-Daudé posted 6 patches 10 months, 1 week ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Greg Kurz <groug@kaod.org>, Christian Schoenebeck <qemu_oss@crudebyte.com>, Gerd Hoffmann <kraxel@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Fam Zheng <fam@euphon.net>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>
[PATCH 5/6] hw/vfio/common: Use RCU_READ macros
Posted by Philippe Mathieu-Daudé 10 months, 1 week ago 
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
Posted by Manos Pitsidianakis 10 months, 1 week ago 
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
Posted by Philippe Mathieu-Daudé 10 months, 1 week ago 
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
Posted by Philippe Mathieu-Daudé 9 months, 2 weeks ago 
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
Posted by Cédric Le Goater 9 months, 2 weeks ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.