Re: [RFC PATCH 1/5] KVM: arm64: pgtables: Change write bit from S2AP_W to DBM
From: Leonardo Bras
Date: Wed Sep 16 2026 - 09:11:58 EST
On Wed, Sep 16, 2026 at 09:30:35AM +0100, Marc Zyngier wrote:
> On Tue, 15 Sep 2026 18:12:45 +0100,
> Leonardo Bras <leo.bras@xxxxxxx> wrote:
> >
> > On Sun, Sep 13, 2026 at 10:00:40AM +0100, Marc Zyngier wrote:
> > > On Tue, 01 Sep 2026 18:15:52 +0100,
> > > Leonardo Bras <leo.bras@xxxxxxx> wrote:
> > > >
> > > > As a first step of changing the encoding for the Stage2 PTE descriptor,
> > > > introduce the DBM bit, and adapt every usage of writable to use the DBM bit
> > > > (51) instead of S2AP[1]/Dirty bit (7).
> > > >
> > > > For this step, we convert usages of RW(Dirty) -> WD(DBM|Dirty).
> > > >
> > > > Signed-off-by: Leonardo Bras <leo.bras@xxxxxxx>
> > > > ---
> > > > arch/arm64/include/asm/kvm_pgtable.h | 3 +++
> > > > arch/arm64/kvm/hyp/pgtable.c | 8 +++++---
> > > > arch/arm64/kvm/nested.c | 4 +++-
> > > > arch/arm64/kvm/ptdump.c | 4 ++--
> > > > 4 files changed, 13 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > > > index 41a8687938eb..37baa86d6fd8 100644
> > > > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > > > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > > > @@ -86,24 +86,27 @@ typedef u64 kvm_pte_t;
> > > > #define KVM_PTE_LEAF_ATTR_HI GENMASK(63, 50)
> > > >
> > > > #define KVM_PTE_LEAF_ATTR_HI_SW GENMASK(58, 55)
> > > >
> > > > #define KVM_PTE_LEAF_ATTR_HI_S1_XN BIT(54)
> > > > #define KVM_PTE_LEAF_ATTR_HI_S1_UXN BIT(54)
> > > > #define KVM_PTE_LEAF_ATTR_HI_S1_PXN BIT(53)
> > > >
> > > > #define KVM_PTE_LEAF_ATTR_HI_S2_XN GENMASK(54, 53)
> > > >
> > > > +#define KVM_PTE_LEAF_ATTR_HI_S2_DBM BIT(51)
> > > > +
> > > > #define KVM_PTE_LEAF_ATTR_HI_S1_GP BIT(50)
> > > >
> > > > #define KVM_PTE_LEAF_ATTR_S2_PERMS (KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R | \
> > > > KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W | \
> > > > + KVM_PTE_LEAF_ATTR_HI_S2_DBM | \
> > > > KVM_PTE_LEAF_ATTR_HI_S2_XN)
> > > >
> > > > /* pKVM invalid pte encodings */
> > > > #define KVM_INVALID_PTE_TYPE_MASK GENMASK(63, 60)
> > > > #define KVM_INVALID_PTE_ANNOT_MASK ~(KVM_PTE_VALID | \
> > > > KVM_INVALID_PTE_TYPE_MASK)
> > > >
> > > > enum kvm_invalid_pte_type {
> > > > /*
> > > > * Used to indicate a pte for which a 'break-before-make'
> > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > > > index b74dd5ce1efd..ca49f1bd7c34 100644
> > > > --- a/arch/arm64/kvm/hyp/pgtable.c
> > > > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > > > @@ -725,42 +725,43 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p
> > > > }
> > > >
> > > > r = stage2_set_xn_attr(prot, &attr);
> > > > if (r)
> > > > return r;
> > > >
> > > > if (prot & KVM_PGTABLE_PROT_R)
> > > > attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
> > > >
> > > > if (prot & KVM_PGTABLE_PROT_W)
> > > > - attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > > > + attr |= KVM_PTE_LEAF_ATTR_HI_S2_DBM | KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > >
> > > What makes it acceptable to always set DBM? This is an optional
> > > feature, and I'm not exactly comfortable setting bits that are
> > > supposed to be RES0.
> >
> > The alternative would be to introduce WC/WD as an alternative encoding
> > infrastructure, and keep making decisions based on having HAFDBS available
> > or in the system. My intention was to adapt things in a way to avoid
> > multiple behaviors if we could avoid.
> >
> > My rationale is, based on our possibility of setting DBM=1:
> > If HAFDBS/HDBSS/HACDBS are not available, or available but disabled,
> > then the DBM bit is just ignored:
> > RO -> RO
> > WC -> RO
> > WD -> RW
> >
> > (for reference:
> > - Read-Only (RO): DBM=0, S2AP[1]=0
> > - Writable-Clean (WC): DBM=1, S2AP[1]=0
> > - Writable-Dirty (WD): DBM=1, S2AP[1]=1
> > )
> >
> > And the behavior should just the same as what we have today.
> >
> >
> > > 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.
> >
> > > > +
> > > >
> > >
> > > Spurious newline.
> > >
> >
> > Oh, ok, will get rid of it.
> >
> > > > if (!kvm_lpa2_is_enabled())
> > > > attr |= FIELD_PREP(KVM_PTE_LEAF_ATTR_LO_S2_SH, sh);
> > > >
> > > > attr |= KVM_PTE_LEAF_ATTR_LO_S2_AF;
> > > > attr |= prot & KVM_PTE_LEAF_ATTR_HI_SW;
> > > > *ptep = attr;
> > > >
> > > > return 0;
> > > > }
> > >
> > > I don't know how you have configured git on your end, but there is *a
> > > lot* of context...
> > >
> >
> > diff.context=10, i.e. 10 lines before and after a change.
> > To me it looks helpful to get more context around the change.
> >
> > I can change it back to 5 if it bothers you.
> >
> > > >
> > > > enum kvm_pgtable_prot kvm_pgtable_stage2_pte_prot(kvm_pte_t pte)
> > > > {
> > > > enum kvm_pgtable_prot prot = pte & KVM_PTE_LEAF_ATTR_HI_SW;
> > > >
> > > > if (!kvm_pte_valid(pte))
> > > > return prot;
> > > >
> > > > if (pte & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R)
> > > > prot |= KVM_PGTABLE_PROT_R;
> > > > - if (pte & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W)
> > > > + if (pte & KVM_PTE_LEAF_ATTR_HI_S2_DBM)
> > > > prot |= KVM_PGTABLE_PROT_W;
> > > >
> > > > switch (FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, pte)) {
> > > > case 0b00:
> > > > prot |= KVM_PGTABLE_PROT_PX | KVM_PGTABLE_PROT_UX;
> > > > break;
> > > > case 0b01:
> > > > prot |= KVM_PGTABLE_PROT_UX;
> > > > break;
> > > > case 0b11:
> > > > @@ -1281,20 +1282,21 @@ static int stage2_update_leaf_attrs(struct kvm_pgtable *pgt, u64 addr,
> > > > *orig_pte = data.pte;
> > > >
> > > > if (level)
> > > > *level = data.level;
> > > > return 0;
> > > > }
> > > >
> > > > int kvm_pgtable_stage2_wrprotect(struct kvm_pgtable *pgt, u64 addr, u64 size)
> > > > {
> > > > return stage2_update_leaf_attrs(pgt, addr, size, 0,
> > > > + KVM_PTE_LEAF_ATTR_HI_S2_DBM |
> > > > KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W,
> > > > NULL, NULL,
> > > > KVM_PGTABLE_WALK_IGNORE_EAGAIN);
> > > > }
> > > >
> > > > void kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr,
> > > > enum kvm_pgtable_walk_flags flags)
> > > > {
> > > > int ret;
> > > >
> > > > @@ -1361,21 +1363,21 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
> > > > s8 level;
> > > > int ret;
> > > >
> > > > if (prot & KVM_PTE_LEAF_ATTR_HI_SW)
> > > > return -EINVAL;
> > > >
> > > > if (prot & KVM_PGTABLE_PROT_R)
> > > > set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
> > > >
> > > > if (prot & KVM_PGTABLE_PROT_W)
> > > > - set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > > > + set |= KVM_PTE_LEAF_ATTR_HI_S2_DBM | KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > > >
> > > > if (prot & KVM_PGTABLE_PROT_X) {
> > > > ret = stage2_set_xn_attr(prot, &xn);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > set |= xn & KVM_PTE_LEAF_ATTR_HI_S2_XN;
> > > > clr |= ~xn & KVM_PTE_LEAF_ATTR_HI_S2_XN;
> > > > }
> > > >
> > > > 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?
> >
> > All I can think is something like a buggy HW implementation setting DBM in
> > a HAFDBS-disabled machine, which has been unnoticed up to now, and now
> > could cause problems.
>
> And pray tell, what is it to have HAFDBS being disabled at S2? Instead
> of throwing the book at me, you should probably read it, and specially
> the effects of VTCR_EL2.HD.
Sorry, I did not mean to "trow a book" at you here, I was just trying to
figure out what was wrong, and given I was not able to do it by myself I
thought it was a good idea to ask you.
I just did not want to do that in an effortless way, so I gathered the
stuff that I previously thougt to understand, and brought them to the
discussion so it could be easier to point what I got wrong.
As for your question, the effects of VTCR_EL2.HD is to actually mark the
page as dirty (S2AP=1) when an writable-clean page (DBM=1, S2AP=0) gets
writen to. DBM seems to act as permission to let the hardware change the
S2AP bit when HAFDBS available & enabled.
So if we want to use DBM as writable, we have to make sure we have
VTCR_EL2.HD=0, or we are indeed intending to use HAFDBS here.
Not sure if I get your point here.
Thanks!
Leo