Re: [PATCH v3 13/21] KVM: selftests: Use KVM's reported TSC KHz as L0's frequency (sanity checked)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] kselftest: random test flakiness due to strict 1% tolerance on sleep(1)
- [Low] kselftest: masked ioctl failure check due to u64 cast and multiplication
--

commit 44c3625d55f5d8d3232ad8e1eb92f5cad1eaa9ab
Author: Sean Christopherson <seanjc@google.com>

KVM: selftests: Use KVM's reported TSC KHz as L0's frequency (sanity checked)

This commit updates the nested TSC scaling test to use KVM's reported
default TSC frequency instead of trying to calibrate it in userspace via 
RDTSC. It also introduces a sanity check to ensure the observed TSC 
frequency roughly matches KVM's reported frequency.

> diff --git a/tools/testing/selftests/kvm/x86/nested_tsc_scaling_test.c b/tools/testing/selftests/kvm/x86/nested_tsc_scaling_test.c
> index d790ab36a198e..a78585af0ffdf 100644
> --- a/tools/testing/selftests/kvm/x86/nested_tsc_scaling_test.c
> +++ b/tools/testing/selftests/kvm/x86/nested_tsc_scaling_test.c

[ ... ]

> @@ -212,18 +209,30 @@ static void test_tsc_scaling(u64 l0_tsc_freq, u64 l1_scale_factor, u64 l2_scale_
>  int main(int argc, char *argv[])
>  {
>  	u64 l0_tsc_freq, tsc_start, tsc_end, l1_scale, l2_scale;
> +	struct kvm_vm *vm;
>  
>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX) ||
>  		     kvm_cpu_has(X86_FEATURE_SVM));
>  	TEST_REQUIRE(kvm_has_cap(KVM_CAP_TSC_CONTROL));
>  	TEST_REQUIRE(sys_clocksource_is_based_on_tsc());
>  
> +	/*
> +	 * Create a dummy VM to get KVM's default TSC frequency.  All CPUs that
> +	 * support TSC scaling should have a constant TSC, i.e. there's no need
> +	 * to calibrate the "real" TSC.  But do sanity check that the observed
> +	 * TSC is within range of KVM's reported TSC frequency.
> +	 */
> +	vm = vm_create_barebones();
> +	l0_tsc_freq = (u64)__vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL) * 1000;
> +	TEST_ASSERT(l0_tsc_freq, "vcpu ioctl KVM_GET_TSC_KHZ failed");

[Severity: Low]
Does this properly catch an ioctl failure? If __vm_ioctl() fails and returns 
-1, casting it to a u64 and multiplying by 1000 results in a very large 
non-zero value (0xfffffffffffffc18). 

Since TEST_ASSERT only checks if l0_tsc_freq is non-zero, won't this pass 
using garbage data instead of failing the test?

> +	kvm_vm_free(vm);
> +
> +	printf("L0 TSC frequency is: %lu\n", l0_tsc_freq);
>  
>  	tsc_start = rdtsc();
>  	sleep(1);
>  	tsc_end = rdtsc();
> -
> -	l0_tsc_freq = tsc_end - tsc_start;
> -	printf("real TSC frequency is around: %lu\n", l0_tsc_freq);
> +	host_check_tsc_freq(0, tsc_end - tsc_start, l0_tsc_freq);

[Severity: Medium]
Could this strict 1% tolerance on sleep(1) introduce random test flakiness 
on loaded systems?

When comparing the constant hardware frequency (l0_tsc_freq) against the 
cycle delta measured across a userspace sleep(1), scheduling delays could 
easily cause sleep to exceed 1.01 seconds. 

If the test environment is heavily loaded, won't a 10ms scheduling delay 
guarantee that the measured cycles exceed the expected constant cycles by 
more than 1%, failing the host_check_tsc_freq() assertion? 

The guest checks in test_tsc_scaling() that call host_check_tsc_freq() 
also appear to be subject to this same issue since l1_tsc_freq is measured 
using the guest's sleep(1).

>  
>  	/* Scale L1 "down" and L2 "up" at a random factor from 2 to 10. */
>  	l1_scale = (kvm_random_u32(&kvm_rng) % 9) + 2;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930173635.3362655-1-seanjc@google.com?part=13