Re: [PATCH 03/10] KVM: arm64: Use guard(hyp_spinlock) in ffa.c

[Fuad Tabba]

Hi Jonathan,

On Tue, 17 Mar 2026 at 17:40, Jonathan Cameron
<jonathan.cameron@xxxxxxxxxx> wrote:
>
> On Mon, 16 Mar 2026 17:35:24 +0000
> Fuad Tabba <tabba@xxxxxxxxxx> wrote:
>
> > Migrate manual hyp_spin_lock() and hyp_spin_unlock() calls managing
> > host_buffers.lock and version_lock to use the guard(hyp_spinlock)
> > macro.
> >
> > This eliminates manual unlock calls on return paths and simplifies
> > error handling by replacing goto labels with direct returns.
> >
> > Change-Id: I52e31c0bed3d2772c800a535af8abdabd81a178b
> > Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx>
>
> See below and read the cleanup.h guidance notes on usage at the top.
> Specifically:
>
>  * Lastly, given that the benefit of cleanup helpers is removal of
>  * "goto", and that the "goto" statement can jump between scopes, the
>  * expectation is that usage of "goto" and cleanup helpers is never
>  * mixed in the same function. I.e. for a given routine, convert all
>  * resources that need a "goto" cleanup to scope-based cleanup, or
>  * convert none of them.
>
> Doing otherwise might mean an encounter with a grumpy Linus :)

I should have read the cleanup.h guidance more closely, thanks for
pointing this out.

> > ---
> >  arch/arm64/kvm/hyp/nvhe/ffa.c | 86 +++++++++++++++++++------------------------
> >  1 file changed, 38 insertions(+), 48 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > index 94161ea1cd60..0c772501c3ba 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > @@ -239,6 +239,8 @@ static void do_ffa_rxtx_map(struct arm_smccc_1_2_regs *res,
> >       int ret = 0;
> >       void *rx_virt, *tx_virt;
> >
> > +     guard(hyp_spinlock)(&host_buffers.lock);
> > +
> >       if (npages != (KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) / FFA_PAGE_SIZE) {
> >               ret = FFA_RET_INVALID_PARAMETERS;
> >               goto out;
> > @@ -249,10 +251,9 @@ static void do_ffa_rxtx_map(struct arm_smccc_1_2_regs *res,
> >               goto out;
> >       }
> >
> > -     hyp_spin_lock(&host_buffers.lock);
> >       if (host_buffers.tx) {
> >               ret = FFA_RET_DENIED;
> > -             goto out_unlock;
> > +             goto out;
> Whilst it isn't a bug as such because you increased the scope to avoid it,
> there is some pretty strong guidance in cleanup.h about not using guard() or
> any of the other magic if a function that also contains gotos.  That came from
> some views Linus expressed pretty clearly about the dangers that brings (the
> problem is a goto that crosses a guard() being defined and the risk of
> refactors that happen to end up with that + general view that cleanup.h
> stuff is about removing gotos, so if you still have them, why bother!
>
> You can usually end up with the best of all worlds via some refactors
> to pull out helper functions, but that's a lot of churn.

If I respin this series (depending on whether the maintainers think
it's worth the hassle), I'll remove all changes that just add churn.

Thanks for having a look at this and the other ones.

Cheers,
/fuad


>
> Jonathan
>
>
> >       }
> >
> >       /*
> > @@ -261,7 +262,7 @@ static void do_ffa_rxtx_map(struct arm_smccc_1_2_regs *res,
> >        */
> >       ret = ffa_map_hyp_buffers(npages);
> >       if (ret)
> > -             goto out_unlock;
> > +             goto out;
> >
> >       ret = __pkvm_host_share_hyp(hyp_phys_to_pfn(tx));
> >       if (ret) {
> > @@ -292,8 +293,6 @@ static void do_ffa_rxtx_map(struct arm_smccc_1_2_regs *res,
> >       host_buffers.tx = tx_virt;
> >       host_buffers.rx = rx_virt;
> >
> > -out_unlock:
> > -     hyp_spin_unlock(&host_buffers.lock);
> >  out:
> >       ffa_to_smccc_res(res, ret);
> >       return;
> > @@ -306,7 +305,7 @@ static void do_ffa_rxtx_map(struct arm_smccc_1_2_regs *res,
> >       __pkvm_host_unshare_hyp(hyp_phys_to_pfn(tx));
> >  err_unmap:
> >       ffa_unmap_hyp_buffers();
> > -     goto out_unlock;
> > +     goto out;
> >  }
> >
>