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 <[email protected]>
> > > ---
> > > 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
