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

[Sean Christopherson <seanjc@google.com>]

On Wed, Sep 30, 2026, sashiko-bot@kernel.org wrote:
> 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?

Eh, yes and no.  KVM_GET_TSC_KHZ existed long before TSC scaling, and while KVM
doesn't sanity check tsc_khz, i.e. KVM_GET_TSC_KHZ could return a "legitimate"
negative frequency (when those ~2.17PHz CPUs come along), in practice checking
for a non-zero value is sufficient because any bogus value will quickly fail the
functional checks.

Heh, actually, kvm_arch_vm_post_create() *already* validates KVM_GET_TSC_KHZ:

	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
	TEST_ASSERT(r > 0, "KVM_GET_TSC_KHZ did not provide a valid TSC frequency.");
	guest_tsc_khz = r;

so this is a non-issue.  I agree it's not the prettiest, I just don't care enough
to clean it up.

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

Yes, but that's pre-existing, and no one has complained, yet...