Date: Tue, 4 Jun 2024 16:49:37 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: <isaku.yamahata@...el.com>, <pbonzini@...hat.com>,
	<erdemaktas@...gle.com>, <vkuznets@...hat.com>, <vannapurve@...gle.com>,
	<jmattson@...gle.com>, <mlevitsk@...hat.com>, <xiaoyao.li@...el.com>,
	<chao.gao@...el.com>, <rick.p.edgecombe@...el.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V5 4/4] KVM: selftests: Add test for configure of x86 APIC
 bus frequency

Hi Sean,

On 6/3/24 10:25 AM, Sean Christopherson wrote:
> On Thu, Apr 25, 2024, Reinette Chatre wrote:
>> From: Isaku Yamahata <isaku.yamahata@...el.com>
>>
>> Test if the APIC bus clock frequency is the expected configured value.
> 
> This is one of the cases where explicitly calling out "code" by name is extremely
> valuable.  E.g.
> 
>      Test if KVM emulates the APIC bus clock at the expected frequency when
>      userspace configures the frequency via KVM_CAP_X86_APIC_BUS_CYCLES_NS.
>      
>      Set APIC timer's initial count to the maximum value and busy wait for 100
>      msec (largely arbitrary) using the TSC. Read the APIC timer's "current
>      count" to calculate the actual APIC bus clock frequency based on TSC
>      frequency.

Thank you very much. (copy&pasted)

> 
>> Set APIC timer's initial count to the maximum value and busy wait for 100
>> msec (any value is okay) with TSC value. Read the APIC timer's "current
>> count" to calculate the actual APIC bus clock frequency based on TSC
>> frequency.
>>
>> diff --git a/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c
>> new file mode 100644
>> index 000000000000..5100b28228af
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c
>> @@ -0,0 +1,166 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Test configure of APIC bus frequency.
>> + *
>> + * Copyright (c) 2024 Intel Corporation
>> + *
>> + * To verify if the APIC bus frequency can be configured this test starts
>> + * by setting the TSC frequency in KVM, and then:
>> + * For every APIC timer frequency supported:
>> + * * In the guest:
>> + * * * Start the APIC timer by programming the APIC TMICT (initial count
>> + *       register) to the largest value possible to guarantee that it will
>> + *       not expire during the test,
>> + * * * Wait for a known duration based on previously set TSC frequency,
>> + * * * Stop the timer and read the APIC TMCCT (current count) register to
>> + *       determine the count at that time (TMCCT is loaded from TMICT when
>> + *       TMICT is programmed and then starts counting down).
>> + * * In the host:
>> + * * * Determine if the APIC counts close to configured APIC bus frequency
>> + *     while taking into account how the APIC timer frequency was modified
>> + *     using the APIC TDCR (divide configuration register).
> 
> I find the asterisks super hard to parse.  And I honestly wouldn't bother breaking
> things down by guest vs. host.  History has shown that file comments that are *too*
> specific eventually become stale, often sooner than later.  E.g. it's entirely
> feasible to do the checking in the guest, not the host.
> 
> How about this?
> 
> /*
>   * Copyright (c) 2024 Intel Corporation
>   *
>   * Verify KVM correctly emulates the APIC bus frequency when the VMM configures
>   * the frequency via KVM_CAP_X86_APIC_BUS_CYCLES_NS.  Start the APIC timer by
>   * programming TMICT (timer initial count) to the largest value possible (so
>   * that the timer will not expire during the test).  Then, after an arbitrary
>   * amount of time has elapsed, verify TMCCT (timer current count) is within 1%
>   * of the expected value based on the time elapsed, the APIC bus frequency, and
>   * the programmed TDCR (timer divide configuration register).
>   */

Thank you very much. (copy&pasted)

> 
>> + */
>> +#define _GNU_SOURCE /* for program_invocation_short_name */
> 
> This can now be dropped.

Right.

> 
>> +#include "apic.h"
>> +#include "test_util.h"
>> +
>> +/*
>> + * Pick one convenient value, 1.5GHz. No special meaning and different from
>> + * the default value, 1GHz.
> 
> I have no idea where the 1GHz comes from.  KVM doesn't force a default TSC, KVM
> uses the underlying CPU's frequency.  Peeking further ahead, I don't understand
> why this test sets KVM_SET_TSC_KHZ.  That brings in a whole different set of
> behavior, and that behavior is already verified by tsc_scaling_sync.c.
> 
> I suspect/assume this test forces a frequency so that it can hardcode the math,
> but (a) that's odd and (b) x86 selftests really should provide a udelay() so that
> goofy stuff like this doesn't end up in random tests.

I believe the "default 1GHz" actually refers to the default APIC bus frequency and
the goal was indeed to (a) make the TSC frequency different from APIC bus frequency,
and (b) make math easier.

Yes, there is no need to use KVM_SET_TSC_KHZ. An implementation of udelay() would
require calibration and to make this simple for KVM I think we can just use
KVM_GET_TSC_KHZ. For now I continue to open code this (see later) since I did not
notice similar patterns in existing tests that may need a utility. I'd be happy
to add a utility if the needed usage pattern is clear since the closest candidate
I could find was xapic_ipi_test.c that does not have a nop loop.

> 
>> + */
>> +#define TSC_HZ			(1500 * 1000 * 1000ULL)
> 
> Definitely do not call this TSC_HZ.  Yeah, it's local to this file, but defining
> generic macros like this is just asking for conflicts, and the value itself has
> nothing to do with the TSC (it's a raw value).  E.g. _if_ we need to keep this,
> something like

Macro is gone. In its place is a new global tsc_hz that is the actual TSC
frequency of the guest.

> 
>    #define FREQ_1_5_GHZ		(1500 * 1000 * 1000ULL)
> 
>> +
>> +/* Wait for 100 msec, not too long, not too short value. */
>> +#define LOOP_MSEC		100ULL
>> +#define TSC_WAIT_DELTA		(TSC_HZ / 1000 * LOOP_MSEC)
> 
> These shouldn't exist.

Gone.

> 
> 
>> +
>> +/*
>> + * Pick a typical value, 25MHz. Different enough from the default value, 1GHz.
>> + */
>> +#define APIC_BUS_CLOCK_FREQ	(25 * 1000 * 1000ULL)
> 
> Rather than hardcode a single frequency, use 25MHz as the default value but let
> the user override it via command line.

Done.

> 
>> +	asm volatile("cli");
> 
> Unless I'm misremembering, the timer still counts when the LVT entry is masked
> so just mask the IRQ in the LVT. Or rather, keep the entry masked in the LVT.

hmmm ... I do not think this is specific to LVT entry but instead an attempt
to ignore all maskable external interrupt that may interfere with the test.
LVT entry is prevented from triggering because if the large configuration value.

> 
> FWIW, you _could_ simply leave APIC_LVT0 at its default value to verify KVM
> correctly emulates that reset value (masked, one-shot).  That'd be mildly amusing,
> but possibly a net negative from readability, so
> 
>> +
>> +	xapic_enable();
> 
> What about x2APIC?  Arguably that's _more_ interesting since it's required for
> TDX.

Added test for x2APIC to test both.

> 
>> +	/*
>> +	 * Setup one-shot timer.  The vector does not matter because the
>> +	 * interrupt does not fire.
> 
> _should_ not fire.

ack.

> 
>> +	 */
>> +	xapic_write_reg(APIC_LVT0, APIC_LVT_TIMER_ONESHOT);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tdcrs); i++) {
>> +		xapic_write_reg(APIC_TDCR, tdcrs[i].tdcr);
>> +
>> +		/* Set the largest value to not trigger the interrupt. */
>> +		tmict = ~0;
>> +		xapic_write_reg(APIC_TMICT, tmict);
>> +
>> +		/* Busy wait for LOOP_MSEC */
>> +		tsc0 = rdtsc();
>> +		tsc1 = tsc0;
>> +		while (tsc1 - tsc0 < TSC_WAIT_DELTA)
>> +			tsc1 = rdtsc();
>> +
>> +		/* Read APIC timer and TSC */
>> +		tmcct = xapic_read_reg(APIC_TMCCT);
>> +		tsc1 = rdtsc();
>> +
>> +		/* Stop timer */
>> +		xapic_write_reg(APIC_TMICT, 0);
>> +
>> +		/* Report it. */
>> +		GUEST_SYNC_ARGS(tdcrs[i].divide_count, tmict - tmcct,
>> +				tsc1 - tsc0, 0, 0);
> 
> Why punt to the host?  I don't see any reason why GUEST_ASSERT() wouldn't work
> here.

GUEST_ASSERT works and changed to it.


...

>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	struct kvm_vm *vm;
>> +
>> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_X86_APIC_BUS_CYCLES_NS));
>> +
>> +	vm = __vm_create(VM_SHAPE_DEFAULT, 1, 0);
>> +	vm_ioctl(vm, KVM_SET_TSC_KHZ, (void *)(TSC_HZ / 1000));
>> +	/*
>> +	 * KVM_CAP_X86_APIC_BUS_CYCLES_NS expects APIC bus clock rate in
>> +	 * nanoseconds and requires that no vCPU is created.
> 
> Meh, I'd drop this comment.  It should be quite obvious that the rate is in
> nanoseconds.  And instead of adding a comment for the vCPU creation, do
> __vm_enable_cap() again _after_ creating a vCPU and verify it fails with -EINVAL.

Done.

> 
>> +	 */
>> +	vm_enable_cap(vm, KVM_CAP_X86_APIC_BUS_CYCLES_NS,
>> +		      NSEC_PER_SEC / APIC_BUS_CLOCK_FREQ);
>> +	vcpu = vm_vcpu_add(vm, 0, guest_code);
>> +
>> +	virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA);
>> +
>> +	test_apic_bus_clock(vcpu);
>> +	kvm_vm_free(vm);
>> +}


Apart from my uncertainty surrounding CLI I believed that I am able to
address all your feedback with the resulting test looking as below. Is this
what you had in mind?

--->8---
From: Isaku Yamahata <isaku.yamahata@...el.com>
Subject: [PATCH] KVM: selftests: Add test for configure of x86 APIC bus frequency

Test if KVM emulates the APIC bus clock at the expected frequency when
userspace configures the frequency via KVM_CAP_X86_APIC_BUS_CYCLES_NS.

Set APIC timer's initial count to the maximum value and busy wait for 100
msec (largely arbitrary) using the TSC. Read the APIC timer's "current
count" to calculate the actual APIC bus clock frequency based on TSC
frequency.

Suggested-by: Sean Christopherson <seanjc@...gle.com>
Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
Co-developed-by: Reinette Chatre <reinette.chatre@...el.com>
Signed-off-by: Reinette Chatre <reinette.chatre@...el.com>
---
Changes v7:
- Drop Maxim Levitsky's Reviewed-by because of significant changes.
- Remove redefine of _GNU_SOURCE. (kernel test robot)
- Rewrite changelog and test description. (Sean)
- Do not set guest TSC frequency but instead discover it.
- Enable user space to set APIC bus frequency. (Sean)
- Use GUEST_ASSERT() from guest instead of TEST_ASSERT() from host. (Sean)
- Test xAPIC as well as x2APIC. (Sean)
- Add check that KVM_CAP_X86_APIC_BUS_CYCLES_NS cannot be set
   after vCPU created. (Sean)
- Remove unnecessary static functions from single file test.
- Be consistent in types by using uint32_t/uint64_t instead of
   u32/u64.

[SNIP older changes]
---
  tools/testing/selftests/kvm/Makefile          |   1 +
  .../selftests/kvm/include/x86_64/apic.h       |   7 +
  .../kvm/x86_64/apic_bus_clock_test.c          | 233 ++++++++++++++++++
  3 files changed, 241 insertions(+)
  create mode 100644 tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index ce8ff8e8ce3a..ad8b5d15f2bd 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -112,6 +112,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_invalid_nested_guest_state
  TEST_GEN_PROGS_x86_64 += x86_64/vmx_set_nested_state_test
  TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
  TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test
+TEST_GEN_PROGS_x86_64 += x86_64/apic_bus_clock_test
  TEST_GEN_PROGS_x86_64 += x86_64/xapic_ipi_test
  TEST_GEN_PROGS_x86_64 += x86_64/xapic_state_test
  TEST_GEN_PROGS_x86_64 += x86_64/xcr0_cpuid_test
diff --git a/tools/testing/selftests/kvm/include/x86_64/apic.h b/tools/testing/selftests/kvm/include/x86_64/apic.h
index bed316fdecd5..b0d2fc62e172 100644
--- a/tools/testing/selftests/kvm/include/x86_64/apic.h
+++ b/tools/testing/selftests/kvm/include/x86_64/apic.h
@@ -60,6 +60,13 @@
  #define		APIC_VECTOR_MASK	0x000FF
  #define	APIC_ICR2	0x310
  #define		SET_APIC_DEST_FIELD(x)	((x) << 24)
+#define APIC_LVT0	0x350
+#define		APIC_LVT_TIMER_ONESHOT		(0 << 17)
+#define		APIC_LVT_TIMER_PERIODIC		(1 << 17)
+#define		APIC_LVT_TIMER_TSCDEADLINE	(2 << 17)
+#define	APIC_TMICT	0x380
+#define	APIC_TMCCT	0x390
+#define	APIC_TDCR	0x3E0
  
  void apic_disable(void);
  void xapic_enable(void);
diff --git a/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c
new file mode 100644
index 000000000000..f2071c002bf5
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Intel Corporation
+ *
+ * Verify KVM correctly emulates the APIC bus frequency when the VMM configures
+ * the frequency via KVM_CAP_X86_APIC_BUS_CYCLES_NS.  Start the APIC timer by
+ * programming TMICT (timer initial count) to the largest value possible (so
+ * that the timer will not expire during the test).  Then, after an arbitrary
+ * amount of time has elapsed, verify TMCCT (timer current count) is within 1%
+ * of the expected value based on the time elapsed, the APIC bus frequency, and
+ * the programmed TDCR (timer divide configuration register).
+ */
+
+#include "apic.h"
+#include "test_util.h"
+
+/* Guest TSC frequency. Used to calibrate delays. */
+unsigned long tsc_hz;
+
+/*
+ * Pick 25MHz for APIC bus frequency. Different enough from the default 1GHz.
+ * User can override via command line.
+ */
+unsigned long apic_hz = 25 * 1000 * 1000;
+
+/*
+ * Possible TDCR values with matching divide count. Used to modify APIC
+ * timer frequency.
+ */
+struct {
+	uint32_t tdcr;
+	uint32_t divide_count;
+} tdcrs[] = {
+	{0x0, 2},
+	{0x1, 4},
+	{0x2, 8},
+	{0x3, 16},
+	{0x8, 32},
+	{0x9, 64},
+	{0xa, 128},
+	{0xb, 1},
+};
+
+void guest_verify(uint64_t tsc_cycles, uint32_t apic_cycles, uint32_t divide_count)
+{
+	uint64_t freq;
+
+	GUEST_ASSERT(tsc_cycles > 0);
+	freq = apic_cycles * divide_count * tsc_hz / tsc_cycles;
+	/* Check if measured frequency is within 1% of configured frequency. */
+	GUEST_ASSERT(freq < apic_hz * 101 / 100);
+	GUEST_ASSERT(freq > apic_hz * 99 / 100);
+}
+
+void x2apic_guest_code(void)
+{
+	uint32_t tmict, tmcct;
+	uint64_t tsc0, tsc1;
+	int i;
+
+	asm volatile("cli");
+
+	x2apic_enable();
+
+	/*
+	 * Setup one-shot timer.  The vector does not matter because the
+	 * interrupt should not fire.
+	 */
+	x2apic_write_reg(APIC_LVT0, APIC_LVT_TIMER_ONESHOT);
+
+	for (i = 0; i < ARRAY_SIZE(tdcrs); i++) {
+		x2apic_write_reg(APIC_TDCR, tdcrs[i].tdcr);
+
+		/* Set the largest value to not trigger the interrupt. */
+		tmict = ~0;
+		x2apic_write_reg(APIC_TMICT, tmict);
+
+		/* Busy wait for 100 msec. */
+		tsc0 = rdtsc();
+		tsc1 = tsc0;
+		while (tsc1 - tsc0 < tsc_hz / 1000 * 100)
+			tsc1 = rdtsc();
+
+		/* Read APIC timer and TSC. */
+		tmcct = x2apic_read_reg(APIC_TMCCT);
+		tsc1 = rdtsc();
+
+		/* Stop timer. */
+		x2apic_write_reg(APIC_TMICT, 0);
+
+		guest_verify(tsc1 - tsc0, tmict - tmcct, tdcrs[i].divide_count);
+	}
+
+	GUEST_DONE();
+}
+
+void xapic_guest_code(void)
+{
+	uint32_t tmict, tmcct;
+	uint64_t tsc0, tsc1;
+	int i;
+
+	asm volatile("cli");
+
+	xapic_enable();
+
+	/*
+	 * Setup one-shot timer.  The vector does not matter because the
+	 * interrupt should not fire.
+	 */
+	xapic_write_reg(APIC_LVT0, APIC_LVT_TIMER_ONESHOT);
+
+	for (i = 0; i < ARRAY_SIZE(tdcrs); i++) {
+		xapic_write_reg(APIC_TDCR, tdcrs[i].tdcr);
+
+		/* Set the largest value to not trigger the interrupt. */
+		tmict = ~0;
+		xapic_write_reg(APIC_TMICT, tmict);
+
+		/* Busy wait for 100 msec. */
+		tsc0 = rdtsc();
+		tsc1 = tsc0;
+		while (tsc1 - tsc0 < tsc_hz / 1000 * 100)
+			tsc1 = rdtsc();
+
+		/* Read APIC timer and TSC. */
+		tmcct = xapic_read_reg(APIC_TMCCT);
+		tsc1 = rdtsc();
+
+		/* Stop timer. */
+		xapic_write_reg(APIC_TMICT, 0);
+
+		guest_verify(tsc1 - tsc0, tmict - tmcct, tdcrs[i].divide_count);
+	}
+
+	GUEST_DONE();
+}
+
+void test_apic_bus_clock(struct kvm_vcpu *vcpu)
+{
+	bool done = false;
+	struct ucall uc;
+
+	while (!done) {
+		vcpu_run(vcpu);
+		TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
+
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_DONE:
+			done = true;
+			break;
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT(uc);
+			break;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+			break;
+		}
+	}
+}
+
+void run_apic_bus_clock_test(bool xapic)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_create(1);
+
+	tsc_hz = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL) * 1000;
+	sync_global_to_guest(vm, tsc_hz);
+	sync_global_to_guest(vm, apic_hz);
+
+	vm_enable_cap(vm, KVM_CAP_X86_APIC_BUS_CYCLES_NS,
+		      NSEC_PER_SEC / apic_hz);
+
+	vcpu = vm_vcpu_add(vm, 0, xapic ? xapic_guest_code : x2apic_guest_code);
+
+	ret = __vm_enable_cap(vm, KVM_CAP_X86_APIC_BUS_CYCLES_NS,
+			      NSEC_PER_SEC / apic_hz);
+	TEST_ASSERT(ret < 0 && errno == EINVAL,
+		    "Setting of APIC bus frequency after vCPU is created should fail.");
+
+	if (xapic)
+		virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA);
+
+	test_apic_bus_clock(vcpu);
+	kvm_vm_free(vm);
+}
+
+void run_xapic_bus_clock_test(void)
+{
+	run_apic_bus_clock_test(true);
+}
+
+void run_x2apic_bus_clock_test(void)
+{
+	run_apic_bus_clock_test(false);
+}
+
+void help(char *name)
+{
+	puts("");
+	printf("usage: %s [-h] [-a APIC bus freq]\n", name);
+	puts("");
+	printf("-a: The APIC bus frequency (in Hz) to be configured for the guest.\n");
+	puts("");
+}
+
+int main(int argc, char *argv[])
+{
+	int opt;
+
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_X86_APIC_BUS_CYCLES_NS));
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_GET_TSC_KHZ));
+
+	while ((opt = getopt(argc, argv, "a:h")) != -1) {
+		switch (opt) {
+		case 'a':
+			apic_hz = atol(optarg);
+			break;
+		case 'h':
+			help(argv[0]);
+			exit(0);
+		default:
+			help(argv[0]);
+			exit(1);
+		}
+	}
+
+	run_xapic_bus_clock_test();
+	run_x2apic_bus_clock_test();
+}
-- 
2.34.1

Powered by blists - more mailing lists