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

[sashiko-bot@kernel.org]

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 <leo.bras@arm.com>

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?

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.

> +		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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729104548.3439958-1-leo.bras@arm.com?part=6