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

[Oliver Upton <oupton@kernel.org>]

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?

> > > 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.

If the guest hypervisor sets VTCR_EL2.HD=0, the expectation is that the
shadow stage-2 MMU treats the corresponding bit in the PTE as RES0.

Thanks,
Oliver