Re: [PATCH 1/1] KVM: selftests: add kvmclock drift test

[Sean Christopherson <seanjc@google.com>]

On Sat, Jan 06, 2024, Dongli Zhang wrote:
> diff --git a/tools/testing/selftests/kvm/x86_64/kvm_clock_drift.c b/tools/testing/selftests/kvm/x86_64/kvm_clock_drift.c
> new file mode 100644
> index 000000000000..324f0dbc5762
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/kvm_clock_drift.c

For better or worse, our selftests naming adds a _test at the end.

However, should we call this kvm_clock_hotplug_test.c?  That way the file level
comment isn't necessary (those these always become stale), and David's live update
suggestion won't conflict.

Or if the live update code fits nicely in this test, then kvm_clock_drift_test.c
is probably a good name.

> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * The kvmclock drift test. Emulate vCPU hotplug and online to verify if
> + * there is kvmclock drift.
> + *
> + * Adapted from steal_time.c

This is really uninteresting.

> + * Copyright (C) 2020, Red Hat, Inc.
> + * Copyright (C) 2024 Oracle and/or its affiliates.
> + */
> +
> +#include <asm/kvm_para.h>
> +#include <asm/pvclock.h>
> +#include <asm/pvclock-abi.h>
> +#include <sys/stat.h>
> +
> +#include "kvm_util.h"
> +#include "processor.h"
> +
> +#define NR_VCPUS		2
> +#define NR_SLOTS		2
> +#define KVMCLOCK_SIZE		sizeof(struct pvclock_vcpu_time_info)
> +/*
> + * KVMCLOCK_GPA is identity mapped
> + */
> +#define KVMCLOCK_GPA		(1 << 30)
> +
> +static uint64_t kvmclock_gpa = KVMCLOCK_GPA;

Why hardcode an address?  Can't the test have a global "struct pvclock_vcpu_time_info"
whose address is then queried by the host by reading the MSR?

Actually, can't the pvclock reads and asserts be done in the guest?  Which is
arguably better since the guest observing drift (or not) is what we really care
about.  Then the host side of the test never needs to resolve a host virtual
address for the pvclock_vcpu_time_info structure.

> +static void guest_code(int cpu)
> +{
> +	struct pvclock_vcpu_time_info *kvmclock;
> +
> +	/*
> +	 * vCPU#0 is to detect the change of pvclock_vcpu_time_info

Just vCPU0, which is the usually terminology in KVM land.

> +	 */

	/* Single line comments should look like this. */

> +	if (cpu == 0) {

Rather than switch inside the guest code, just use vcpu_arch_set_entry_point()
to point vCPU1 at a different function entirely.  Then most of the commetns go
away, and the code is generally much easier to read.

> +		GUEST_SYNC(0);
> +
> +		kvmclock = (struct pvclock_vcpu_time_info *) kvmclock_gpa;
> +		wrmsr(MSR_KVM_SYSTEM_TIME_NEW, kvmclock_gpa | KVM_MSR_ENABLED);
> +
> +		/*
> +		 * Backup the pvclock_vcpu_time_info before vCPU#1 hotplug
> +		 */
> +		kvmclock[1] = kvmclock[0];
> +
> +		GUEST_SYNC(2);
> +		/*
> +		 * Enter the guest to update pvclock_vcpu_time_info
> +		 */
> +		GUEST_SYNC(4);
> +	}
> +
> +	/*
> +	 * vCPU#1 is to emulate the vCPU hotplug
> +	 */
> +	if (cpu == 1) {
> +		GUEST_SYNC(1);
> +		/*
> +		 * This is after the host side MSR_IA32_TSC
> +		 */

Rather than add comments, use an enum to describe the stages:

	enum {
		SYNC_VCPU0_???
		SYNC_VCPU1_???
		SYNC_VCPU0_???
		SYNC_VCPU1_???
	}

That said, why does this test "emulate" hotplug?  Why not literally hotplug a
vCPU?  It's probably _less_ code, and the annoying NR_VCPUS #define goes away too,
because you can do:

	vm = vm_create_with_one_vcpu(&vcpu0, vcpu0_code);

	<setup kvmclock>

	vcpu1 = vm_vcpu_add(vm, 1, vcpu1_code);

Then you probably only need one sync, from vCPU to alert the host that kvmclock
is setup.

> +		GUEST_SYNC(3);
> +	}
> +}
> +#define CLOCKSOURCE_PATH "/sys/devices/system/clocksource/clocksource0/current_clocksource"
> +
> +static void check_clocksource(void)
> +{
> +	char *clk_name;
> +	struct stat st;
> +	FILE *fp;
> +
> +	fp = fopen(CLOCKSOURCE_PATH, "r");
> +	if (!fp) {
> +		pr_info("failed to open clocksource file: %d; assuming TSC.\n",
> +			errno);
> +		return;
> +	}
> +
> +	if (fstat(fileno(fp), &st)) {
> +		pr_info("failed to stat clocksource file: %d; assuming TSC.\n",
> +			errno);
> +		goto out;
> +	}
> +
> +	clk_name = malloc(st.st_size);
> +	TEST_ASSERT(clk_name, "failed to allocate buffer to read file\n");
> +
> +	if (!fgets(clk_name, st.st_size, fp)) {
> +		pr_info("failed to read clocksource file: %d; assuming TSC.\n",
> +			ferror(fp));
> +		goto out;
> +	}
> +
> +	TEST_ASSERT(!strncmp(clk_name, "tsc\n", st.st_size),

TEST_REQUIRE, not TEST_ASSERT, i.e. skip the test if the clocksource isn't
supported, don't cause a failure.

> +		    "clocksource not supported: %s", clk_name);
> +out:
> +	fclose(fp);
> +}

This can all be replaced with:

	TEST_REQUIRE(sys_clocksource_is_based_on_tsc());