Re: [PATCH v2 09/14] KVM: arm64: Add helper for swapping guest descriptor

[Marc Zyngier <maz@kernel.org>]

On Mon, 17 Nov 2025 22:43:20 +0000,
Oliver Upton <oupton@kernel.org> wrote:
> 
> Implementing FEAT_HAFDBS in KVM's software PTWs requires the ability to
> CAS a descriptor to update the in-memory value. Add an accessor to do
> exactly that, coping with the fact that guest descriptors are in user
> memory (duh).
> 
> While FEAT_LSE required on any system that implements NV, KVM now uses
> the stage-1 PTW for non-nested use cases meaning an LL/SC implementation
> is necessary as well.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_nested.h |  2 +
>  arch/arm64/kvm/at.c                 | 87 +++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> index 5d967b60414c..6dbc2908aed9 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -403,4 +403,6 @@ void kvm_handle_s1e2_tlbi(struct kvm_vcpu *vcpu, u32 inst, u64 val);
>  		(FIX_VNCR - __c);				\
>  	})
>  
> +int __kvm_at_swap_desc(struct kvm *kvm, gpa_t ipa, u64 old, u64 new);
> +
>  #endif /* __ARM64_KVM_NESTED_H */
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index a295a37dd3b1..22e2ee4e182e 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -1650,3 +1650,90 @@ int __kvm_find_s1_desc_level(struct kvm_vcpu *vcpu, u64 va, u64 ipa, int *level)
>  		return ret;
>  	}
>  }
> +
> +static int __lse_swap_desc(u64 __user *ptep, u64 old, u64 new)
> +{
> +	u64 tmp = old;
> +	int ret = 0;
> +
> +	uaccess_enable_privileged();
> +
> +	asm volatile(__LSE_PREAMBLE
> +		     "1: cas	%[old], %[new], %[addr]\n"
> +		     "2:\n"
> +		     _ASM_EXTABLE_UACCESS_ERR(1b, 2b, %w[ret])
> +		     : [old] "+r" (old), [addr] "+Q" (*ptep), [ret] "+r" (ret)
> +		     : [new] "r" (new)
> +		     : "memory");
> +
> +	uaccess_disable_privileged();
> +
> +	if (ret)
> +		return ret;
> +	if (tmp != old)
> +		return -EAGAIN;
> +
> +	return ret;
> +}
> +
> +static int __llsc_swap_desc(u64 __user *ptep, u64 old, u64 new)
> +{
> +	u64 tmp;
> +	int ret;
> +
> +	uaccess_enable_privileged();
> +
> +	asm volatile("prfm	pstl1strm, %[addr]\n"
> +		     "1: ldxr	%[tmp], %[addr]\n"
> +		     "sub	%[tmp], %[tmp], %[old]\n"
> +		     "cbnz	%[tmp], 3f\n"
> +		     "2: stlxr	%w[ret], %[new], %[addr]\n"

So if we don't find the correct value, we skip the store, which means
that ret is not updated.

> +		     "3:\n"
> +		     _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w[ret])
> +		     _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w[ret])
> +		     : [ret] "=r" (ret), [addr] "+Q" (*ptep), [tmp] "=&r" (tmp)
> +		     : [old] "r" (old), [new] "r" (new)
> +		     : "memory");
> +
> +	uaccess_disable_privileged();
> +
> +	/* STLXR didn't update the descriptor */
> +	if (ret == 1)
> +		return -EAGAIN;
> +
> +	return ret;

But then ret has an undefined value, and bad things will happen. I
reckon you need to initialise ret to 0, and evaluate tmp, much like
you do in the CAS case. Something like this:

diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index 7c13e69a7d9a1..67a93c7954c7d 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -1724,8 +1724,8 @@ static int __lse_swap_desc(u64 __user *ptep, u64 old, u64 new)
 
 static int __llsc_swap_desc(u64 __user *ptep, u64 old, u64 new)
 {
+	int ret = 0;
 	u64 tmp;
-	int ret;
 
 	uaccess_enable_privileged();
 
@@ -1743,8 +1743,11 @@ static int __llsc_swap_desc(u64 __user *ptep, u64 old, u64 new)
 
 	uaccess_disable_privileged();
 
-	/* STLXR didn't update the descriptor */
-	if (ret == 1)
+	if (ret < 0)
+		return ret;
+
+	/* STLXR didn't update the descriptor, or the compare failed */
+	if (tmp || ret == 1)
 		return -EAGAIN;
 
 	return ret;

> +}
> +
> +int __kvm_at_swap_desc(struct kvm *kvm, gpa_t ipa, u64 old, u64 new)
> +{
> +	struct kvm_memory_slot *slot;
> +	unsigned long hva;
> +	u64 __user *ptep;
> +	bool writable;
> +	int offset;
> +	gfn_t gfn;
> +	int r;
> +
> +	gfn = ipa >> PAGE_SHIFT;
> +	offset = offset_in_page(ipa);
> +	slot = gfn_to_memslot(kvm, gfn);
> +	hva = gfn_to_hva_memslot_prot(slot, gfn, &writable);

There is a very strong assumption that we hold the srcu read lock
here. Can we add a

      lockdep_assert(srcu_read_lock_held(&kvm->srcu));

at the beginning of this function?

> +	if (kvm_is_error_hva(hva))
> +		return -EINVAL;
> +	if (!writable)
> +		return -EPERM;
> +
> +	ptep = (u64 __user *)hva + offset;
> +	if (cpus_have_final_cap(ARM64_HAS_LSE_ATOMICS))
> +		r = __lse_swap_desc(ptep, old, new);
> +	else
> +		r = __llsc_swap_desc(ptep, old, new);
> +
> +	if (r < 0)
> +		return r;
> +	if (r)
> +		return -EAGAIN;

Isn't that case always caught by the first check?

> +
> +	mark_page_dirty_in_slot(kvm, slot, gfn);
> +	return 0;
> +}

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.