diff mbox series

[v6,9/9] KVM: selftests: Add a basic SEV-SNP smoke test

Message ID 20250203223205.36121-10-prsampat@amd.com (mailing list archive)
State New
Headers show
Series Basic SEV-SNP Selftests | expand

Commit Message

Pratik Rajesh Sampat Feb. 3, 2025, 10:32 p.m. UTC 
Extend sev_smoke_test to also run a minimal SEV-SNP smoke test that
initializes and sets up private memory regions required to run a simple
SEV-SNP guest.

Similar to its SEV-ES smoke test counterpart, this also does not
support GHCB and ucall yet and uses the GHCB MSR protocol to trigger an
exit of the type KVM_EXIT_SYSTEM_EVENT.

Tested-by: Srikanth Aithal <sraithal@amd.com>
Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
---
v5..v6:

* Rename KVM CPU feature X86_FEATURE_SNP to X86_FEATURE_SEV_SNP (Nikunj)
* Collected tags from Srikanth.
---
 .../selftests/kvm/x86/sev_smoke_test.c        | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Sean Christopherson Feb. 12, 2025, 2:31 a.m. UTC | #1 
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
> @@ -217,5 +244,20 @@ int main(int argc, char *argv[])
>  		}
>  	}
>  
> +	if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
> +		uint64_t snp_policy = snp_default_policy();
> +
> +		test_snp(snp_policy);
> +		/* Test minimum firmware level */
> +		test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |
> +			SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));

Ah, this is where the firmware policy stuff is used.  Refresh me, can userspace
request _any_ major/minor as the min, and expect failure if the version isn't
supported?  If so, the test should iterate over the major/minor combinations that
are guaranteed to fail.  And if userspace can query the supported minor/major,
the test should iterate over all the happy versions too. 

Unless there's nothing interesting to test, I would move the major/minor stuff to
a separate patch.

> +
> +		test_snp_shutdown(snp_policy);
> +
> +		if (kvm_has_cap(KVM_CAP_XCRS) &&
> +		    (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask)
> +			test_sync_vmsa_snp(snp_policy);

This is all copy+paste from SEV-ES tests, minus SEV_POLICY_NO_DBG.  There's gotta
be a way to dedup this code.

Something like this?

static void needs_a_better_name(uint32_t type, uint64_t policy)
{
	const u64 xf_mask = XFEATURE_MASK_X87_AVX;

	test_sev(guest_sev_code, policy | SEV_POLICY_NO_DBG);
	test_sev(guest_sev_code, policy);

	if (type == KVM_X86_SEV_VM)
		return;

	test_sev_shutdown(policy);

	if (kvm_has_cap(KVM_CAP_XCRS) &&
	    (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask) {
		test_sync_vmsa(policy);
		test_sync_vmsa(policy | SEV_POLICY_NO_DBG);
	}
}

int main(int argc, char *argv[])
{
	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));

	needs_a_better_name(KVM_X86_SEV_VM, 0);

	if (kvm_cpu_has(X86_FEATURE_SEV_ES))
		needs_a_better_name(KVM_X86_SEV_ES_VM, 0);

	if (kvm_cpu_has(X86_FEATURE_SEV_SNP))
		needs_a_better_name(KVM_X86_SEV_SNP_VM, 0);

	return 0;
}
Pratik Rajesh Sampat Feb. 14, 2025, 6:14 p.m. UTC | #2 
On 2/11/25 8:31 PM, Sean Christopherson wrote:
> On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
>> @@ -217,5 +244,20 @@ int main(int argc, char *argv[])
>>  		}
>>  	}
>>  
>> +	if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
>> +		uint64_t snp_policy = snp_default_policy();
>> +
>> +		test_snp(snp_policy);
>> +		/* Test minimum firmware level */
>> +		test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |
>> +			SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));
> 
> Ah, this is where the firmware policy stuff is used.  Refresh me, can userspace
> request _any_ major/minor as the min, and expect failure if the version isn't
> supported?  If so, the test should iterate over the major/minor combinations that
> are guaranteed to fail.  And if userspace can query the supported minor/major,
> the test should iterate over all the happy versions too. 
> 

Yes, any policy greater than the min policy (defined in sev-dev.c)
should be supported. The sad path tests were intended to be added in the
upcoming negative test patch series so that we could have the proper
infrastructure to handle and report failures.

> Unless there's nothing interesting to test, I would move the major/minor stuff to
> a separate patch.

Would you rather prefer I do the happy tests here (something like -
min_policy and min_policy + 1?) and defer the failure tests for the
next patchset? Or, I can remove policy testing from here entirely and
introduce it only when the sad path testing infrastructure is ready, so
that we can test this completely at once?

> 
>> +
>> +		test_snp_shutdown(snp_policy);
>> +
>> +		if (kvm_has_cap(KVM_CAP_XCRS) &&
>> +		    (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask)
>> +			test_sync_vmsa_snp(snp_policy);
> 
> This is all copy+paste from SEV-ES tests, minus SEV_POLICY_NO_DBG.  There's gotta
> be a way to dedup this code.
> 
> Something like this?
> 
> static void needs_a_better_name(uint32_t type, uint64_t policy)
> {
> 	const u64 xf_mask = XFEATURE_MASK_X87_AVX;
> 
> 	test_sev(guest_sev_code, policy | SEV_POLICY_NO_DBG);
> 	test_sev(guest_sev_code, policy);
> 
> 	if (type == KVM_X86_SEV_VM)
> 		return;
> 
> 	test_sev_shutdown(policy);
> 
> 	if (kvm_has_cap(KVM_CAP_XCRS) &&
> 	    (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask) {
> 		test_sync_vmsa(policy);
> 		test_sync_vmsa(policy | SEV_POLICY_NO_DBG);
> 	}
> }
> 
> int main(int argc, char *argv[])
> {
> 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
> 
> 	needs_a_better_name(KVM_X86_SEV_VM, 0);
> 
> 	if (kvm_cpu_has(X86_FEATURE_SEV_ES))
> 		needs_a_better_name(KVM_X86_SEV_ES_VM, 0);
> 
> 	if (kvm_cpu_has(X86_FEATURE_SEV_SNP))
> 		needs_a_better_name(KVM_X86_SEV_SNP_VM, 0);
> 
> 	return 0;
> }

Sure, I can definitely clean this up so that we have less duplication of
code all around for this test.

Thanks again!
Pratik
Sean Christopherson Feb. 19, 2025, 12:54 a.m. UTC | #3 
On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote:
> 
> 
> On 2/11/25 8:31 PM, Sean Christopherson wrote:
> > On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
> >> @@ -217,5 +244,20 @@ int main(int argc, char *argv[])
> >>  		}
> >>  	}
> >>  
> >> +	if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
> >> +		uint64_t snp_policy = snp_default_policy();
> >> +
> >> +		test_snp(snp_policy);
> >> +		/* Test minimum firmware level */
> >> +		test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |
> >> +			SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));
> > 
> > Ah, this is where the firmware policy stuff is used.  Refresh me, can userspace
> > request _any_ major/minor as the min, and expect failure if the version isn't
> > supported?  If so, the test should iterate over the major/minor combinations that
> > are guaranteed to fail.  And if userspace can query the supported minor/major,
> > the test should iterate over all the happy versions too. 
> > 
> 
> Yes, any policy greater than the min policy (defined in sev-dev.c)
> should be supported. The sad path tests were intended to be added in the
> upcoming negative test patch series so that we could have the proper
> infrastructure to handle and report failures.
> 
> > Unless there's nothing interesting to test, I would move the major/minor stuff to
> > a separate patch.
> 
> Would you rather prefer I do the happy tests here (something like -
> min_policy and min_policy + 1?) and defer the failure tests for the
> next patchset? Or, I can remove policy testing from here entirely and
> introduce it only when the sad path testing infrastructure is ready, so
> that we can test this completely at once?

Let's do the latter.  For the initial series, do the bare minimum so that we can
get that merged, and then focus on the min API version stuff in a separate series.
The version testing shouldn't be terribly complex, but it doesn't seem like it's
entirely trivial either, and I don't want it to block the base SNP support.
Pratik Rajesh Sampat Feb. 19, 2025, 2:58 p.m. UTC | #4 
On 2/18/25 6:54 PM, Sean Christopherson wrote:
> On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote:
>>
>>
>> On 2/11/25 8:31 PM, Sean Christopherson wrote:
>>> On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
>>>> @@ -217,5 +244,20 @@ int main(int argc, char *argv[])
>>>>  		}
>>>>  	}
>>>>  
>>>> +	if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
>>>> +		uint64_t snp_policy = snp_default_policy();
>>>> +
>>>> +		test_snp(snp_policy);
>>>> +		/* Test minimum firmware level */
>>>> +		test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |
>>>> +			SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));
>>>
>>> Ah, this is where the firmware policy stuff is used.  Refresh me, can userspace
>>> request _any_ major/minor as the min, and expect failure if the version isn't
>>> supported?  If so, the test should iterate over the major/minor combinations that
>>> are guaranteed to fail.  And if userspace can query the supported minor/major,
>>> the test should iterate over all the happy versions too. 
>>>
>>
>> Yes, any policy greater than the min policy (defined in sev-dev.c)
>> should be supported. The sad path tests were intended to be added in the
>> upcoming negative test patch series so that we could have the proper
>> infrastructure to handle and report failures.
>>
>>> Unless there's nothing interesting to test, I would move the major/minor stuff to
>>> a separate patch.
>>
>> Would you rather prefer I do the happy tests here (something like -
>> min_policy and min_policy + 1?) and defer the failure tests for the
>> next patchset? Or, I can remove policy testing from here entirely and
>> introduce it only when the sad path testing infrastructure is ready, so
>> that we can test this completely at once?
> 
> Let's do the latter.  For the initial series, do the bare minimum so that we can
> get that merged, and then focus on the min API version stuff in a separate series.
> The version testing shouldn't be terribly complex, but it doesn't seem like it's
> entirely trivial either, and I don't want it to block the base SNP support.

Sure thing, will do.
Thanks!
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/x86/sev_smoke_test.c b/tools/testing/selftests/kvm/x86/sev_smoke_test.c
index 3a36cd3ca151..3336550152c0 100644
--- a/tools/testing/selftests/kvm/x86/sev_smoke_test.c
+++ b/tools/testing/selftests/kvm/x86/sev_smoke_test.c
@@ -16,6 +16,18 @@ 
 
 #define XFEATURE_MASK_X87_AVX (XFEATURE_MASK_FP | XFEATURE_MASK_SSE | XFEATURE_MASK_YMM)
 
+static void guest_snp_code(void)
+{
+	uint64_t sev_msr = rdmsr(MSR_AMD64_SEV);
+
+	GUEST_ASSERT(sev_msr & MSR_AMD64_SEV_ENABLED);
+	GUEST_ASSERT(sev_msr & MSR_AMD64_SEV_ES_ENABLED);
+	GUEST_ASSERT(sev_msr & MSR_AMD64_SEV_SNP_ENABLED);
+
+	wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ);
+	VMGEXIT();
+}
+
 static void guest_sev_es_code(void)
 {
 	/* TODO: Check CPUID after GHCB-based hypercall support is added. */
@@ -157,11 +169,21 @@  static void test_sev_es(uint64_t policy)
 	__test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, policy);
 }
 
+static void test_snp(uint64_t policy)
+{
+	__test_sev(guest_snp_code, KVM_X86_SNP_VM, policy);
+}
+
 static void test_sync_vmsa_sev_es(uint64_t policy)
 {
 	__test_sync_vmsa(KVM_X86_SEV_ES_VM, policy);
 }
 
+static void test_sync_vmsa_snp(uint64_t policy)
+{
+	__test_sync_vmsa(KVM_X86_SNP_VM, policy);
+}
+
 static void guest_shutdown_code(void)
 {
 	struct desc_ptr idt;
@@ -195,6 +217,11 @@  static void test_sev_es_shutdown(uint64_t policy)
 	__test_sev_shutdown(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
 }
 
+static void test_snp_shutdown(uint64_t policy)
+{
+	__test_sev_shutdown(KVM_X86_SNP_VM, policy);
+}
+
 int main(int argc, char *argv[])
 {
 	const u64 xf_mask = XFEATURE_MASK_X87_AVX;
@@ -217,5 +244,20 @@  int main(int argc, char *argv[])
 		}
 	}
 
+	if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
+		uint64_t snp_policy = snp_default_policy();
+
+		test_snp(snp_policy);
+		/* Test minimum firmware level */
+		test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |
+			SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));
+
+		test_snp_shutdown(snp_policy);
+
+		if (kvm_has_cap(KVM_CAP_XCRS) &&
+		    (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask)
+			test_sync_vmsa_snp(snp_policy);
+	}
+
 	return 0;
 }