[PATCH v1 02/11] KVM: arm64: Use guard(hyp_spinlock) in pKVM hypervisor code
Vincent Donnefort
vdonnefort at google.com
Mon Jun 15 05:53:35 PDT 2026
On Fri, Jun 12, 2026 at 07:59:16AM +0100, tabba at 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 at 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
>
More information about the linux-arm-kernel
mailing list