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

On Wed, Jul 29, 2026 at 11:14:51AM +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

It's included in proper order

> 
> > +					    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()?
> 

It's a package, you have to provide both. 
If not, provide a return -ENXIO inlined function such as above.


> 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?

Yes, since it skips everything else in the function, it's required to do 
the dirty-ring management as well.

Okay, will keep in mind that this file has the comments about this and log 
being NULL from before.

Leo