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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

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 :)
> ---
>  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.

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;
>  }
>