[PATCH v1 02/11] KVM: arm64: Use guard(hyp_spinlock) in pKVM hypervisor code
Fuad Tabba
tabba at google.com
Mon Jun 15 06:11:37 PDT 2026
Hi Vincent,
On Mon, 15 Jun 2026 at 13:53, Vincent Donnefort <vdonnefort at google.com> wrote:
>
> 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
Ack.
>
> >
> > - 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.
Sure, will add parentheses around the second operand.
>
> > + 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.
Agreed, the single rollback callsite is better. I'll use a flag to
keep the original control flow readable:
>
> > }
> > -
> > -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.
The scoped_guard makes the lock scope visually explicit here: the
lock must be dropped before the long teardown tail that follows, and
scoped_guard makes that boundary clear. It also prevents forgetting
the unlock on the early return. I think the extra tab is worth it for the
clarity it provides.
Thanks,
/fuad
>
> > }
> >
> > 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