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

[Manali Shukla <manali.shukla@amd.com>]

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