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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Format string type mismatch causing undefined behavior on 64-bit big-endian architectures
- [Low] Misleading error message contradicts inclusive boundary check
- [Low] Misleading assertion failure when dirty ring capabilities are unsupported
--

commit 8555e295b2631adeaf18b3b867988e6adc58eb4c
Author: Leonardo Bras <leo.bras@arm.com>

KVM: selftests: Check dirty-ring size before enabling

This commit changes vm_enable_dirty_ring() to dynamically fetch the maximum
dirty-ring size via KVM_CAP_DIRTY_LOG_RING_ACQ_REL or KVM_CAP_DIRTY_LOG_RING.
It then validates that the requested ring size is less than or equal to this
maximum and is a power of two, printing a specific error message if the check
fails.

> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c

[ ... ]

>  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);
> +	}
> +

[Severity: Low]
If the host KVM subsystem does not support these capabilities, max_size
will evaluate to 0.

Since the assertion below will then fail for any positive ring_size, does
this create a misleading error message about "smaller than 0 entries"
rather than indicating the feature is unsupported?

> +	TEST_ASSERT(ring_size <= max_size && is_power_of_2(ring_size),
> +		    "Invalid dirty-ring size: Should be a power of two "
> +		    "smaller than %d entries\n",

[Severity: Low]
The condition checks if ring_size is less than or equal to max_size, but the
error message text specifies it must be "smaller than".

Does this mismatch make it confusing to debug failures when the requested size
is exactly equal to the maximum size?

> +		    max_size / sizeof(struct kvm_dirty_gfn));

[Severity: Low]
The expression max_size / sizeof(struct kvm_dirty_gfn) evaluates to a size_t,
but the format string uses %d.

Can this cause format warnings, and result in incorrectly printing 0 on
64-bit big-endian architectures since the format parsing will read the most
significant 32 bits?

> +
> +	vm_enable_cap(vm, cap, ring_size);
>  	vm->dirty_ring_size = ring_size;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624171656.1737580-1-leo.bras@arm.com?part=2