Re: [PATCH v4 4/4] KVM: selftests: KVM: SVM: Add Idle HLT intercept test

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.

+#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?

+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.

• 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.

• TEST_REQUIRE(this_cpu_has(X86_FEATURE_IDLE_HLT));

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.

• kvm_vm_free(vm);
• return 0;

+}

2.34.1