Re: [PATCH v2 07/28] KVM: arm64: nv: Set dirty state at stage-2

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

On Fri, Oct 23, 2026 at 01:55:49PM -0700, Oliver Upton wrote:
> Set the dirty state on descriptors at stage-2 for write accesses when
> DBM is set.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/kvm/nested.c | 51 +++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index dea5c31f10df..1d581d3bd557 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -151,6 +151,7 @@ struct s2_walk_info {
>  	unsigned int	t0sz;
>  	bool		be;
>  	bool		ha;
> +	bool		hd;
>  };
>  
>  struct s2_walk_step {
> @@ -246,6 +247,20 @@ static int read_guest_s2_desc(struct kvm_vcpu *vcpu, struct s2_walk_step *ws,
>  	return 0;
>  }
>  
> +static bool should_set_dirty_state(struct s2_walk_info *wi, struct s2_walk_step *ws,
> +				   struct kvm_s2_trans *out, struct kvm_walk_access *access)
> +{
> +	switch (access->type) {
> +	/* R_ZYGHJ */
> +	case WALK_ACCESS_CMO:
> +	case WALK_ACCESS_AT:
> +		return false;
> +	default:
> +		/* R_NSXRD */
> +		return access->write && wi->hd && out->writable;
> +	}
> +}
> +
>  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)
> @@ -258,6 +273,9 @@ static int handle_desc_update(struct kvm_vcpu *vcpu, struct s2_walk_info *wi,
>  	if (wi->ha)
>  		new |= KVM_PTE_LEAF_ATTR_LO_S2_AF;
>  
> +	if (should_set_dirty_state(wi, ws, out, access))
> +		new |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> +
>  	if (old == new)
>  		return 0;
>  
> @@ -272,11 +290,15 @@ static int handle_desc_update(struct kvm_vcpu *vcpu, struct s2_walk_info *wi,
>  	}
>  
>  	ret = __kvm_at_swap_desc(vcpu->kvm, ws->desc_pa, old, new);
> -	if (!ret || ret == -EAGAIN)
> +	if (ret == -EAGAIN)
>  		return ret;
> +	if (!ret) {
> +		out->desc = ws->desc;
> +		out->dirty = out->desc & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> +		return 0;
> +	}
>  
>  	out->esr = ESR_ELx_FSC_SEA_TTW(ws->level);
> -	out->desc = ws->desc;
>  	return 1;
>  }


Before that out->desc would only be updated if __kvm_at_swap_desc() 
returned != (0 or -EAGAIN). Now it only gets updated if ret == 0.
Is this intentional?

>  
> @@ -422,16 +444,6 @@ static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, struct kvm_walk_access *acc
>  		return 1;
>  	}
>  
> -	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);
> -		out->desc = ws.desc;
> -		return 1;
> -	}
> -
>  	addr_bottom += contiguous_bit_shift(ws.desc, wi, ws.level);
>  
>  	/* Calculate and return the result */
> @@ -441,6 +453,20 @@ static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, struct kvm_walk_access *acc
>  	compute_s2_permissions(vcpu, wi, &ws, out);
>  	out->level = ws.level;
>  	out->desc = ws.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 = (struct kvm_s2_trans) {
> +			.esr	= compute_fsc(ws.level, ESR_ELx_FSC_ACCESS),
> +			.desc	= ws.desc,
> +		};
> +
> +		return 1;
> +	}
> +
>  	return 0;
>  }

The block got moved down in walk_nested_s2_pgd(), and IIUC if the 
descriptor lacks the AF, we zero all other fields except esr and desc, 
cleaning everything that just got set in the lines above (out->writable, 
etc).

Any particular reason for doing it like this, instead of keeping where it 
was, and avoid calculating the other out->* fields?


>  
> @@ -537,6 +563,7 @@ static void setup_s2_walk(struct kvm_vcpu *vcpu, struct s2_walk_info *wi)
>  			      ps_to_output_size(FIELD_GET(VTCR_EL2_PS_MASK, vtcr), false));
>  	wi->ha = vtcr & VTCR_EL2_HA;
>  	wi->be = vcpu_read_sys_reg(vcpu, SCTLR_EL2) & SCTLR_ELx_EE;
> +	wi->hd = wi->ha && (vtcr & VTCR_EL2_HD);
>  }
>  
>  int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, struct kvm_walk_access *access,
> -- 
> 2.47.3
> 

Thanks!
Leo