Re: [PATCH v12 18/24] KVM: selftests: Add master clock offset test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] KVM: selftests: get_pvtis() never reads guest pvclock pages
  at KVMCLOCK_GPA
--

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

KVM: selftests: Add master clock offset test

This commit adds an x86 KVM selftest to verify that master clock mode
remains active and pvclock readings stay consistent across vCPUs with
different TSC offsets, both initially and after advancing the clock via
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..faf81872538eb
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/masterclock_offset_test.c
> @@ -0,0 +1,203 @@

[ ... ]

> +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]);

[ ... ]

> +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");

[Severity: Medium]
Should get_pvtis() read the per-vCPU pvclock_vcpu_time_info structures
from guest memory at KVMCLOCK_GPA (or check both guest memory and
vcpu_get_clock_guest())?

The test sets up guest memory at KVMCLOCK_GPA, enables
MSR_KVM_SYSTEM_TIME_NEW in guest_code(), and runs each vCPU after
vm_set_clock() so kvm_guest_time_update() refreshes the guest pvclock
structures.  However, get_pvtis() only calls vcpu_get_clock_guest()
(KVM_GET_CLOCK_GUEST), which synthesizes hv_clock directly from the
VM-wide master clock fields in struct kvm_arch without reading the guest
pvclock pages:

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

As a result, the guest memory at KVMCLOCK_GPA is never inspected, and
checking that tsc_to_system_mul, tsc_shift, and system_time match across
vCPUs compares the same VM-wide ka->master_* fields against themselves
regardless of whether vcpu_run() updated the guest pvclock structures.
In addition, because vcpu_set_tsc_offset() on vcpus[2] in main() queues
KVM_REQ_MASTERCLOCK_UPDATE only on vcpus[2], running vcpus[0..2] once in
order populates the guest pvclock structures for vcpus[0] and vcpus[1]
before vcpus[2] runs kvm_update_masterclock() and broadcasts
KVM_REQ_CLOCK_UPDATE, leaving vcpus[0] and vcpus[1] with stale guest
memory state until the next vcpu_run().

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