Re: [RFC PATCH 1/5] KVM: arm64: pgtables: Change write bit from S2AP_W to DBM

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

On Wed, Sep 16, 2026 at 01:20:33PM +0100, Marc Zyngier wrote:
> On Wed, 16 Sep 2026 12:22:05 +0100,
> Leonardo Bras <leo.bras@arm.com> wrote:
> > 
> > On Tue, Sep 15, 2026 at 05:37:15PM -0700, Oliver Upton wrote:
> > > On Tue, Sep 15, 2026 at 06:12:45PM +0100, Leonardo Bras wrote:
> > > > > Yes, the HW should ignore it. But we have also
> > > > > seen quite a few broken designs in this area...
> > > > > 
> > > > 
> > > > I lack experience on what bad thing could happen. So I will expand on what 
> > > > I belive to understand up to here:
> > > > 
> > > > - The PTE is in memory, so the DBM bit can be set regardless of being RES0
> > > > - For SW pagetable walking, I don't think 'bit 51 == 0' is checked
> > > > - For HW pagetable walking, maybe some faulty implementation may rely on 
> > > >   bit51 being RES0, and fault otherwise.
> > > > 
> > > > If that's the case, then we would have to actually support both encodings, 
> > > > and only enable the new one if HAFDBS is available in the system. 
> > > > 
> > > > I just wonder how high are the chances to have such a broken design, 
> > > > or other broken designs did not come to my mind, and if we have to start 
> > > > with that multiple-encoding option.
> > > 
> > > FWIW, the host stage-1 already uses the DBM bit unconditionally,
> > > treating it as a software bit on implementations without HAFDBS.
> > > Although given the quality of any garden variety Arm MMU I understand
> > > where Marc is coming from.
> > > 
> > > I don't think the HAFDBS enablement is complicated enough to be done in
> > > a separate series without any meaningful users, nor would I really be
> > > interested in taking it without, say, HDBSS.
> > > 
> > > Can you please work with Tian to get a combined series out for this?
> > > 
> > 
> > Hi Oliver, thanks for reviewing!
> > 
> > Sure, one of the reasons I sent like this is so Tian could use it as a base 
> > for his next version.
> > 
> > 
> > > > > > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > > > > > index 17123f0b6dab..eb8dfffc32c7 100644
> > > > > > --- a/arch/arm64/kvm/nested.c
> > > > > > +++ b/arch/arm64/kvm/nested.c
> > > > > > @@ -379,21 +379,23 @@ static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, phys_addr_t ipa,
> > > > > >  	}
> > > > > >  
> > > > > >  	addr_bottom += contiguous_bit_shift(desc, wi, level);
> > > > > >  
> > > > > >  	/* Calculate and return the result */
> > > > > >  	paddr = (desc & GENMASK_ULL(47, addr_bottom)) |
> > > > > >  		(ipa & GENMASK_ULL(addr_bottom - 1, 0));
> > > > > >  	out->output = paddr;
> > > > > >  	out->block_size = 1UL << ((3 - level) * stride + wi->pgshift);
> > > > > >  	out->readable = desc & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
> > > > > > -	out->writable = desc & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > > > > > +	/* Takes care of both RO/RW and RO/WC/WD encodings */
> > > > > > +	out->writable = desc & (KVM_PTE_LEAF_ATTR_HI_S2_DBM |
> > > > > > +				KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W);
> > > > > 
> > > > > Absolutely NOT. For a start, NV doesn't support FEAT_HAFDBS. But even
> > > > > if it did, you are now actively corrupting memory by turning a RO
> > > > > mapping with a spurious DBM bit set into a writable mapping.
> > > > > VTCR_EL2.HD exists for a reason.
> > > > > 
> > > > > Do you see why your blanket approach of equating DBM with writable is
> > > > > plain wrong?
> > > > 
> > > > Sorry, not really... please help me understand it.
> > > > 
> > > > When you say a spurious DBM bit, what does it mean?
> > > 
> > > You've implemented the exact sort of bug that was alluded to above. In
> > > this case it's a software page table walker consuming DBM regardless of
> > > the value of VTCR_EL2.HD.
> > > 
> > 
> > So you mean that DBM being treated as "writable" could _only_ happen if we 
> > have VTCR_EL2.HD=1? I was previously under the impression that it could be 
> > used regardless of HD value.
> 
> Then you have failed to understand the architecture. When
> VTCR_EL2.HD==0, DBM can be treated as RES0, RES0, or RES0.
> 
> R_XZFQH and R_BRFGY are pretty clear that DBM can only be evaluated
> when dirty hw update is enabled, and I_MHJZP tells you what it means
> for S2 to have this enabled.

Right, I_MHJZP says that if VTCR_EL2.HD is 1, then dirty state hardware 
management is enabled.

Then, R_XZFQH and R_BRFGY say that if (DBM+S2AP) bit combination is the 
given and the dirty state hardware management is enabled then a block 
descriptor is WC/WD.

R_XZFQH header, as an example:
For each translation stage using Direct permissions, if all of the 
following apply, then a Block descriptor or Page descriptor is described 
as writable-clean[...]

It says: 
"if all apply, the block is WC", 
it does not say 
"only if all apply, the block is WC".
so it forces one way, but not the other.

So I previously understood that we could also use the encoding for WC/WD in 
software, to avoid having 2 distinct encodings, when VTCR_EL2.HD=0. 

If that's not possible, as you mentioned, then yes, I failed to get that 
part. I also see that you mention it being RES0 when VTCR_EL2.HD==0, but I 
honestly could not find reference to that. :(

What I could find on Arm ARM M.cc was Table D8-53, which states for 
bit 51 that it's DBM if indirect permissions are disabled, or PIINDEX[1] 
if they are enabled. There is also no such information in D8.5.2 about this 
res0 behavior. 

So I am possibly missing the proper place to look for that information. 
Could you please share that with me so I can improve and maybe avoid being 
a inconvenience in the future?

Thanks!
Leo