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 02:56:38PM -0700, Sean Christopherson wrote:
> On Thu, Oct 01, 2026, Leonardo Bras wrote:
> > 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:
> > > > >  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,
> 
> No, the VM-scoped version checks if the VM *can* support the capability in its
> current configuration, e.g. some capabilities are fundamentally incompatible with
> certain VM types.
> 
> > 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?
> 
> It's not necessary in this case.  All KVM_CHECK_EXTENSION calls feed into
> kvm_vm_ioctl_check_extension_generic(), the only difference is that the @kvm
> pointer will be NULL when called on /dev/kvm.  While some capabilities do behave
> differently if @kvm is valid, the majority do not, including the dirty ring ones:
> 
> 	case KVM_CAP_DIRTY_LOG_RING:
> #ifdef CONFIG_HAVE_KVM_DIRTY_RING_TSO
> 		return KVM_DIRTY_RING_MAX_ENTRIES * sizeof(struct kvm_dirty_gfn);
> #else
> 		return 0;
> #endif
> 	case KVM_CAP_DIRTY_LOG_RING_ACQ_REL:
> #ifdef CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL
> 		return KVM_DIRTY_RING_MAX_ENTRIES * sizeof(struct kvm_dirty_gfn);
> #else
> 		return 0;
> #endif
> 
> I.e. the VM version and /dev/kvm version will always yield the same result.  Very
> theoretically that could change, but it's unlikely.  And we can always update
> KVM selftests in the unlikely case a specific VM type comes along that doesn't
> support the dirty ring even when the broader architectures does.

All right then, thanks!
Leo