Re: [PATCH 10/22] KVM: arm64: Plumb through access descriptor for stage-1

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

On Tue, Jun 23, 2026 at 11:41:49AM -0700, Oliver Upton wrote:
> Pass sufficient context to the stage-1 walk such that access-dependent
> features like FEAT_HAFDBS can implement the correct behavior.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_nested.h |  4 +++-
>  arch/arm64/kvm/at.c                 | 32 ++++++++++++++++++++++-------
>  arch/arm64/kvm/nested.c             | 10 ++++++---
>  3 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> index 71814c4aac3e..347d79fd350c 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -112,6 +112,8 @@ struct kvm_walk_access {
>  		WALK_ACCESS_CMO,
>  		WALK_ACCESS_AT,
>  		WALK_ACCESS_S1PTW,
> +		WALK_ACCESS_NV2,
> +		WALK_ACCESS_NONARCH,
>  	} type;
>  
>  	u64	ia;
> @@ -357,7 +359,7 @@ static inline void fail_s1_walk(struct s1_walk_result *wr, u8 fst, bool s1ptw)
>  }
>  
>  int __kvm_translate_va(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
> -		       struct s1_walk_result *wr, u64 va);
> +		       struct s1_walk_result *wr, struct kvm_walk_access *access);
>  int __kvm_find_s1_desc_level(struct kvm_vcpu *vcpu, u64 va, u64 ipa,
>  			     int *level);
>  
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 6930bc3bc86b..597e9cddfc7e 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -466,12 +466,13 @@ static void compute_s1_permissions(struct kvm_vcpu *vcpu,
>  				   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)
> +		   struct s1_walk_result *wr, struct kvm_walk_access *access)
>  {
> -	u64 va_top, va_bottom, baddr, desc, new_desc, ipa;
> +	u64 va_top, va_bottom, baddr, desc, new_desc, ipa, va;
>  	struct kvm_s2_trans s2_trans = {};
>  	int level, stride, ret;
>  
> +	va = access->ia;
>  	level = wi->sl;
>  	stride = wi->pgshift - 3;
>  	baddr = wi->baddr;
> @@ -1340,6 +1341,7 @@ static void compute_s1_permissions(struct kvm_vcpu *vcpu,
>  
>  static int handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr, u64 *par)
>  {
> +	struct kvm_walk_access access = {};
>  	struct s1_walk_result wr = {};
>  	struct s1_walk_info wi = {};
>  	bool perm_fail = false;
> @@ -1357,9 +1359,21 @@ static int handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr, u64 *par)
>  	if (wr.level == S1_MMU_DISABLED)
>  		goto compute_par;
>  
> +	access.type = WALK_ACCESS_AT;
> +	access.ia = vaddr;
> +	switch (op) {
> +	case OP_AT_S1E1WP:
> +	case OP_AT_S1E1W:
> +	case OP_AT_S1E2W:
> +	case OP_AT_S1E0W:
> +		access.write = true;
> +		break;
> +	default:
> +	}

Do we need a default clause that does nothing?

> +
>  	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  
> -	ret = walk_s1(vcpu, &wi, &wr, vaddr);
> +	ret = walk_s1(vcpu, &wi, &wr, &access);
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
>  
> @@ -1683,11 +1697,11 @@ int __kvm_at_s12(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>   * set. The rest of the wi and wr should be 0-initialised.
>   */
>  int __kvm_translate_va(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
> -		       struct s1_walk_result *wr, u64 va)
> +		       struct s1_walk_result *wr, struct kvm_walk_access *access)
>  {
>  	int ret;
>  
> -	ret = setup_s1_walk(vcpu, wi, wr, va);
> +	ret = setup_s1_walk(vcpu, wi, wr, access->ia);
>  	if (ret)
>  		return ret;
>  
> @@ -1697,7 +1711,7 @@ int __kvm_translate_va(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  		return 0;
>  	}
>  
> -	return walk_s1(vcpu, wi, wr, va);
> +	return walk_s1(vcpu, wi, wr, access);
>  }
>  
>  struct desc_match {
> @@ -1735,6 +1749,10 @@ int __kvm_find_s1_desc_level(struct kvm_vcpu *vcpu, u64 va, u64 ipa, int *level)
>  		.as_el0	= false,
>  		.pan	= false,
>  	};
> +	struct kvm_walk_access access = {
> +		.type	= WALK_ACCESS_NONARCH,
> +		.ia	= va,
> +	};
>  	struct s1_walk_result wr = {};
>  	int ret;
>  
> @@ -1754,7 +1772,7 @@ int __kvm_find_s1_desc_level(struct kvm_vcpu *vcpu, u64 va, u64 ipa, int *level)
>  	}
>  
>  	/* Walk the guest's PT, looking for a match along the way */
> -	ret = walk_s1(vcpu, &wi, &wr, va);
> +	ret = walk_s1(vcpu, &wi, &wr, &access);
>  	switch (ret) {
>  	case -EINTR:
>  		/* We interrupted the walk on a match, return the level */
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 4f13f37e560b..54228db30371 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -1425,6 +1425,7 @@ static u64 read_vncr_el2(struct kvm_vcpu *vcpu)
>  
>  static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
>  {
> +	struct kvm_walk_access access = {};
>  	struct kvm_memory_slot *memslot;
>  	bool write_fault, writable;
>  	unsigned long mmu_seq;
> @@ -1434,6 +1435,7 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
>  	int ret;
>  
>  	vt = vcpu->arch.vncr_tlb;
> +	write_fault = kvm_is_write_fault(vcpu);
>  
>  	/*
>  	 * If we're about to walk the EL2 S1 PTs, we must invalidate the
> @@ -1459,12 +1461,14 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
>  
>  	va =  read_vncr_el2(vcpu);
>  
> -	ret = __kvm_translate_va(vcpu, &vt->wi, &vt->wr, va);
> +	access.type = WALK_ACCESS_NV2;
> +	access.ia = va;
> +	access.write = write_fault;

It's not clear on why we need the proxy var write_fault.

> +
> +	ret = __kvm_translate_va(vcpu, &vt->wi, &vt->wr, &access);
>  	if (ret)
>  		return ret;
>  
> -	write_fault = kvm_is_write_fault(vcpu);
> -
>  	mmu_seq = vcpu->kvm->mmu_invalidate_seq;
>  	smp_rmb();
>  
> -- 
> 2.47.3
> 

IIUC:
In general terms, change some function signatures to replace va to a struct 
pointer that contains extra information, such as walk type and it being a 
write fault.

On top of that, there are 2 extra kvm_walk_access flags added here, and I 
see them being attributed, but no information on what they do different.

Since there are already flags being treated somewhere, maybe a patch just 
adding these new flags and showing what they do differently on the 
processing, then adding the remaining of this patch in a new one would make 
them easier to understand. 

(I still haven't read what those flags do differently in the next patches, 
but seems like a no-op at this point, even though it should be doing 
something different here. Please help me understand if I am missing some 
context)

Thanks!
Leo