Re: [PATCH v1 02/11] KVM: arm64: Use guard(hyp_spinlock) in pKVM hypervisor code

[Vincent Donnefort <vdonnefort@google.com>]

On Fri, Jun 12, 2026 at 07:59:16AM +0100, tabba@google.com wrote:
> Convert the manual hyp_spin_lock()/hyp_spin_unlock() pairs in
> arch/arm64/kvm/hyp/nvhe/{pkvm,mm,page_alloc,ffa}.c to
> guard(hyp_spinlock) and scoped_guard(hyp_spinlock), dropping several
> unlock-only goto labels in favour of direct returns.
> 
> hyp_fixblock_lock in mm.c is left as an explicit lock/unlock pair: it is
> acquired in hyp_fixblock_map() and released in hyp_fixblock_unmap(), so
> its critical section spans two functions and cannot be expressed as a
> single lexical scope.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/ffa.c        | 154 +++++++++++----------------
>  arch/arm64/kvm/hyp/nvhe/mm.c         |  37 ++-----
>  arch/arm64/kvm/hyp/nvhe/page_alloc.c |  13 +--
>  arch/arm64/kvm/hyp/nvhe/pkvm.c       |  86 +++++----------
>  4 files changed, 105 insertions(+), 185 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 1af722771178..46cd4fa924be 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -313,17 +313,16 @@ static void do_ffa_rxtx_unmap(struct arm_smccc_1_2_regs *res,
>  			      struct kvm_cpu_context *ctxt)
>  {
>  	DECLARE_REG(u32, id, ctxt, 1);
> -	int ret = 0;
>  
>  	if (id != HOST_FFA_ID) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
> -	hyp_spin_lock(&host_buffers.lock);
> +	guard(hyp_spinlock)(&host_buffers.lock);
>  	if (!host_buffers.tx) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out_unlock;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
>  	hyp_unpin_shared_mem(host_buffers.tx, host_buffers.tx + 1);
> @@ -336,10 +335,7 @@ static void do_ffa_rxtx_unmap(struct arm_smccc_1_2_regs *res,
>  
>  	ffa_unmap_hyp_buffers();
>  
> -out_unlock:
> -	hyp_spin_unlock(&host_buffers.lock);
> -out:
> -	ffa_to_smccc_res(res, ret);
> +	ffa_to_smccc_res(res, 0);
>  }
>  
>  static u32 __ffa_host_share_ranges(struct ffa_mem_region_addr_range *ranges,
> @@ -418,18 +414,20 @@ static void do_ffa_mem_frag_tx(struct arm_smccc_1_2_regs *res,
>  	DECLARE_REG(u32, fraglen, ctxt, 3);
>  	DECLARE_REG(u32, endpoint_id, ctxt, 4);
>  	struct ffa_mem_region_addr_range *buf;
> -	int ret = FFA_RET_INVALID_PARAMETERS;
> +	int ret;
>  	u32 nr_ranges;

nit: inverted christmas tree

>  
> -	if (fraglen > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE)
> -		goto out;
> +	if (fraglen > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE ||
> +	    fraglen % sizeof(*buf)) {

nit: I don't know if we wouldn't want extra parenthesis here for readability.

> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
> +	}
>  
> -	if (fraglen % sizeof(*buf))
> -		goto out;
> -
> -	hyp_spin_lock(&host_buffers.lock);
> -	if (!host_buffers.tx)
> -		goto out_unlock;
> +	guard(hyp_spinlock)(&host_buffers.lock);
> +	if (!host_buffers.tx) {
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
> +	}
>  
>  	buf = hyp_buffers.tx;
>  	memcpy(buf, host_buffers.tx, fraglen);
> @@ -444,19 +442,14 @@ static void do_ffa_mem_frag_tx(struct arm_smccc_1_2_regs *res,
>  		 */
>  		ffa_mem_reclaim(res, handle_lo, handle_hi, 0);
>  		WARN_ON(res->a0 != FFA_SUCCESS);
> -		goto out_unlock;
> +		ffa_to_smccc_res(res, ret);
> +		return;
>  	}
>  
>  	ffa_mem_frag_tx(res, handle_lo, handle_hi, fraglen, endpoint_id);
>  	if (res->a0 != FFA_SUCCESS && res->a0 != FFA_MEM_FRAG_RX)
>  		WARN_ON(ffa_host_unshare_ranges(buf, nr_ranges));
>  
> -out_unlock:
> -	hyp_spin_unlock(&host_buffers.lock);
> -out:
> -	if (ret)
> -		ffa_to_smccc_res(res, ret);
> -
>  	/*
>  	 * If for any reason this did not succeed, we're in trouble as we have
>  	 * now lost the content of the previous fragments and we can't rollback
> @@ -465,7 +458,6 @@ static void do_ffa_mem_frag_tx(struct arm_smccc_1_2_regs *res,
>  	 * sharing/donating them again and may possibly lead to subsequent
>  	 * failures, but this will not compromise confidentiality.
>  	 */
> -	return;
>  }
>  
>  static void __do_ffa_mem_xfer(const u64 func_id,
> @@ -480,29 +472,29 @@ static void __do_ffa_mem_xfer(const u64 func_id,
>  	struct ffa_composite_mem_region *reg;
>  	struct ffa_mem_region *buf;
>  	u32 offset, nr_ranges, checked_offset;
> -	int ret = 0;
> +	int ret;
>  
>  	if (addr_mbz || npages_mbz || fraglen > len ||
>  	    fraglen > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
>  	if (fraglen < sizeof(struct ffa_mem_region) +
>  		      sizeof(struct ffa_mem_region_attributes)) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
> -	hyp_spin_lock(&host_buffers.lock);
> +	guard(hyp_spinlock)(&host_buffers.lock);
>  	if (!host_buffers.tx) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out_unlock;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
>  	if (len > ffa_desc_buf.len) {
> -		ret = FFA_RET_NO_MEMORY;
> -		goto out_unlock;
> +		ffa_to_smccc_res(res, FFA_RET_NO_MEMORY);
> +		return;
>  	}
>  
>  	buf = hyp_buffers.tx;
> @@ -512,53 +504,41 @@ static void __do_ffa_mem_xfer(const u64 func_id,
>  			ffa_mem_desc_offset(buf, 0, hyp_ffa_version);
>  	offset = ep_mem_access->composite_off;
>  	if (!offset || buf->ep_count != 1 || buf->sender_id != HOST_FFA_ID) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out_unlock;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
>  	if (check_add_overflow(offset, sizeof(struct ffa_composite_mem_region), &checked_offset)) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out_unlock;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
>  	if (fraglen < checked_offset) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out_unlock;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
>  	reg = (void *)buf + offset;
>  	nr_ranges = ((void *)buf + fraglen) - (void *)reg->constituents;
>  	if (nr_ranges % sizeof(reg->constituents[0])) {
> -		ret = FFA_RET_INVALID_PARAMETERS;
> -		goto out_unlock;
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
>  	}
>  
>  	nr_ranges /= sizeof(reg->constituents[0]);
>  	ret = ffa_host_share_ranges(reg->constituents, nr_ranges);
> -	if (ret)
> -		goto out_unlock;
> +	if (ret) {
> +		ffa_to_smccc_res(res, ret);
> +		return;
> +	}
>  
>  	ffa_mem_xfer(res, func_id, len, fraglen);
>  	if (fraglen != len) {
> -		if (res->a0 != FFA_MEM_FRAG_RX)
> -			goto err_unshare;
> -
> -		if (res->a3 != fraglen)
> -			goto err_unshare;
> +		if (res->a0 != FFA_MEM_FRAG_RX || res->a3 != fraglen)
> +			WARN_ON(ffa_host_unshare_ranges(reg->constituents, nr_ranges));
>  	} else if (res->a0 != FFA_SUCCESS) {
> -		goto err_unshare;
> +		WARN_ON(ffa_host_unshare_ranges(reg->constituents, nr_ranges));

I am not sure this is really better for this function. At least we had a single
callsite to this WARN_ON(ffa_host_unshare_ranges) ... 

Or alternatively if we really want guard() this can just set ret = XXX and then

  if (ret)
      WARN_ON(ffa_host_unshare_ranges(reg->constituents, nr_ranges));

So we can keep a single call site for the rollback.

>  	}
> -
> -out_unlock:
> -	hyp_spin_unlock(&host_buffers.lock);
> -out:
> -	if (ret)
> -		ffa_to_smccc_res(res, ret);
> -	return;
> -
> -err_unshare:
> -	WARN_ON(ffa_host_unshare_ranges(reg->constituents, nr_ranges));
> -	goto out_unlock;
>  }
>  

[...]

>  int __pkvm_finalize_teardown_vm(pkvm_handle_t handle)
> @@ -996,22 +975,19 @@ int __pkvm_finalize_teardown_vm(pkvm_handle_t handle)
>  	struct kvm *host_kvm;
>  	unsigned int idx;
>  	size_t vm_size;
> -	int err;
>  
> -	hyp_spin_lock(&vm_table_lock);
> -	hyp_vm = get_pkvm_unref_hyp_vm_locked(handle);
> -	if (!hyp_vm || !hyp_vm->kvm.arch.pkvm.is_dying) {
> -		err = -EINVAL;
> -		goto err_unlock;
> +	scoped_guard(hyp_spinlock, &vm_table_lock) {
> +		hyp_vm = get_pkvm_unref_hyp_vm_locked(handle);
> +		if (!hyp_vm || !hyp_vm->kvm.arch.pkvm.is_dying)
> +			return -EINVAL;
> +
> +		host_kvm = hyp_vm->host_kvm;
> +
> +		/* Ensure the VMID is clean before it can be reallocated */
> +		__kvm_tlb_flush_vmid(&hyp_vm->kvm.arch.mmu);
> +		remove_vm_table_entry(handle);
>  	}
>  
> -	host_kvm = hyp_vm->host_kvm;
> -
> -	/* Ensure the VMID is clean before it can be reallocated */
> -	__kvm_tlb_flush_vmid(&hyp_vm->kvm.arch.mmu);
> -	remove_vm_table_entry(handle);
> -	hyp_spin_unlock(&vm_table_lock);
> -
>  	/* Reclaim guest pages (including page-table pages) */
>  	mc = &host_kvm->arch.pkvm.teardown_mc;
>  	stage2_mc = &host_kvm->arch.pkvm.stage2_teardown_mc;
> @@ -1042,10 +1018,6 @@ int __pkvm_finalize_teardown_vm(pkvm_handle_t handle)
>  	teardown_donated_memory(mc, hyp_vm, vm_size);
>  	hyp_unpin_shared_mem(host_kvm, host_kvm + 1);
>  	return 0;
> -
> -err_unlock:
> -	hyp_spin_unlock(&vm_table_lock);
> -	return err;

For this one too I doubt this is really interesting: only one path using
err_unlock and actually the entire label could be just removed to to simply do
hyp_spin_unlock() return -EINVAL;

This would avoid adding another tab with that scoped_guard(). But that's
probably my aversion to scoped_guard() talking.

>  }
>  
>  static u64 __pkvm_memshare_page_req(struct kvm_vcpu *vcpu, u64 ipa)
> -- 
> 2.54.0.1136.gdb2ca164c4-goog
>