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.