| Message ID | eac8c5e0431529282e7887aad0ba66506df28e9e.1714081726.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Make bus clock frequency for vAPIC timer configurable | expand |
On 4/25/2024 3:07 PM, Reinette Chatre wrote: > 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 Nit: some typos here? > +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); Use vm_create() instead?
Hi Zide, On 4/26/2024 4:06 PM, Chen, Zide wrote: > > > On 4/25/2024 3:07 PM, Reinette Chatre wrote: >> 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 > > Nit: some typos here? Apologies but this is not obvious to me. Could you please help by pointing out all those typos to me? > >> +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); > > Use vm_create() instead? Sure. This is easier on the eye while generating exactly the same code. 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 index 5100b28228af..6ed253875971 100644 --- a/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c +++ b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c @@ -149,7 +149,7 @@ int main(int argc, char *argv[]) TEST_REQUIRE(kvm_has_cap(KVM_CAP_X86_APIC_BUS_CYCLES_NS)); - vm = __vm_create(VM_SHAPE_DEFAULT, 1, 0); + vm = vm_create(1); vm_ioctl(vm, KVM_SET_TSC_KHZ, (void *)(TSC_HZ / 1000)); /* * KVM_CAP_X86_APIC_BUS_CYCLES_NS expects APIC bus clock rate in Reinette
On 4/26/2024 4:26 PM, Reinette Chatre wrote: > Hi Zide, > > On 4/26/2024 4:06 PM, Chen, Zide wrote: >> >> >> On 4/25/2024 3:07 PM, Reinette Chatre wrote: >>> 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 >> >> Nit: some typos here? > > Apologies but this is not obvious to me. Could you please help > by pointing out all those typos to me? Do you think it's more readable to add a ","? - * To verify if the APIC bus frequency can be configured this test starts + * To verify if the APIC bus frequency can be configured, this test starts * by setting the TSC frequency in KVM, and then:
Hi Zide, On 4/26/2024 10:38 PM, Chen, Zide wrote: > On 4/26/2024 4:26 PM, Reinette Chatre wrote: >> On 4/26/2024 4:06 PM, Chen, Zide wrote: >>> On 4/25/2024 3:07 PM, Reinette Chatre wrote: >>>> 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 >>> >>> Nit: some typos here? >> >> Apologies but this is not obvious to me. Could you please help >> by pointing out all those typos to me? > > Do you think it's more readable to add a ","? > > - * To verify if the APIC bus frequency can be configured this test starts > + * To verify if the APIC bus frequency can be configured, this test starts > * by setting the TSC frequency in KVM, and then: Sure. I'll do so in the next version. Thank you. Reinette
On Thu, Apr 25, 2024, Reinette Chatre wrote: > From: Isaku Yamahata <isaku.yamahata@intel.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. > 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). */ > + */ > +#define _GNU_SOURCE /* for program_invocation_short_name */ This can now be dropped. > +#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. > + */ > +#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 #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. > + > +/* > + * 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. > + 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. 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. > + /* > + * Setup one-shot timer. The vector does not matter because the > + * interrupt does not fire. _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 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_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; > + case UCALL_SYNC: { > + u32 divide_counter = uc.args[1]; > + u32 apic_cycles = uc.args[2]; > + u64 tsc_cycles = uc.args[3]; > + u64 freq; > + > + TEST_ASSERT(tsc_cycles > 0, > + "TSC cycles must not be zero."); > + > + /* Allow 1% slack. */ > + freq = apic_cycles * divide_counter * TSC_HZ / tsc_cycles; > + TEST_ASSERT(freq < APIC_BUS_CLOCK_FREQ * 101 / 100, > + "APIC bus clock frequency is too large"); > + TEST_ASSERT(freq > APIC_BUS_CLOCK_FREQ * 99 / 100, > + "APIC bus clock frequency is too small"); > + break; > + } > + default: > + TEST_FAIL("Unknown ucall %lu", uc.cmd); > + break; > + } > + } > +} > + > +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. > + */ > + 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); > +} > -- > 2.34.1 >
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@intel.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@intel.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@google.com> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> Co-developed-by: Reinette Chatre <reinette.chatre@intel.com> Signed-off-by: Reinette Chatre <reinette.chatre@intel.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(); +}
On Tue, Jun 04, 2024, Reinette Chatre wrote: > > > +/* > > > + * 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. Please add a utility. ARM and RISC-V already implement udelay(), and this isn't the first test that's wanted udelay() functionality. At the very least, it'd be nice to have for debug, e.g. to be able to insert artificial delay if a test is failing due to a suspected race suspected. I.e. this is likely an "if you build it, they will come" situations. > > 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. I doubt it. And if that really is the motiviation, that's a fools errand. This is _guest_ code. Disabling IRQs in the guest doesn't prevent host IRQs from being delivered, it only blocks virtual IRQs. And KVM selftests guests should never receive virtual IRQs unless the test itself explicitly sends them. > LVT entry is prevented from triggering because if the large configuration value. Yes and no. A large configuration value _should_ avoid a timer IRQ, but the entire point of this test is to verify KVM correctly emulates the timer. If this test does its job and finds a KVM bug that causes the timer to prematurely expire, then unmasking the LVT entry will generate an unexpected IRQ. Of course, the test doesn't configure a legal vector so the IRQ will never be delivered. We could fix that problem, but then a test failure would manifest as a hard-to-triage unexpected event, compared to an explicit TEST_ASSERT() on the timer value. That said, I'm not totally pposed to letting the guest die if KVM unexpectedly injects a timer IRQ, e.g. if all is well, it's a cheap way to provide a bit of extra coverage. On the other hand, masking the interrupt is even simpler, and the odds of false pass are low.
Hi Sean, On 6/5/24 7:22 AM, Sean Christopherson wrote: > On Tue, Jun 04, 2024, Reinette Chatre wrote: >>>> +/* >>>> + * 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. > > Please add a utility. ARM and RISC-V already implement udelay(), and this isn't > the first test that's wanted udelay() functionality. At the very least, it'd be > nice to have for debug, e.g. to be able to insert artificial delay if a test is > failing due to a suspected race suspected. I.e. this is likely an "if you build > it, they will come" situations. Will do. > >>> 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. > > I doubt it. And if that really is the motiviation, that's a fools errand. This > is _guest_ code. Disabling IRQs in the guest doesn't prevent host IRQs from being > delivered, it only blocks virtual IRQs. And KVM selftests guests should never > receive virtual IRQs unless the test itself explicitly sends them. Thank you for clarifying. > >> LVT entry is prevented from triggering because if the large configuration value. > > Yes and no. A large configuration value _should_ avoid a timer IRQ, but the > entire point of this test is to verify KVM correctly emulates the timer. If this > test does its job and finds a KVM bug that causes the timer to prematurely expire, > then unmasking the LVT entry will generate an unexpected IRQ. > > Of course, the test doesn't configure a legal vector so the IRQ will never be > delivered. We could fix that problem, but then a test failure would manifest as > a hard-to-triage unexpected event, compared to an explicit TEST_ASSERT() on the > timer value. > > That said, I'm not totally pposed to letting the guest die if KVM unexpectedly > injects a timer IRQ, e.g. if all is well, it's a cheap way to provide a bit of > extra coverage. On the other hand, masking the interrupt is even simpler, and > the odds of false pass are low. Understood. Will mask the interrupt in LVT entry as you suggested initially. While checking which bits to set I realized that the existing test was enabling oneshot mode in an entry where those bits are actually reserved. This is now fixed also. I plan to send the changes as next version of this work, with merged patches dropped from series. Reinette
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 871e2de3eb05..b65c7c88008a 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -111,6 +111,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..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). + */ +#define _GNU_SOURCE /* for program_invocation_short_name */ + +#include "apic.h" +#include "test_util.h" + +/* + * Pick one convenient value, 1.5GHz. No special meaning and different from + * the default value, 1GHz. + */ +#define TSC_HZ (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) + +/* + * Pick a typical value, 25MHz. Different enough from the default value, 1GHz. + */ +#define APIC_BUS_CLOCK_FREQ (25 * 1000 * 1000ULL) + +static void guest_code(void) +{ + /* + * Possible TDCR values and its divide count. Used to modify APIC + * timer frequency. + */ + struct { + u32 tdcr; + u32 divide_count; + } tdcrs[] = { + {0x0, 2}, + {0x1, 4}, + {0x2, 8}, + {0x3, 16}, + {0x8, 32}, + {0x9, 64}, + {0xa, 128}, + {0xb, 1}, + }; + + u32 tmict, tmcct; + u64 tsc0, tsc1; + int i; + + asm volatile("cli"); + + xapic_enable(); + + /* + * Setup one-shot timer. The vector does not matter because the + * interrupt does 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 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); + } + + 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; + case UCALL_SYNC: { + u32 divide_counter = uc.args[1]; + u32 apic_cycles = uc.args[2]; + u64 tsc_cycles = uc.args[3]; + u64 freq; + + TEST_ASSERT(tsc_cycles > 0, + "TSC cycles must not be zero."); + + /* Allow 1% slack. */ + freq = apic_cycles * divide_counter * TSC_HZ / tsc_cycles; + TEST_ASSERT(freq < APIC_BUS_CLOCK_FREQ * 101 / 100, + "APIC bus clock frequency is too large"); + TEST_ASSERT(freq > APIC_BUS_CLOCK_FREQ * 99 / 100, + "APIC bus clock frequency is too small"); + break; + } + default: + TEST_FAIL("Unknown ucall %lu", uc.cmd); + break; + } + } +} + +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. + */ + 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); +}