Re: [PATCH 4/4] KVM: selftests: Update nSVM EFER test to expect #GP when clearing SVME in L2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] KVM: selftests: l1_clear_efer_svme() misses L2 exit and completion checks
- [Medium] KVM: selftests: test_host_initiated_lmsle() not updated for EFER CPUID check
- [Low] KVM: selftests: l2_clear_efer_svme() assert message says EFER.LME not SVME
--

commit bfd1afa646e17ebc8c26cbfd62dcc99d73a0c1f3
Author: Sean Christopherson <seanjc@google.com>

KVM: selftests: Update nSVM EFER test to expect #GP when clearing SVME in L2

This commit updates svm_nested_efer_test to expect a #GP fault in L2 when
clearing EFER.SVME via WRMSR instead of a triple-fault shutdown of L1, and
factors out a helper to run the guest EFER test cases.

> diff --git a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c b/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
> index fa0d76a9c390b..f612146d23ac2 100644
> --- a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
> +++ b/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
> @@ -15,13 +15,18 @@ static bool l2_ran;
>  static void l2_clear_efer_svme(void)
>  {
>  	u64 efer = rdmsr(MSR_EFER);
> +	u8 vector;
>  
>  	/* generic_svm_setup() initializes EFER_SVME set for L2 */
>  	GUEST_ASSERT(efer & EFER_SVME);
> -	wrmsr(MSR_EFER, efer & ~EFER_SVME);
>  
> -	/* Unreachable, L1 should be shutdown */
> -	GUEST_ASSERT(0);
> +	vector = wrmsr_safe(MSR_EFER, efer & ~EFER_SVME);
> +	__GUEST_ASSERT(vector == GP_VECTOR,
> +		       "Expected #GP when clearing EFER.LME in L2, got %s",
> +		       ex_str(vector));

[Severity: Low]
In l2_clear_efer_svme(), should the __GUEST_ASSERT() format string say
EFER.SVME instead of EFER.LME when wrmsr_safe(MSR_EFER, efer & ~EFER_SVME)
returns a vector other than GP_VECTOR?

> +
> +	l2_ran = true;
> +	vmmcall();
>  }
>  
>  static void l1_clear_efer_svme(struct svm_test_data *svm)
> @@ -29,8 +34,7 @@ static void l1_clear_efer_svme(struct svm_test_data *svm)
>  	generic_svm_setup(svm, l2_clear_efer_svme);
>  	run_guest(svm->vmcb, svm->vmcb_gpa);
>  
> -	/* Unreachable, L1 should be shutdown */
> -	GUEST_ASSERT(0);
> +	GUEST_DONE();
>  }

[Severity: Medium]
When test_clear_efer_svme() runs l1_clear_efer_svme(), run_guest() enters
l2_clear_efer_svme(), which sets l2_ran = true and executes vmmcall().
However, when run_guest() returns to l1_clear_efer_svme(), it immediately
calls GUEST_DONE() without checking l2_ran or svm->vmcb->control.exit_code
(unlike l1_lmsle()):

tools/testing/selftests/kvm/x86/svm_nested_efer_test.c:l1_lmsle() {
	...
	} else {
		GUEST_ASSERT_EQ(vmcb->control.exit_code, SVM_EXIT_VMMCALL);
		GUEST_ASSERT(l2_ran);
		GUEST_ASSERT(vmcb->save.efer & EFER_LMSLE);
	}
}

If VMRUN fails or if clearing EFER.SVME in L2 triggers a nested VM-Exit to L1
instead of injecting #GP into L2, wouldn't run_guest() return to
l1_clear_efer_svme() with l2_ran == false and still pass the test? Also,
should l2_clear_efer_svme() re-read MSR_EFER after wrmsr_safe() to verify
that EFER_SVME remained set?

[ ... ]

> @@ -248,7 +257,7 @@ int main(int argc, char *argv[])
>  		test_clear_efer_svme();
>  		test_lmsle(true);
>  	} else {
> -		ksft_test_result_skip("L2 clearing EFER.SVME shuts down L1\n");
> +		ksft_test_result_skip("L2 clearing EFER.SVME gets #GP\n");
>  		ksft_test_result_skip("Guest EFER_LMSLE_MBZ=1\n");
>  	}
>  

[Severity: Medium]
Earlier in this series, commit f8fc3a0796b0 ("Revert \"KVM: x86: Skip EFER
vs. guest CPUID checks for host-initiated writes\"") updated set_efer() in
arch/x86/kvm/msrs.c to enforce kvm_valid_efer() unconditionally on all
MSR_EFER writes, including host-initiated writes:

arch/x86/kvm/msrs.c:set_efer() {
	...
	if (!kvm_valid_efer(vcpu, efer))
		return 1;
	...
}

Because kvm_valid_efer() rejects EFER_LMSLE when X86_FEATURE_EFER_LMSLE_MBZ
is set in guest CPUID, won't main() -> test_host_initiated_lmsle() in
tools/testing/selftests/kvm/x86/svm_nested_efer_test.c now fail on hosts
where has_lmsle is true?

tools/testing/selftests/kvm/x86/svm_nested_efer_test.c:test_host_initiated_lmsle() {
	...
	vcpu_set_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ);
	...
	efer = vcpu_get_msr(vcpu, MSR_EFER);
	TEST_ASSERT(!(efer & EFER_LMSLE), "EFER.LMSLE unexpectedly set");

	vcpu_set_msr(vcpu, MSR_EFER, efer | EFER_LMSLE);
	...
}

Does test_host_initiated_lmsle() also need to be updated or removed to expect
host-initiated KVM_SET_MSRS writes to reject EFER_LMSLE when
X86_FEATURE_EFER_LMSLE_MBZ is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022223052.646891-1-seanjc@google.com?part=4