Re: [PATCH v3 09/11] kvm/dirty_ring: Add arch-generic interface for hw-accelerated dirty-ring cleaning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] The monolithic `#ifndef CONFIG_HAVE_KVM_HW_DIRTY_BIT` guard causes a build failure for architectures that implement only one of the hardware-accelerated dirty logging hooks.
- [Low] The undocumented API contract of `kvm_arch_dirty_ring_clear` silently shifts the responsibility of updating KVM dirty ring state to the architecture hook.
- [Low] Missing forward declaration for `struct kvm_dirty_ring` in `include/linux/kvm_dirty_bit.h`.
--

--- Patch [9]: [PATCH v3 09/11] kvm/dirty_ring: Add arch-generic interface for hw-accelerated dirty-ring cleaning ---
commit 54695fde1ebfc7f27e678570e500f9d9eb6ecff5
Author: Leonardo Bras <leo.bras@arm.com>

kvm/dirty_ring: Add arch-generic interface for hw-accelerated dirty-ring cleaning

Introduce kvm_arch_dirty_ring_clear() to allow architecture-specific
hardware-accelerated dirty-ring routines. The call is added to
kvm_dirty_ring_reset() and falls back to a software version if not
implemented or if an error is detected.

> diff --git a/include/linux/kvm_dirty_bit.h b/include/linux/kvm_dirty_bit.h
> index fa4f6b67b6235..8492979d694e7 100644
> --- a/include/linux/kvm_dirty_bit.h
> +++ b/include/linux/kvm_dirty_bit.h
> @@ -18,6 +18,13 @@ static inline int kvm_arch_dirty_log_clear(struct kvm *kvm,
>  	return -ENXIO;
>  }
>  
> +static inline int kvm_arch_dirty_ring_clear(struct kvm *kvm,
> +					    struct kvm_dirty_ring *ring,

[Severity: Low]
Does this struct need a forward declaration?

If this header is included before include/linux/kvm_dirty_ring.h, might this
cause a compiler warning for struct kvm_dirty_ring being declared inside a
parameter list?

> +					    int *nr_entries_reset)
> +{
> +	return -ENXIO;
> +}
> +
>  #else /* CONFIG_HAVE_KVM_HW_DIRTY_BIT */

[Severity: Low]
Could this monolithic config guard cause a build failure if an architecture
enables CONFIG_HAVE_KVM_HW_DIRTY_BIT to provide kvm_arch_dirty_log_clear(),
but does not implement kvm_arch_dirty_ring_clear()?

Because the fallback for kvm_arch_dirty_ring_clear() is excluded when the
config is enabled, omitting it in the arch-specific header would leave it
undeclared.

>  
>  #include <asm/kvm_dirty_bit.h>
> diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
> index 807c45fb1fc7b..3fa62894f3d24 100644
> --- a/virt/kvm/dirty_ring.c
> +++ b/virt/kvm/dirty_ring.c

[ ... ]

> @@ -133,6 +134,9 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring,
>  	 */
>  	lockdep_assert_held(&kvm->slots_lock);
>  
> +	if (kvm_arch_dirty_ring_clear(kvm, ring, nr_entries_reset) >= 0)
> +		goto out;
> +

[Severity: Low]
Does this bypass implicitly require the arch-specific hook to manually update
the internal state of the dirty ring?

If an architecture implements kvm_arch_dirty_ring_clear() and returns success,
this goto skips the generic software loop. The arch hook would need to
manually increment ring->reset_index, update *nr_entries_reset, and call
kvm_dirty_gfn_set_invalid() for each processed entry to avoid corrupting the
ring state. Is this API contract expected to be documented here?

>  	while (likely((*nr_entries_reset) < INT_MAX)) {
>  		if (signal_pending(current))
>  			return -EINTR;

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