| Message ID | 20241022054810.23369-5-manali.shukla@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for the Idle HLT intercept feature | expand |
Hi Sean, Thank you for reviewing my patches. On 12/20/2024 6:54 AM, Sean Christopherson wrote: > On Tue, Oct 22, 2024, Manali Shukla wrote: >> diff --git a/tools/testing/selftests/kvm/x86_64/svm_idle_hlt_test.c b/tools/testing/selftests/kvm/x86_64/svm_idle_hlt_test.c >> new file mode 100644 >> index 000000000000..fe2ea96695e4 >> --- /dev/null >> +++ b/tools/testing/selftests/kvm/x86_64/svm_idle_hlt_test.c >> @@ -0,0 +1,89 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2024 Advanced Micro Devices, Inc. >> + * >> + */ >> +#include <kvm_util.h> >> +#include <processor.h> >> +#include <test_util.h> >> +#include "svm_util.h" >> +#include "apic.h" >> + >> +#define VINTR_VECTOR 0x30 > > Drop the "V". From the guest's perspective, it's simply the interrupt vector. > The "V" suggests there's nested SVM stuff going on, e.g. to virtualize an interrupt > for L2 or something. Sure I will remove "V". > >> +#define NUM_ITERATIONS 1000 >> + >> +static bool irq_received; >> + >> +/* >> + * The guest code instruments the scenario where there is a V_INTR pending >> + * event available while hlt instruction is executed. The HLT VM Exit doesn't >> + * occur in above-mentioned scenario if Idle HLT intercept feature is enabled. >> + */ > > So the only thing thing that is idle-HLT specific in this test is that final > TEST_ASSERT_EQ(). Rather than make this test depend on idle-HLT, we should > tweak it run on all hardware, and then: > > if (kvm_cpu_has(X86_FEATURE_IDLE_HLT)) > TEST_ASSERT_EQ(halt_exits, 0); > else > TEST_ASSERT_EQ(halt_exits, NUM_ITERATIONS); > > Not sure about the name. Maybe hlt_ipi_test or ipi_hlt_test? Yeah makes sense. I will keep the name of the test as ipi_hlt_test and make the test common to run on all hardware. > >> +static void guest_code(void) >> +{ >> + uint32_t icr_val; >> + int i; >> + >> + xapic_enable(); > > Hmm, I think we should have this test force x2APIC mode. KVM emulates x2APIC > in software (if it's not accerlated by APICv), i.e. it's always available. That > will allow using this test to do performance testing of KVM's fastpath handling > of handle_fastpath_set_x2apic_icr_irqoff(). > > Of course, KVM only uses the fastpath for non-shorthand IPIs, and any setup that > can do self-IPI fully in the fastpath (via virtual interrupt delivery) won't exit > in the first place (virtualized by hardware), i.e. there's probably no point in > adding self-IPIs to the fastpath. > > But maybe in the future I can convince someone to enhance this test to do > cross-vCPU IPI testing. > Sure. I will make this test work with x2APIC mode. >> + >> + icr_val = (APIC_DEST_SELF | APIC_INT_ASSERT | VINTR_VECTOR); >> + >> + for (i = 0; i < NUM_ITERATIONS; i++) { >> + cli(); >> + xapic_write_reg(APIC_ICR, icr_val); >> + safe_halt(); >> + GUEST_ASSERT(READ_ONCE(irq_received)); >> + WRITE_ONCE(irq_received, false); >> + } >> + GUEST_DONE(); >> +} >> + >> +static void guest_vintr_handler(struct ex_regs *regs) >> +{ >> + WRITE_ONCE(irq_received, true); >> + xapic_write_reg(APIC_EOI, 0x00); >> +} >> + >> +int main(int argc, char *argv[]) >> +{ >> + struct kvm_vm *vm; >> + struct kvm_vcpu *vcpu; >> + struct ucall uc; >> + uint64_t halt_exits, vintr_exits; >> + >> + /* Check the extension for binary stats */ > > Pointless comment, the code below is self-explanatory. > Sure will remove the comment. >> + TEST_REQUIRE(this_cpu_has(X86_FEATURE_IDLE_HLT)); I think this is not needed, since I am planning to make this test independent of idle-HLT as per above comments. > > This needs to check *KVM* support. I.e. kvm_cpu_has(). > >> + TEST_REQUIRE(kvm_has_cap(KVM_CAP_BINARY_STATS_FD)); >> + >> + vm = vm_create_with_one_vcpu(&vcpu, guest_code); >> + >> + vm_install_exception_handler(vm, VINTR_VECTOR, guest_vintr_handler); >> + virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA); >> + >> + vcpu_run(vcpu); >> + TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO); >> + >> + halt_exits = vcpu_get_stat(vcpu, halt_exits); >> + vintr_exits = vcpu_get_stat(vcpu, irq_window_exits); >> + >> + switch (get_ucall(vcpu, &uc)) { >> + case UCALL_ABORT: >> + REPORT_GUEST_ASSERT(uc); >> + /* NOT REACHED */ >> + case UCALL_DONE: >> + break; >> + >> + default: >> + TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd); >> + } >> + >> + TEST_ASSERT_EQ(halt_exits, 0); >> + pr_debug("Guest executed VINTR followed by halts: %d times.\n" >> + "The guest exited due to halt: %ld times and number\n" >> + "of vintr exits: %ld.\n", >> + NUM_ITERATIONS, halt_exits, vintr_exits); > > halt_exits obviously is '0' at this point, so I don't see any point in printing > it out. > > As for vintr_exits, I vote to drop it, for now at least. At some point in the > future, I would like to expand this test so that it can be used for a rudimentary > IPI+HLT perf test. But for now, I think it makes sense to keep it simple, e.g. > so that nothing needs to be unwound if improvements are made in the future. > Sure I will remove this pr_debug for now. >> + >> + kvm_vm_free(vm); >> + return 0; >> +} >> -- >> 2.34.1 >> - Manali
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 960cf6a77198..25e4bc9473d5 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -94,6 +94,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/smaller_maxphyaddr_emulation_test TEST_GEN_PROGS_x86_64 += x86_64/smm_test TEST_GEN_PROGS_x86_64 += x86_64/state_test TEST_GEN_PROGS_x86_64 += x86_64/vmx_preemption_timer_test +TEST_GEN_PROGS_x86_64 += x86_64/svm_idle_hlt_test TEST_GEN_PROGS_x86_64 += x86_64/svm_vmcall_test TEST_GEN_PROGS_x86_64 += x86_64/svm_int_ctl_test TEST_GEN_PROGS_x86_64 += x86_64/svm_nested_shutdown_test diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h index 8e36de85b68f..41815d6ae500 100644 --- a/tools/testing/selftests/kvm/include/x86_64/processor.h +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h @@ -197,6 +197,7 @@ struct kvm_x86_cpu_feature { #define X86_FEATURE_PAUSEFILTER KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 10) #define X86_FEATURE_PFTHRESHOLD KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 12) #define X86_FEATURE_VGIF KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 16) +#define X86_FEATURE_IDLE_HLT KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 30) #define X86_FEATURE_SEV KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 1) #define X86_FEATURE_SEV_ES KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 3) diff --git a/tools/testing/selftests/kvm/x86_64/svm_idle_hlt_test.c b/tools/testing/selftests/kvm/x86_64/svm_idle_hlt_test.c new file mode 100644 index 000000000000..fe2ea96695e4 --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/svm_idle_hlt_test.c @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 Advanced Micro Devices, Inc. + * + */ +#include <kvm_util.h> +#include <processor.h> +#include <test_util.h> +#include "svm_util.h" +#include "apic.h" + +#define VINTR_VECTOR 0x30 +#define NUM_ITERATIONS 1000 + +static bool irq_received; + +/* + * The guest code instruments the scenario where there is a V_INTR pending + * event available while hlt instruction is executed. The HLT VM Exit doesn't + * occur in above-mentioned scenario if Idle HLT intercept feature is enabled. + */ + +static void guest_code(void) +{ + uint32_t icr_val; + int i; + + xapic_enable(); + + icr_val = (APIC_DEST_SELF | APIC_INT_ASSERT | VINTR_VECTOR); + + for (i = 0; i < NUM_ITERATIONS; i++) { + cli(); + xapic_write_reg(APIC_ICR, icr_val); + safe_halt(); + GUEST_ASSERT(READ_ONCE(irq_received)); + WRITE_ONCE(irq_received, false); + } + GUEST_DONE(); +} + +static void guest_vintr_handler(struct ex_regs *regs) +{ + WRITE_ONCE(irq_received, true); + xapic_write_reg(APIC_EOI, 0x00); +} + +int main(int argc, char *argv[]) +{ + struct kvm_vm *vm; + struct kvm_vcpu *vcpu; + struct ucall uc; + uint64_t halt_exits, vintr_exits; + + /* Check the extension for binary stats */ + TEST_REQUIRE(this_cpu_has(X86_FEATURE_IDLE_HLT)); + TEST_REQUIRE(kvm_has_cap(KVM_CAP_BINARY_STATS_FD)); + + vm = vm_create_with_one_vcpu(&vcpu, guest_code); + + vm_install_exception_handler(vm, VINTR_VECTOR, guest_vintr_handler); + virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA); + + vcpu_run(vcpu); + TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO); + + halt_exits = vcpu_get_stat(vcpu, halt_exits); + vintr_exits = vcpu_get_stat(vcpu, irq_window_exits); + + switch (get_ucall(vcpu, &uc)) { + case UCALL_ABORT: + REPORT_GUEST_ASSERT(uc); + /* NOT REACHED */ + case UCALL_DONE: + break; + + default: + TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd); + } + + TEST_ASSERT_EQ(halt_exits, 0); + pr_debug("Guest executed VINTR followed by halts: %d times.\n" + "The guest exited due to halt: %ld times and number\n" + "of vintr exits: %ld.\n", + NUM_ITERATIONS, halt_exits, vintr_exits); + + kvm_vm_free(vm); + return 0; +}