Re: [PATCH v4 3/6] KVM: arm64: Add auto DBM support for hardware dirty tracking

[Tian Zheng <zhengtian10@huawei.com>]

On 7/16/2026 3:39 PM, Oliver Upton wrote:
> Hi Tian,
>
> On Thu, Jul 09, 2026 at 06:40:23PM +0800, Tian Zheng wrote:
>> -	if (prot & KVM_PGTABLE_PROT_W)
>> +	if (prot & KVM_PGTABLE_PROT_W) {
>>   		set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
>>
>> +		/*
>> +		 * No DEVICE filter needed here: relax_perms is only called
>> +		 * on FSC_PERM faults. Device pages always get full RW from
>> +		 * initial mapping and are never write-protected during
>> +		 * migration, so they never trigger a permission fault.
>> +		 */
>> +		if (pgt->flags & KVM_PGTABLE_S2_DBM)
>> +			set |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;
>> +	} else {
>> +		/*
>> +		 * Clear DBM on W→RO downgrade to prevent hardware from
>> +		 * silently upgrading RO+DBM back to W+dirty, which would
>> +		 * bypass KVM's write tracking and cause data corruption.
>> +		 */
>> +		clr |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;
>> +	}
>> +
> This block makes it pretty evident that the DBM bit really *is* the
> write permission bit. I'd much rather we introduce the concept of dirty
> state to the page table library and migrate the abstract write
> permission to the DBM field, even if we don't have FEAT_HAFDBS.
>
> That way everything 'just works' from outside the page-table library:
> write-protecting hugepages would have the effect of clearing DBM and we
> can separately reap dirty state from page descriptors.
>
> If/when the architecture forces FEAT_S2PIE upon us we will need to make
> this change anyway since dirty state management is unconditional and
> handled separately from the actual permissions.
>
> Thanks,
> Oliver


Hi Oliver,

Thanks again for your insightful review. Following your suggestion, I've

reworked the design around a unified three-state model that works regardless

of whether FEAT_HAFDBS is implemented:

**State table**
State                       | DBM | S2AP[1] | Without HTTU         | 
With HTTU (HAFDBS)
Non-writable   (N)  |  0     |    0         | write -> fault, inject    
     | write -> fault, inject
Writable-clean (C)  |  1     |    0         | write -> fault, sw C->D  
  | write -> hw C->D, no fault, HDBSS logs
Writable-dirty (D)   |  1     |    1         | writable, no fault       
     | writable, no fault

**Proposed changes**
1. Remove KVM_PGTABLE_S2_DBM from enum kvm_pgtable_stage2_flags

— VTCR_EL2.{HD,HDBSS,HA} enablement in kvm_arm_enable_hdbss_global()

already keys off kvm->arch.enable_hdbss / system_supports_hdbss().

2. stage2_set_prot_attr() — set DBM unconditionally on writable pages:
```
if (prot & KVM_PGTABLE_PROT_W) {
     attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
     /* Writable-dirty: DBM=1 conveys write intent, S2AP[1]=1 marks dirty */
     attr |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;
}
```

3. kvm_pgtable_stage2_relax_perms() — drop the else branch entirely:
```
if (prot & KVM_PGTABLE_PROT_W) {
     set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
     /* Non-writable -> Writable-dirty: restore both write intent and 
dirty state */
     set |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;
}
/* no else: callers passing !W (e.g. exec faults) must not touch DBM */
```

4. kvm_pgtable_stage2_wrprotect() — unchanged: it only clears S2AP1 (D->C).

DBM is preserved so HDBSS re-arms next round.
  ```
int kvm_pgtable_stage2_wrprotect(struct kvm_pgtable *pgt, u64 addr, u64 
size)
{
    /* Writable-dirty -> Writable-clean: clear dirty state (S2AP_W),
    * preserve write intent (DBM) so HDBSS re-arms for next write.
     */
     return stage2_update_leaf_attrs(pgt, addr, size, 0,
                          KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W,
                          NULL, NULL,
                          KVM_PGTABLE_WALK_IGNORE_EAGAIN);
}
```

**One clarification**
In the three-state model above, wrprotect() clears S2AP[1] but preserves 
DBM (D->C).

This allows HDBSS to re-arm on the next write. If we instead cleared DBM 
as well (->N),

HDBSS would be permanently disabled on that page and we'd lose the 
benefit of hardware

dirty tracking.


So my understanding is:

wrprotect() (dirty tracking): D->C — clears S2AP[1], preserves DBM

mkreadonly() (true RO, future): ->N — clears both S2AP[1] and DBM

Does this match what you had in mind?

Looking forward to your thoughts.

Thanks,
Tian