Re: [PATCH v11 16/23] 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 5 potential issue(s) to consider:
- [Medium] KVM: selftests: missing TEST_REQUIRE() checks in pvclock_migration_test
- [Low] KVM: x86: doc per-vCPU KVM_SET_TSC_KHZ order breaks KVM_SET_CLOCK_GUEST
- [Low] KVM: x86: vcpu.rst drops source KVM_GET_TSC_KHZ step still used on dest
- [Low] KVM: selftests: VM memory leak in main() from using kvm_vm_release()
- [Low] KVM: selftests: duplicated pvclock and TSC scaling helpers in test
--

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

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

This commit adds the KVM_VCPU_TSC_SCALE device attribute to expose a vCPU's
TSC scaling ratio and fraction bits to userspace when hardware TSC scaling is
supported. It also updates the vCPU TSC migration documentation and adds a
selftest to verify kvmclock precision across simulated live migration.

> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index deb5c51bc00c8..3daa77c5d2150 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -239,6 +239,7 @@ including the layout of the stolen time structure.
>  :Architectures: x86
>  
>  4.1 ATTRIBUTE: KVM_VCPU_TSC_OFFSET
> +----------------------------------

[ ... ]

> @@ -261,44 +265,110 @@ following describes a possible algorithm to use for this purpose.
>  
>  From the source VMM process:
>  
> -1. Invoke the KVM_GET_CLOCK ioctl to record the host TSC (tsc_src),
> +1. Invoke the KVM_GET_CLOCK ioctl to record the host TSC (host_tsc_src),
>     kvmclock nanoseconds (guest_src), and host CLOCK_REALTIME nanoseconds
> -   (host_src).
> +   (time_src) at a given moment (Tsrc). The host_tsc field is only valid
> +   when the returned flags include KVM_CLOCK_HOST_TSC.
> +
> +2. For each vCPU[i]:
> +
> +   a. Read the KVM_VCPU_TSC_OFFSET attribute to record the guest TSC offset
> +      (ofs_src[i]).
> +
> +   b. Read the KVM_VCPU_TSC_SCALE attribute to record the guest TSC scaling
> +      ratio (ratio_src[i], frac_bits_src[i]). If the attribute is not
> +      supported (-ENXIO), there is no hardware TSC scaling and identity
> +      scaling applies: use ratio 1, frac_bits 0.
>  
> -2. Read the KVM_VCPU_TSC_OFFSET attribute for every vCPU to record the
> -   guest TSC offset (ofs_src[i]).
> +   c. Use host_tsc_src and the scaling/offset factors to calculate this
> +      vCPU's TSC at time Tsrc:
>  
> -3. Invoke the KVM_GET_TSC_KHZ ioctl to record the frequency of the
> -   guest's TSC (freq).
> +      tsc_src[i] = ((host_tsc_src * ratio_src[i]) >> frac_bits_src[i]) + ofs_src[i]
> +
> +      Note that frac_bits may be as high as 48, so this multiplication
> +      needs a 128-bit intermediate to avoid overflow.
> +
> +3. Invoke the KVM_GET_CLOCK_GUEST ioctl on the boot vCPU to return the KVM
> +   clock as a function of the guest TSC (pvti_src). (This ioctl may not
> +   succeed if the host and guest TSCs are not consistent and well-behaved.)

[Severity: Low]
Previously, step 3 on the source side instructed the VMM to invoke
KVM_GET_TSC_KHZ to record the guest TSC frequency (freq), which was replaced
here by KVM_GET_CLOCK_GUEST.

However, destination step 4 and step 7b below still reference freq and
freq[i]:

    tsc_dst[i] = tsc_src[i] + (ΔT * freq[i] / 10^6)

Should step 2 on the source side also include invoking KVM_GET_TSC_KHZ to
record freq[i] (or freq) for each vCPU?

>  From the destination VMM process:
>  
> -4. Invoke the KVM_SET_CLOCK ioctl, providing the source nanoseconds from
> -   kvmclock (guest_src) and CLOCK_REALTIME (host_src) in their respective
> -   fields.  Ensure that the KVM_CLOCK_REALTIME flag is set in the provided
> -   structure.
> +4. Invoke the KVM_SET_TSC_KHZ ioctl to set the scaled frequency of the
> +   guest's TSC (freq) — either on the VM before creating the vCPUs, or
> +   on each vCPU after creating it (as the exemplar test does).
> +
> +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),
> +   again checking KVM_CLOCK_HOST_TSC in the returned flags.
> +
> +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, taking care
> +      with the units (ΔT is in nanoseconds; freq, as set by
> +      KVM_SET_TSC_KHZ, is in kHz):
> +
> +      tsc_dst[i] = tsc_src[i] + (ΔT * freq[i] / 10^6)
>  
> -   KVM will advance the VM's kvmclock to account for elapsed time since
> -   recording the clock values.  Note that this will cause problems in
> -   the guest (e.g., timeouts) unless CLOCK_REALTIME is synchronized
> -   between the source and destination, and a reasonably short time passes
> -   between the source pausing the VMs and the destination executing
> -   steps 4-7.
> +   c. Use host_tsc_dst and the scaling factors to calculate this vCPU's
> +      raw scaled TSC at time Tdst without offsetting:
> +
> +      raw_dst[i] = ((host_tsc_dst * ratio_dst[i]) >> frac_bits_dst[i])
> +
> +   d. Calculate ofs_dst[i] = tsc_dst[i] - raw_dst[i] and set the resulting
> +      offset using the KVM_VCPU_TSC_OFFSET attribute.
> +
> +8. If pvti_src was provided, invoke the KVM_SET_CLOCK_GUEST ioctl on the boot
> +   vCPU to restore the KVM clock as a precise function of the guest TSC.

[Severity: Low]
Can following steps 4, 7, and 8 as written cause KVM_SET_CLOCK_GUEST in step 8
to fail with -ENODATA when a VMM uses per-vCPU KVM_SET_TSC_KHZ?

Step 4 allows calling KVM_SET_TSC_KHZ on each vCPU after creating it, while
step 7 is described as happening "As each vCPU[i] is created". If a VMM
reads KVM_VCPU_TSC_SCALE in step 7a as each vCPU is created before calling
per-vCPU KVM_SET_TSC_KHZ, KVM_VCPU_TSC_SCALE will return the unscaled default
ratio (1 << tsc_frac_bits) rather than the scaled ratio for freq.

Furthermore, if a VMM creates and configures one vCPU at a time (creating
vCPU[0], calling per-vCPU KVM_SET_TSC_KHZ(vCPU[0], freq), and running step 7,
then creating vCPU[1]), creating vCPU[1] invokes:

  kvm_arch_vcpu_create()
    -> kvm_set_tsc_khz(vcpu, kvm->arch.default_tsc_khz)
      -> __kvm_synchronize_tsc()

Because per-vCPU KVM_SET_TSC_KHZ does not update kvm->arch.default_tsc_khz,
vCPU[1] is initialized with the unscaled default_tsc_khz. In
arch/x86/kvm/x86.c:__kvm_synchronize_tsc():

	if (vcpu->arch.virtual_tsc_khz != kvm->arch.cur_tsc_khz) {
		kvm->arch.cur_tsc_freq_generation++;
		kvm->arch.all_vcpus_matched_freq = false;
		kvm->arch.nr_vcpus_matched_freq = 0;
	}

This bumps cur_tsc_freq_generation to default_tsc_khz, and the subsequent
KVM_SET_TSC_KHZ(vCPU[1], freq) bumps cur_tsc_freq_generation again back to
freq, leaving vCPU[0] in an older generation with all_vcpus_matched_freq set
to false. When step 8 then invokes KVM_SET_CLOCK_GUEST,
pvclock_update_vm_gtod_copy() disables use_master_clock and
KVM_SET_CLOCK_GUEST fails with -ENODATA.

Should the documentation clarify that if per-vCPU KVM_SET_TSC_KHZ is used
instead of VM-scoped KVM_SET_TSC_KHZ, all vCPUs must be created and have
KVM_SET_TSC_KHZ applied before reading KVM_VCPU_TSC_SCALE and setting
KVM_VCPU_TSC_OFFSET in step 7?

[ ... ]

> 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..c4a64b6a2e429
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/pvclock_migration_test.c

[ ... ]

> +static u64 pvclock_read_cycles(struct pvclock_vcpu_time_info *src,
> +				    u64 tsc)
> +{
> +	u64 delta = tsc - src->tsc_timestamp;
> +	u64 ns;
> +
> +	if (src->tsc_shift >= 0)
> +		delta <<= src->tsc_shift;
> +	else
> +		delta >>= -(s32)src->tsc_shift;
> +
> +	ns = (unsigned __int128)delta * src->tsc_to_system_mul >> 32;
> +	return src->system_time + ns;
> +}

[Severity: Low]
This isn't a bug, but pvclock_read_cycles() duplicates __pvclock_read_cycles()
from tools/arch/x86/include/asm/pvclock.h (already used by
tools/testing/selftests/kvm/x86/kvm_clock_test.c). In addition, main() later
open-codes the 128-bit TSC scaling calculation for raw_dst:

	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;

Could this test reuse __pvclock_read_cycles() and call
host_to_guest_tsc(&scale_dst, 0, host_tsc_dst) for raw_dst?

[ ... ]

> +int main(void)
> +{
> +	struct pvclock_vcpu_time_info pvti_src[NR_VCPUS];
> +	struct pvclock_vcpu_time_info pvti_dst_ioctl[NR_VCPUS];
> +	struct kvm_clock_data clock_src, clock_dst;
> +	struct kvm_vcpu_tsc_scale scale_src[NR_VCPUS], scale_dst;
> +	struct kvm_vcpu *vcpus[NR_VCPUS];
> +	struct kvm_vm *vm;
> +	u64 mono_before, kvm_before, kvm_after;
> +	s64 delta_before;
> +	u64 ofs_src[NR_VCPUS], tsc_src[NR_VCPUS], tsc_dst, raw_dst;
> +	u64 ofs_dst[NR_VCPUS];
> +	u64 host_tsc_src, host_tsc_dst;
> +	u64 time_src, time_dst;
> +	s64 delta_t;
> +	u32 freq_khz = 1500000; /* 1.5 GHz — forces TSC scaling */
> +	int i, pass;
> +
> +	TEST_REQUIRE(sys_clocksource_is_based_on_tsc());
> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_TSC_CONTROL));

[Severity: Medium]
What happens if this selftest is run on a kernel that supports
KVM_CAP_TSC_CONTROL, but does not support KVM_CAP_CLOCK_GUEST (or
KVM_VCPU_TSC_SCALE)?

In main(), the subsequent calls to vcpu_get_clock_guest(),
vcpu_set_clock_guest(), and vcpu_device_attr_get(..., KVM_VCPU_TSC_CTRL,
KVM_VCPU_TSC_SCALE, ...) unconditionally assert ioctl success, so the test
will fail with an assertion abort instead of skipping.

Should main() also check TEST_REQUIRE(kvm_has_cap(KVM_CAP_CLOCK_GUEST)) (like
pvclock_test.c, masterclock_offset_test.c, and xen_migration_test.c) and
verify that the vCPU has KVM_VCPU_TSC_SCALE via __vcpu_has_device_attr()?

[ ... ]

> +	/* Step 3: KVM_GET_CLOCK_GUEST (already fetched above as pvti_src[]) */
> +
> +	kvm_vm_release(vm);
> +
> +	/* === PAUSE (simulate migration) === */
> +	pr_info("=== Pausing 1 second ===\n");
> +	sleep(1);
> +
> +	/* === DESTINATION SIDE === */
> +	pr_info("=== Destination VM ===\n");
> +	/* Step 4: create + KVM_SET_TSC_KHZ (in create_vm) */
> +	vm = create_vm(vcpus, freq_khz);

[Severity: Low]
Does calling kvm_vm_release(vm) instead of kvm_vm_free(vm) here (and at the
end of main()) leak the VM's userspace_mem_regions mappings, sparsebit arrays,
and struct kvm_vm when vm is overwritten by create_vm()?

In tools/testing/selftests/kvm/lib/kvm_util.c, kvm_vm_release() only frees the
vCPUs and closes the KVM file descriptors, whereas kvm_vm_free() unmaps the
memory regions, frees vpages_valid and vpages_mapped, calls kvm_vm_release(),
and frees struct kvm_vm.

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