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

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

On Tue, Jun 23, 2026 at 11:41:46AM -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 | 43 +++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index a70af3b3f05d..e5a407fc0880 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -132,6 +132,7 @@ struct s2_walk_info {
>  	unsigned int	t0sz;
>  	bool		be;
>  	bool		ha;
> +	bool		hd;
>  };
>  
>  struct s2_walk_step {
> @@ -227,6 +228,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_RKMHW */
> +	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)
> @@ -239,6 +254,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;
>  
> @@ -403,16 +421,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 */
> @@ -422,6 +430,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;
>  }
>  
> @@ -518,6 +540,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);

Should we not check if the feature is available before setting the HD bit?
Or does it being RES0 mean that it can't be 1 in any case? 
(The Arm ARM's Glossary seems to allow direct writes to RES0 sometimes, 
IIUC)


>  }
>  
>  int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, struct kvm_walk_access *access,
> -- 
> 2.47.3
> 


IIUC here you add the hd bit for the walk info, and set it based on VTCR, 
using it to decide on automatically uptating the dirty-state if it's also a 
write fault and the page is writable.

Then you move the the kvm_walk_nested_s2() and AF testing code to after the 
functions that fill the out struct, so you can use it to figure out the 
writable part used above.

Everything seems right, but one part:

IIRC, HAFDBS would need HD=1, and DBM=1 set to be able to properly update 
the dirty bit. DBM should be the 'writable' here, but you are using S2AP[1] 
to decide on writable instead, which I understand to be incorrect according 
to documentation. I am sure you have reason for that here, thogh.

On the other hand, since you are effectively making dirty=writable in a
previous patch, the HAFDBS mechanism here seems not to work at all:

compute_s2_permissions(): trans->writable = ws->desc.S2AP[1];
('trans' is 'out')
handle_desc_update() 	: new = ws->desc;
			  if (... && out->writable)
				new |= S2AP[1];
 
So 'new' will only get the dirty bit, if it already has the dirty bit. :(

(I can be missing something here, though)

Thanks!
Leo