Re: [PATCH v1 3/3] KVM: selftests: Add a test case for KVM_X86_DISABLE_EXITS_HLT

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

Hi Muhammad Usama Anjum,

Thank you for reviewing my patch.

On 3/30/2024 1:43 AM, Muhammad Usama Anjum wrote:
> On 3/27/24 10:42 AM, Manali Shukla wrote:
>> By default, HLT instruction executed by guest is intercepted by hypervisor.
>> However, KVM_CAP_X86_DISABLE_EXITS capability can be used to not intercept
>> HLT by setting KVM_X86_DISABLE_EXITS_HLT.
>>
>> Add a test case to test KVM_X86_DISABLE_EXITS_HLT functionality.
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
> Thank you for the new test patch. We have been trying to ensure TAP
> conformance for tests which cannot be achieved if new tests aren't using
> TAP already. Please make your test TAP compliant.

As per my understanding about TAP interface, kvm_test_harness.h file includes a MACRO,
which is used to create VM with one vcpu using vm_create_with_one_vcpu(), but
halt_disable_exit_test creates a customized VM with KVM_CAP_X86_DISABLE_EXITS
capability set and different vm_shape parameters to start a VM without in-kernel
APIC support. AFAIU, I won't be able to use KVM_ONE_VCPU_TEST_SUITE MACRO as is.
How do you suggest to proceed with this issue? 

> 
>> ---
>>  tools/testing/selftests/kvm/Makefile          |   1 +
>>  .../kvm/x86_64/halt_disable_exit_test.c       | 113 ++++++++++++++++++
> Add generated object to .gitignore file.

Sure. I will do it.
> 
>>  2 files changed, 114 insertions(+)
>>  create mode 100644 tools/testing/selftests/kvm/x86_64/halt_disable_exit_test.c
>>
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index c75251d5c97c..9f72abb95d2e 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -89,6 +89,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/set_sregs_test
>>  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/halt_disable_exit_test
>>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_preemption_timer_test
>>  TEST_GEN_PROGS_x86_64 += x86_64/svm_vmcall_test
>>  TEST_GEN_PROGS_x86_64 += x86_64/svm_int_ctl_test
>> diff --git a/tools/testing/selftests/kvm/x86_64/halt_disable_exit_test.c b/tools/testing/selftests/kvm/x86_64/halt_disable_exit_test.c
>> new file mode 100644
>> index 000000000000..b7279dd0eaff
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/x86_64/halt_disable_exit_test.c
>> @@ -0,0 +1,113 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * KVM disable halt exit test
>> + *
>> + *  Copyright (C) 2024 Advanced Micro Devices, Inc.
>> + */
>> +#include <pthread.h>
>> +#include <signal.h>
>> +#include "kvm_util.h"
>> +#include "svm_util.h"
>> +#include "processor.h"
>> +#include "test_util.h"
>> +
>> +pthread_t task_thread, vcpu_thread;
>> +#define SIG_IPI SIGUSR1
>> +
>> +static void guest_code(uint8_t is_hlt_exec)
>> +{
>> +	while (!READ_ONCE(is_hlt_exec))
>> +		;
>> +
>> +	safe_halt();
>> +	GUEST_DONE();
>> +}
>> +
>> +static void *task_worker(void *arg)
>> +{
>> +	uint8_t *is_hlt_exec = (uint8_t *)arg;
>> +
>> +	usleep(1000);
>> +	WRITE_ONCE(*is_hlt_exec, 1);
>> +	pthread_kill(vcpu_thread, SIG_IPI);
>> +	return 0;
>> +}
>> +
>> +static void *vcpu_worker(void *arg)
>> +{
>> +	int ret;
>> +	int sig = -1;
>> +	uint8_t *is_hlt_exec = (uint8_t *)arg;
>> +	struct kvm_vm *vm;
>> +	struct kvm_run *run;
>> +	struct kvm_vcpu *vcpu;
>> +	struct kvm_signal_mask *sigmask = alloca(offsetof(struct kvm_signal_mask, sigset)
>> +						 + sizeof(sigset_t));
>> +	sigset_t *sigset = (sigset_t *) &sigmask->sigset;
>> +
>> +	/* Create a VM without in kernel APIC support */
>> +	vm = __vm_create(VM_SHAPE_DEFAULT, 1, 0, false);
>> +	vm_enable_cap(vm, KVM_CAP_X86_DISABLE_EXITS, KVM_X86_DISABLE_EXITS_HLT);
>> +	vcpu = vm_vcpu_add(vm, 0, guest_code);
>> +	vcpu_args_set(vcpu, 1, *is_hlt_exec);
>> +
>> +	/*
>> +	 * SIG_IPI is unblocked atomically while in KVM_RUN.  It causes the
>> +	 * ioctl to return with -EINTR, but it is still pending and we need
>> +	 * to accept it with the sigwait.
>> +	 */
>> +	sigmask->len = 8;
>> +	pthread_sigmask(0, NULL, sigset);
>> +	sigdelset(sigset, SIG_IPI);
>> +	vcpu_ioctl(vcpu, KVM_SET_SIGNAL_MASK, sigmask);
>> +	sigemptyset(sigset);
>> +	sigaddset(sigset, SIG_IPI);
>> +	run = vcpu->run;
>> +
>> +again:
>> +	ret = __vcpu_run(vcpu);
>> +	TEST_ASSERT_EQ(errno, EINTR);
>> +
>> +	if (ret == -1 && errno == EINTR) {
>> +		sigwait(sigset, &sig);
>> +		assert(sig == SIG_IPI);
>> +		TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_INTR);
>> +		goto again;
>> +	}
>> +
>> +	if (run->exit_reason == KVM_EXIT_HLT)
>> +		TEST_FAIL("Expected KVM_EXIT_INTR, got KVM_EXIT_HLT");
>> +
>> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>> +	kvm_vm_free(vm);
>> +	return 0;
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	int ret;
>> +	void *retval;
>> +	uint8_t is_halt_exec;
>> +	sigset_t sigset;
>> +
>> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_X86_DISABLE_EXITS));
>> +
>> +	/* Ensure that vCPU threads start with SIG_IPI blocked.  */
>> +	sigemptyset(&sigset);
>> +	sigaddset(&sigset, SIG_IPI);
>> +	pthread_sigmask(SIG_BLOCK, &sigset, NULL);
>> +
>> +	ret = pthread_create(&vcpu_thread, NULL, vcpu_worker, &is_halt_exec);
>> +	TEST_ASSERT(ret == 0, "pthread_create vcpu thread failed errno=%d", errno);
>> +
>> +	ret = pthread_create(&task_thread, NULL, task_worker, &is_halt_exec);
>> +	TEST_ASSERT(ret == 0, "pthread_create task thread failed errno=%d", errno);
>> +
>> +	pthread_join(vcpu_thread, &retval);
>> +	TEST_ASSERT(ret == 0, "pthread_join on vcpu thread failed with errno=%d", ret);
>> +
>> +	pthread_join(task_thread, &retval);
>> +	TEST_ASSERT(ret == 0, "pthread_join on task thread failed with errno=%d", ret);
>> +
>> +	return 0;
>> +}
> 
- Manali