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

[Oliver Upton <oupton@kernel.org>]

On Wed, Sep 23, 2026 at 03:09:11PM +0100, Leonardo Bras wrote:
> > @@ -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)

VTCR_EL2 is a sanitised sysreg, the enforcement of RESx happens
implicitly on read... Start looking at config.c and follow the masks.

> >  }
> >  
> >  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)

Patch ordering could be better here, have a look at patch #8.
compute_s2_permissions() will interpret the DBM bit as writable when
VTCR_EL2.HD is set.

Thanks,
Oliver