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

["Cédric Le Goater" <clg@redhat.com>]

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.