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!