Re: [PATCH v5 08/34] KVM: x86: Add KVM_VCPU_TSC_SCALE and fix the documentation on TSC migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing capability check for TSC scaling in `pvclock_migration_test` causes unconditional test failure on unsupported hardware.
- [Medium] Integer overflow in `tsc_dst` and `tsc_ofs_delta_ns` calculations in `pvclock_migration_test`.
--

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

KVM: x86: Add KVM_VCPU_TSC_SCALE and fix the documentation on TSC migration

Adds KVM_VCPU_TSC_SCALE to extract the scale ratio and frac_bits, and
documents the process for userspace to preserve the TSC across migration.
Adds a selftest to verify KVM clock precision across simulated live migration.

> diff --git a/tools/testing/selftests/kvm/x86/pvclock_migration_test.c b/tools/testing/selftests/kvm/x86/pvclock_migration_test.c
> new file mode 100644
> index 0000000000000..6a7eaf627d1a7
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/pvclock_migration_test.c

[ ... ]

> +int main(void)
> +{

[ ... ]

> +	uint32_t freq_khz = 1500000; /* 1.5 GHz — forces TSC scaling */
> +	int ret;
> +
> +	TEST_REQUIRE(sys_clocksource_is_based_on_tsc());
> +
> +	/* === SOURCE SIDE === */
> +	pr_info("=== Source VM ===\n");
> +	vm = create_vm(&vcpu);
> +
> +	/* Set guest TSC frequency (may trigger scaling) */
> +	vcpu_ioctl(vcpu, KVM_SET_TSC_KHZ, (void *)(unsigned long)freq_khz);

[Severity: Medium]
Will this unconditionally fail on hardware that lacks TSC scaling?

The test unconditionally forces a 1.5 GHz guest TSC frequency. If the host
lacks hardware TSC scaling (kvm_caps.has_tsc_control is false) and the
physical TSC frequency is greater than 1.5 GHz, KVM returns -EINVAL.

Should there be a check like TEST_REQUIRE(kvm_has_cap(KVM_CAP_TSC_CONTROL))
before forcing this, or a way to gracefully handle the error?

[ ... ]

> +	/* Step 6: ΔT */
> +	delta_t = (int64_t)(time_dst - time_src);
> +
> +	/* Step 7: Compute destination offset */
> +	{
> +		struct kvm_device_attr attr = {
> +			.group = KVM_VCPU_TSC_CTRL,
> +			.attr = KVM_VCPU_TSC_SCALE,
> +			.addr = (uint64_t)(uintptr_t)&scale_dst,
> +		};
> +		memset(&scale_dst, 0, sizeof(scale_dst));
> +		__vcpu_ioctl(vcpu, KVM_GET_DEVICE_ATTR, &attr);
> +	}
> +
> +	tsc_dst = tsc_src + (uint64_t)((int64_t)freq_khz * 1000 * delta_t / 1000000000LL);

[Severity: Medium]
Can this calculation overflow under heavy load?

With freq_khz set to 1500000, the intermediate multiplication is
1.5e9 * delta_t. This exceeds INT64_MAX if delta_t is greater than ~6.14
seconds. While the test uses sleep(1), VM teardown and creation under heavy
CI load might cause the time delta to exceed this.

Could this be rearranged to avoid intermediate overflow, for example:
delta_t * freq_khz / 1000000LL?

> +
> +	if (scale_dst.tsc_frac_bits)
> +		raw_dst = (unsigned __int128)host_tsc_dst * scale_dst.tsc_ratio
> +			  >> scale_dst.tsc_frac_bits;
> +	else
> +		raw_dst = host_tsc_dst;
> +
> +	ofs_dst = tsc_dst - raw_dst;
> +
> +	/*
> +	 * The TSC offset delta introduced by using CLOCK_REALTIME to
> +	 * estimate elapsed time. On same host, the correct offset is
> +	 * ofs_src; the difference is the CLOCK_REALTIME-vs-TSC error.
> +	 */
> +	int64_t tsc_ofs_delta = (int64_t)(ofs_dst - ofs_src);
> +	int64_t tsc_ofs_delta_ns = tsc_ofs_delta * 1000000000LL / ((int64_t)freq_khz * 1000);

[Severity: Medium]
Could a similar integer overflow happen here if the TSC offset delta cycles
are abnormally large?

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