Re: [PATCH 09/22] KVM: arm64: Compute S1 permissions as part of s1_walk()

[Leonardo Bras <leo.bras@arm.com>]

On Tue, Jun 23, 2026 at 11:41:48AM -0700, Oliver Upton wrote:
> Implementing support for hardware dirty state means that the table
> walker needs to have visibility into the permissions on the final
> translation.
> 
> Compute the S1 permissions as part of s1_walk() and initialize
> s1_walk_result before checking all fault conditions. The appropriate
> fields will be reinitialized if the walk happens to fail at a later
> point.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/kvm/at.c | 41 ++++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 083014e9d86a..6930bc3bc86b 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -461,6 +461,10 @@ static int kvm_swap_s1_desc(struct kvm_vcpu *vcpu, u64 pa, u64 old, u64 new,
>  	return __kvm_at_swap_desc(vcpu->kvm, pa, old, new);
>  }
>  
> +static void compute_s1_permissions(struct kvm_vcpu *vcpu,
> +				   struct s1_walk_info *wi,
> +				   struct s1_walk_result *wr);
> +
>  static int walk_s1(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  		   struct s1_walk_result *wr, u64 va)
>  {
> @@ -590,6 +594,20 @@ 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;
>  
> +	va_bottom += contiguous_bit_shift(desc, wi, level);
> +
> +	wr->failed = false;
> +	wr->level = level;
> +	wr->desc = desc;
> +	wr->pa = baddr & GENMASK(52, va_bottom);
> +	wr->pa |= va & GENMASK_ULL(va_bottom - 1, 0);
> +
> +	wr->nG = (wi->regime != TR_EL2) && (desc & PTE_NG);
> +	if (wr->nG)
> +		wr->asid = get_asid_by_regime(vcpu, wi->regime);
> +
> +	compute_s1_permissions(vcpu, wi, wr);
> +
>  	if (wi->ha)
>  		new_desc |= PTE_AF;
>  
> @@ -615,18 +633,6 @@ static int walk_s1(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  		return -EACCES;
>  	}
>  
> -	va_bottom += contiguous_bit_shift(desc, wi, level);
> -
> -	wr->failed = false;
> -	wr->level = level;
> -	wr->desc = desc;
> -	wr->pa = baddr & GENMASK(52, va_bottom);
> -	wr->pa |= va & GENMASK_ULL(va_bottom - 1, 0);
> -
> -	wr->nG = (wi->regime != TR_EL2) && (desc & PTE_NG);
> -	if (wr->nG)
> -		wr->asid = get_asid_by_regime(vcpu, wi->regime);
> -
>  	return 0;
>  
>  addrsz:
> @@ -1365,8 +1371,6 @@ static int handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr, u64 *par)
>  	if (ret)
>  		goto compute_par;
>  
> -	compute_s1_permissions(vcpu, &wi, &wr);
> -
>  	switch (op) {
>  	case OP_AT_S1E1RP:
>  	case OP_AT_S1E1R:


Up to this point, everything seems straightforward with the commit message:
wr setting is moved up in walk_s1(), and compute_s1_permissions() is moved 
to inside walk_s1(), from it's place in handle_at_slow() where it was 
called after walk_s1() so it should be fine for any other user that needed 
it before. 

> @@ -1690,15 +1694,10 @@ int __kvm_translate_va(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  	if (wr->level == S1_MMU_DISABLED) {
>  		wr->ur = wr->uw = wr->ux = true;
>  		wr->pr = wr->pw = wr->px = true;
> -	} else {
> -		ret = walk_s1(vcpu, wi, wr, va);
> -		if (ret)
> -			return ret;
> -
> -		compute_s1_permissions(vcpu, wi, wr);
> +		return 0;
>  	}
>  
> -	return 0;
> +	return walk_s1(vcpu, wi, wr, va);
>  }

Here the else clause is optimized-out, as the if returns in the end.
Since you run compute_s1_permissions() inside walk_s1() now, there is no 
need to test walk_s1() return value anymore. Good improvement.

FWIW:
Reviewed-by: Leonardo Bras <leo.bras@arm.com>

Thanks!
Leo