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

[sashiko-bot@kernel.org]

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

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?

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.

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?

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/20260729104548.3439958-1-leo.bras@arm.com?part=10