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

On Wed, Jul 29, 2026 at 11:21:24AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Critical] Hardware-accelerated dirty ring clear directly accesses EL2 system registers from the host kernel, causing fatal unhandled exceptions on nVHE and pKVM systems.
> - [High] The `hw_entries` buffer is allocated using kmalloc, which does not guarantee the page alignment strictly required by the `SYS_HACDBSBR_EL2` hardware register.
> 
> Pre-existing issues:
> - [High] Writes to control-plane system registers are batched without an intervening `isb()`, violating ARM64 architectural synchronization requirements.
> --
> 
> --- Patch [10]: [PATCH v3 10/11] KVM: arm64: Add hardware-accelerated dirty-ring cleaning routine ---
> commit f9fc2a3f2ff348143923dea638ecd996e369580d
> Author: Leonardo Bras <[email protected]>
> 
> KVM: arm64: Add hardware-accelerated dirty-ring cleaning routine
>     
> Implement arm64 version of kvm_arch_dirty_ring_clear() making use of
> FEAT_HACDBS.
>     
> It works by transversing the dirty-ring and converting its entries into
> HDBSS entries based on the slot offset.
>     
> The resulting HDBSS array is then fed to the HACDBS mechanism that walks
> the pagetable marking writable-dirty pages as writable-clean.
> 
> > diff --git a/arch/arm64/include/asm/kvm_dirty_bit.h b/arch/arm64/include/asm/kvm_dirty_bit.h
> > index d368a23a84b30..f785a7aad9e88 100644
> > --- a/arch/arm64/include/asm/kvm_dirty_bit.h
> > +++ b/arch/arm64/include/asm/kvm_dirty_bit.h
> > @@ -15,6 +15,9 @@ int __kvm_arch_dirty_log_clear(struct kvm *kvm,
> >  			       unsigned long *bitmap,
> >  			       bool *flush);
> >  
> > +int __kvm_arch_dirty_ring_clear(struct kvm *kvm, struct kvm_dirty_ring *ring,
> > +				int *nr_entries_reset);
> > +
> >  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 gating function also need to verify the VHE execution mode?

it's already done before setting KVM_PGTABLE_S2_DBM.

> 
> By checking only system_supports_hacdbs(), it appears this permits the
> accelerated path to be invoked on nVHE and pKVM host systems.
> 
> Since dirty_bit_clear() performs direct accesses to EL2 system registers
> from the host kernel:
> 
> arch/arm64/kvm/dirty_bit.c:dirty_bit_clear() {
>     ...
>     __load_stage2(&kvm->arch.mmu);
>     sysreg_clear_set_hcr(0, HCR_EL2_VM);
>     ...
> }
> 
> Wouldn't this trigger an unhandled hypervisor trap or undefined instruction
> exception when triggered by userspace on nVHE systems?
> 
> [ ... ]
> > diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> > index d7e624f9b6308..ca9cfefe0893b 100644
> > --- a/arch/arm64/kvm/dirty_bit.c
> > +++ b/arch/arm64/kvm/dirty_bit.c
> [ ... ]
> > @@ -359,3 +359,80 @@ int __kvm_arch_dirty_log_clear(struct kvm *kvm,
> >  
> >  	return ret;
> >  }
> > +
> > +int __kvm_arch_dirty_ring_clear(struct kvm *kvm, struct kvm_dirty_ring *ring,
> > +				int *nr_entries_reset)
> > +{
> > +	u64 *hw_entries __free(kfree) = NULL;
> > +	u64 ttwl;
> > +	s64 cur_slot = S64_MAX;
> > +	int i, ret;
> > +	struct kvm_memory_slot *memslot;
> > +
> > +	if (signal_pending(current))
> > +		return -EINTR;
> > +
> > +	ttwl = HDBSS_ENTRY_TTWL(KVM_PGTABLE_LAST_LEVEL);
> > +
> > +	hw_entries = kmalloc(max(ring->size * sizeof(u64), PAGE_SIZE), GFP_KERNEL);
> 
> [Severity: High]
> Could this allocation cause the hardware accelerator to read from the wrong
> memory address?
> 
> When debugging features like SLUB debugging are enabled, kmalloc allocations
> are not strictly page-aligned because of prepended metadata.
> 

Argh, I was not aware of this.
Will change it to use recommended allocators instead of kmalloc.


> The SYS_HACDBSBR_EL2 hardware register requires a page-aligned physical
> base address. When hacdbs_start() masks out the offset using
> HACDBSBR_EL2_BADDR_MASK, an unaligned base address gets truncated.
> 
> Will this result in the hardware reading from the start of the physical
> page instead of the actual hw_entries array? It might be necessary to use
> alloc_pages_exact() or __get_free_pages() here to guarantee alignment.
> 
> > +	if (!hw_entries)
> > +		return -ENOMEM;
> > +
> [ ... ]
> > +	if (i == 0)
> > +		return 0;
> > +
> > +	write_lock(&kvm->mmu_lock);
> > +	ret = dirty_bit_clear(kvm, hw_entries, i);
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible that the hardware routines
> called here violate architectural synchronization requirements?
> 

Already discussed before.

> Looking at the underlying functions, writes to control-plane system
> registers seem to be batched without an intervening isb() as the very next
> instruction.
> 
> For example, in hacdbs_start():
> 
> arch/arm64/kvm/dirty_bit.c:hacdbs_start()
>     ...
>     sysreg_clear_set_s(SYS_HACDBSCONS_EL2,
>                        HACDBSCONS_EL2_ERR_REASON | HACDBSCONS_EL2_INDEX, 0);
>                        
>     br = (virt_to_phys(hw_entries) & HACDBSBR_EL2_BADDR_MASK) | ...
>     ...
> 
> And in hacdbs_sched_in():
> 
> arch/arm64/kvm/dirty_bit.c:hacdbs_sched_in()
>     ...
>     write_sysreg_s(save->save.vtcr_el2, SYS_VTCR_EL2);
>     write_sysreg_s(save->save.vttbr_el2, SYS_VTTBR_EL2);
>     isb();
> 
> Could this allow the CPU to execute the intervening instructions in an
> architecturally undefined pipeline state?
> 
> > +	write_unlock(&kvm->mmu_lock);
> > +
> > +	/* Set as invalid all successfully cleaned entries */
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10