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

[Marc Zyngier]

On Wed, 16 Sep 2026 12:22:05 +0100,
Leonardo Bras <leo.bras@xxxxxxx> 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.

	M.

-- 
Without deviation from the norm, progress is not possible.