Hi Neeraj, Thanks for reviewing my patches.
On 1/28/2025 1:34 PM, Neeraj Upadhyay wrote: > > > On 1/3/2025 1:48 PM, Manali Shukla wrote: >> From: Manali Shukla <[email protected]> >> >> The IPI HLT test simulates a scenario where a pending event is present >> while the HLT instruction is executed. >> >> Evaluates the idle HLT intercept feature of the AMD architecture, if > > Nit: Start as "The test evaluates idle HLT intercept feature ..." ? Sure. > >> available. If the feature is not present, this selftest can be extended > > If idle halt intercept feature is not present the test exercises > halt-exits/guest-entry for pending interrupts. So, maybe the test in > its current form is still useful if idle halt intercept feature is > not present. Is that understanding wrong? Cross-vCPU IPI or performance> > testing of halt exits are future extensions to this test. > Test in its current form is useful when idle halt intercept feature is not present. I will reword the commit message. >> in the future to include cross-vCPU IPI testing. >> >> Suggested-by: Sean Christopherson <[email protected]> >> Signed-off-by: Manali Shukla <[email protected]> >> --- >> tools/testing/selftests/kvm/Makefile.kvm | 1 + >> .../selftests/kvm/include/x86/processor.h | 1 + >> tools/testing/selftests/kvm/ipi_hlt_test.c | 83 +++++++++++++++++++ >> 3 files changed, 85 insertions(+) >> create mode 100644 tools/testing/selftests/kvm/ipi_hlt_test.c >> >> diff --git a/tools/testing/selftests/kvm/Makefile.kvm >> b/tools/testing/selftests/kvm/Makefile.kvm >> index 4277b983cace..d6eda8c19fed 100644 >> --- a/tools/testing/selftests/kvm/Makefile.kvm >> +++ b/tools/testing/selftests/kvm/Makefile.kvm >> @@ -135,6 +135,7 @@ TEST_GEN_PROGS_x86 += steal_time >> TEST_GEN_PROGS_x86 += kvm_binary_stats_test >> TEST_GEN_PROGS_x86 += system_counter_offset_test >> TEST_GEN_PROGS_x86 += pre_fault_memory_test >> +TEST_GEN_PROGS_x86 += ipi_hlt_test >> >> # Compiled outputs used by test targets >> TEST_GEN_PROGS_EXTENDED_x86 += x86/nx_huge_pages_test >> diff --git a/tools/testing/selftests/kvm/include/x86/processor.h >> b/tools/testing/selftests/kvm/include/x86/processor.h >> index 1f9798ed71f1..23a34a12be48 100644 >> --- a/tools/testing/selftests/kvm/include/x86/processor.h >> +++ b/tools/testing/selftests/kvm/include/x86/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/ipi_hlt_test.c >> b/tools/testing/selftests/kvm/ipi_hlt_test.c >> new file mode 100644 >> index 000000000000..09ed8011450f >> --- /dev/null >> +++ b/tools/testing/selftests/kvm/ipi_hlt_test.c >> @@ -0,0 +1,83 @@ >> +// 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 "apic.h" >> + >> +#define INTR_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. >> + */ >> + >> +static void guest_code(void) >> +{ >> + uint32_t icr_val; > > uint64_t ? Sure. > >> + int i; >> + >> + x2apic_enable(); >> + >> + icr_val = (APIC_DEST_SELF | APIC_INT_ASSERT | INTR_VECTOR); >> + >> + for (i = 0; i < NUM_ITERATIONS; i++) { >> + cli(); >> + x2apic_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) > > s/guest_vintr_handler/guest_intr_handler/ ? > Sure. > >> +{ >> + WRITE_ONCE(irq_received, true); >> + x2apic_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; > > Extra space Sure. > >> + >> + TEST_REQUIRE(kvm_has_cap(KVM_CAP_BINARY_STATS_FD)); >> + >> + vm = vm_create_with_one_vcpu(&vcpu, guest_code); >> + >> + vm_install_exception_handler(vm, INTR_VECTOR, guest_vintr_handler); >> + virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA); > > virt_pg_map() is not required for x2apic. So, remove? > I missed to remove it. I will remove it. >> + >> + vcpu_run(vcpu); >> + TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO); >> + >> + halt_exits = vcpu_get_stat(vcpu, halt_exits); >> + >> + switch (get_ucall(vcpu, &uc)) { >> + case UCALL_ABORT: >> + REPORT_GUEST_ASSERT(uc); >> + /* NOT REACHED */ >> + case UCALL_DONE: >> + break; >> + > > Nit: empty line not required? Sure. > > > - Neeraj > >> + default: >> + TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd); >> + } >> + >> + if (kvm_cpu_has(X86_FEATURE_IDLE_HLT)) >> + TEST_ASSERT_EQ(halt_exits, 0); >> + else >> + TEST_ASSERT_EQ(halt_exits, NUM_ITERATIONS); >> + >> + kvm_vm_free(vm); >> + return 0; >> +} > -Manali
