Re: [PATCH v7 33/36] KVM: selftests: Add KVM/PV clock selftest to prove timer correction

[Sean Christopherson]

> +int main(int argc, char *argv[])
> +{
> +	static const struct option long_opts[] = {
> +		{ "sleep", required_argument, NULL, 's' },
> +		{ "help",  no_argument,       NULL, 'h' },
> +		{ NULL,    0,                  NULL,  0  },
> +	};
> +	unsigned int sleep_sec = 2;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	uint64_t host_khz;
> +	uint64_t freq;
> +	int opt;
> +
> +	while ((opt = getopt_long(argc, argv, "s:h", long_opts, NULL)) != -1) {
> +		switch (opt) {
> +		case 's':
> +			sleep_sec = atoi(optarg);
> +			break;
> +		case 'h':
> +		default:
> +			usage(argv[0]);
> +			return opt == 'h' ? 0 : 1;
> +		}
> +	}
> +
> +	TEST_REQUIRE(sys_clocksource_is_based_on_tsc());
> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_TSC_CONTROL));
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +	configure_pvclock(vm);
> +
> +	/* Check KVM_GET_CLOCK_GUEST is supported */
> +	{
> +		struct pvclock_vcpu_time_info tmp;
> +		int ret = __vcpu_ioctl(vcpu, KVM_GET_CLOCK_GUEST, &tmp);
> +		TEST_REQUIRE(ret == 0);

It will likely be a moot point since we should have a CAP, but don't do TEST_REQUIRE()
on a local variable like this, it completely defeates the purpose of the macro
shenanigans.  Becuase this:

  1..0 # SKIP - Requirement not met: ret == 0

is useless information, whereas this:

  1..0 # SKIP - Requirement not met: !__vcpu_ioctl(vcpu, KVM_GET_CLOCK_GUEST, &tmp)

gives the user a starting point without having to go search through the test code.

> +	}

...

> +static volatile uint32_t vcpu_counter;
> +static void guest_code_stable_bit(void)
> +{
> +	uint32_t idx = __atomic_fetch_add(&vcpu_counter, 1, __ATOMIC_SEQ_CST);
> +	uint64_t gpa = KVMCLOCK_GPA + idx * sizeof(struct pvclock_vcpu_time_info);

This series needs to be updated to catch up to upstream.  Selftests now use
u32, u64, etc.  And at least one patch missed an obvious opportunity for guard(). 

> +	wrmsr(MSR_KVM_SYSTEM_TIME_NEW, gpa | KVM_MSR_ENABLED);
> +	GUEST_SYNC(0);
> +	GUEST_SYNC(0);
> +	GUEST_SYNC(0);
> +}
> +
> +static void set_tsc_offset(struct kvm_vcpu *vcpu, uint64_t offset)
> +{
> +	struct kvm_device_attr attr = {
> +		.group = KVM_VCPU_TSC_CTRL,
> +		.attr = KVM_VCPU_TSC_OFFSET,
> +		.addr = (__u64)(uintptr_t)&offset,
> +	};
> +
> +	TEST_REQUIRE(__vcpu_has_device_attr(vcpu, KVM_VCPU_TSC_CTRL,
> +					    KVM_VCPU_TSC_OFFSET) == 0);
> +	vcpu_ioctl(vcpu, KVM_SET_DEVICE_ATTR, &attr);

This quite clearly belongs in library code.

> +}
> +
> +static void run_vcpu_once(struct kvm_vcpu *vcpu)
> +{
> +	struct ucall uc;
> +
> +	vcpu_run(vcpu);
> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
> +	switch (get_ucall(vcpu, &uc)) {
> +	case UCALL_ABORT:
> +		REPORT_GUEST_ASSERT(uc);

Gah, we really need to have vcpu_run() handle guest asserts.

> +		break;
> +	case UCALL_SYNC:
> +		break;
> +	default:
> +		TEST_FAIL("Unexpected ucall");
> +	}
> +}
> +
> +static void test_tsc_stable_bit(void)
> +{
> +	struct pvclock_vcpu_time_info pvti;
> +	struct kvm_vcpu *vcpus[2];
> +	struct kvm_vm *vm;
> +	int ret;
> +
> +	pr_info("Testing PVCLOCK_TSC_STABLE_BIT with matched/unmatched TSCs\n");
> +
> +	vm = vm_create_with_vcpus(2, guest_code_stable_bit, vcpus);
> +	configure_pvclock(vm);
> +
> +	/*
> +	 * Case 1: All TSCs matched (same frequency and offset).
> +	 * Master clock should be active, PVCLOCK_TSC_STABLE_BIT set.
> +	 */
> +	run_vcpu_once(vcpus[0]);
> +
> +	ret = __vcpu_ioctl(vcpus[0], KVM_GET_CLOCK_GUEST, &pvti);
> +	TEST_ASSERT(!ret, "GET_CLOCK_GUEST should succeed with matched TSCs");
> +	TEST_ASSERT(pvti.flags & PVCLOCK_TSC_STABLE_BIT,
> +		    "PVCLOCK_TSC_STABLE_BIT should be set with matched TSCs");
> +
> +	/*
> +	 * Case 2: Different TSC offset, same frequency.
> +	 * Master clock should still be active (frequency matches), but
> +	 * PVCLOCK_TSC_STABLE_BIT should be cleared (offsets differ).
> +	 */
> +	set_tsc_offset(vcpus[1], 12345678);
> +	run_vcpu_once(vcpus[1]);
> +	run_vcpu_once(vcpus[0]);
> +
> +	ret = __vcpu_ioctl(vcpus[0], KVM_GET_CLOCK_GUEST, &pvti);
> +	if (ret) {
> +		/* Master clock disabled by offset mismatch — old kernel */
> +		pr_info("  Skipping offset tests (master clock requires matched offsets)\n");
> +		goto out_stable;
> +	}
> +	TEST_ASSERT(!(pvti.flags & PVCLOCK_TSC_STABLE_BIT),
> +		    "PVCLOCK_TSC_STABLE_BIT should be clear with offset-mismatched TSCs");
> +
> +	/*
> +	 * Case 3: Different TSC frequency.
> +	 * Master clock should be disabled entirely.
> +	 */
> +	vcpu_ioctl(vcpus[1], KVM_SET_TSC_KHZ,
> +		   (void *)(unsigned long)(__vcpu_ioctl(vcpus[1], KVM_GET_TSC_KHZ, NULL) / 2));
> +	/* Write TSC to trigger kvm_synchronize_tsc / kvm_track_tsc_matching */
> +	vcpu_set_msr(vcpus[1], MSR_IA32_TSC, 0);
> +	run_vcpu_once(vcpus[1]);
> +
> +	ret = __vcpu_ioctl(vcpus[0], KVM_GET_CLOCK_GUEST, &pvti);
> +	TEST_ASSERT(ret && errno == EINVAL,
> +		    "GET_CLOCK_GUEST should fail with frequency-mismatched TSCs, got %d (errno %d)",
> +		    ret, errno);
> +
> +out_stable:
> +	kvm_vm_free(vm);
> +}
> +
> +static void test_clock_guest_with_offsets(void)
> +{
> +	struct pvclock_vcpu_time_info pvti0, pvti1, pvti1_after;
> +	struct kvm_vcpu *vcpus[2];
> +	struct kvm_vm *vm;
> +	int64_t delta;
> +	int ret;
> +
> +	pr_info("Testing KVM_[GS]ET_CLOCK_GUEST with different TSC offsets\n");
> +
> +	vm = vm_create_with_vcpus(2, guest_code_stable_bit, vcpus);
> +	configure_pvclock(vm);
> +
> +	/* Set different TSC offsets on the two vCPUs */
> +	set_tsc_offset(vcpus[0], 0);
> +	set_tsc_offset(vcpus[1], 1000000000ull);
> +
> +	/* Run both to establish kvmclock */
> +	run_vcpu_once(vcpus[0]);
> +	run_vcpu_once(vcpus[1]);
> +
> +	/* GET_CLOCK_GUEST on both — should succeed (master clock active) */
> +	ret = __vcpu_ioctl(vcpus[0], KVM_GET_CLOCK_GUEST, &pvti0);
> +	if (ret) {
> +		pr_info("  Skipping (master clock requires matched offsets on this kernel)\n");
> +		kvm_vm_free(vm);
> +		return;
> +	}
> +	ret = __vcpu_ioctl(vcpus[1], KVM_GET_CLOCK_GUEST, &pvti1);
> +	TEST_ASSERT(!ret, "GET_CLOCK_GUEST on vcpu1 failed");
> +
> +	/* The tsc_timestamps should differ (different offsets) */
> +	TEST_ASSERT(pvti0.tsc_timestamp != pvti1.tsc_timestamp,
> +		    "tsc_timestamps should differ with different offsets");
> +
> +	/* Sleep to let time elapse, then restore vcpu0's clock */
> +	sleep(1);
> +	vcpu_ioctl(vcpus[0], KVM_SET_CLOCK_GUEST, &pvti0);
> +
> +	/* Run vcpu0 to process the clock update */
> +	run_vcpu_once(vcpus[0]);
> +
> +	/* GET_CLOCK_GUEST on vcpu1 — should reflect the correction */
> +	ret = __vcpu_ioctl(vcpus[1], KVM_GET_CLOCK_GUEST, &pvti1_after);
> +	TEST_ASSERT(!ret, "GET_CLOCK_GUEST on vcpu1 after SET failed");

Please add proper APIs instead of copy+pasting the same code everywhere.  E.g.
this should really be something like

	vcpu_get_clock_guest(vcpus[1], ...);

where vcpu_ioctl() asserts success.