Re: [PATCH v5 3/3] KVM: selftests: Add self IPI HLT test

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

Hi Neeraj,

Thanks for reviewing my patches.

On 1/28/2025 1:34 PM, Neeraj Upadhyay wrote:
> 
> 
> On 1/3/2025 1:48 PM, Manali Shukla wrote:
>> From: Manali Shukla <Manali.Shukla@amd.com>
>>
>> The IPI HLT test simulates a scenario where a pending event is present
>> while the HLT instruction is executed.
>>
>> Evaluates the idle HLT intercept feature of the AMD architecture, if
> 
> Nit: Start as "The test evaluates idle HLT intercept feature ..." ?

Sure.

> 
>> available. If the feature is not present, this selftest can be extended
> 
> If idle halt intercept feature is not present the test exercises
> halt-exits/guest-entry for pending interrupts. So, maybe the test in
> its current form is still useful if idle halt intercept feature is
> not present. Is that understanding wrong? Cross-vCPU IPI or performance> testing of halt exits are future extensions to this test.
> 

Test in its current form is useful when idle halt intercept feature is
not present. I will reword the commit message.


>> in the future to include cross-vCPU IPI testing.
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Manali Shukla <Manali.Shukla@amd.com>
>> ---
>>  tools/testing/selftests/kvm/Makefile.kvm      |  1 +
>>  .../selftests/kvm/include/x86/processor.h     |  1 +
>>  tools/testing/selftests/kvm/ipi_hlt_test.c    | 83 +++++++++++++++++++
>>  3 files changed, 85 insertions(+)
>>  create mode 100644 tools/testing/selftests/kvm/ipi_hlt_test.c
>>
>> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
>> index 4277b983cace..d6eda8c19fed 100644
>> --- a/tools/testing/selftests/kvm/Makefile.kvm
>> +++ b/tools/testing/selftests/kvm/Makefile.kvm
>> @@ -135,6 +135,7 @@ TEST_GEN_PROGS_x86 += steal_time
>>  TEST_GEN_PROGS_x86 += kvm_binary_stats_test
>>  TEST_GEN_PROGS_x86 += system_counter_offset_test
>>  TEST_GEN_PROGS_x86 += pre_fault_memory_test
>> +TEST_GEN_PROGS_x86 += ipi_hlt_test
>>  
>>  # Compiled outputs used by test targets
>>  TEST_GEN_PROGS_EXTENDED_x86 += x86/nx_huge_pages_test
>> diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h
>> index 1f9798ed71f1..23a34a12be48 100644
>> --- a/tools/testing/selftests/kvm/include/x86/processor.h
>> +++ b/tools/testing/selftests/kvm/include/x86/processor.h
>> @@ -197,6 +197,7 @@ struct kvm_x86_cpu_feature {
>>  #define X86_FEATURE_PAUSEFILTER         KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 10)
>>  #define X86_FEATURE_PFTHRESHOLD         KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 12)
>>  #define	X86_FEATURE_VGIF		KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 16)
>> +#define X86_FEATURE_IDLE_HLT		KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 30)
>>  #define X86_FEATURE_SEV			KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 1)
>>  #define X86_FEATURE_SEV_ES		KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 3)
>>  
>> diff --git a/tools/testing/selftests/kvm/ipi_hlt_test.c b/tools/testing/selftests/kvm/ipi_hlt_test.c
>> new file mode 100644
>> index 000000000000..09ed8011450f
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/ipi_hlt_test.c
>> @@ -0,0 +1,83 @@
>> +// 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 "apic.h"
>> +
>> +#define INTR_VECTOR     0x30
>> +#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.
>> + */
>> +
>> +static void guest_code(void)
>> +{
>> +	uint32_t icr_val;
> 
> uint64_t ?

Sure.

> 
>> +	int i;
>> +
>> +	x2apic_enable();
>> +
>> +	icr_val = (APIC_DEST_SELF | APIC_INT_ASSERT | INTR_VECTOR);
>> +
>> +	for (i = 0; i < NUM_ITERATIONS; i++) {
>> +		cli();
>> +		x2apic_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)
> 
> s/guest_vintr_handler/guest_intr_handler/ ?
> 

Sure.

> 
>> +{
>> +	WRITE_ONCE(irq_received, true);
>> +	x2apic_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;
> 
> Extra space

Sure.
> 
>> +
>> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_BINARY_STATS_FD));
>> +
>> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
>> +
>> +	vm_install_exception_handler(vm, INTR_VECTOR, guest_vintr_handler);
>> +	virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA);
> 
> virt_pg_map() is not required for x2apic. So, remove?
> 

I missed to remove it. I will remove it.

>> +
>> +	vcpu_run(vcpu);
>> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>> +
>> +	halt_exits = vcpu_get_stat(vcpu, halt_exits);
>> +
>> +	switch (get_ucall(vcpu, &uc)) {
>> +	case UCALL_ABORT:
>> +		REPORT_GUEST_ASSERT(uc);
>> +		/* NOT REACHED */
>> +	case UCALL_DONE:
>> +		break;
>> +
> 
> Nit: empty line not required?

Sure.

> 
> 
> - Neeraj
> 
>> +	default:
>> +		TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
>> +	}
>> +
>> +	if (kvm_cpu_has(X86_FEATURE_IDLE_HLT))
>> +		TEST_ASSERT_EQ(halt_exits, 0);
>> +	else
>> +		TEST_ASSERT_EQ(halt_exits, NUM_ITERATIONS);
>> +
>> +	kvm_vm_free(vm);
>> +	return 0;
>> +}
> 

-Manali