Re: [PATCH v3 06/11] KVM: arm64: Add hardware-accelerated dirty-bitmap cleaning routine

On Wed, Jul 29, 2026 at 11:18:41AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] Dropping `mmu_lock` during eager page splitting allows concurrent guest writes to be permanently lost.
> - [Critical] Direct EL2 system register access from EL1 host kernel triggers an UNDEFINED exception on nVHE/pKVM systems.
> - [High] Infinite hardware polling loop with preemption disabled risks hard lockups and renders the preempt notifier dead code.
> --
> 
> --- Patch [6]: [PATCH v3 06/11] KVM: arm64: Add hardware-accelerated dirty-bitmap cleaning routine ---
> commit 67bd2745169abaea5a656577d6b7add50b3e09f1
> Author: Leonardo Bras <[email protected]>
> 
> KVM: arm64: Add hardware-accelerated dirty-bitmap cleaning routine
> 
> Implement arm64 version of kvm_arch_dirty_log_clear() making use of FEAT_HACDBS.
> It converts dirty bitmap entries into HDBSS entries in 64-page blocks and feeds
> them to the HACDBS mechanism to clear dirty states. It also supports hugepage
> splitting.
> 
> > diff --git a/arch/arm64/include/asm/kvm_dirty_bit.h b/arch/arm64/include/asm/kvm_dirty_bit.h
> > index dd16438f06518..d368a23a84b30 100644
> > --- a/arch/arm64/include/asm/kvm_dirty_bit.h
> > +++ b/arch/arm64/include/asm/kvm_dirty_bit.h
> [ ... ]
> > +static inline bool kvm_arch_dirty_clear_enabled(struct kvm *kvm)
> > +{
> > +	return system_supports_hacdbs() &&  kvm->arch.mmu.pgt &&
> > +	       (kvm->arch.mmu.pgt->flags & KVM_PGTABLE_S2_DBM);
> > +}
> 
> [Severity: Critical]
> Does this check need to verify if the kernel is running in VHE mode?

kvm->arch.mmu.pgt->flags & KVM_PGTABLE_S2_DBM will only be true when HDBSS 
is enabled, and that only happens in VHE=1.

> 
> If this executes on an nVHE or pKVM system (where the host runs at EL1), 
> and the hardware supports HACDBS, the ioctl will proceed and call 
> dirty_bit_clear(). That function directly accesses EL2 system registers 
> like SYS_HACDBSCONS_EL2, which will trap or UNDEF when accessed from EL1.
> 
> > diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> > index 33826bbd16d91..5a6266205295e 100644
> > --- a/arch/arm64/kvm/dirty_bit.c
> > +++ b/arch/arm64/kvm/dirty_bit.c
> [ ... ]
> > +int __kvm_arch_dirty_log_clear(struct kvm *kvm,
> > +			       struct kvm_memory_slot *memslot,
> > +			       struct kvm_clear_dirty_log *log,
> > +			       unsigned long *bitmap,
> > +			       bool *flush)
> > +{
> [ ... ]
> > +	write_lock(&kvm->mmu_lock);
> > +
> > +	for (unsigned long i = start; i < end; i++) {
> > +		unsigned long mask;
> > +		gfn_t offset;
> > +		atomic_long_t *p;
> > +
> > +		if (log) { /* Clean only what is in the input bitmap */
> > +			mask = bitmap[i - start];
> > +			if (!mask)
> > +				continue;
> > +
> > +			p = (atomic_long_t *)&dirty_bitmap[i];
> > +			mask &= atomic_long_fetch_andnot(mask, p);
> > +		} else { /* Clean everything, save to output bitmap */
> [ ... ]
> > +		if (kvm_dirty_log_manual_protect_and_init_set(kvm))
> > +			kvm_mmu_split_huge_pages(kvm,
> > +						 gfn_to_gpa(offset + __ffs(mask)),
> > +						 gfn_to_gpa(offset + __fls(mask) + 1));
> 
> [Severity: Critical]
> Could this lead to silent memory corruption due to dropped locks?
> 
> The software dirty bit is cleared above via atomic_long_fetch_andnot(). 
> Then we call kvm_mmu_split_huge_pages(), which drops mmu_lock internally:
> 
> kvm_mmu_split_huge_pages()
>   need_split_memcache_topup_or_resched()
>   write_unlock(&kvm->mmu_lock);
>   cond_resched();
> 
> While the lock is dropped, a vCPU can write to the page without trapping
> because the hardware DBM bit is still set. Once the lock is reacquired, 
> dirty_bit_clear() clears the hardware DBM bit. The write gets lost because
> both software and hardware bits are now clean.

If the new bit is set before the atomic read, it will be cleaned and 
reported as dirty to user. If it's manual cleaning, it will be available 
for cleaning in the next round.

If the new bit is set after the atomic read, it will be there for the next 
iteration, so no problem. 

> 
> > +		do {
> > +			idx = mask_to_hdbss(&mask, hw_entries, offset, ttwl, idx, entries_sz);
> > +			if (idx >= entries_sz) {
> > +				ret = dirty_bit_clear(kvm, hw_entries, idx);
> 
> [Severity: High]
> Is there a risk of a hard lockup here during hardware polling?
> 
> Looking at dirty_bit_clear(), it polls SYS_HACDBSCONS_EL2 in a while (true)
> loop without a timeout fallback. 
> 
> Since __kvm_arch_dirty_log_clear() takes write_lock(&kvm->mmu_lock) before 
> calling dirty_bit_clear(), preemption is disabled on non-RT kernels. This 
> makes the preempt_notifier registered inside dirty_bit_clear() dead code 
> and guarantees the CPU will spin forever if the hardware hangs.

Good catch!
So on non-RT kernels we need no preempt_notifiers.
OTOH, we will need them on RT kernels, so I suppose we need to find a way 
of using them seemlessly.

Thanks!
Leo