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

Fuad Tabba
2026-03-17T18:01:55Z

Hi Jonathan,

On Tue, 17 Mar 2026 at 17:40, Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Mon, 16 Mar 2026 17:35:24 +0000
> Fuad Tabba <tabba@google.com> 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@google.com>
>
> 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;
> > }
> >
>