Re: [PATCH v12 16/24] 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
issues to consider:
- [Low] KVM: x86: doc per-vCPU KVM_SET_TSC_KHZ order breaks
  KVM_SET_CLOCK_GUEST
- [Low] KVM: selftests: duplicated pvclock and TSC scaling helpers in
  test
--

commit 83ddc67114d3c0896cacdd30d2c4eb5ecc38fbfe
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 vCPU device attribute to expose the
guest TSC scaling ratio and fractional bits to userspace, updates the TSC
migration documentation, and adds a selftest for pvclock and TSC migration.

> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index deb5c51bc00c8..8dc6ad904429d 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
[ ... ]
> @@ -261,44 +265,117 @@ following describes a possible algorithm to use for this purpose.
[ ... ]
> +From the destination VMM process:
> +
> +4. Invoke the KVM_SET_TSC_KHZ ioctl to set the scaled frequency of the
> +   guest's TSC (freq[i]) — either on the VM before creating the vCPUs, or
> +   on each vCPU after creating it (as the exemplar test does). When using
> +   the per-vCPU form, set the frequency before performing step 7 on that
> +   vCPU: KVM_VCPU_TSC_SCALE reports the ratio for the currently
> +   configured frequency.
> +
> +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:

[Severity: Low]
When using the per-vCPU KVM_SET_TSC_KHZ ioctl, does creating and
configuring vCPUs one at a time disable the master clock?

Without the VM-scoped KVM_SET_TSC_KHZ ioctl, each KVM_CREATE_VCPU runs
kvm_arch_vcpu_postcreate() -> kvm_synchronize_tsc() at default_tsc_khz.
If a VMM sets freq[0] on vCPU[0] and then creates vCPU[1], vCPU[1]'s
creation bumps cur_tsc_freq_generation back to default_tsc_khz and resets
nr_vcpus_matched_freq, and setting freq[1] on vCPU[1] bumps
cur_tsc_freq_generation again, leaving vCPU[0] in an older generation:

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;
	}

	/* Count each vCPU once per freq generation */
	if (vcpu->arch.this_tsc_freq_generation != kvm->arch.cur_tsc_freq_generation) {
		vcpu->arch.this_tsc_freq_generation = kvm->arch.cur_tsc_freq_generation;
		kvm->arch.nr_vcpus_matched_freq++;
	}
	...
}

Because all_vcpus_matched_freq stays false, KVM_GET_CLOCK in step 5 will
not set KVM_CLOCK_HOST_TSC (or KVM_SET_CLOCK_GUEST in step 8 will fail
with -ENODATA if vCPUs are created during step 7).  Should step 4 note
that when using the per-vCPU KVM_SET_TSC_KHZ ioctl, all vCPUs must be
created before setting the frequency on any of them (as create_vm() in
pvclock_migration_test.c does)?

> 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..c18c9132308d6
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/pvclock_migration_test.c
> @@ -0,0 +1,521 @@
[ ... ]
> +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;
> +}
[ ... ]
> +	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;

[Severity: Low]
This isn't a bug, but pvclock_read_cycles() duplicates
__pvclock_read_cycles() from tools/arch/x86/include/asm/pvclock.h, and
raw_dst open-codes host_to_guest_tsc(&scale_dst, 0, host_tsc_dst) defined
earlier in this file.

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