Re: [PATCH] selftests: kvm: revamp MONITOR/MWAIT tests

[Paolo Bonzini <pbonzini@redhat.com>]

On 3/20/25 17:52, Paolo Bonzini wrote:
> Run each testcase in a separate VMs to cover more possibilities;
> move WRMSR close to MONITOR/MWAIT to test updating CPUID bits
> while in the VM.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Queued now.

Paolo

> ---
>   .../selftests/kvm/x86/monitor_mwait_test.c    | 108 +++++++++---------
>   1 file changed, 57 insertions(+), 51 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86/monitor_mwait_test.c b/tools/testing/selftests/kvm/x86/monitor_mwait_test.c
> index 2b550eff35f1..390ae2d87493 100644
> --- a/tools/testing/selftests/kvm/x86/monitor_mwait_test.c
> +++ b/tools/testing/selftests/kvm/x86/monitor_mwait_test.c
> @@ -7,6 +7,7 @@
>   
>   #include "kvm_util.h"
>   #include "processor.h"
> +#include "kselftest.h"
>   
>   #define CPUID_MWAIT (1u << 3)
>   
> @@ -14,6 +15,8 @@ enum monitor_mwait_testcases {
>   	MWAIT_QUIRK_DISABLED = BIT(0),
>   	MISC_ENABLES_QUIRK_DISABLED = BIT(1),
>   	MWAIT_DISABLED = BIT(2),
> +	CPUID_DISABLED = BIT(3),
> +	TEST_MAX = CPUID_DISABLED * 2 - 1,
>   };
>   
>   /*
> @@ -35,11 +38,19 @@ do {									\
>   			       testcase, vector);			\
>   } while (0)
>   
> -static void guest_monitor_wait(int testcase)
> +static void guest_monitor_wait(void *arg)
>   {
> +	int testcase = (int) (long) arg;
>   	u8 vector;
>   
> -	GUEST_SYNC(testcase);
> +	u64 val = rdmsr(MSR_IA32_MISC_ENABLE) & ~MSR_IA32_MISC_ENABLE_MWAIT;
> +	if (!(testcase & MWAIT_DISABLED))
> +		val |= MSR_IA32_MISC_ENABLE_MWAIT;
> +	wrmsr(MSR_IA32_MISC_ENABLE, val);
> +
> +	__GUEST_ASSERT(this_cpu_has(X86_FEATURE_MWAIT) == !(testcase & MWAIT_DISABLED),
> +		       "Expected CPUID.MWAIT %s\n",
> +		       (testcase & MWAIT_DISABLED) ? "cleared" : "set");
>   
>   	/*
>   	 * Arbitrarily MONITOR this function, SVM performs fault checks before
> @@ -50,19 +61,6 @@ static void guest_monitor_wait(int testcase)
>   
>   	vector = kvm_asm_safe("mwait", "a"(guest_monitor_wait), "c"(0), "d"(0));
>   	GUEST_ASSERT_MONITOR_MWAIT("MWAIT", testcase, vector);
> -}
> -
> -static void guest_code(void)
> -{
> -	guest_monitor_wait(MWAIT_DISABLED);
> -
> -	guest_monitor_wait(MWAIT_QUIRK_DISABLED | MWAIT_DISABLED);
> -
> -	guest_monitor_wait(MISC_ENABLES_QUIRK_DISABLED | MWAIT_DISABLED);
> -	guest_monitor_wait(MISC_ENABLES_QUIRK_DISABLED);
> -
> -	guest_monitor_wait(MISC_ENABLES_QUIRK_DISABLED | MWAIT_QUIRK_DISABLED | MWAIT_DISABLED);
> -	guest_monitor_wait(MISC_ENABLES_QUIRK_DISABLED | MWAIT_QUIRK_DISABLED);
>   
>   	GUEST_DONE();
>   }
> @@ -74,56 +72,64 @@ int main(int argc, char *argv[])
>   	struct kvm_vm *vm;
>   	struct ucall uc;
>   	int testcase;
> +	char test[80];
>   
> -	TEST_REQUIRE(this_cpu_has(X86_FEATURE_MWAIT));
>   	TEST_REQUIRE(kvm_has_cap(KVM_CAP_DISABLE_QUIRKS2));
>   
> -	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> -	vcpu_clear_cpuid_feature(vcpu, X86_FEATURE_MWAIT);
> +	ksft_print_header();
> +	ksft_set_plan(12);
> +	for (testcase = 0; testcase <= TEST_MAX; testcase++) {
> +		vm = vm_create_with_one_vcpu(&vcpu, guest_monitor_wait);
> +		vcpu_args_set(vcpu, 1, (void *)(long)testcase);
> +
> +		disabled_quirks = 0;
> +		if (testcase & MWAIT_QUIRK_DISABLED) {
> +			disabled_quirks |= KVM_X86_QUIRK_MWAIT_NEVER_UD_FAULTS;
> +			strcpy(test, "MWAIT can fault");
> +		} else {
> +			strcpy(test, "MWAIT never faults");
> +		}
> +		if (testcase & MISC_ENABLES_QUIRK_DISABLED) {
> +			disabled_quirks |= KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT;
> +			strcat(test, ", MISC_ENABLE updates CPUID");
> +		} else {
> +			strcat(test, ", no CPUID updates");
> +		}
> +
> +		vm_enable_cap(vm, KVM_CAP_DISABLE_QUIRKS2, disabled_quirks);
> +
> +		if (!(testcase & MISC_ENABLES_QUIRK_DISABLED) &&
> +		    (!!(testcase & CPUID_DISABLED) ^ !!(testcase & MWAIT_DISABLED)))
> +			continue;
> +
> +		if (testcase & CPUID_DISABLED) {
> +			strcat(test, ", CPUID clear");
> +			vcpu_clear_cpuid_feature(vcpu, X86_FEATURE_MWAIT);
> +		} else {
> +			strcat(test, ", CPUID set");
> +			vcpu_set_cpuid_feature(vcpu, X86_FEATURE_MWAIT);
> +		}
> +
> +		if (testcase & MWAIT_DISABLED)
> +			strcat(test, ", MWAIT disabled");
>   
> -	while (1) {
>   		vcpu_run(vcpu);
>   		TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>   
>   		switch (get_ucall(vcpu, &uc)) {
> -		case UCALL_SYNC:
> -			testcase = uc.args[1];
> -			break;
>   		case UCALL_ABORT:
> -			REPORT_GUEST_ASSERT(uc);
> -			goto done;
> +			/* Detected in vcpu_run */
> +			break;
>   		case UCALL_DONE:
> -			goto done;
> +			ksft_test_result_pass("%s\n", test);
> +			break;
>   		default:
>   			TEST_FAIL("Unknown ucall %lu", uc.cmd);
> -			goto done;
> +			break;
>   		}
> -
> -		disabled_quirks = 0;
> -		if (testcase & MWAIT_QUIRK_DISABLED)
> -			disabled_quirks |= KVM_X86_QUIRK_MWAIT_NEVER_UD_FAULTS;
> -		if (testcase & MISC_ENABLES_QUIRK_DISABLED)
> -			disabled_quirks |= KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT;
> -		vm_enable_cap(vm, KVM_CAP_DISABLE_QUIRKS2, disabled_quirks);
> -
> -		/*
> -		 * If the MISC_ENABLES quirk (KVM neglects to update CPUID to
> -		 * enable/disable MWAIT) is disabled, toggle the ENABLE_MWAIT
> -		 * bit in MISC_ENABLES accordingly.  If the quirk is enabled,
> -		 * the only valid configuration is MWAIT disabled, as CPUID
> -		 * can't be manually changed after running the vCPU.
> -		 */
> -		if (!(testcase & MISC_ENABLES_QUIRK_DISABLED)) {
> -			TEST_ASSERT(testcase & MWAIT_DISABLED,
> -				    "Can't toggle CPUID features after running vCPU");
> -			continue;
> -		}
> -
> -		vcpu_set_msr(vcpu, MSR_IA32_MISC_ENABLE,
> -			     (testcase & MWAIT_DISABLED) ? 0 : MSR_IA32_MISC_ENABLE_MWAIT);
> +		kvm_vm_free(vm);
>   	}
> +	ksft_finished();
>   
> -done:
> -	kvm_vm_free(vm);
>   	return 0;
>   }