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

[Sean Christopherson <seanjc@google.com>]

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
>