Hi Sean,
Thank you for reviewing my patches.
On 3/7/2024 11:52 PM, Sean Christopherson wrote:
> On Thu, Mar 07, 2024, Manali Shukla wrote:
>> From: Manali Shukla <Manali.Shukla@xxxxxxx>
>>
>> The Execution of the HLT instruction results in a VMEXIT. Hypervisor
>> observes pending V_INTR and V_NMI events just after the VMEXIT
>> generated by the HLT for the vCPU and causes VM entry to service the
>> pending events. The Idle HLT intercept feature avoids the wasteful
>> VMEXIT during the halt if there are pending V_INTR and V_NMI events
>> for the vCPU.
>>
>> The selftest for the Idle HLT intercept instruments the above-mentioned
>> scenario.
>>
>> Signed-off-by: Manali Shukla <Manali.Shukla@xxxxxxx>
>> ---
>> tools/testing/selftests/kvm/Makefile | 1 +
>> .../selftests/kvm/x86_64/svm_idlehlt_test.c | 118 ++++++++++++++++++
>> 2 files changed, 119 insertions(+)
>> create mode 100644 tools/testing/selftests/kvm/x86_64/svm_idlehlt_test.c
>>
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index 492e937fab00..7ad03dc4f35e 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -89,6 +89,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/smaller_maxphyaddr_emulation_test
>> TEST_GEN_PROGS_x86_64 += x86_64/smm_test
>> TEST_GEN_PROGS_x86_64 += x86_64/state_test
>> TEST_GEN_PROGS_x86_64 += x86_64/vmx_preemption_timer_test
>> +TEST_GEN_PROGS_x86_64 += x86_64/svm_idlehlt_test
>
> Uber nit, maybe svm_idle_hlt_test? I find "idlehlt" hard to parse.
Sure I will change it to svm_idle_hlt_test.
>
>> TEST_GEN_PROGS_x86_64 += x86_64/svm_vmcall_test
>> TEST_GEN_PROGS_x86_64 += x86_64/svm_int_ctl_test
>> TEST_GEN_PROGS_x86_64 += x86_64/svm_nested_shutdown_test
>> diff --git a/tools/testing/selftests/kvm/x86_64/svm_idlehlt_test.c b/tools/testing/selftests/kvm/x86_64/svm_idlehlt_test.c
>> new file mode 100644
>> index 000000000000..1564511799d4
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/x86_64/svm_idlehlt_test.c
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * svm_idlehalt_test
>> + *
>
> Please omit this, file comments that state the name of the test inevitably
> become stale (see above).
Sure. I will remove it.
>
>> + * Copyright (C) 2024 Advanced Micro Devices, Inc.
>> + *
>> + * For licencing details see kernel-base/COPYING
>
> This seems gratuitous, doesn't the SPDX stuff take care this?
Agreed, I will remove this.
>
>> + *
>> + * Author:
>> + * Manali Shukla <manali.shukla@xxxxxxx>
>> + */
>> +#include "kvm_util.h"
>> +#include "svm_util.h"
>> +#include "processor.h"
>> +#include "test_util.h"
>> +#include "apic.h"
>> +
>> +#define VINTR_VECTOR 0x30
>> +#define NUM_ITERATIONS 100000
>
> What's the runtime? If it's less than a second, then whatever, but if it's at
> all longer than that, then I'd prefer to use a lower default and make this user-
> configurable.
It takes ~34s to run this test.
>
>> +/*
>> + * Incremented in the VINTR handler. Provides evidence to the sender that the
>> + * VINR is arrived at the destination.
>
> Evidence is useless if there's no detective looking for it. Yeah, it gets
> printed out in the end, but in reality, no one is going to look at that.
>
> Rather than read this from the host, just make it a non-volatile bool and assert
> that it set after every
>
Sure. I can do that.
>> + */
>> +static volatile uint64_t vintr_rcvd;
>> +
>> +void verify_apic_base_addr(void)
>> +{
>> + uint64_t msr = rdmsr(MSR_IA32_APICBASE);
>> + uint64_t base = GET_APIC_BASE(msr);
>> +
>> + GUEST_ASSERT(base == APIC_DEFAULT_GPA);
>> +}
>> +
>> +/*
>> + * The halting 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 the Idle HLT intercept feature is enabled.
>> + */
>> +
>> +static void halter_guest_code(void)
>
> Just "guest_code()". Yeah, it's a weird generic name, but at this point it's so
> ubiquitous that it's analogous to main(), i.e. readers know that guest_code() is
> the entry point. And deviating from that suggests that there is a second vCPU
> running _other_ guest code (otherwise, why differentiate?), which isn't the case.
>
Sure.
>> +{
>> + uint32_t icr_val;
>> + int i;
>> +
>> + verify_apic_base_addr();
>
> Why? The test will fail if the APIC is borked, this is just unnecessary noise
> that distracts readers.
>
> Sure. I will remove it in V2.
>> + xapic_enable();
>> +
>> + icr_val = (APIC_DEST_SELF | APIC_INT_ASSERT | VINTR_VECTOR);
>> +
>> + for (i = 0; i < NUM_ITERATIONS; i++) {
>> + xapic_write_reg(APIC_ICR, icr_val);
>> + asm volatile("sti; hlt; cli");
>
> Please add safe_halt() and cli() helpers in processor.h. And then do:
>
> cli();
> xapic_write_reg(APIC_ICR, icr_val);
> safe_halt();
>
> to guarantee that interrupts are disabled when the IPI is sent. And as above,
>
> GUEST_ASSERT(READ_ONCE(irq_received));
> WRITE_ONCE(irq_received, false);
>
Sure.
>> + }
>> + GUEST_DONE();
>> +}
>> +
>> +static void guest_vintr_handler(struct ex_regs *regs)
>> +{
>> + vintr_rcvd++;
>> + xapic_write_reg(APIC_EOI, 0x30);
>
> EOI is typically written with '0', not the vector, because the legacy EOI register
> clears the highest ISR vector, not whatever is specified. IIRC, one of the Intel
> or AMD specs even says to use '0'.
>
> AMD's Specific EOI register does allow targeting a specific vector, but that's
> not what's being used here.
Sure.
>
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + struct kvm_vm *vm;
>> + struct kvm_vcpu *vcpu;
>> + struct ucall uc;
>> + uint64_t halt_exits, vintr_exits;
>> + uint64_t *pvintr_rcvd;
>> +
>> + TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
>
> No, this test doesn't require SVM, which is KVM advertising *nested* SVM. This
> test does require idle-hlt support though...
Sure. I will add it in V2.
>
>> + /* Check the extension for binary stats */
>> + TEST_REQUIRE(kvm_has_cap(KVM_CAP_BINARY_STATS_FD));
>> +
>> + vm = vm_create_with_one_vcpu(&vcpu, halter_guest_code);
>> +
>> + vm_init_descriptor_tables(vm);
>> + vcpu_init_descriptor_tables(vcpu);
>> + 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");
>
> Is there really no way to get binary stats without having to pass in strings?
I could see one of the test case is passing the strings to get binary stats of
vm. That is why I used strings to get binary stats of vcpu. I will try to find another
way to get the binary stats.
>
>> + vintr_exits = vcpu_get_stat(vcpu, "irq_window_exits");
>> + pvintr_rcvd = (uint64_t *)addr_gva2hva(vm, (uint64_t)&vintr_rcvd);
>> +
>> + switch (get_ucall(vcpu, &uc)) {
>> + case UCALL_ABORT:
>> + REPORT_GUEST_ASSERT(uc);
>> + /* NOT REACHED */
>
> Eh, just put a "break;" here and drop the comment.
>
Sure.
>> + case UCALL_DONE:
>> + goto done;
>
> break;
>
>> + default:
>> + TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
>> + }
>> +
>> +done:
>> + TEST_ASSERT(halt_exits == 0,
>
> So in all honesty, this isn't a very interesting test. It's more of a CPU test
> than a KVM test. I do think it's worth adding, because why not.
>
> But I'd also like to see a testcase for KVM_X86_DISABLE_EXITS_HLT. It would be
> a generic test, i.e. not specific to idle-hlt since there is no dependency and
> the test shouldn't care. I _think_ it would be fairly straightforward: create
> a VM without an in-kernel APIC (so that HLT exits to userspace), disable HLT
> interception, send a signal from a different task after some delay, and execute
> HLT in the guest. Then verify the vCPU exited because of -EINTR and not HLT.
I will create this test.
>
>> + "Test Failed:\n"
>> + "Guest executed VINTR followed by halts: %d times\n"
>> + "The guest exited due to halt: %ld times and number\n"
>> + "of vintr exits: %ld and vintr got re-injected: %ld times\n",
>> + NUM_ITERATIONS, halt_exits, vintr_exits, *pvintr_rcvd);
>
> I appreciate the effort to provide more info, but this is way too noisy. If
> anything, print gory details in a pr_debug() *before* the assert (see below),
> and then simply do:
>
> TEST_ASSERT_EQ(halt_exits, 0);
>
Sure.
>> + fprintf(stderr,
>> + "Test Successful:\n"
>> + "Guest executed VINTR followed by halts: %d times\n"
>> + "The guest exited due to halt: %ld times and number\n"
>> + "of vintr exits: %ld and vintr got re-injected: %ld times\n",
>> + NUM_ITERATIONS, halt_exits, vintr_exits, *pvintr_rcvd);
>
> And this should be pr_debug(), because no human is going to look at this except
> when the test isn't working correctly.
I will change it to pr_debug() in V2.
>
>> +
>> + kvm_vm_free(vm);
>> + return 0;
>> +}
>> --
>> 2.34.1
>> /pvintr_rcvd
>
