Re: [PATCH 06/22] KVM: arm64: nv: Use a helper for stage-2 descriptor updates

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

On Tue, Jun 23, 2026 at 11:41:45AM -0700, Oliver Upton wrote:
> Use a helper for handling stage-2 descriptor updates so the
> implementation can be shared with updates to table descriptors.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/kvm/nested.c | 47 +++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index c2fb7290f0c8..a70af3b3f05d 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -227,9 +227,23 @@ static int read_guest_s2_desc(struct kvm_vcpu *vcpu, struct s2_walk_step *ws,
>  	return 0;
>  }
>  
> -static int swap_guest_s2_desc(struct kvm_vcpu *vcpu, phys_addr_t pa, u64 old, u64 new,
> -			      struct s2_walk_info *wi)
> +static int handle_desc_update(struct kvm_vcpu *vcpu, struct s2_walk_info *wi,
> +			      struct s2_walk_step *ws, struct kvm_s2_trans *out,
> +			      struct kvm_walk_access *access)
>  {
> +	u64 old, new;
> +	int ret;
> +
> +	old = new = ws->desc;
> +
> +	if (wi->ha)
> +		new |= KVM_PTE_LEAF_ATTR_LO_S2_AF;
> +
> +	if (old == new)
> +		return 0;
> +
> +	ws->desc = new;
> +
>  	if (wi->be) {
>  		old = (__force u64)cpu_to_be64(old);
>  		new = (__force u64)cpu_to_be64(new);
> @@ -238,7 +252,13 @@ static int swap_guest_s2_desc(struct kvm_vcpu *vcpu, phys_addr_t pa, u64 old, u6
>  		new = (__force u64)cpu_to_le64(new);
>  	}
>  
> -	return __kvm_at_swap_desc(vcpu->kvm, pa, old, new);
> +	ret = __kvm_at_swap_desc(vcpu->kvm, ws->desc_pa, old, new);
> +	if (!ret || ret == -EAGAIN)
> +		return ret;
> +
> +	out->esr = ESR_ELx_FSC_SEA_TTW(ws->level);
> +	out->desc = ws->desc;
> +	return 1;
>  }
>  
>  static void compute_s2_permissions(struct kvm_vcpu *vcpu, struct s2_walk_info *wi,
> @@ -287,7 +307,6 @@ static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, struct kvm_walk_access *acc
>  	struct s2_walk_step ws = {};
>  	phys_addr_t base_addr;
>  	unsigned int addr_top, addr_bottom;
> -	u64 new_desc;  /* page table entry */
>  	int ret;
>  
>  	switch (BIT(wi->pgshift)) {
> @@ -340,8 +359,6 @@ static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, struct kvm_walk_access *acc
>  			return ret;
>  		}
>  
> -		new_desc = ws.desc;
> -
>  		/* Check for valid descriptor at this point */
>  		if (!(ws.desc & KVM_PTE_VALID)) {
>  			out->esr = compute_fsc(ws.level, ESR_ELx_FSC_FAULT);
> @@ -386,21 +403,9 @@ static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, struct kvm_walk_access *acc
>  		return 1;
>  	}
>  
> -	if (wi->ha)
> -		new_desc |= KVM_PTE_LEAF_ATTR_LO_S2_AF;
> -
> -	if (new_desc != ws.desc) {
> -		ret = swap_guest_s2_desc(vcpu, ws.desc_pa, ws.desc, new_desc, wi);
> -		if (ret == -EAGAIN)
> -			return ret;
> -		if (ret) {
> -			out->esr = ESR_ELx_FSC_SEA_TTW(ws.level);
> -			out->desc = ws.desc;
> -			return 1;
> -		}
> -
> -		ws.desc = new_desc;
> -	}
> +	ret = handle_desc_update(vcpu, wi, &ws, out, access);
> +	if (ret)
> +		return ret;
>  
>  	if (!(ws.desc & KVM_PTE_LEAF_ATTR_LO_S2_AF)) {
>  		out->esr = compute_fsc(ws.level, ESR_ELx_FSC_ACCESS);
> -- 
> 2.47.3
> 

IIUC, you pulled stuff around swap_guest_s2_desc() and put it inside it, 
while changing the function name to handle_desc_update(), so it should be 
the same logic.

It looks much better this way, and most things seem to have the same efect.

The only thing that I notice was "ws->desc = new":
The previous version would do that only after running __kvm_at_swap_desc() 
and only when the return value is zero. 

The new version does that every time "old != new", and before running 
__kvm_at_swap_desc() and does not take into account it's return value.

This means that on this new version if __kvm_at_swap_desc() returns 1:
	ws->desc = new;
	ret = 1;
	out->desc = ws->desc; //(i.e. new)

And in the previous version, it would make out->desc = ws->desc, that being 
the original value, not the new one.

Is this ok? 
(I am commenting because it could be unintentional)

Thanks!
Leo