Re: [PATCH v11 21/23] KVM: selftests: Test that the first user TSC write is honoured exactly

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: selftests: misplaced timing window and missing KVM_SET_TSC_KHZ in main()
- [Low] KVM: selftests: duplicate MSR_IA32_TSC test binary and helpers
--

commit c39adcb632ae1981c065df1ae8906c472090451f
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: selftests: Test that the first user TSC write is honoured exactly

This commit adds an x86 KVM selftest that creates a vCPU, writes a guest TSC
value corresponding to 500ms of runtime via MSR_IA32_TSC, and checks that the
value reads back without being snapped back to the default generation by the
1-second slop heuristic in kvm_synchronize_tsc().

> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index 1edc78845fd35..fd063806a7fb9 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -151,6 +151,7 @@ TEST_GEN_PROGS_x86 += x86/xcr0_cpuid_test
>  TEST_GEN_PROGS_x86 += x86/xss_msr_test
>  TEST_GEN_PROGS_x86 += x86/debug_regs
>  TEST_GEN_PROGS_x86 += x86/tsc_msrs_test
> +TEST_GEN_PROGS_x86 += x86/tsc_user_set_test

[Severity: Low]
This isn't a bug, but tools/testing/selftests/kvm/x86/tsc_msrs_test.c already
tests host and guest reads and writes to MSR_IA32_TSC and MSR_IA32_TSC_ADJUST.
Should this narrow MSR_IA32_TSC host-write check be added as a test case in
x86/tsc_msrs_test.c rather than introducing a separate standalone test binary?

> diff --git a/tools/testing/selftests/kvm/x86/tsc_user_set_test.c b/tools/testing/selftests/kvm/x86/tsc_user_set_test.c
> new file mode 100644
> index 0000000000000..6ab76e817f9fe
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/tsc_user_set_test.c
> @@ -0,0 +1,86 @@

[ ... ]

> +int main(void)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	u64 freq_khz, t_write, t_read, slack_cycles, mono_ns;
> +	struct timespec ts;
> +	int r;
> +
> +	/*
> +	 * Start the measured window before vCPU creation: a snapped-back
> +	 * value tracks time since the kernel's default generation (i.e.
> +	 * since creation), so the window the slack is derived from must
> +	 * include everything from creation onwards.
> +	 */
> +	clock_gettime(CLOCK_MONOTONIC_RAW, &ts);
> +	mono_ns = ts.tv_sec * 1000000000ULL + ts.tv_nsec;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, NULL);
> +
> +	r = __vcpu_ioctl(vcpu, KVM_GET_TSC_KHZ, NULL);
> +	TEST_ASSERT(r > 0, "KVM_GET_TSC_KHZ failed");
> +	freq_khz = r;

[Severity: Low]
This isn't a bug, but vm_create_with_one_vcpu() calls
kvm_arch_vm_post_create(), which already invokes KVM_GET_TSC_KHZ, asserts that
it succeeded, and stores the result in the global guest_tsc_khz. In addition,
test_util.h (included via kvm_util.h) provides timespec_to_ns() and
timespec_sub(). Could main() reuse guest_tsc_khz and timespec_to_ns() instead
of re-querying KVM_GET_TSC_KHZ and open-coding the timespec conversion?

> +
> +	/*
> +	 * The first user write: a guest TSC corresponding to 500ms of
> +	 * runtime. This is within 1s (of virtual cycle time) of the
> +	 * kernel's default generation, so it is exactly the case that
> +	 * the slop heuristic would destroy if it were (incorrectly)
> +	 * armed by the kernel's own internal TSC synchronization.
> +	 */
> +	t_write = freq_khz * 500ULL;		/* khz * ms = cycles */
> +
> +	vcpu_set_msr(vcpu, MSR_IA32_TSC, t_write);
> +
> +	t_read = vcpu_get_msr(vcpu, MSR_IA32_TSC);
> +
> +	clock_gettime(CLOCK_MONOTONIC_RAW, &ts);
> +	mono_ns = (ts.tv_sec * 1000000000ULL + ts.tv_nsec) - mono_ns;
> +
> +	/*
> +	 * The TSC advances between the write and the read; bound the
> +	 * difference by the measured elapsed time around the two ioctls
> +	 * plus 10ms of slop, so a descheduled test cannot flake. If the
> +	 * write was snapped to the kernel's default generation, the
> +	 * value read back is ~500ms low (i.e. below t_write), which is
> +	 * unambiguous.
> +	 */
> +	slack_cycles = freq_khz * (mono_ns / 1000000ULL + 10);
> +
> +	TEST_ASSERT(t_read >= t_write,
> +		    "TSC went backwards from explicit write: wrote %lu, read %lu"
> +		    " (first userspace write was subjected to 1-second slop?)",
> +		    t_write, t_read);
> +	TEST_ASSERT(t_read - t_write < slack_cycles,
> +		    "TSC read back too far from written value: wrote %lu, read %lu",
> +		    t_write, t_read);

[Severity: Medium]
Should the initial clock_gettime(CLOCK_MONOTONIC_RAW, &ts) call in main() be
placed immediately before vcpu_set_msr(vcpu, MSR_IA32_TSC, t_write) rather
than before vm_create_with_one_vcpu(&vcpu, NULL)?

When the kernel honours vcpu_set_msr(vcpu, MSR_IA32_TSC, t_write), the guest
TSC is set to t_write at the time of vcpu_set_msr() and only advances between
vcpu_set_msr() and vcpu_get_msr(), matching the second comment that
slack_cycles bounds the elapsed time around the two MSR ioctls.

Conversely, if a buggy kernel snaps the write back to the default generation
established during vCPU creation, t_read tracks the elapsed time since vCPU
creation. Because mono_ns is measured from before vm_create_with_one_vcpu(),
t_read is always less than mono_ns, so t_read - t_write < slack_cycles is
guaranteed to hold for any snapped-back value. If vm_create_with_one_vcpu()
takes between 500ms and 1500ms on a loaded host, a snapped-back TSC will
satisfy both t_read >= t_write and t_read - t_write < slack_cycles, causing
the test to pass on a buggy kernel.

Also, both the commit message and the file header comment note that
kernel-internal TSC synchronization on vCPU creation and frequency changes
must not arm user_set_tsc. During kvm_arch_vcpu_create(), kvm_set_tsc_khz()
runs while l1_tsc_scaling_ratio and l1_tsc_offset are still zero, whereas an
explicit KVM_SET_TSC_KHZ ioctl on an existing vCPU runs with non-zero
l1_tsc_scaling_ratio and l1_tsc_offset. Should main() also test calling
KVM_SET_TSC_KHZ before the first MSR_IA32_TSC write?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105154955.2525419-1-dwmw2@infradead.org?part=21