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

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

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 :)

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