Re: [PATCH 10/12] KVM: arm64: Implement HW access flag management in stage-1 SW PTW

[Marc Zyngier <maz@kernel.org>]

On Wed, 12 Nov 2025 18:34:04 +0000,
Oliver Upton <oupton@kernel.org> wrote:
> 
> Atomically update the Access flag at stage-1 when the guest has
> configured the MMU to do so. Make the implementation choice (and liberal
> interpretation of speculation) that any access type updates the Access
> flag, including AT and CMO instructions.
> 
> Restart the entire walk by returning to the exception-generating
> instruction in the case of a failed Access flag update.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_asm.h    |  6 +--
>  arch/arm64/include/asm/kvm_nested.h |  1 +
>  arch/arm64/kvm/at.c                 | 74 +++++++++++++++++++++++------
>  arch/arm64/kvm/sys_regs.c           |  9 ++--
>  4 files changed, 69 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 9da54d4ee49e..090f7b740bdc 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -246,9 +246,9 @@ extern void __kvm_tlb_flush_vmid(struct kvm_s2_mmu *mmu);
>  extern int __kvm_tlbi_s1e2(struct kvm_s2_mmu *mmu, u64 va, u64 sys_encoding);
>  
>  extern void __kvm_timer_set_cntvoff(u64 cntvoff);
> -extern void __kvm_at_s1e01(struct kvm_vcpu *vcpu, u32 op, u64 vaddr);
> -extern void __kvm_at_s1e2(struct kvm_vcpu *vcpu, u32 op, u64 vaddr);
> -extern void __kvm_at_s12(struct kvm_vcpu *vcpu, u32 op, u64 vaddr);
> +extern int __kvm_at_s1e01(struct kvm_vcpu *vcpu, u32 op, u64 vaddr);
> +extern int __kvm_at_s1e2(struct kvm_vcpu *vcpu, u32 op, u64 vaddr);
> +extern int __kvm_at_s12(struct kvm_vcpu *vcpu, u32 op, u64 vaddr);
>  
>  extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
>  
> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> index 6dbc2908aed9..905c658057a4 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -353,6 +353,7 @@ struct s1_walk_info {
>  	bool	     		be;
>  	bool	     		s2;
>  	bool			pa52bit;
> +	bool			ha;
>  };
>  
>  struct s1_walk_result {
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 74f3be46fa66..9778a4241c19 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -346,6 +346,8 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  
>  	wi->baddr &= GENMASK_ULL(wi->max_oa_bits - 1, x);
>  
> +	wi->ha = tcr & TCR_HA;
> +
>  	return 0;
>  
>  addrsz:
> @@ -380,10 +382,24 @@ static int kvm_read_s1_desc(struct kvm_vcpu *vcpu, u64 pa, u64 *desc,
>  	return 0;
>  }
>  
> +static int kvm_swap_s1_desc(struct kvm_vcpu *vcpu, u64 pa, u64 old, u64 new,
> +			    struct s1_walk_info *wi)
> +{
> +	if (wi->be) {
> +		old = cpu_to_be64(old);
> +		new = cpu_to_be64(new);
> +	} else {
> +		old = cpu_to_be64(old);
> +		new = cpu_to_be64(new);
> +	}
> +
> +	return __kvm_at_swap_desc(vcpu->kvm, pa, old, new);
> +}
> +
>  static int walk_s1(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  		   struct s1_walk_result *wr, u64 va)
>  {
> -	u64 va_top, va_bottom, baddr, desc;
> +	u64 va_top, va_bottom, baddr, desc, new_desc, ipa;
>  	int level, stride, ret;
>  
>  	level = wi->sl;
> @@ -393,7 +409,7 @@ static int walk_s1(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  	va_top = get_ia_size(wi) - 1;
>  
>  	while (1) {
> -		u64 index, ipa;
> +		u64 index;
>  
>  		va_bottom = (3 - level) * stride + wi->pgshift;
>  		index = (va & GENMASK_ULL(va_top, va_bottom)) >> (va_bottom - 3);
> @@ -490,6 +506,17 @@ static int walk_s1(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  	if (check_output_size(baddr & GENMASK(52, va_bottom), wi))
>  		goto addrsz;
>  
> +	if (wi->ha)
> +		new_desc |= PTE_AF;

What initialised new_desc the first place? Shouldn't there be a
'new_desc = desc;' somewhere before that?

> +
> +	if (new_desc != desc) {
> +		ret = kvm_swap_s1_desc(vcpu, ipa, desc, new_desc, wi);
> +		if (ret)
> +			return ret;
> +
> +		desc = new_desc;
> +	}
> +
>  	if (!(desc & PTE_AF)) {
>  		fail_s1_walk(wr, ESR_ELx_FSC_ACCESS_L(level), false);
>  		return -EACCES;
> @@ -1234,7 +1261,7 @@ static void compute_s1_permissions(struct kvm_vcpu *vcpu,
>  	wr->pr &= !pan;
>  }
>  
> -static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> +static int handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr, u64 *par)
>  {
>  	struct s1_walk_result wr = {};
>  	struct s1_walk_info wi = {};
> @@ -1259,6 +1286,11 @@ static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
>  
> +	/*
> +	 * Race to update a descriptor -- restart the walk.
> +	 */
> +	if (ret == -EAGAIN)
> +		return ret;
>  	if (ret)
>  		goto compute_par;
>  
> @@ -1292,7 +1324,8 @@ static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  		fail_s1_walk(&wr, ESR_ELx_FSC_PERM_L(wr.level), false);
>  
>  compute_par:
> -	return compute_par_s1(vcpu, &wi, &wr);
> +	*par = compute_par_s1(vcpu, &wi, &wr);
> +	return 0;
>  }
>  
>  /*
> @@ -1420,9 +1453,10 @@ static bool par_check_s1_access_fault(u64 par)
>  		 !(par & SYS_PAR_EL1_S));
>  }
>  
> -void __kvm_at_s1e01(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> +int __kvm_at_s1e01(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  {
>  	u64 par = __kvm_at_s1e01_fast(vcpu, op, vaddr);
> +	int ret;
>  
>  	/*
>  	 * If PAR_EL1 reports that AT failed on a S1 permission or access
> @@ -1434,15 +1468,20 @@ void __kvm_at_s1e01(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  	 */
>  	if ((par & SYS_PAR_EL1_F) &&
>  	    !par_check_s1_perm_fault(par) &&
> -	    !par_check_s1_access_fault(par))
> -		par = handle_at_slow(vcpu, op, vaddr);
> +	    !par_check_s1_access_fault(par)) {
> +		ret = handle_at_slow(vcpu, op, vaddr, &par);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	vcpu_write_sys_reg(vcpu, par, PAR_EL1);
> +	return 0;
>  }
>  
> -void __kvm_at_s1e2(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> +int __kvm_at_s1e2(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  {
>  	u64 par;
> +	int ret;
>  
>  	/*
>  	 * We've trapped, so everything is live on the CPU. As we will be
> @@ -1489,13 +1528,17 @@ void __kvm_at_s1e2(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  	}
>  
>  	/* We failed the translation, let's replay it in slow motion */
> -	if ((par & SYS_PAR_EL1_F) && !par_check_s1_perm_fault(par))
> -		par = handle_at_slow(vcpu, op, vaddr);
> +	if ((par & SYS_PAR_EL1_F) && !par_check_s1_perm_fault(par)) {
> +		ret = handle_at_slow(vcpu, op, vaddr, &par);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	vcpu_write_sys_reg(vcpu, par, PAR_EL1);
> +	return 0;
>  }

There is a quite a bit of churn in this patch changing the signature
of the __kvm_at_s*() functions (and whatever calls them to propagate
the errors). It'd be worth pulling this refactor as a preliminary
patch, and then focus on the functional change.

Thanks,

	M.

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