Re: [PATCH v11 18/23] KVM: selftests: Add master clock offset test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: selftests: get_pvtis() never reads guest pvclock pages at KVMCLOCK_GPA
- [Low] KVM: selftests: pvclock_calc() and test duplicate pvclock helpers and tests
--

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

KVM: selftests: Add master clock offset test

This commit adds a KVM selftest on x86 to verify that master clock mode
remains active and consistent across vCPUs when one vCPU is given a
different TSC offset and the VM clock is advanced with KVM_SET_CLOCK.

> diff --git a/tools/testing/selftests/kvm/x86/masterclock_offset_test.c b/tools/testing/selftests/kvm/x86/masterclock_offset_test.c
> new file mode 100644
> index 0000000000000..47a1313eadc9f
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/masterclock_offset_test.c
> @@ -0,0 +1,215 @@
[ ... ]
> +static u64 pvclock_calc(struct pvclock_vcpu_time_info *pvti, u64 guest_tsc)
> +{
> +	u64 delta = guest_tsc - pvti->tsc_timestamp;
> +
> +	if (pvti->tsc_shift >= 0)
> +		delta <<= pvti->tsc_shift;
> +	else
> +		delta >>= -(int)pvti->tsc_shift;
> +
> +	return pvti->system_time + ((__uint128_t)delta * pvti->tsc_to_system_mul >> 32);
> +}

[Severity: Low]
This isn't a bug, but pvclock_calc() in
tools/testing/selftests/kvm/x86/masterclock_offset_test.c duplicates
__pvclock_read_cycles() from tools/arch/x86/include/asm/pvclock.h (already
used by tools/testing/selftests/kvm/x86/kvm_clock_test.c):

tools/arch/x86/include/asm/pvclock.h:__pvclock_read_cycles() {
	u64 delta = tsc - src->tsc_timestamp;
	u64 offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
					     src->tsc_shift);
	return src->system_time + offset;
}

Also, tools/testing/selftests/kvm/x86/pvclock_test.c (in
test_tsc_stable_bit() and test_clock_guest_with_offsets()) and
tools/testing/selftests/kvm/x86/pvclock_migration_test.c (in
check_cross_vcpu_coherence()) already test master clock persistence,
PVCLOCK_TSC_STABLE_BIT clearing, and cross-vCPU pvclock agreement when vCPUs
have divergent TSC offsets.

Could this use __pvclock_read_cycles() and be folded as a test case into
tools/testing/selftests/kvm/x86/pvclock_test.c rather than adding a separate
test binary?

> +static void guest_code(u64 pvti_gpa)
> +{
> +	wrmsr(MSR_KVM_SYSTEM_TIME_NEW, pvti_gpa | KVM_MSR_ENABLED);
> +	for (;;)
> +		GUEST_SYNC(0);
> +}
> +
> +static void get_pvtis(struct kvm_vcpu **vcpus,
> +		      struct pvclock_vcpu_time_info *pvti)
> +{
> +	int i;
> +
> +	for (i = 0; i < NR_VCPUS; i++) {
> +		int idx = vcpu_order[i];
> +
> +		vcpu_get_clock_guest(vcpus[idx], &pvti[idx]);
> +		pr_info("vCPU %d: tsc_timestamp=%lu system_time=%lu "
> +			"mul=%u shift=%d flags=0x%x\n",
> +			idx, (unsigned long)pvti[idx].tsc_timestamp,
> +			(unsigned long)pvti[idx].system_time,
> +			pvti[idx].tsc_to_system_mul, pvti[idx].tsc_shift,
> +			pvti[idx].flags);
> +	}
> +}
[ ... ]
> +static void test_set_clock_consistency(struct kvm_vm *vm,
> +				       struct kvm_vcpu **vcpus,
> +				       struct pvclock_vcpu_time_info *pvti,
> +				       struct kvm_clock_data *clock,
> +				       u64 offset0)
> +{
> +	struct kvm_clock_data setclk = { .clock = clock->clock + ONE_HOUR_NS };
> +	u64 clk0;
> +	int i;
> +
> +	/*
> +	 * Add an hour to the VM kvmclock via KVM_SET_CLOCK, run each
> +	 * vCPU to pick up the update, and check they're still in sync.
> +	 */
> +	vm_set_clock(vm, &setclk);
> +
> +	/* Guest code does GUEST_SYNC then exits — run each to see update */
> +	for (i = 0; i < NR_VCPUS; i++) {
> +		vcpu_run(vcpus[vcpu_order[i]]);
> +		TEST_ASSERT_KVM_EXIT_REASON(vcpus[vcpu_order[i]], KVM_EXIT_IO);
> +	}
> +
> +	get_pvtis(vcpus, pvti);
> +
> +	TEST_ASSERT(pvti[0].system_time == pvti[1].system_time &&
> +		    pvti[0].system_time == pvti[2].system_time,
> +		    "system_time should still match after KVM_SET_CLOCK");
[ ... ]
> +int main(void)
> +{
[ ... ]
> +	/* Give each vCPU its own pvclock structure within the page */
> +	for (i = 0; i < NR_VCPUS; i++)
> +		vcpu_args_set(vcpus[i], 1, KVMCLOCK_GPA +
> +			      i * sizeof(struct pvclock_vcpu_time_info) * 2);
> +
> +	/* Get vCPU 0's default offset and set vCPU 2's offset higher */
> +	offset0 = vcpu_get_tsc_offset(vcpus[0]);
> +	vcpu_set_tsc_offset(vcpus[2], offset0 + TSC_OFFSET);
> +
> +	/* Run each vCPU to enable kvmclock (with offset already set) */
> +	for (i = 0; i < NR_VCPUS; i++) {
> +		vcpu_run(vcpus[i]);
> +		TEST_ASSERT_KVM_EXIT_REASON(vcpus[i], KVM_EXIT_IO);
> +	}

[Severity: Medium]
Should get_pvtis() also inspect the guest-visible pvclock structures mapped
at KVMCLOCK_GPA rather than only calling vcpu_get_clock_guest()?

In main(), the test maps guest memory at KVMCLOCK_GPA, gives each vCPU its
own pvclock GPA, runs guest_code() to write MSR_KVM_SYSTEM_TIME_NEW, and in
test_set_clock_consistency() runs each vCPU after vm_set_clock() to refresh
the per-vCPU pvclock state.

However, get_pvtis() only calls vcpu_get_clock_guest()
(KVM_GET_CLOCK_GUEST). In arch/x86/kvm/x86.c,
kvm_vcpu_ioctl_get_clock_guest() does not read the guest pvclock page or go
through kvm_guest_time_update(); instead, it synthesizes hv_clock directly
from the VM-wide master clock fields in struct kvm_arch:

arch/x86/kvm/x86.c:kvm_vcpu_ioctl_get_clock_guest() {
	...
		hv_clock.tsc_timestamp = kvm_read_l1_tsc(v, ka->master_cycle_now);
		hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
		hv_clock.tsc_shift = ka->master_tsc_shift;
		hv_clock.tsc_to_system_mul = ka->master_tsc_mul;
		hv_clock.flags = READ_ONCE(ka->all_vcpus_matched_tsc) ?
				 PVCLOCK_TSC_STABLE_BIT : 0;
	...
}

Because kvm_vm_ioctl_set_clock() updates ka->kvmclock_offset synchronously,
get_pvtis() never observes the guest memory at KVMCLOCK_GPA written by
kvm_guest_time_update(), and comparing system_time, tsc_to_system_mul, and
tsc_shift across vCPUs only compares the same VM-wide master_* fields to
themselves regardless of whether the vCPUs were run.

In addition, when main() calls vcpu_set_tsc_offset(vcpus[2], offset0 +
TSC_OFFSET), KVM_REQ_MASTERCLOCK_UPDATE is raised only on vcpus[2]. Running
vcpus[0], vcpus[1], and vcpus[2] in a single pass causes vcpus[0] and
vcpus[1] to populate their guest pvclock pages at KVMCLOCK_GPA before
vcpus[2] runs kvm_update_masterclock() and broadcasts KVM_REQ_CLOCK_UPDATE,
leaving the guest-visible pvclock pages for vcpus[0] and vcpus[1] with a
stale pre-update snapshot where PVCLOCK_TSC_STABLE_BIT is still set.

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