Re: [PATCH v1 2/3] KVM: selftests: Check dirty-ring size before enabling

[Leonardo Bras <leo.bras@arm.com>]

On Thu, Oct 01, 2026 at 03:46:51PM +0100, Leonardo Bras wrote:
> On Wed, Sep 30, 2026 at 01:31:25PM -0700, Sean Christopherson wrote:
> > On Tue, Sep 29, 2026, Leonardo Bras wrote:
> > > As of today, trying to enable dirty-ring with a size bigger than the
> > > maximum will return an "argument list too long" error.
> > > 
> > > Change vm_enable_dirty_ring() to get the maximum size, then compare it to
> > > the desired size before enabling. If the value is invalid, print a more
> > > precise error message.
> > > 
> > > Signed-off-by: Leonardo Bras <leo.bras@arm.com>
> > > ---
> > >  tools/testing/selftests/kvm/lib/kvm_util.c | 21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > > index 9ddc047d5c27..15a671b71553 100644
> > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > > @@ -168,24 +168,37 @@ unsigned int kvm_check_cap(long cap)
> > >  	ret = __kvm_ioctl(kvm_fd, KVM_CHECK_EXTENSION, (void *)cap);
> > >  	TEST_ASSERT(ret >= 0, KVM_IOCTL_ERROR(KVM_CHECK_EXTENSION, ret));
> > >  
> > >  	kvm_free_fd(kvm_fd);
> > >  
> > >  	return (unsigned int)ret;
> > >  }
> > >  
> > >  void vm_enable_dirty_ring(struct kvm_vm *vm, u32 ring_size)
> > >  {
> > > -	if (vm_check_cap(vm, KVM_CAP_DIRTY_LOG_RING_ACQ_REL))
> > > -		vm_enable_cap(vm, KVM_CAP_DIRTY_LOG_RING_ACQ_REL, ring_size);
> > > -	else
> > > -		vm_enable_cap(vm, KVM_CAP_DIRTY_LOG_RING, ring_size);
> > > +	long cap = KVM_CAP_DIRTY_LOG_RING_ACQ_REL;
> > > +	int max_size = vm_check_cap(vm, cap);
> > > +
> > > +	if (!max_size) {
> > > +		cap = KVM_CAP_DIRTY_LOG_RING;
> > > +		max_size = vm_check_cap(vm, cap);
> > > +	}
> > > +
> > > +	TEST_ASSERT(max_size > 0,
> > 
> > Rather than open code this, which is kinda sorta going to show up in multiple
> > places, what if we do this as a prep patch?  Then vm_enable_dirty_ring() can use
> > kvm_get_dirty_ring_cap() (completely untested).
> 
> Sure, if you think it's useful :)

Oh, vm_check_cap() is different than kvm_has_cap()/kvm_check_cap():
IIUC, the vm* version will check if the extension is enabled in the current 
VM, while the kvm* version will open a new /dev/kvm fd and check the 
extension there, probably meaning the CAP is available in the system.

So, maybe we would need a slightly different approach?
I.E. have the kvm_get_dirty_ring_cap() to use the VM version, and 
have dirty_ring_supported() to start the new /dev/kvm fd and free it later?

What do you think?

Thanks!
Leo 




> 
> > 
> > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> > index af5eb0334a74..558a71d631da 100644
> > --- a/tools/testing/selftests/kvm/dirty_log_test.c
> > +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> > @@ -291,8 +291,7 @@ static void default_after_vcpu_run(struct kvm_vcpu *vcpu)
> >  
> >  static bool dirty_ring_supported(void)
> >  {
> > -	return (kvm_has_cap(KVM_CAP_DIRTY_LOG_RING) ||
> > -		kvm_has_cap(KVM_CAP_DIRTY_LOG_RING_ACQ_REL));
> > +	return kvm_get_dirty_ring_cap();
> >  }
> >  
> >  static void dirty_ring_create_vm_done(struct kvm_vm *vm)
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index e3b122719262..c08d1581221e 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -339,6 +339,17 @@ static inline bool kvm_has_cap(long cap)
> >  	return kvm_check_cap(cap);
> >  }
> >  
> > +static inline long kvm_get_dirty_ring_cap(void)
> > +{
> > +	if (kvm_has_cap(KVM_CAP_DIRTY_LOG_RING_ACQ_REL))
> > +		return KVM_CAP_DIRTY_LOG_RING_ACQ_REL;
> > +
> > +	if (kvm_has_cap(KVM_CAP_DIRTY_LOG_RING))
> > +		return KVM_CAP_DIRTY_LOG_RING;
> > +
> > +	return 0;	
> > +}
> > +
> >  /*
> >   * Use the "inner", double-underscore macro when reporting errors from within
> >   * other macros so that the name of ioctl() and not its literal numeric value
> > 
> > 
> > > "Dirty-ring not supported in this kernel\n");
> > 
> > "this kernel" could be misleading, some architectures simply don't support the
> > dirty ring.
> 
> So, do you suggest keeping it simple by printing "Dirty-ring not supported"?
> 
> 
> > 
> > > +	TEST_ASSERT(ring_size <= max_size && is_power_of_2(ring_size) &&
> > > +		    ring_size >= getpagesize(),
> > > +		    "Invalid dirty-ring size: Should be a power of two "
> > > +		    "between %lu and %lu entries\n",
> > 
> > Don't wrap strings.  The "Invalid dirty-ring size:" part is redudant with the
> > expressions, just drop that to shorten things.  This should also spit out the
> > requested ring_size.  Stating the range as a number of entries is also confusing;
> > as a debugger, I don't want to have to go look at the size of kvm_dirty_gfn to
> > understand why ring_size is invalid.
> > 
> > And maybe split up the asserts?  If the goal is to make it easier for developers
> > to know when they messed up, might as well make it as easy as possible.  E.g.
> > leaning on the above diff, something like this?
> > 
> > 	long cap = kvm_get_dirty_ring_cap();
> > 
> > 	TEST_ASSERT(cap, "Dirty-ring not supported");
> > 
> > 	TEST_ASSERT(is_power_of_2(ring_size),
> > 		    "Dirty-ring size '0x%x' must be a power-of-2", ring_size);
> > 	TEST_ASSERT(ring_size >= getpagesize(),
> > 		    "Dirty-ring size '0x%x' must be at least one (host) page", ring_size);
> > 	TEST_ASSERT(ring_size >= vm_check_cap(vm, cap),
> > 		    "Dirty-ring size '0x%x' is larger than KVM's limit of '0x%x'",
> > 		    ring_size, vm_check_cap(vm, cap));
> > 
> > 	vm_enable_cap(vm, cap, ring_size);
> > 	vm->dirty_ring_size = ring_size;
> > 
> 
> Humm, I see the point. 
> Will do as suggested then, thanks!
> 
> Leo