Re: [PATCH v7 32/36] 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] The documented algorithm for TSC migration calculates the destination TSC incorrectly by omitting a unit conversion factor.
- [Medium] The exemplar test contradicts the documented procedure for setting guest TSC frequency.
--

--- Patch [32]: [PATCH v7 32/36] KVM: x86: Add KVM_VCPU_TSC_SCALE and fix the documentation on TSC migration ---
commit 24bf59a7caece6b1df9bc880e92f05672eb883ff
Author: David Woodhouse <dwmw@amazon.co.uk>
KVM: x86: Add KVM_VCPU_TSC_SCALE and fix the documentation on TSC migration

This commit adds KVM_VCPU_TSC_SCALE to extract the actual scale ratio and
frac_bits, documents the process that userspace needs to follow to preserve
the TSC across migration, and adds an exemplar selftest.

> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index 66e714f2fcfa7..2b8ab8995079c 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
[ ... ]
> +From the destination VMM process:
> +
> +4. Before creating the vCPUs, invoke the KVM_SET_TSC_KHZ ioctl on the VM, to
> +   set the scaled frequency of the guest's TSC (freq).
> +
> +5. Invoke the KVM_GET_CLOCK ioctl to record the host TSC (host_tsc_dst) and
> +   host CLOCK_REALTIME nanoseconds (time_dst) at a given moment (Tdst).
> +
> +6. Calculate the number of nanoseconds elapsed between Tsrc and Tdst:
> +
> +   ΔT = time_dst - time_src
> +
> +7. As each vCPU[i] is created:
> +
> +   a. Read the KVM_VCPU_TSC_SCALE attribute to record the guest TSC scaling
> +      ratio (ratio_dst[i], frac_bits_dst[i]).
> +
> +   b. Calculate the intended guest TSC value at time Tdst:
> +
> +      tsc_dst[i] = tsc_src[i] + (ΔT * freq[i])

[Severity: Medium]
Does this formula need a unit conversion factor? If ΔT is in nanoseconds and
freq is in kHz, multiplying them directly yields a result that is off by a
factor of 1,000,000.

The new test performs this calculation with the division, which appears
correct:
tsc_dst = tsc_src + (uint64_t)((int64_t)freq_khz * delta_t / 1000000LL);

> 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..3b675827aec34
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/pvclock_migration_test.c
[ ... ]
> +	/* === DESTINATION SIDE === */
> +	pr_info("=== Destination VM ===\n");
> +	vm = create_vm(&vcpu);
> +
> +	/* Step 4: KVM_SET_TSC_KHZ */
> +	vcpu_ioctl(vcpu, KVM_SET_TSC_KHZ, (void *)(unsigned long)freq_khz);

[Severity: Medium]
Is the documentation correct that KVM_SET_TSC_KHZ should be invoked on the
VM before creating vCPUs?

The code here invokes KVM_SET_TSC_KHZ on the vCPU after creating it, which
contradicts step 4 of the documented procedure above.

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