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
