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

[Tian Zheng <zhengtian10@huawei.com>]

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

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