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

[Marc Zyngier <maz@kernel.org>]

On Wed, 12 Nov 2025 18:34:03 +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                 | 86 +++++++++++++++++++++++++++++
>  2 files changed, 88 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..74f3be46fa66 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -1650,3 +1650,89 @@ 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)
> +{
> +	unsigned int loops = 128;
> +	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"
> +		     "cbz	%w[ret], 4f\n"
> +		     "sub	%w[loops], %w[loops], #1\n"
> +		     "cbnz	%w[loops], 1b\n"
> +		     "3: mov	%w[ret], %w[eagain]\n"
> +		     "4:\n"
> +		     : [ret] "=r" (ret), [addr] "+Q" (*ptep), [tmp] "=&r" (tmp),
> +		       [loops] "+r" (loops)
> +		     : [old] "r" (old), [new] "r" (new), [eagain] "Ir" (-EAGAIN)
> +		     : "memory");

Why doesn't this need an exception table as well? I'd expect it to
fault just as much as the LSE version (and ret cannot report -EFAULT,
for example).

I'm also on the fence about the bounded loop. Yes, forward progress is
a problem, but it should only affect large systems which can readily
use the atomic instructions. I'd rather we get rid of it until proven
that we really need something like it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.