Re: [RFC PATCH 1/5] KVM: arm64: pgtables: Change write bit from S2AP_W to DBM
From: Tian Zheng
Date: Fri Sep 18 2026 - 08:06:03 EST
On 9/16/2026 9:25 PM, Leonardo Bras wrote:
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@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.
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
Hi Leo, Marc,
On the NV side, KVM itself already limits the L1-visible
ID_AA64MMFR1_EL1.HAFDBS to AF-only in limit_nv_id_reg(), so bit 51 has
no architectural meaning to L1, and an L1 using it as software metadata
would have its read-only pages misread as writable by the union read in
walk_nested_s2_pgd() — the memory corruption Marc described. Since NV
has no HAFDBS emulation today, I'd suggest we simply revert this hunk
for now and keep reading writability from S2AP[1] alone:
```
out->writable = desc & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
```
It seems we can address the NV side later, once nested HAFDBS is
actually supported.
That's what I did when picking the series up for HDBSS v5: patch 1 drops
the nested.c hunk.
Thanks,
Tian