[PATCH v3 0/9] SEV Kernel Selftests

[PATCH v3 0/9] SEV Kernel Selftests

[Pratik R. Sampat]

This series primarily introduces SEV-SNP test for the kernel selftest
framework. It tests boot, ioctl, pre fault, and fallocate in various
combinations to exercise both positive and negative launch flow paths.

Patch 1 - Adds a wrapper for the ioctl calls that decouple ioctl and
asserts, which enables the use of negative test cases. No functional
change intended.
Patch 2 - Extend the sev smoke tests to use the SNP specific ioctl
calls and sets up memory to boot a SNP guest VM
Patch 3 - Adds SNP to shutdown testing
Patch 4, 5 - Tests the ioctl path for SEV, SEV-ES and SNP
Patch 6 - Adds support for SNP in KVM_SEV_INIT2 tests
Patch 7,8,9 - Enable Prefault tests for SEV, SEV-ES and SNP

The patchset is rebased on top of kvm-x86/next branch.

v3:
1. Remove the assignments for the prefault and fallocate test type
   enums.
2. Fix error message for sev launch measure and finish.
3. Collect tested-by tags [Peter, Srikanth]

v2:
https://lore.kernel.org/kvm/20240816192310.117456-1-pratikrajesh.sampat@amd.com/
1. Add SMT parsing check to populate SNP policy flags
2. Extend Peter Gonda's shutdown test to include SNP
3. Introduce new tests for prefault which include exercising prefault,
   fallocate, hole-punch in various combinations.
4. Decouple ioctl patch reworked to introduce private variants of the
   the functions that call into the ioctl. Also reordered the patch for
   it to arrive first so that new APIs are not written right after
   their introduction.
5. General cleanups - adding comments, avoiding local booleans, better
   error message. Suggestions incorporated from Peter, Tom, and Sean.

RFC:
https://lore.kernel.org/kvm/20240710220540.188239-1-pratikrajesh.sampat@amd.com/

Any feedback/review is highly appreciated!

Michael Roth (2):
  KVM: selftests: Add interface to manually flag protected/encrypted
    ranges
  KVM: selftests: Add a CoCo-specific test for KVM_PRE_FAULT_MEMORY

Pratik R. Sampat (7):
  KVM: selftests: Decouple SEV ioctls from asserts
  KVM: selftests: Add a basic SNP smoke test
  KVM: selftests: Add SNP to shutdown testing
  KVM: selftests: SEV IOCTL test
  KVM: selftests: SNP IOCTL test
  KVM: selftests: SEV-SNP test for KVM_SEV_INIT2
  KVM: selftests: Interleave fallocate for KVM_PRE_FAULT_MEMORY

 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/include/kvm_util.h  |  13 +
 .../selftests/kvm/include/x86_64/processor.h  |   1 +
 .../selftests/kvm/include/x86_64/sev.h        |  76 +++-
 tools/testing/selftests/kvm/lib/kvm_util.c    |  53 ++-
 .../selftests/kvm/lib/x86_64/processor.c      |   6 +-
 tools/testing/selftests/kvm/lib/x86_64/sev.c  | 190 +++++++-
 .../kvm/x86_64/coco_pre_fault_memory_test.c   | 421 ++++++++++++++++++
 .../selftests/kvm/x86_64/sev_init2_tests.c    |  13 +
 .../selftests/kvm/x86_64/sev_smoke_test.c     | 297 +++++++++++-
 10 files changed, 1023 insertions(+), 48 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c

-- 
2.34.1

[PATCH v3 1/9] KVM: selftests: Decouple SEV ioctls from asserts

[Pratik R. Sampat]

Add variants of sev, sev-es launch path that return the status of the
ioctl call instead of asserting for success. This enables both positive
and negative testing of the path.

No functional impact intended.

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 .../selftests/kvm/include/x86_64/sev.h        | 22 +++++-
 tools/testing/selftests/kvm/lib/x86_64/sev.c  | 78 +++++++++++++++----
 2 files changed, 80 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
index 82c11c81a956..3998152cc081 100644
--- a/tools/testing/selftests/kvm/include/x86_64/sev.h
+++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
@@ -27,6 +27,12 @@ enum sev_guest_state {
 
 #define GHCB_MSR_TERM_REQ	0x100
 
+/* Variants of the SEV launch path that do not assert the ioctl status */
+int __sev_vm_launch_start(struct kvm_vm *vm, uint32_t policy);
+int __sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy);
+int __sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement);
+int __sev_vm_launch_finish(struct kvm_vm *vm);
+
 void sev_vm_launch(struct kvm_vm *vm, uint32_t policy);
 void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement);
 void sev_vm_launch_finish(struct kvm_vm *vm);
@@ -82,15 +88,23 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
 	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
 }
 
-static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
-					  uint64_t size)
+static inline int __sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
+					   uint64_t hva, uint64_t size)
 {
 	struct kvm_sev_launch_update_data update_data = {
-		.uaddr = (unsigned long)addr_gpa2hva(vm, gpa),
+		.uaddr = hva,
 		.len = size,
 	};
 
-	vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
+	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
+}
+
+static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
+					  uint64_t hva, uint64_t size)
+{
+	int ret = __sev_launch_update_data(vm, gpa, hva, size);
+
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm);
 }
 
 #endif /* SELFTEST_KVM_SEV_H */
diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
index e9535ee20b7f..125a72246e09 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
@@ -14,15 +14,16 @@
  * and find the first range, but that's correct because the condition
  * expression would cause us to quit the loop.
  */
-static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
+static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
 {
 	const struct sparsebit *protected_phy_pages = region->protected_phy_pages;
 	const vm_paddr_t gpa_base = region->region.guest_phys_addr;
 	const sparsebit_idx_t lowest_page_in_region = gpa_base >> vm->page_shift;
 	sparsebit_idx_t i, j;
+	int ret;
 
 	if (!sparsebit_any_set(protected_phy_pages))
-		return;
+		return 0;
 
 	sev_register_encrypted_memory(vm, region);
 
@@ -30,8 +31,15 @@ static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *regio
 		const uint64_t size = (j - i + 1) * vm->page_size;
 		const uint64_t offset = (i - lowest_page_in_region) * vm->page_size;
 
-		sev_launch_update_data(vm, gpa_base + offset, size);
+		ret = __sev_launch_update_data(vm, gpa_base + offset,
+					       (uint64_t)addr_gpa2hva(vm, gpa_base + offset),
+					       size);
+		if (ret)
+			return ret;
+
 	}
+
+	return 0;
 }
 
 void sev_vm_init(struct kvm_vm *vm)
@@ -60,38 +68,74 @@ void sev_es_vm_init(struct kvm_vm *vm)
 	}
 }
 
-void sev_vm_launch(struct kvm_vm *vm, uint32_t policy)
+int __sev_vm_launch_start(struct kvm_vm *vm, uint32_t policy)
 {
 	struct kvm_sev_launch_start launch_start = {
 		.policy = policy,
 	};
+
+	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_START, &launch_start);
+}
+
+int __sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy)
+{
 	struct userspace_mem_region *region;
-	struct kvm_sev_guest_status status;
 	int ctr;
 
-	vm_sev_ioctl(vm, KVM_SEV_LAUNCH_START, &launch_start);
-	vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
+	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) {
+		int ret = encrypt_region(vm, region);
 
-	TEST_ASSERT_EQ(status.policy, policy);
-	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_LAUNCH_UPDATE);
-
-	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node)
-		encrypt_region(vm, region);
+		if (ret)
+			return ret;
+	}
 
 	if (policy & SEV_POLICY_ES)
 		vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
 
 	vm->arch.is_pt_protected = true;
+
+	return 0;
 }
 
-void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement)
+int __sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement)
 {
 	struct kvm_sev_launch_measure launch_measure;
-	struct kvm_sev_guest_status guest_status;
 
 	launch_measure.len = 256;
 	launch_measure.uaddr = (__u64)measurement;
-	vm_sev_ioctl(vm, KVM_SEV_LAUNCH_MEASURE, &launch_measure);
+
+	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_MEASURE, &launch_measure);
+}
+
+int __sev_vm_launch_finish(struct kvm_vm *vm)
+{
+	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_FINISH, NULL);
+}
+
+void sev_vm_launch(struct kvm_vm *vm, uint32_t policy)
+{
+	struct kvm_sev_guest_status status;
+	int ret;
+
+	ret = __sev_vm_launch_start(vm, policy);
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_START, ret, vm);
+
+	vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
+
+	TEST_ASSERT_EQ(status.policy, policy);
+	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_LAUNCH_UPDATE);
+
+	ret = __sev_vm_launch_update(vm, policy);
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm);
+}
+
+void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement)
+{
+	struct kvm_sev_guest_status guest_status;
+	int ret;
+
+	ret = __sev_vm_launch_measure(vm, measurement);
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_MEASURE, ret, vm);
 
 	vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &guest_status);
 	TEST_ASSERT_EQ(guest_status.state, SEV_GUEST_STATE_LAUNCH_SECRET);
@@ -100,13 +144,15 @@ void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement)
 void sev_vm_launch_finish(struct kvm_vm *vm)
 {
 	struct kvm_sev_guest_status status;
+	int ret;
 
 	vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
 	TEST_ASSERT(status.state == SEV_GUEST_STATE_LAUNCH_UPDATE ||
 		    status.state == SEV_GUEST_STATE_LAUNCH_SECRET,
 		    "Unexpected guest state: %d", status.state);
 
-	vm_sev_ioctl(vm, KVM_SEV_LAUNCH_FINISH, NULL);
+	ret = __sev_vm_launch_finish(vm);
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_FINISH, ret, vm);
 
 	vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
 	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING);
-- 
2.34.1

Re: [PATCH v3 1/9] KVM: selftests: Decouple SEV ioctls from asserts

[Sean Christopherson]

On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
> +static inline int __sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> +					   uint64_t hva, uint64_t size)
>  {
>  	struct kvm_sev_launch_update_data update_data = {
> -		.uaddr = (unsigned long)addr_gpa2hva(vm, gpa),
> +		.uaddr = hva,
>  		.len = size,
>  	};
>  
> -	vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
> +	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
> +}
> +
> +static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> +					  uint64_t hva, uint64_t size)
> +{
> +	int ret = __sev_launch_update_data(vm, gpa, hva, size);
> +
> +	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm);
>  }
>  
>  #endif /* SELFTEST_KVM_SEV_H */
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> index e9535ee20b7f..125a72246e09 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> @@ -14,15 +14,16 @@
>   * and find the first range, but that's correct because the condition
>   * expression would cause us to quit the loop.
>   */
> -static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
> +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)

This is all kinds of wrong.  encrypt_region() should never fail.  And by allowing
it to fail, any unexpected failure becomes harder to debug.  It's also a lie,
because sev_register_encrypted_memory() isn't allowed to fail, and I would bet
that most readers would expect _that_ call to fail given the name.

The granularity is also poor, and the complete lack of idempotency is going to
be problematic.  E.g. only the first region is actually tested, and if someone
tries to do negative testing on multiple regions, sev_register_encrypted_memory()
will fail due to trying to re-encrypt a region.

__sev_vm_launch_update() has similar issues.  encrypt_region() is allowed to
fail, but its call to KVM_SEV_LAUNCH_UPDATE_VMSA is not.

And peeking ahead, passing an @assert parameter to __test_snp_launch_start() (or
any helper) is a non-starter.  Readers should not have to dive into a helper's
implementation to understand that this

  __test_snp_launch_start(type, policy, 0, true);

is a happy path and this

  ret = __test_snp_launch_start(type, policy, BIT(i), false);

is a sad path.

And re-creating the VM every time is absurdly wasteful.  While performance isn't
a priority for selftests, there's no reason to make everything as slow as possible.

Even just passing the page type to encrypt_region() is confusing.  When the test
is actually going to run the guest, applying ZERO and CPUID types to _all_ pages
is completely nonsensical.

In general, I think trying to reuse the happy path's infrastructure is going to
do more harm than good.  This is what I was trying to get at in my feedback for
the previous version.

For negative tests, I would honestly say development them "from scratch", i.e.
deliberately don't reuse the existing SEV-MEM/ES infrastructure.  It'll require
more copy+paste to get rolling, but I suspect that the end result will be less
churn and far easier to read.

Re: [PATCH v3 1/9] KVM: selftests: Decouple SEV ioctls from asserts

[Pratik R. Sampat]

Hi Sean,

On 10/14/2024 5:18 PM, Sean Christopherson wrote:
> On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
>> +static inline int __sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>> +					   uint64_t hva, uint64_t size)
>>  {
>>  	struct kvm_sev_launch_update_data update_data = {
>> -		.uaddr = (unsigned long)addr_gpa2hva(vm, gpa),
>> +		.uaddr = hva,
>>  		.len = size,
>>  	};
>>  
>> -	vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
>> +	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
>> +}
>> +
>> +static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>> +					  uint64_t hva, uint64_t size)
>> +{
>> +	int ret = __sev_launch_update_data(vm, gpa, hva, size);
>> +
>> +	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm);
>>  }
>>  
>>  #endif /* SELFTEST_KVM_SEV_H */
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> index e9535ee20b7f..125a72246e09 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> @@ -14,15 +14,16 @@
>>   * and find the first range, but that's correct because the condition
>>   * expression would cause us to quit the loop.
>>   */
>> -static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
>> +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
> 
> This is all kinds of wrong.  encrypt_region() should never fail.  And by allowing
> it to fail, any unexpected failure becomes harder to debug.  It's also a lie,
> because sev_register_encrypted_memory() isn't allowed to fail, and I would bet
> that most readers would expect _that_ call to fail given the name.
> 
> The granularity is also poor, and the complete lack of idempotency is going to
> be problematic.  E.g. only the first region is actually tested, and if someone
> tries to do negative testing on multiple regions, sev_register_encrypted_memory()
> will fail due to trying to re-encrypt a region.
> 
> __sev_vm_launch_update() has similar issues.  encrypt_region() is allowed to
> fail, but its call to KVM_SEV_LAUNCH_UPDATE_VMSA is not.
> 
> And peeking ahead, passing an @assert parameter to __test_snp_launch_start() (or
> any helper) is a non-starter.  Readers should not have to dive into a helper's
> implementation to understand that this
> 
>   __test_snp_launch_start(type, policy, 0, true);
> 
> is a happy path and this
> 
>   ret = __test_snp_launch_start(type, policy, BIT(i), false);
> 
> is a sad path.
> 
> And re-creating the VM every time is absurdly wasteful.  While performance isn't
> a priority for selftests, there's no reason to make everything as slow as possible.
> 
> Even just passing the page type to encrypt_region() is confusing.  When the test
> is actually going to run the guest, applying ZERO and CPUID types to _all_ pages
> is completely nonsensical.
> 
> In general, I think trying to reuse the happy path's infrastructure is going to
> do more harm than good.  This is what I was trying to get at in my feedback for
> the previous version.
> 
> For negative tests, I would honestly say development them "from scratch", i.e.
> deliberately don't reuse the existing SEV-MEM/ES infrastructure.  It'll require
> more copy+paste to get rolling, but I suspect that the end result will be less
> churn and far easier to read.

This makes sense. I was trying to be as minimal as possible without a
lot of replication while trying to introduce the negative tests. I see
that this has created several issues of granularity, even general
correctness and overall has created more problems than it solves.

I will try to develop the negative interface separately tailored for
this specific use-case rather than piggybacking on the happy path when I
send out the patchset #2.

[PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Pratik R. Sampat]

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.

Also, decouple policy and type and require functions to provide both
such that there is no assumption regarding the type using policy.

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 .../selftests/kvm/include/x86_64/processor.h  |   1 +
 .../selftests/kvm/include/x86_64/sev.h        |  54 +++++++-
 tools/testing/selftests/kvm/lib/kvm_util.c    |   8 +-
 .../selftests/kvm/lib/x86_64/processor.c      |   6 +-
 tools/testing/selftests/kvm/lib/x86_64/sev.c  | 116 +++++++++++++++++-
 .../selftests/kvm/x86_64/sev_smoke_test.c     |  67 ++++++++--
 6 files changed, 230 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index e247f99e0473..1dfa2c03b40f 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -199,6 +199,7 @@ struct kvm_x86_cpu_feature {
 #define	X86_FEATURE_VGIF		KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 16)
 #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)
+#define X86_FEATURE_SNP		KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 4)
 
 /*
  * KVM defined paravirt features.
diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
index 3998152cc081..658c3cca208d 100644
--- a/tools/testing/selftests/kvm/include/x86_64/sev.h
+++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
@@ -22,8 +22,21 @@ enum sev_guest_state {
 	SEV_GUEST_STATE_RUNNING,
 };
 
+/* Minimum firmware version required for the SEV-SNP support */
+#define SNP_FW_REQ_VER_MAJOR	1
+#define SNP_FW_REQ_VER_MINOR	51
+#define SNP_POLICY_MINOR_BIT	0
+#define SNP_POLICY_MAJOR_BIT	8
+
 #define SEV_POLICY_NO_DBG	(1UL << 0)
 #define SEV_POLICY_ES		(1UL << 2)
+#define SNP_POLICY_SMT		(1ULL << 16)
+#define SNP_POLICY_RSVD_MBO	(1ULL << 17)
+#define SNP_POLICY_DBG		(1ULL << 19)
+#define SNP_POLICY		(SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
+
+#define SNP_FW_VER_MAJOR(maj)	((uint8_t)(maj) << SNP_POLICY_MAJOR_BIT)
+#define SNP_FW_VER_MINOR(min)	((uint8_t)(min) << SNP_POLICY_MINOR_BIT)
 
 #define GHCB_MSR_TERM_REQ	0x100
 
@@ -32,14 +45,22 @@ int __sev_vm_launch_start(struct kvm_vm *vm, uint32_t policy);
 int __sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy);
 int __sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement);
 int __sev_vm_launch_finish(struct kvm_vm *vm);
+int __snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy, uint8_t flags);
+int __snp_vm_launch_update(struct kvm_vm *vm, uint8_t page_type);
+int __snp_vm_launch_finish(struct kvm_vm *vm, uint16_t flags);
 
 void sev_vm_launch(struct kvm_vm *vm, uint32_t policy);
 void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement);
 void sev_vm_launch_finish(struct kvm_vm *vm);
+void snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy);
+void snp_vm_launch_update(struct kvm_vm *vm);
+void snp_vm_launch_finish(struct kvm_vm *vm);
+
+bool is_kvm_snp_supported(void);
 
 struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
 					   struct kvm_vcpu **cpu);
-void vm_sev_launch(struct kvm_vm *vm, uint32_t policy, uint8_t *measurement);
+void vm_sev_launch(struct kvm_vm *vm, uint64_t policy, uint8_t *measurement);
 
 kvm_static_assert(SEV_RET_SUCCESS == 0);
 
@@ -74,8 +95,18 @@ kvm_static_assert(SEV_RET_SUCCESS == 0);
 	__TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd,	ret, vm);		\
 })
 
+/* Ensure policy is within bounds for SEV, SEV-ES */
+#define ASSERT_SEV_POLICY(type, policy)				\
+({									\
+	if (type == KVM_X86_SEV_VM || type == KVM_X86_SEV_ES_VM) {	\
+		TEST_ASSERT(policy < ((uint32_t)~0U),			\
+			    "Policy beyond bounds for SEV");		\
+	}								\
+})									\
+
 void sev_vm_init(struct kvm_vm *vm);
 void sev_es_vm_init(struct kvm_vm *vm);
+void snp_vm_init(struct kvm_vm *vm);
 
 static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
 						 struct userspace_mem_region *region)
@@ -99,6 +130,19 @@ static inline int __sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
 	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
 }
 
+static inline int __snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
+					   uint64_t hva, uint64_t size, uint8_t type)
+{
+	struct kvm_sev_snp_launch_update update_data = {
+		.uaddr = hva,
+		.gfn_start = gpa >> PAGE_SHIFT,
+		.len = size,
+		.type = type,
+	};
+
+	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
+}
+
 static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
 					  uint64_t hva, uint64_t size)
 {
@@ -107,4 +151,12 @@ static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
 	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm);
 }
 
+static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
+					 uint64_t hva, uint64_t size, uint8_t type)
+{
+	int ret = __snp_launch_update_data(vm, gpa, hva, size, type);
+
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_UPDATE, ret, vm);
+}
+
 #endif /* SELFTEST_KVM_SEV_H */
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index a2b7df5f1d39..bbf90ad224da 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -413,14 +413,18 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus,
 						 nr_extra_pages);
 	struct userspace_mem_region *slot0;
 	struct kvm_vm *vm;
-	int i;
+	int i, flags = 0;
 
 	pr_debug("%s: mode='%s' type='%d', pages='%ld'\n", __func__,
 		 vm_guest_mode_string(shape.mode), shape.type, nr_pages);
 
 	vm = ____vm_create(shape);
 
-	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, nr_pages, 0);
+	if (shape.type == KVM_X86_SNP_VM)
+		flags |=  KVM_MEM_GUEST_MEMFD;
+
+	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, nr_pages, flags);
+
 	for (i = 0; i < NR_MEM_REGIONS; i++)
 		vm->memslots[i] = 0;
 
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 974bcd2df6d7..981f3c9fd1cf 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -625,7 +625,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
 	sync_global_to_guest(vm, host_cpu_is_amd);
 	sync_global_to_guest(vm, is_forced_emulation_enabled);
 
-	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
+	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
+	    vm->type == KVM_X86_SNP_VM) {
 		struct kvm_sev_init init = { 0 };
 
 		vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
@@ -1134,7 +1135,8 @@ void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits)
 
 void kvm_init_vm_address_properties(struct kvm_vm *vm)
 {
-	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
+	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
+	    vm->type == KVM_X86_SNP_VM) {
 		vm->arch.sev_fd = open_sev_dev_path_or_exit();
 		vm->arch.c_bit = BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT));
 		vm->gpa_tag_mask = vm->arch.c_bit;
diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
index 125a72246e09..ff3824564854 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
@@ -14,7 +14,8 @@
  * and find the first range, but that's correct because the condition
  * expression would cause us to quit the loop.
  */
-static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
+static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region,
+			  uint8_t page_type)
 {
 	const struct sparsebit *protected_phy_pages = region->protected_phy_pages;
 	const vm_paddr_t gpa_base = region->region.guest_phys_addr;
@@ -25,12 +26,23 @@ static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region
 	if (!sparsebit_any_set(protected_phy_pages))
 		return 0;
 
-	sev_register_encrypted_memory(vm, region);
+	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM)
+		sev_register_encrypted_memory(vm, region);
 
 	sparsebit_for_each_set_range(protected_phy_pages, i, j) {
 		const uint64_t size = (j - i + 1) * vm->page_size;
 		const uint64_t offset = (i - lowest_page_in_region) * vm->page_size;
 
+		if (vm->type == KVM_X86_SNP_VM) {
+			vm_mem_set_private(vm, gpa_base + offset, size);
+			ret = __snp_launch_update_data(vm, gpa_base + offset,
+						      (uint64_t)addr_gpa2hva(vm, gpa_base + offset),
+						      size, page_type);
+			if (ret)
+				return ret;
+			continue;
+		}
+
 		ret = __sev_launch_update_data(vm, gpa_base + offset,
 					       (uint64_t)addr_gpa2hva(vm, gpa_base + offset),
 					       size);
@@ -68,6 +80,14 @@ void sev_es_vm_init(struct kvm_vm *vm)
 	}
 }
 
+void snp_vm_init(struct kvm_vm *vm)
+{
+	struct kvm_sev_init init = { 0 };
+
+	assert(vm->type == KVM_X86_SNP_VM);
+	vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
+}
+
 int __sev_vm_launch_start(struct kvm_vm *vm, uint32_t policy)
 {
 	struct kvm_sev_launch_start launch_start = {
@@ -83,7 +103,7 @@ int __sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy)
 	int ctr;
 
 	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) {
-		int ret = encrypt_region(vm, region);
+		int ret = encrypt_region(vm, region, 0);
 
 		if (ret)
 			return ret;
@@ -112,6 +132,41 @@ int __sev_vm_launch_finish(struct kvm_vm *vm)
 	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_FINISH, NULL);
 }
 
+int __snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy, uint8_t flags)
+{
+	struct kvm_sev_snp_launch_start launch_start = {
+		.policy = policy,
+		.flags = flags,
+	};
+
+	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_START, &launch_start);
+}
+
+int __snp_vm_launch_update(struct kvm_vm *vm, uint8_t page_type)
+{
+	struct userspace_mem_region *region;
+	int ctr, ret;
+
+	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) {
+		ret = encrypt_region(vm, region, page_type);
+		if (ret)
+			return ret;
+	}
+
+	vm->arch.is_pt_protected = true;
+
+	return 0;
+}
+
+int __snp_vm_launch_finish(struct kvm_vm *vm, uint16_t flags)
+{
+	struct kvm_sev_snp_launch_finish launch_finish = {
+		.flags = flags,
+	};
+
+	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_FINISH, &launch_finish);
+}
+
 void sev_vm_launch(struct kvm_vm *vm, uint32_t policy)
 {
 	struct kvm_sev_guest_status status;
@@ -158,6 +213,45 @@ void sev_vm_launch_finish(struct kvm_vm *vm)
 	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING);
 }
 
+void snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy)
+{
+	int ret = __snp_vm_launch_start(vm, policy, 0);
+
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_START, ret, vm);
+}
+
+void snp_vm_launch_update(struct kvm_vm *vm)
+{
+	int ret = __snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
+
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_UPDATE, ret, vm);
+}
+
+void snp_vm_launch_finish(struct kvm_vm *vm)
+{
+	int ret = __snp_vm_launch_finish(vm, 0);
+
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_FINISH, ret, vm);
+}
+
+bool is_kvm_snp_supported(void)
+{
+	int sev_fd = open_sev_dev_path_or_exit();
+	struct sev_user_data_status sev_status;
+
+	struct sev_issue_cmd arg = {
+		.cmd = SEV_PLATFORM_STATUS,
+		.data = (unsigned long)&sev_status,
+	};
+
+	kvm_ioctl(sev_fd, SEV_ISSUE_CMD, &arg);
+	close(sev_fd);
+
+	return sev_status.api_major > SNP_FW_REQ_VER_MAJOR ||
+		(sev_status.api_major == SNP_FW_REQ_VER_MAJOR &&
+		sev_status.api_minor >= SNP_FW_REQ_VER_MINOR);
+}
+
 struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
 					   struct kvm_vcpu **cpu)
 {
@@ -174,8 +268,22 @@ struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
 	return vm;
 }
 
-void vm_sev_launch(struct kvm_vm *vm, uint32_t policy, uint8_t *measurement)
+void vm_sev_launch(struct kvm_vm *vm, uint64_t policy, uint8_t *measurement)
 {
+	if (vm->type == KVM_X86_SNP_VM) {
+		vm_enable_cap(vm, KVM_CAP_EXIT_HYPERCALL, (1 << KVM_HC_MAP_GPA_RANGE));
+
+		snp_vm_launch_start(vm, policy);
+
+		snp_vm_launch_update(vm);
+
+		snp_vm_launch_finish(vm);
+
+		return;
+	}
+
+	ASSERT_SEV_POLICY(vm->type, policy);
+
 	sev_vm_launch(vm, policy);
 
 	if (!measurement)
diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
index 2e9197eb1652..12d466915074 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
@@ -16,6 +16,27 @@
 
 #define XFEATURE_MASK_X87_AVX (XFEATURE_MASK_FP | XFEATURE_MASK_SSE | XFEATURE_MASK_YMM)
 
+static bool is_smt_active(void)
+{
+	FILE *f;
+
+	f = fopen("/sys/devices/system/cpu/smt/active", "r");
+	if (!f)
+		return false;
+
+	return fgetc(f) - '0';
+}
+
+static void guest_snp_code(void)
+{
+	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED);
+	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED);
+	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_SNP_ENABLED);
+
+	wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ);
+	__asm__ __volatile__("rep; vmmcall");
+}
+
 static void guest_sev_es_code(void)
 {
 	/* TODO: Check CPUID after GHCB-based hypercall support is added. */
@@ -61,7 +82,7 @@ static void compare_xsave(u8 *from_host, u8 *from_guest)
 		abort();
 }
 
-static void test_sync_vmsa(uint32_t policy)
+static void test_sync_vmsa(uint32_t type, uint64_t policy)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
@@ -77,7 +98,10 @@ static void test_sync_vmsa(uint32_t policy)
 		.xcrs[0].value = XFEATURE_MASK_X87_AVX,
 	};
 
-	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_code_xsave, &vcpu);
+	TEST_ASSERT(type != KVM_X86_SEV_VM,
+		    "sync_vmsa only supported for SEV-ES and SNP VM types");
+
+	vm = vm_sev_create_with_one_vcpu(type, guest_code_xsave, &vcpu);
 	gva = vm_vaddr_alloc_shared(vm, PAGE_SIZE, KVM_UTIL_MIN_VADDR,
 				    MEM_REGION_TEST_DATA);
 	hva = addr_gva2hva(vm, gva);
@@ -99,7 +123,7 @@ static void test_sync_vmsa(uint32_t policy)
 	    : "ymm4", "st", "st(1)", "st(2)", "st(3)", "st(4)", "st(5)", "st(6)", "st(7)");
 	vcpu_xsave_set(vcpu, &xsave);
 
-	vm_sev_launch(vm, SEV_POLICY_ES | policy, NULL);
+	vm_sev_launch(vm, policy, NULL);
 
 	/* This page is shared, so make it decrypted.  */
 	memset(hva, 0, 4096);
@@ -118,14 +142,12 @@ static void test_sync_vmsa(uint32_t policy)
 	kvm_vm_free(vm);
 }
 
-static void test_sev(void *guest_code, uint64_t policy)
+static void test_sev(void *guest_code, uint32_t type, uint64_t policy)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 	struct ucall uc;
 
-	uint32_t type = policy & SEV_POLICY_ES ? KVM_X86_SEV_ES_VM : KVM_X86_SEV_VM;
-
 	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
 
 	/* TODO: Validate the measurement is as expected. */
@@ -134,7 +156,7 @@ static void test_sev(void *guest_code, uint64_t policy)
 	for (;;) {
 		vcpu_run(vcpu);
 
-		if (policy & SEV_POLICY_ES) {
+		if (vm->type == KVM_X86_SEV_ES_VM || vm->type == KVM_X86_SNP_VM) {
 			TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT,
 				    "Wanted SYSTEM_EVENT, got %s",
 				    exit_reason_str(vcpu->run->exit_reason));
@@ -194,19 +216,38 @@ int main(int argc, char *argv[])
 {
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
 
-	test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
-	test_sev(guest_sev_code, 0);
+	test_sev(guest_sev_code, KVM_X86_SEV_VM, SEV_POLICY_NO_DBG);
+	test_sev(guest_sev_code, KVM_X86_SEV_VM, 0);
 
 	if (kvm_cpu_has(X86_FEATURE_SEV_ES)) {
-		test_sev(guest_sev_es_code, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
-		test_sev(guest_sev_es_code, SEV_POLICY_ES);
+		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
+		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
 
 		test_sev_es_shutdown();
 
 		if (kvm_has_cap(KVM_CAP_XCRS) &&
 		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
-			test_sync_vmsa(0);
-			test_sync_vmsa(SEV_POLICY_NO_DBG);
+			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
+			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
+		}
+	}
+
+	if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
+		unsigned long snp_policy = SNP_POLICY;
+
+		if (unlikely(!is_smt_active()))
+			snp_policy &= ~SNP_POLICY_SMT;
+
+		test_sev(guest_snp_code, KVM_X86_SNP_VM, snp_policy);
+		/* Test minimum firmware level */
+		test_sev(guest_snp_code, KVM_X86_SNP_VM,
+			 snp_policy |
+			 SNP_FW_VER_MAJOR(SNP_FW_REQ_VER_MAJOR) |
+			 SNP_FW_VER_MINOR(SNP_FW_REQ_VER_MINOR));
+
+		if (kvm_has_cap(KVM_CAP_XCRS) &&
+		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
+			test_sync_vmsa(KVM_X86_SNP_VM, snp_policy);
 		}
 	}
 
-- 
2.34.1

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Sean Christopherson]

On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
> 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.
> 
> Also, decouple policy and type and require functions to provide both
> such that there is no assumption regarding the type using policy.
> 
> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
> Tested-by: Peter Gonda <pgonda@google.com>
> Tested-by: Srikanth Aithal <sraithal@amd.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h  |   1 +
>  .../selftests/kvm/include/x86_64/sev.h        |  54 +++++++-
>  tools/testing/selftests/kvm/lib/kvm_util.c    |   8 +-
>  .../selftests/kvm/lib/x86_64/processor.c      |   6 +-
>  tools/testing/selftests/kvm/lib/x86_64/sev.c  | 116 +++++++++++++++++-
>  .../selftests/kvm/x86_64/sev_smoke_test.c     |  67 ++++++++--
>  6 files changed, 230 insertions(+), 22 deletions(-)

There is *way* too much going on in this one patch.  There's at least 6+ patches
worth of stuff here:

 1. Add x86 architectural defines
 2. SNP ioctl() plumbing
 3..N. Other misc plumbing, e.g. is_smt_active()
 N+1. __vm_create() change to force GUEST_MEMFD for SNP
 N+2. Whatever the ASSERT_SEV_POLICY() thing is doing, if it's actually necessary
 N+3..M. Refactorings to existing code to prep for SNP
 M+1. SNP support

In general, if you feel the urge to start a changelog paragraph with "Also,"
that's a sign you need to break up the patch.

> @@ -74,8 +95,18 @@ kvm_static_assert(SEV_RET_SUCCESS == 0);
>  	__TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd,	ret, vm);		\
>  })
>  
> +/* Ensure policy is within bounds for SEV, SEV-ES */
> +#define ASSERT_SEV_POLICY(type, policy)				\
> +({									\
> +	if (type == KVM_X86_SEV_VM || type == KVM_X86_SEV_ES_VM) {	\
> +		TEST_ASSERT(policy < ((uint32_t)~0U),			\
> +			    "Policy beyond bounds for SEV");		\

This is asinine.  First, there's one user, i.e. I see no reason to have a funky
macro to assert on the type.  Second, _if_ this is a common macro, "type" can and
should be incorporated into the assert.  Third, I have no idea why selftests is
validating its own inputs to KVM.

> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 974bcd2df6d7..981f3c9fd1cf 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -625,7 +625,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
>  	sync_global_to_guest(vm, host_cpu_is_amd);
>  	sync_global_to_guest(vm, is_forced_emulation_enabled);
>  
> -	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
> +	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
> +	    vm->type == KVM_X86_SNP_VM) {

Probably time to add a helper, e.g. is_sev_vm() or something.  If we follow KVM's
lead, then we'd have is_sev_vm(), is_sev_es_vm(), and is_sev_snp_vm(), where an
SNP VM returns true for all of them.  Not sure I love that idea, just throwing it
out there as one possibility.

>  		struct kvm_sev_init init = { 0 };
>  
>  		vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
> @@ -1134,7 +1135,8 @@ void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits)
>  
>  void kvm_init_vm_address_properties(struct kvm_vm *vm)
>  {
> -	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
> +	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
> +	    vm->type == KVM_X86_SNP_VM) {
>  		vm->arch.sev_fd = open_sev_dev_path_or_exit();
>  		vm->arch.c_bit = BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT));
>  		vm->gpa_tag_mask = vm->arch.c_bit;
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> index 125a72246e09..ff3824564854 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> @@ -14,7 +14,8 @@
>   * and find the first range, but that's correct because the condition
>   * expression would cause us to quit the loop.
>   */
> -static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
> +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region,
> +			  uint8_t page_type)
>  {
>  	const struct sparsebit *protected_phy_pages = region->protected_phy_pages;
>  	const vm_paddr_t gpa_base = region->region.guest_phys_addr;
> @@ -25,12 +26,23 @@ static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region
>  	if (!sparsebit_any_set(protected_phy_pages))
>  		return 0;
>  
> -	sev_register_encrypted_memory(vm, region);
> +	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM)

And then this would be

	if (!is_sev_snp_vm())

> +		sev_register_encrypted_memory(vm, region);
>  
>  	sparsebit_for_each_set_range(protected_phy_pages, i, j) {
>  		const uint64_t size = (j - i + 1) * vm->page_size;
>  		const uint64_t offset = (i - lowest_page_in_region) * vm->page_size;
>  
> +		if (vm->type == KVM_X86_SNP_VM) {
> +			vm_mem_set_private(vm, gpa_base + offset, size);

Setting memory private seems like something that should be done by common code,
not by SNP specific code.

> @@ -158,6 +213,45 @@ void sev_vm_launch_finish(struct kvm_vm *vm)
>  	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING);
>  }
>  
> +void snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy)
> +{
> +	int ret = __snp_vm_launch_start(vm, policy, 0);
> +
> +	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_START, ret, vm);

Please use vm_ioctl().  TEST_ASSERT_VM_VCPU_IOCTL() should pretty much never be
used directly, the only exceptions are cases where '0' is not the only success
value, e.g. for ioctls that return a file descriptor.
> +static void guest_snp_code(void)
> +{
> +	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED);
> +	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED);
> +	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_SNP_ENABLED);

Read the MSR once.

> +
> +	wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ);
> +	__asm__ __volatile__("rep; vmmcall");

Please add a vmgexit() helper (which I should have done as part of commit
40e09b3ccfac ("KVM: selftests: Add a basic SEV-ES smoke test")).

> +}
> +
>  static void guest_sev_es_code(void)
>  {
>  	/* TODO: Check CPUID after GHCB-based hypercall support is added. */
> @@ -61,7 +82,7 @@ static void compare_xsave(u8 *from_host, u8 *from_guest)
>  		abort();
>  }
>  
> -static void test_sync_vmsa(uint32_t policy)
> +static void test_sync_vmsa(uint32_t type, uint64_t policy)
>  {
>  	struct kvm_vcpu *vcpu;
>  	struct kvm_vm *vm;
> @@ -77,7 +98,10 @@ static void test_sync_vmsa(uint32_t policy)
>  		.xcrs[0].value = XFEATURE_MASK_X87_AVX,
>  	};
>  
> -	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_code_xsave, &vcpu);
> +	TEST_ASSERT(type != KVM_X86_SEV_VM,
> +		    "sync_vmsa only supported for SEV-ES and SNP VM types");
> +
> +	vm = vm_sev_create_with_one_vcpu(type, guest_code_xsave, &vcpu);
>  	gva = vm_vaddr_alloc_shared(vm, PAGE_SIZE, KVM_UTIL_MIN_VADDR,
>  				    MEM_REGION_TEST_DATA);
>  	hva = addr_gva2hva(vm, gva);
> @@ -99,7 +123,7 @@ static void test_sync_vmsa(uint32_t policy)
>  	    : "ymm4", "st", "st(1)", "st(2)", "st(3)", "st(4)", "st(5)", "st(6)", "st(7)");
>  	vcpu_xsave_set(vcpu, &xsave);
>  
> -	vm_sev_launch(vm, SEV_POLICY_ES | policy, NULL);
> +	vm_sev_launch(vm, policy, NULL);
>  
>  	/* This page is shared, so make it decrypted.  */
>  	memset(hva, 0, 4096);
> @@ -118,14 +142,12 @@ static void test_sync_vmsa(uint32_t policy)
>  	kvm_vm_free(vm);
>  }
>  
> -static void test_sev(void *guest_code, uint64_t policy)
> +static void test_sev(void *guest_code, uint32_t type, uint64_t policy)
>  {
>  	struct kvm_vcpu *vcpu;
>  	struct kvm_vm *vm;
>  	struct ucall uc;
>  
> -	uint32_t type = policy & SEV_POLICY_ES ? KVM_X86_SEV_ES_VM : KVM_X86_SEV_VM;
> -
>  	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
>  
>  	/* TODO: Validate the measurement is as expected. */
> @@ -134,7 +156,7 @@ static void test_sev(void *guest_code, uint64_t policy)
>  	for (;;) {
>  		vcpu_run(vcpu);
>  
> -		if (policy & SEV_POLICY_ES) {
> +		if (vm->type == KVM_X86_SEV_ES_VM || vm->type == KVM_X86_SNP_VM) {
>  			TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT,
>  				    "Wanted SYSTEM_EVENT, got %s",
>  				    exit_reason_str(vcpu->run->exit_reason));
> @@ -194,19 +216,38 @@ int main(int argc, char *argv[])
>  {
>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
>  
> -	test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
> -	test_sev(guest_sev_code, 0);
> +	test_sev(guest_sev_code, KVM_X86_SEV_VM, SEV_POLICY_NO_DBG);
> +	test_sev(guest_sev_code, KVM_X86_SEV_VM, 0);

To cut down on the amount of copy+paste, and line lengths, I vote to add separate
wrappers, e.g.

	test_sev(<policy>)
	test_sev_es(<policy>)
	test_sev_snp(<policy>);
>  
>  	if (kvm_cpu_has(X86_FEATURE_SEV_ES)) {
> -		test_sev(guest_sev_es_code, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
> -		test_sev(guest_sev_es_code, SEV_POLICY_ES);
> +		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);

Please wrap at ~80 chars.


> +		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
>  
>  		test_sev_es_shutdown();
>  
>  		if (kvm_has_cap(KVM_CAP_XCRS) &&
>  		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
> -			test_sync_vmsa(0);
> -			test_sync_vmsa(SEV_POLICY_NO_DBG);
> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
> +		}
> +	}
> +
> +	if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {

Why do we need both?  KVM shouldn't advertise SNP if it's not supported.

> +		unsigned long snp_policy = SNP_POLICY;

u64, no?

> +
> +		if (unlikely(!is_smt_active()))
> +			snp_policy &= ~SNP_POLICY_SMT;

Why does SNP_POLICY assume SMT?  And what is RSVD_MBO?  E.g. why not this?

		u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;

> +
> +		test_sev(guest_snp_code, KVM_X86_SNP_VM, snp_policy);
> +		/* Test minimum firmware level */
> +		test_sev(guest_snp_code, KVM_X86_SNP_VM,
> +			 snp_policy |
> +			 SNP_FW_VER_MAJOR(SNP_FW_REQ_VER_MAJOR) |
> +			 SNP_FW_VER_MINOR(SNP_FW_REQ_VER_MINOR));
> +
> +		if (kvm_has_cap(KVM_CAP_XCRS) &&
> +		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {

Curly braces are unnecessary.

> +			test_sync_vmsa(KVM_X86_SNP_VM, snp_policy);
>  		}
>  	}
>  
> -- 
> 2.34.1
> 

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Pratik R. Sampat]

Hi Sean,

Thank you for your comments.
...

>>  .../selftests/kvm/include/x86_64/processor.h  |   1 +
>>  .../selftests/kvm/include/x86_64/sev.h        |  54 +++++++-
>>  tools/testing/selftests/kvm/lib/kvm_util.c    |   8 +-
>>  .../selftests/kvm/lib/x86_64/processor.c      |   6 +-
>>  tools/testing/selftests/kvm/lib/x86_64/sev.c  | 116 +++++++++++++++++-
>>  .../selftests/kvm/x86_64/sev_smoke_test.c     |  67 ++++++++--
>>  6 files changed, 230 insertions(+), 22 deletions(-)
> 
> There is *way* too much going on in this one patch.  There's at least 6+ patches
> worth of stuff here:
> 
>  1. Add x86 architectural defines
>  2. SNP ioctl() plumbing
>  3..N. Other misc plumbing, e.g. is_smt_active()
>  N+1. __vm_create() change to force GUEST_MEMFD for SNP
>  N+2. Whatever the ASSERT_SEV_POLICY() thing is doing, if it's actually necessary
>  N+3..M. Refactorings to existing code to prep for SNP
>  M+1. SNP support
> 
> In general, if you feel the urge to start a changelog paragraph with "Also,"
> that's a sign you need to break up the patch.

Sure. I will split this into multiple patches which should
form the basis of the #1 patch series.

> 
>> @@ -74,8 +95,18 @@ kvm_static_assert(SEV_RET_SUCCESS == 0);
>>  	__TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd,	ret, vm);		\
>>  })
>>  
>> +/* Ensure policy is within bounds for SEV, SEV-ES */
>> +#define ASSERT_SEV_POLICY(type, policy)				\
>> +({									\
>> +	if (type == KVM_X86_SEV_VM || type == KVM_X86_SEV_ES_VM) {	\
>> +		TEST_ASSERT(policy < ((uint32_t)~0U),			\
>> +			    "Policy beyond bounds for SEV");		\
> 
> This is asinine.  First, there's one user, i.e. I see no reason to have a funky
> macro to assert on the type.  Second, _if_ this is a common macro, "type" can and
> should be incorporated into the assert.  Third, I have no idea why selftests is
> validating its own inputs to KVM.

It wasn't strictly necessary to validate this. Since the function
vm_sev_launch() now took a u64 in policy (for SNP), I thought it maybe
useful to validate u32 for the rest, as library function can be called
by other selftests as well. However, I do see your point that it doesn't
make too much sense for selftests to try and validate it's own inputs.

I'm open to both - reducing the macro to a just a check within the
function or removing the macro altogether.

> 
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> index 974bcd2df6d7..981f3c9fd1cf 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> @@ -625,7 +625,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
>>  	sync_global_to_guest(vm, host_cpu_is_amd);
>>  	sync_global_to_guest(vm, is_forced_emulation_enabled);
>>  
>> -	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
>> +	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
>> +	    vm->type == KVM_X86_SNP_VM) {
> 
> Probably time to add a helper, e.g. is_sev_vm() or something.  If we follow KVM's
> lead, then we'd have is_sev_vm(), is_sev_es_vm(), and is_sev_snp_vm(), where an
> SNP VM returns true for all of them.  Not sure I love that idea, just throwing it
> out there as one possibility.

Agreed. It will be cleaner to have helpers since similar checks are
being made in multiple places.

> 
>>  		struct kvm_sev_init init = { 0 };
>>  
>>  		vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
>> @@ -1134,7 +1135,8 @@ void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits)
>>  
>>  void kvm_init_vm_address_properties(struct kvm_vm *vm)
>>  {
>> -	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
>> +	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
>> +	    vm->type == KVM_X86_SNP_VM) {
>>  		vm->arch.sev_fd = open_sev_dev_path_or_exit();
>>  		vm->arch.c_bit = BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT));
>>  		vm->gpa_tag_mask = vm->arch.c_bit;
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> index 125a72246e09..ff3824564854 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> @@ -14,7 +14,8 @@
>>   * and find the first range, but that's correct because the condition
>>   * expression would cause us to quit the loop.
>>   */
>> -static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
>> +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region,
>> +			  uint8_t page_type)
>>  {
>>  	const struct sparsebit *protected_phy_pages = region->protected_phy_pages;
>>  	const vm_paddr_t gpa_base = region->region.guest_phys_addr;
>> @@ -25,12 +26,23 @@ static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region
>>  	if (!sparsebit_any_set(protected_phy_pages))
>>  		return 0;
>>  
>> -	sev_register_encrypted_memory(vm, region);
>> +	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM)
> 
> And then this would be
> 
> 	if (!is_sev_snp_vm())

Ack.

> 
>> +		sev_register_encrypted_memory(vm, region);
>>  
>>  	sparsebit_for_each_set_range(protected_phy_pages, i, j) {
>>  		const uint64_t size = (j - i + 1) * vm->page_size;
>>  		const uint64_t offset = (i - lowest_page_in_region) * vm->page_size;
>>  
>> +		if (vm->type == KVM_X86_SNP_VM) {
>> +			vm_mem_set_private(vm, gpa_base + offset, size);
> 
> Setting memory private seems like something that should be done by common code,
> not by SNP specific code.

That's fair. I will make a helper for this and call this common code
separate from encrypt_region()

> 
>> @@ -158,6 +213,45 @@ void sev_vm_launch_finish(struct kvm_vm *vm)
>>  	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING);
>>  }
>>  
>> +void snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy)
>> +{
>> +	int ret = __snp_vm_launch_start(vm, policy, 0);
>> +
>> +	TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_START, ret, vm);
> 
> Please use vm_ioctl().  TEST_ASSERT_VM_VCPU_IOCTL() should pretty much never be
> used directly, the only exceptions are cases where '0' is not the only success
> value, e.g. for ioctls that return a file descriptor.

Sure. This was done for trying to be inclusive for the negative cases by
decoupling ioctl calls from their asserts. Since, code for negative
tests is are going to architected separately altogether, I will make
sure to clean this up with vm_ioctl() everywhere.

>> +static void guest_snp_code(void)
>> +{
>> +	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED);
>> +	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED);
>> +	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_SNP_ENABLED);
> 
> Read the MSR once.
> 
>> +
>> +	wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ);
>> +	__asm__ __volatile__("rep; vmmcall");
> 
> Please add a vmgexit() helper (which I should have done as part of commit
> 40e09b3ccfac ("KVM: selftests: Add a basic SEV-ES smoke test")).
> 

Sure, will do so for this and apply to the other guest_code as well.

>> +}
>> +
>>  static void guest_sev_es_code(void)
>>  {
>>  	/* TODO: Check CPUID after GHCB-based hypercall support is added. */
>> @@ -61,7 +82,7 @@ static void compare_xsave(u8 *from_host, u8 *from_guest)
>>  		abort();
>>  }
>>  
>> -static void test_sync_vmsa(uint32_t policy)
>> +static void test_sync_vmsa(uint32_t type, uint64_t policy)
>>  {
>>  	struct kvm_vcpu *vcpu;
>>  	struct kvm_vm *vm;
>> @@ -77,7 +98,10 @@ static void test_sync_vmsa(uint32_t policy)
>>  		.xcrs[0].value = XFEATURE_MASK_X87_AVX,
>>  	};
>>  
>> -	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_code_xsave, &vcpu);
>> +	TEST_ASSERT(type != KVM_X86_SEV_VM,
>> +		    "sync_vmsa only supported for SEV-ES and SNP VM types");
>> +
>> +	vm = vm_sev_create_with_one_vcpu(type, guest_code_xsave, &vcpu);
>>  	gva = vm_vaddr_alloc_shared(vm, PAGE_SIZE, KVM_UTIL_MIN_VADDR,
>>  				    MEM_REGION_TEST_DATA);
>>  	hva = addr_gva2hva(vm, gva);
>> @@ -99,7 +123,7 @@ static void test_sync_vmsa(uint32_t policy)
>>  	    : "ymm4", "st", "st(1)", "st(2)", "st(3)", "st(4)", "st(5)", "st(6)", "st(7)");
>>  	vcpu_xsave_set(vcpu, &xsave);
>>  
>> -	vm_sev_launch(vm, SEV_POLICY_ES | policy, NULL);
>> +	vm_sev_launch(vm, policy, NULL);
>>  
>>  	/* This page is shared, so make it decrypted.  */
>>  	memset(hva, 0, 4096);
>> @@ -118,14 +142,12 @@ static void test_sync_vmsa(uint32_t policy)
>>  	kvm_vm_free(vm);
>>  }
>>  
>> -static void test_sev(void *guest_code, uint64_t policy)
>> +static void test_sev(void *guest_code, uint32_t type, uint64_t policy)
>>  {
>>  	struct kvm_vcpu *vcpu;
>>  	struct kvm_vm *vm;
>>  	struct ucall uc;
>>  
>> -	uint32_t type = policy & SEV_POLICY_ES ? KVM_X86_SEV_ES_VM : KVM_X86_SEV_VM;
>> -
>>  	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
>>  
>>  	/* TODO: Validate the measurement is as expected. */
>> @@ -134,7 +156,7 @@ static void test_sev(void *guest_code, uint64_t policy)
>>  	for (;;) {
>>  		vcpu_run(vcpu);
>>  
>> -		if (policy & SEV_POLICY_ES) {
>> +		if (vm->type == KVM_X86_SEV_ES_VM || vm->type == KVM_X86_SNP_VM) {
>>  			TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT,
>>  				    "Wanted SYSTEM_EVENT, got %s",
>>  				    exit_reason_str(vcpu->run->exit_reason));
>> @@ -194,19 +216,38 @@ int main(int argc, char *argv[])
>>  {
>>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
>>  
>> -	test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
>> -	test_sev(guest_sev_code, 0);
>> +	test_sev(guest_sev_code, KVM_X86_SEV_VM, SEV_POLICY_NO_DBG);
>> +	test_sev(guest_sev_code, KVM_X86_SEV_VM, 0);
> 
> To cut down on the amount of copy+paste, and line lengths, I vote to add separate
> wrappers, e.g.
> 
> 	test_sev(<policy>)
> 	test_sev_es(<policy>)
> 	test_sev_snp(<policy>);

Sure.

>>  
>>  	if (kvm_cpu_has(X86_FEATURE_SEV_ES)) {
>> -		test_sev(guest_sev_es_code, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
>> -		test_sev(guest_sev_es_code, SEV_POLICY_ES);
>> +		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
> 
> Please wrap at ~80 chars.

Ack. Will do.

> 
> 
>> +		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
>>  
>>  		test_sev_es_shutdown();
>>  
>>  		if (kvm_has_cap(KVM_CAP_XCRS) &&
>>  		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
>> -			test_sync_vmsa(0);
>> -			test_sync_vmsa(SEV_POLICY_NO_DBG);
>> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
>> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
>> +		}
>> +	}
>> +
>> +	if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
> 
> Why do we need both?  KVM shouldn't advertise SNP if it's not supported.

My rationale behind needing this was that the feature can be advertised
but it may not have the right API major or minor release which could be
updated post boot and could determine it's support during runtime.

> 
>> +		unsigned long snp_policy = SNP_POLICY;
> 
> u64, no?

Yes, sorry for the oversight. Will change it to u64.

> 
>> +
>> +		if (unlikely(!is_smt_active()))
>> +			snp_policy &= ~SNP_POLICY_SMT;
> 
> Why does SNP_POLICY assume SMT?  And what is RSVD_MBO?  E.g. why not this?
> 
> 		u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
> 

I think most systems support SMT so I enabled the bit in by default and
only unset it when there isn't any support.

RSVD_MBO is a reserved bit that must be set to one for SNP guest policy
to be enabled. The FW spec specifies this - Sec 4.3 Pg 27 - Guest Policy
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf

>> +
>> +		test_sev(guest_snp_code, KVM_X86_SNP_VM, snp_policy);
>> +		/* Test minimum firmware level */
>> +		test_sev(guest_snp_code, KVM_X86_SNP_VM,
>> +			 snp_policy |
>> +			 SNP_FW_VER_MAJOR(SNP_FW_REQ_VER_MAJOR) |
>> +			 SNP_FW_VER_MINOR(SNP_FW_REQ_VER_MINOR));
>> +
>> +		if (kvm_has_cap(KVM_CAP_XCRS) &&
>> +		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
> 
> Curly braces are unnecessary.

Ack.

Thanks again for the detailed review!

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Sean Christopherson]

On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
> >> +		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
> >>  
> >>  		test_sev_es_shutdown();
> >>  
> >>  		if (kvm_has_cap(KVM_CAP_XCRS) &&
> >>  		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
> >> -			test_sync_vmsa(0);
> >> -			test_sync_vmsa(SEV_POLICY_NO_DBG);
> >> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
> >> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
> >> +		}
> >> +	}
> >> +
> >> +	if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
> > 
> > Why do we need both?  KVM shouldn't advertise SNP if it's not supported.
> 
> My rationale behind needing this was that the feature can be advertised
> but it may not have the right API major or minor release which could be
> updated post boot and could determine it's support during runtime.

KVM will never determine support after KVM has been loaded.  If *KVM* has a
dependency on the API major.minor, then X86_FEATURE_SNP must be set if and only
if the supported API version is available.

If the API major.minor is purely a userspace thing, then is_kvm_snp_supported()
is misnamed, because the check has nothing to do with KVM.  E.g. something like
is_snp_api_version_supported() would be more appropriate.

> >> +		unsigned long snp_policy = SNP_POLICY;
> > 
> > u64, no?
> 
> Yes, sorry for the oversight. Will change it to u64.
> 
> > 
> >> +
> >> +		if (unlikely(!is_smt_active()))
> >> +			snp_policy &= ~SNP_POLICY_SMT;
> > 
> > Why does SNP_POLICY assume SMT?  And what is RSVD_MBO?  E.g. why not this?
> > 
> > 		u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
> > 
> 
> I think most systems support SMT so I enabled the bit in by default and
> only unset it when there isn't any support.

That's confusing though, because you're mixing architectural defines with semi-
arbitrary selftests behavior.  RSVD_MBO on the other is apparently tightly coupled
with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO
needs to be part of SNP_POLICY

If you want to have a *software*-defined default policy, then make it obvious that
it's software defined.  E.g. name the #define SNP_DEFAULT_POLICY, not simply
SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy,
which it is not.  That said, IIUC, SMT *must* match the host configuration, i.e.
whether or not SMT is set is non-negotiable.  In that case, there's zero value in
defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems.

Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, 
and that they are mutualy exclusive?  E.g. what happens if the full policy is simply
SNP_POLICY_RSVD_MBO?

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Pratik R. Sampat]

Hi Sean,

On 10/28/2024 12:55 PM, Sean Christopherson wrote:
> On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
>>>> +		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
>>>>  
>>>>  		test_sev_es_shutdown();
>>>>  
>>>>  		if (kvm_has_cap(KVM_CAP_XCRS) &&
>>>>  		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
>>>> -			test_sync_vmsa(0);
>>>> -			test_sync_vmsa(SEV_POLICY_NO_DBG);
>>>> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
>>>> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
>>>
>>> Why do we need both?  KVM shouldn't advertise SNP if it's not supported.
>>
>> My rationale behind needing this was that the feature can be advertised
>> but it may not have the right API major or minor release which could be
>> updated post boot and could determine it's support during runtime.
> 
> KVM will never determine support after KVM has been loaded.  If *KVM* has a
> dependency on the API major.minor, then X86_FEATURE_SNP must be set if and only
> if the supported API version is available.
> 
> If the API major.minor is purely a userspace thing, then is_kvm_snp_supported()
> is misnamed, because the check has nothing to do with KVM.  E.g. something like
> is_snp_api_version_supported() would be more appropriate.

That's fair. It is related to the FW supplied to it from userspace and
naming it with kvm prefix is misleading. I'll change that.

> 
>>>> +		unsigned long snp_policy = SNP_POLICY;
>>>
>>> u64, no?
>>
>> Yes, sorry for the oversight. Will change it to u64.
>>
>>>
>>>> +
>>>> +		if (unlikely(!is_smt_active()))
>>>> +			snp_policy &= ~SNP_POLICY_SMT;
>>>
>>> Why does SNP_POLICY assume SMT?  And what is RSVD_MBO?  E.g. why not this?
>>>
>>> 		u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
>>>
>>
>> I think most systems support SMT so I enabled the bit in by default and
>> only unset it when there isn't any support.
> 
> That's confusing though, because you're mixing architectural defines with semi-
> arbitrary selftests behavior.  RSVD_MBO on the other is apparently tightly coupled
> with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO
> needs to be part of SNP_POLICY
> 
> If you want to have a *software*-defined default policy, then make it obvious that
> it's software defined.  E.g. name the #define SNP_DEFAULT_POLICY, not simply
> SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy,
> which it is not.  That said, IIUC, SMT *must* match the host configuration, i.e.
> whether or not SMT is set is non-negotiable.  In that case, there's zero value in
> defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems.
> 

Right, SMT should match the host configuration. Would a
SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro?

Instead of,
#define SNP_POLICY	(SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)

Have something like this instead to make it generic and less ambiguous?
#define SNP_DEFAULT_POLICY()		 			       \
({								       \
	SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0);  \
})

> Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, 
> and that they are mutualy exclusive?  E.g. what happens if the full policy is simply
> SNP_POLICY_RSVD_MBO?

SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and
SEV-ES - pg 31, Table 2
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf

and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg
27, Table 9
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf

In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is
set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set.

An SNP guest can certainly just have the policy SNP_POLICY_RSVD_MBO,
barring the case on a SMT system where that bit must be set too for a
successful launch.

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Sean Christopherson]

On Mon, Oct 28, 2024, Pratik R. Sampat wro4te:
> On 10/28/2024 12:55 PM, Sean Christopherson wrote:
> > On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
> >>>> +		if (unlikely(!is_smt_active()))
> >>>> +			snp_policy &= ~SNP_POLICY_SMT;
> >>>
> >>> Why does SNP_POLICY assume SMT?  And what is RSVD_MBO?  E.g. why not this?
> >>>
> >>> 		u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
> >>>
> >>
> >> I think most systems support SMT so I enabled the bit in by default and
> >> only unset it when there isn't any support.
> > 
> > That's confusing though, because you're mixing architectural defines with semi-
> > arbitrary selftests behavior.  RSVD_MBO on the other is apparently tightly coupled
> > with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO
> > needs to be part of SNP_POLICY
> > 
> > If you want to have a *software*-defined default policy, then make it obvious that
> > it's software defined.  E.g. name the #define SNP_DEFAULT_POLICY, not simply
> > SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy,
> > which it is not.  That said, IIUC, SMT *must* match the host configuration, i.e.
> > whether or not SMT is set is non-negotiable.  In that case, there's zero value in
> > defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems.
> > 
> 
> Right, SMT should match the host configuration. Would a
> SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro?
> 
> Instead of,
> #define SNP_POLICY	(SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
> 
> Have something like this instead to make it generic and less ambiguous?
> #define SNP_DEFAULT_POLICY()		 			       \
> ({								       \
> 	SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0);  \
> })

No, unless it's the least awful option, don't hide dynamic functionality in a macro
that looks like it holds static data.  The idea is totally fine, but put it in an
actual helper, not a macro, _if_ there's actually a need for a default policy.
If there's only ever one main path that creates SNP VMs, then I don't see the point
in specifying a default policy.

> > Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, 
> > and that they are mutualy exclusive?  E.g. what happens if the full policy is simply
> > SNP_POLICY_RSVD_MBO?
> 
> SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and
> SEV-ES - pg 31, Table 2
> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf
> 
> and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg
> 27, Table 9
> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
> 
> In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is
> set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set.

Ugh, one is SEV_xxx, the other is SNP_xxx.  Argh!  And IIUC, they are mutually
exclusive (totally separate thigns?), because SNP guests use an 8-byte structure,
whereas SEV/SEV-ES use a 4-byte structure, and with different layouts.

That means this is _extremely_ confusing.  Separate the SEV_xxx defines from the
SNP_xxx defines, because other than a name, they have nothing in common.

+/* Minimum firmware version required for the SEV-SNP support */
+#define SNP_FW_REQ_VER_MAJOR   1
+#define SNP_FW_REQ_VER_MINOR   51

Side topic, why are these hardcoded?  And where did they come from?  If they're
arbitrary KVM selftests values, make that super duper clear.

+#define SNP_POLICY_MINOR_BIT   0
+#define SNP_POLICY_MAJOR_BIT   8

s/BIT/SHIFT.  "BIT" implies they are a single bit, which is obviously not the
case.  But I vote to omit the extra #define entirely and just open code the shift
in the SNP_FW_VER_{MAJOR,MINOR} macros.

 #define SEV_POLICY_NO_DBG      (1UL << 0)
 #define SEV_POLICY_ES          (1UL << 2)
+#define SNP_POLICY_SMT         (1ULL << 16)
+#define SNP_POLICY_RSVD_MBO    (1ULL << 17)
+#define SNP_POLICY_DBG         (1ULL << 19)
+#define SNP_POLICY             (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
+
+#define SNP_FW_VER_MAJOR(maj)  ((uint8_t)(maj) << SNP_POLICY_MAJOR_BIT)
+#define SNP_FW_VER_MINOR(min)  ((uint8_t)(min) << SNP_POLICY_MINOR_BIT)

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Pratik R. Sampat]

Hi Sean,

On 10/30/2024 8:46 AM, Sean Christopherson wrote:
> On Mon, Oct 28, 2024, Pratik R. Sampat wro4te:
>> On 10/28/2024 12:55 PM, Sean Christopherson wrote:
>>> On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
>>>>>> +		if (unlikely(!is_smt_active()))
>>>>>> +			snp_policy &= ~SNP_POLICY_SMT;
>>>>>
>>>>> Why does SNP_POLICY assume SMT?  And what is RSVD_MBO?  E.g. why not this?
>>>>>
>>>>> 		u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
>>>>>
>>>>
>>>> I think most systems support SMT so I enabled the bit in by default and
>>>> only unset it when there isn't any support.
>>>
>>> That's confusing though, because you're mixing architectural defines with semi-
>>> arbitrary selftests behavior.  RSVD_MBO on the other is apparently tightly coupled
>>> with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO
>>> needs to be part of SNP_POLICY
>>>
>>> If you want to have a *software*-defined default policy, then make it obvious that
>>> it's software defined.  E.g. name the #define SNP_DEFAULT_POLICY, not simply
>>> SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy,
>>> which it is not.  That said, IIUC, SMT *must* match the host configuration, i.e.
>>> whether or not SMT is set is non-negotiable.  In that case, there's zero value in
>>> defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems.
>>>
>>
>> Right, SMT should match the host configuration. Would a
>> SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro?
>>
>> Instead of,
>> #define SNP_POLICY	(SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
>>
>> Have something like this instead to make it generic and less ambiguous?
>> #define SNP_DEFAULT_POLICY()		 			       \
>> ({								       \
>> 	SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0);  \
>> })
> 
> No, unless it's the least awful option, don't hide dynamic functionality in a macro
> that looks like it holds static data.  The idea is totally fine, but put it in an
> actual helper, not a macro, _if_ there's actually a need for a default policy.
> If there's only ever one main path that creates SNP VMs, then I don't see the point
> in specifying a default policy.
> 

Currently, there just seems to be one path of doing (later the prefault
tests exercise it) so I'm not too averse to just dropping it and having
what bits needs to be set during the main path.

I had only introduced it so that it would be easy to specify a minimal
working SNP policy as it was for SEV and SEV-ES without too much
ambiguity. But if it's causing more issues than resolving, I can
definitely get rid of it.

>>> Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, 
>>> and that they are mutualy exclusive?  E.g. what happens if the full policy is simply
>>> SNP_POLICY_RSVD_MBO?
>>
>> SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and
>> SEV-ES - pg 31, Table 2
>> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf
>>
>> and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg
>> 27, Table 9
>> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
>>
>> In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is
>> set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set.
> 
> Ugh, one is SEV_xxx, the other is SNP_xxx.  Argh!  And IIUC, they are mutually
> exclusive (totally separate thigns?), because SNP guests use an 8-byte structure,
> whereas SEV/SEV-ES use a 4-byte structure, and with different layouts.
> 
> That means this is _extremely_ confusing.  Separate the SEV_xxx defines from the
> SNP_xxx defines, because other than a name, they have nothing in common.
> 

Right. I see how that can be confusing. Sure I can make sure not to
bundle up these defines together.

> +/* Minimum firmware version required for the SEV-SNP support */
> +#define SNP_FW_REQ_VER_MAJOR   1
> +#define SNP_FW_REQ_VER_MINOR   51
> 
> Side topic, why are these hardcoded?  And where did they come from?  If they're
> arbitrary KVM selftests values, make that super duper clear.

Well, it's not entirely arbitrary. This was the version that SNP GA'd
with first so that kind of became the minimum required version needed.

I think the only place we've documented this is here -
https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.

Maybe, I can modify the comment above to say something like -
Minimum general availability release firmware required for SEV-SNP support.

> 
> +#define SNP_POLICY_MINOR_BIT   0
> +#define SNP_POLICY_MAJOR_BIT   8
> 
> s/BIT/SHIFT.  "BIT" implies they are a single bit, which is obviously not the
> case.  But I vote to omit the extra #define entirely and just open code the shift
> in the SNP_FW_VER_{MAJOR,MINOR} macros.

Sure, I'll get rid of those couple of #defines and use them directly in
the macros.

Thanks!
Pratik

> 
>  #define SEV_POLICY_NO_DBG      (1UL << 0)
>  #define SEV_POLICY_ES          (1UL << 2)
> +#define SNP_POLICY_SMT         (1ULL << 16)
> +#define SNP_POLICY_RSVD_MBO    (1ULL << 17)
> +#define SNP_POLICY_DBG         (1ULL << 19)
> +#define SNP_POLICY             (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
> +
> +#define SNP_FW_VER_MAJOR(maj)  ((uint8_t)(maj) << SNP_POLICY_MAJOR_BIT)
> +#define SNP_FW_VER_MINOR(min)  ((uint8_t)(min) << SNP_POLICY_MINOR_BIT)

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Sean Christopherson]

On Wed, Oct 30, 2024, Pratik R. Sampat wrote:
> On 10/30/2024 8:46 AM, Sean Christopherson wrote:
> > +/* Minimum firmware version required for the SEV-SNP support */
> > +#define SNP_FW_REQ_VER_MAJOR   1
> > +#define SNP_FW_REQ_VER_MINOR   51
> > 
> > Side topic, why are these hardcoded?  And where did they come from?  If they're
> > arbitrary KVM selftests values, make that super duper clear.
> 
> Well, it's not entirely arbitrary. This was the version that SNP GA'd
> with first so that kind of became the minimum required version needed.
> 
> I think the only place we've documented this is here -
> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
> 
> Maybe, I can modify the comment above to say something like -
> Minimum general availability release firmware required for SEV-SNP support.

Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on
earth is that not checked and enforced by the kernel?  Relying on userspace to
not crash the host (or worse) because of unsupported firmware is not a winning
strategy.

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Pratik R. Sampat]

Hi Sean,

On 10/30/2024 12:57 PM, Sean Christopherson wrote:
> On Wed, Oct 30, 2024, Pratik R. Sampat wrote:
>> On 10/30/2024 8:46 AM, Sean Christopherson wrote:
>>> +/* Minimum firmware version required for the SEV-SNP support */
>>> +#define SNP_FW_REQ_VER_MAJOR   1
>>> +#define SNP_FW_REQ_VER_MINOR   51
>>>
>>> Side topic, why are these hardcoded?  And where did they come from?  If they're
>>> arbitrary KVM selftests values, make that super duper clear.
>>
>> Well, it's not entirely arbitrary. This was the version that SNP GA'd
>> with first so that kind of became the minimum required version needed.
>>
>> I think the only place we've documented this is here -
>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
>>
>> Maybe, I can modify the comment above to say something like -
>> Minimum general availability release firmware required for SEV-SNP support.
> 
> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on
> earth is that not checked and enforced by the kernel?  Relying on userspace to
> not crash the host (or worse) because of unsupported firmware is not a winning
> strategy.

We do check against the firmware level 1.51 while setting things up
first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail
out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any
other corresponding SNP calls should fail cleanly without any adverse
effects to the host.

From the positive selftest perspective though, we want to make sure it's
both supported and enabled, and skip the test if not.
I believe we can tell if it's supported by the platform using the MSR -
MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM
capabilities. However, to determine if it's enabled from the kernel, I
made this check here. Having said that, I do agree that there should
probably be a better way to expose this support to the userspace.

Thanks
Pratik

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Sean Christopherson]

On Thu, Oct 31, 2024, Pratik R. Sampat wrote:
> Hi Sean,
> 
> On 10/30/2024 12:57 PM, Sean Christopherson wrote:
> > On Wed, Oct 30, 2024, Pratik R. Sampat wrote:
> >> On 10/30/2024 8:46 AM, Sean Christopherson wrote:
> >>> +/* Minimum firmware version required for the SEV-SNP support */
> >>> +#define SNP_FW_REQ_VER_MAJOR   1
> >>> +#define SNP_FW_REQ_VER_MINOR   51
> >>>
> >>> Side topic, why are these hardcoded?  And where did they come from?  If they're
> >>> arbitrary KVM selftests values, make that super duper clear.
> >>
> >> Well, it's not entirely arbitrary. This was the version that SNP GA'd
> >> with first so that kind of became the minimum required version needed.
> >>
> >> I think the only place we've documented this is here -
> >> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
> >>
> >> Maybe, I can modify the comment above to say something like -
> >> Minimum general availability release firmware required for SEV-SNP support.
> > 
> > Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on
> > earth is that not checked and enforced by the kernel?  Relying on userspace to
> > not crash the host (or worse) because of unsupported firmware is not a winning
> > strategy.
> 
> We do check against the firmware level 1.51 while setting things up
> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail
> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any
> other corresponding SNP calls should fail cleanly without any adverse
> effects to the host.

And I'm saying, that's not good enough.  If the platform doesn't support SNP,
the KVM *must not* advertise support for SNP.

> From the positive selftest perspective though, we want to make sure it's
> both supported and enabled, and skip the test if not.

No, we want the test to assert that KVM reports SNP support if and only if SNP
is 100% supported.

> I believe we can tell if it's supported by the platform using the MSR -
> MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM
> capabilities. However, to determine if it's enabled from the kernel, I
> made this check here. Having said that, I do agree that there should
> probably be a better way to expose this support to the userspace.
> 
> Thanks
> Pratik

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Pratik R. Sampat]

On 10/31/2024 11:27 AM, Sean Christopherson wrote:
> On Thu, Oct 31, 2024, Pratik R. Sampat wrote:
>> Hi Sean,
>>
>> On 10/30/2024 12:57 PM, Sean Christopherson wrote:
>>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote:
>>>> On 10/30/2024 8:46 AM, Sean Christopherson wrote:
>>>>> +/* Minimum firmware version required for the SEV-SNP support */
>>>>> +#define SNP_FW_REQ_VER_MAJOR   1
>>>>> +#define SNP_FW_REQ_VER_MINOR   51
>>>>>
>>>>> Side topic, why are these hardcoded?  And where did they come from?  If they're
>>>>> arbitrary KVM selftests values, make that super duper clear.
>>>>
>>>> Well, it's not entirely arbitrary. This was the version that SNP GA'd
>>>> with first so that kind of became the minimum required version needed.
>>>>
>>>> I think the only place we've documented this is here -
>>>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
>>>>
>>>> Maybe, I can modify the comment above to say something like -
>>>> Minimum general availability release firmware required for SEV-SNP support.
>>>
>>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on
>>> earth is that not checked and enforced by the kernel?  Relying on userspace to
>>> not crash the host (or worse) because of unsupported firmware is not a winning
>>> strategy.
>>
>> We do check against the firmware level 1.51 while setting things up
>> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail
>> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any
>> other corresponding SNP calls should fail cleanly without any adverse
>> effects to the host.
> 
> And I'm saying, that's not good enough.  If the platform doesn't support SNP,
> the KVM *must not* advertise support for SNP.
> 

Sure, fair to expect this. Currently, if the FW check fails, SNP is not
setup and there is nothing that indicates in the KVM capabilities (apart
from one dmesg error) that the support does not exist.

One thing I could do (as an independent patch) is to introduce a CC API
that abstracts the FW version check made by the CCP module. Since sev
platform status can be gotten before INIT to extract the major and minor
version numbers, KVM can also call into this API and use that to decide
if the KVM capabilities for SNP must be set or not.

Thanks!
Pratik

>> From the positive selftest perspective though, we want to make sure it's
>> both supported and enabled, and skip the test if not.
> 
> No, we want the test to assert that KVM reports SNP support if and only if SNP
> is 100% supported.
> 
>> I believe we can tell if it's supported by the platform using the MSR -
>> MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM
>> capabilities. However, to determine if it's enabled from the kernel, I
>> made this check here. Having said that, I do agree that there should
>> probably be a better way to expose this support to the userspace.
>>
>> Thanks
>> Pratik

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Sean Christopherson]

On Mon, Nov 04, 2024, Pratik R. Sampat wrote:
> 
> 
> On 10/31/2024 11:27 AM, Sean Christopherson wrote:
> > On Thu, Oct 31, 2024, Pratik R. Sampat wrote:
> >> Hi Sean,
> >>
> >> On 10/30/2024 12:57 PM, Sean Christopherson wrote:
> >>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote:
> >>>> On 10/30/2024 8:46 AM, Sean Christopherson wrote:
> >>>>> +/* Minimum firmware version required for the SEV-SNP support */
> >>>>> +#define SNP_FW_REQ_VER_MAJOR   1
> >>>>> +#define SNP_FW_REQ_VER_MINOR   51
> >>>>>
> >>>>> Side topic, why are these hardcoded?  And where did they come from?  If they're
> >>>>> arbitrary KVM selftests values, make that super duper clear.
> >>>>
> >>>> Well, it's not entirely arbitrary. This was the version that SNP GA'd
> >>>> with first so that kind of became the minimum required version needed.
> >>>>
> >>>> I think the only place we've documented this is here -
> >>>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
> >>>>
> >>>> Maybe, I can modify the comment above to say something like -
> >>>> Minimum general availability release firmware required for SEV-SNP support.
> >>>
> >>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on
> >>> earth is that not checked and enforced by the kernel?  Relying on userspace to
> >>> not crash the host (or worse) because of unsupported firmware is not a winning
> >>> strategy.
> >>
> >> We do check against the firmware level 1.51 while setting things up
> >> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail
> >> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any
> >> other corresponding SNP calls should fail cleanly without any adverse
> >> effects to the host.
> > 
> > And I'm saying, that's not good enough.  If the platform doesn't support SNP,
> > the KVM *must not* advertise support for SNP.
> > 
> 
> Sure, fair to expect this. Currently, if the FW check fails, SNP is not
> setup and there is nothing that indicates in the KVM capabilities (apart
> from one dmesg error) that the support does not exist.
> 
> One thing I could do (as an independent patch) is to introduce a CC API
> that abstracts the FW version check made by the CCP module. Since sev
> platform status can be gotten before INIT to extract the major and minor
> version numbers, KVM can also call into this API and use that to decide
> if the KVM capabilities for SNP must be set or not.

Why is CC_ATTR_HOST_SEV_SNP set if hardware/firmware can't actually support SNP?
KVM shouldn't have to care about some arbitrary firmware API version, the whole
point of a driver is so that KVM doesn't have to deal with such details.

I'm a-ok with a KVM selftest *asserting* that the kernel isn't broken, but KVM
itself shouldn't need to manually check the firmware version.

Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Pratik R. Sampat]

On 11/4/2024 5:47 PM, Sean Christopherson wrote:
> On Mon, Nov 04, 2024, Pratik R. Sampat wrote:
>>
>>
>> On 10/31/2024 11:27 AM, Sean Christopherson wrote:
>>> On Thu, Oct 31, 2024, Pratik R. Sampat wrote:
>>>> Hi Sean,
>>>>
>>>> On 10/30/2024 12:57 PM, Sean Christopherson wrote:
>>>>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote:
>>>>>> On 10/30/2024 8:46 AM, Sean Christopherson wrote:
>>>>>>> +/* Minimum firmware version required for the SEV-SNP support */
>>>>>>> +#define SNP_FW_REQ_VER_MAJOR   1
>>>>>>> +#define SNP_FW_REQ_VER_MINOR   51
>>>>>>>
>>>>>>> Side topic, why are these hardcoded?  And where did they come from?  If they're
>>>>>>> arbitrary KVM selftests values, make that super duper clear.
>>>>>>
>>>>>> Well, it's not entirely arbitrary. This was the version that SNP GA'd
>>>>>> with first so that kind of became the minimum required version needed.
>>>>>>
>>>>>> I think the only place we've documented this is here -
>>>>>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
>>>>>>
>>>>>> Maybe, I can modify the comment above to say something like -
>>>>>> Minimum general availability release firmware required for SEV-SNP support.
>>>>>
>>>>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on
>>>>> earth is that not checked and enforced by the kernel?  Relying on userspace to
>>>>> not crash the host (or worse) because of unsupported firmware is not a winning
>>>>> strategy.
>>>>
>>>> We do check against the firmware level 1.51 while setting things up
>>>> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail
>>>> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any
>>>> other corresponding SNP calls should fail cleanly without any adverse
>>>> effects to the host.
>>>
>>> And I'm saying, that's not good enough.  If the platform doesn't support SNP,
>>> the KVM *must not* advertise support for SNP.
>>>
>>
>> Sure, fair to expect this. Currently, if the FW check fails, SNP is not
>> setup and there is nothing that indicates in the KVM capabilities (apart
>> from one dmesg error) that the support does not exist.
>>
>> One thing I could do (as an independent patch) is to introduce a CC API
>> that abstracts the FW version check made by the CCP module. Since sev
>> platform status can be gotten before INIT to extract the major and minor
>> version numbers, KVM can also call into this API and use that to decide
>> if the KVM capabilities for SNP must be set or not.
> 
> Why is CC_ATTR_HOST_SEV_SNP set if hardware/firmware can't actually support SNP?
> KVM shouldn't have to care about some arbitrary firmware API version, the whole
> point of a driver is so that KVM doesn't have to deal with such details.
> 
> I'm a-ok with a KVM selftest *asserting* that the kernel isn't broken, but KVM
> itself shouldn't need to manually check the firmware version.

Clearing CC_ATTR_HOST_SEV_SNP when the init fails is one approach to go
about it. Here we could clear it from here and eventually that would
prevent the the SNP feature being set in KVM capability.

+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1099,6 +1099,7 @@ static int __sev_snp_init_locked(int *error)
                return 0;

        if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
+               cc_platform_clear(CC_ATTR_HOST_SEV_SNP);

A suggestion where we could more directly approach this could be by
exporting an explicit check from ccp instead?

--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -122,6 +122,12 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
        return false;
 }

+bool sev_snp_fw_available(void)
+{
+    return sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
+}
+EXPORT_SYMBOL_GPL(sev_snp_fw_available);

which could be then called on will as follows:

--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void)
        sev_es_asid_count = min_sev_asid - 1;
        WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
        sev_es_supported = true;
-       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
+       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP) && sev_snp_fw_available();

 out:
        if (boot_cpu_has(X86_FEATURE_SEV))

This would ensure that we could enable and disable the SNP capability
even in the case where maybe the firmware can get hotloaded using the
proposed download_firmware_ex[1] or in cases where the INIT could be
deferred; all while KVM wouldn't need to be bothered with the API
details.

[1]: https://lore.kernel.org/lkml/20241029183907.3536683-1-dionnaglaze@google.com/

[PATCH v3 3/9] KVM: selftests: Add SNP to shutdown testing

[Pratik R. Sampat]

Parameterize the shutdown test to include the SEV-SNP VM type

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 tools/testing/selftests/kvm/x86_64/sev_smoke_test.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
index 12d466915074..8e798f5a2a53 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
@@ -193,16 +193,14 @@ static void guest_shutdown_code(void)
 	__asm__ __volatile__("ud2");
 }
 
-static void test_sev_es_shutdown(void)
+static void test_sev_shutdown(uint32_t type, uint64_t policy)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 
-	uint32_t type = KVM_X86_SEV_ES_VM;
-
 	vm = vm_sev_create_with_one_vcpu(type, guest_shutdown_code, &vcpu);
 
-	vm_sev_launch(vm, SEV_POLICY_ES, NULL);
+	vm_sev_launch(vm, policy, NULL);
 
 	vcpu_run(vcpu);
 	TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SHUTDOWN,
@@ -223,7 +221,7 @@ int main(int argc, char *argv[])
 		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
 		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
 
-		test_sev_es_shutdown();
+		test_sev_shutdown(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
 
 		if (kvm_has_cap(KVM_CAP_XCRS) &&
 		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
@@ -245,6 +243,8 @@ int main(int argc, char *argv[])
 			 SNP_FW_VER_MAJOR(SNP_FW_REQ_VER_MAJOR) |
 			 SNP_FW_VER_MINOR(SNP_FW_REQ_VER_MINOR));
 
+		test_sev_shutdown(KVM_X86_SNP_VM, snp_policy);
+
 		if (kvm_has_cap(KVM_CAP_XCRS) &&
 		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
 			test_sync_vmsa(KVM_X86_SNP_VM, snp_policy);
-- 
2.34.1

[PATCH v3 4/9] KVM: selftests: SEV IOCTL test

[Pratik R. Sampat]

Introduce tests for sev and sev-es ioctl that exercises the boot path
of launch, update and finish on an invalid policy.

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 .../selftests/kvm/x86_64/sev_smoke_test.c     | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
index 8e798f5a2a53..5fa4ee27609b 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
@@ -142,12 +142,96 @@ static void test_sync_vmsa(uint32_t type, uint64_t policy)
 	kvm_vm_free(vm);
 }
 
+static void sev_guest_neg_status_assert(struct kvm_vm *vm, uint32_t type)
+{
+	struct kvm_sev_guest_status status;
+	int ret;
+
+	ret = __vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
+	TEST_ASSERT(ret, "KVM_SEV_GUEST_STATUS should fail, invalid VM Type.");
+}
+
+static void vm_sev_es_launch_neg(struct kvm_vm *vm, uint32_t type, uint64_t policy)
+{
+	int ret;
+
+	/* Launch start with policy SEV_POLICY_NO_DBG (0x0) */
+	ret = __sev_vm_launch_start(vm, 0);
+	TEST_ASSERT(ret,
+		    "KVM_SEV_LAUNCH_START should fail due to type (%d) - policy(0x0) mismatch",
+		    type);
+
+	ret = __sev_vm_launch_update(vm, policy);
+	TEST_ASSERT(ret,
+		    "KVM_SEV_LAUNCH_UPDATE should fail due to LAUNCH_START. type: %d policy: 0x%lx",
+		    type, policy);
+	sev_guest_neg_status_assert(vm, type);
+
+	ret = __sev_vm_launch_measure(vm, alloca(256));
+	TEST_ASSERT(ret,
+		    "KVM_SEV_LAUNCH_UPDATE should fail due to LAUNCH_START. type: %d policy: 0x%lx",
+		    type, policy);
+	sev_guest_neg_status_assert(vm, type);
+
+	ret = __sev_vm_launch_finish(vm);
+	TEST_ASSERT(ret,
+		    "KVM_SEV_LAUNCH_UPDATE should fail due to LAUNCH_START. type: %d policy: 0x%lx",
+		    type, policy);
+	sev_guest_neg_status_assert(vm, type);
+}
+
+/*
+ * Test for SEV ioctl launch path
+ * VMs of the type SEV and SEV-ES are created, however they are launched with
+ * an empty policy to observe the effect on the control flow of launching a VM.
+ *
+ * SEV - Expected to pass through the path of launch start, update, measure,
+ * and finish. vcpu_run expected to fail with error KVM_EXIT_IO.
+ *
+ * SEV-ES - Expected to fail the launch start as vm created with type
+ * KVM_X86_DEFAULT_VM but policy passed to launch start is KVM_X86_SEV_ES_VM.
+ * Post this, calls that pass the correct policy to update, measure, and finish
+ * are also expected to fail cascading.
+ */
+static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
+{
+	struct kvm_vcpu *vcpu;
+	int exp_exit_reason;
+	struct kvm_vm *vm;
+	struct ucall uc;
+
+	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
+
+	if (type == KVM_X86_SEV_VM) {
+		sev_vm_launch(vm, 0);
+		sev_vm_launch_measure(vm, alloca(256));
+		sev_vm_launch_finish(vm);
+	} else {
+		vm_sev_es_launch_neg(vm, type, policy);
+	}
+
+	vcpu_run(vcpu);
+	get_ucall(vcpu, &uc);
+	if (type == KVM_X86_SEV_VM)
+		exp_exit_reason = KVM_EXIT_IO;
+	else
+		exp_exit_reason = KVM_EXIT_FAIL_ENTRY;
+
+	TEST_ASSERT(vcpu->run->exit_reason == exp_exit_reason,
+		    "vcpu_run failed exit expected: %d, got: %d",
+		    exp_exit_reason, vcpu->run->exit_reason);
+
+	kvm_vm_free(vm);
+}
+
 static void test_sev(void *guest_code, uint32_t type, uint64_t policy)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 	struct ucall uc;
 
+	test_sev_launch(guest_code, type, policy);
+
 	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
 
 	/* TODO: Validate the measurement is as expected. */
-- 
2.34.1

[PATCH v3 5/9] KVM: selftests: SNP IOCTL test

[Pratik R. Sampat]

Introduce testing of SNP ioctl calls. Tests attributes such as flags,
page types, and policies in various combinations along the SNP launch
path.

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 .../testing/selftests/kvm/include/kvm_util.h  |  11 ++
 .../selftests/kvm/x86_64/sev_smoke_test.c     | 140 +++++++++++++++++-
 2 files changed, 150 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index bc7c242480d6..ab213708b551 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -912,6 +912,17 @@ static inline struct kvm_vm *vm_create(uint32_t nr_runnable_vcpus)
 	return __vm_create(VM_SHAPE_DEFAULT, nr_runnable_vcpus, 0);
 }
 
+static inline struct kvm_vm *vm_create_type(unsigned long type,
+					    uint32_t nr_runnable_vcpus)
+{
+	const struct vm_shape shape = {
+		.mode = VM_MODE_DEFAULT,
+		.type = type,
+	};
+
+	return __vm_create(shape, nr_runnable_vcpus, 0);
+}
+
 struct kvm_vm *__vm_create_with_vcpus(struct vm_shape shape, uint32_t nr_vcpus,
 				      uint64_t extra_mem_pages,
 				      void *guest_code, struct kvm_vcpu *vcpus[]);
diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
index 5fa4ee27609b..9a7efbe214ce 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
@@ -224,13 +224,151 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
 	kvm_vm_free(vm);
 }
 
+static int __test_snp_launch_start(uint32_t type, uint64_t policy,
+				   uint8_t flags, bool assert)
+{
+	struct kvm_vm *vm;
+	int ret = 0;
+
+	vm = vm_create_type(type, 1);
+	ret = __snp_vm_launch_start(vm, policy, flags);
+	if (assert)
+		TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_START, ret, vm);
+	kvm_vm_free(vm);
+
+	return ret;
+}
+
+static void test_snp_launch_start(uint32_t type, uint64_t policy)
+{
+	uint8_t i;
+	int ret;
+
+	/* Flags must be zero for success */
+	__test_snp_launch_start(type, policy, 0, true);
+
+	for (i = 1; i < 8; i++) {
+		ret = __test_snp_launch_start(type, policy, BIT(i), false);
+		TEST_ASSERT(ret && errno == EINVAL,
+			    "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag\n"
+			    "(type: %d policy: 0x%lx, flag: 0x%lx)",
+			    type, policy, BIT(i));
+	}
+
+	ret = __test_snp_launch_start(type, SNP_POLICY_SMT, 0, false);
+	TEST_ASSERT(ret && errno == EINVAL,
+		    "KVM_SEV_SNP_LAUNCH_START should fail, SNP_POLICY_RSVD_MBO policy bit not set\n"
+		    "(type: %d policy: 0x%llx, flags: 0x0)",
+		    type, SNP_POLICY_SMT);
+
+	ret = __test_snp_launch_start(type, SNP_POLICY_RSVD_MBO, 0, false);
+	if (unlikely(!is_smt_active())) {
+		TEST_ASSERT(!ret,
+			    "KVM_SEV_SNP_LAUNCH_START should succeed, SNP_POLICY_SMT not required on non-SMT systems\n"
+			    "(type: %d policy: 0x%llx, flags: 0x0)",
+			    type, SNP_POLICY_RSVD_MBO);
+	} else {
+		TEST_ASSERT(ret && errno == EINVAL,
+			    "KVM_SEV_SNP_LAUNCH_START should fail, SNP_POLICY_SMT is not set on a SMT system\n"
+			    "(type: %d policy: 0x%llx, flags: 0x0)",
+			    type, SNP_POLICY_RSVD_MBO);
+	}
+
+	ret = __test_snp_launch_start(type, SNP_POLICY |
+				      SNP_FW_VER_MAJOR(UINT8_MAX) |
+				      SNP_FW_VER_MINOR(UINT8_MAX), 0, false);
+	TEST_ASSERT(ret && errno == EIO,
+		    "KVM_SEV_SNP_LAUNCH_START should fail, invalid version\n"
+		    "expected: %d.%d got: %d.%d (type: %d policy: 0x%llx, flags: 0x0)",
+		    SNP_FW_REQ_VER_MAJOR, SNP_FW_REQ_VER_MINOR,
+		    UINT8_MAX, UINT8_MAX, type,
+		    SNP_POLICY | SNP_FW_VER_MAJOR(UINT8_MAX) | SNP_FW_VER_MINOR(UINT8_MAX));
+}
+
+static void test_snp_launch_update(uint32_t type, uint64_t policy)
+{
+	struct kvm_vm *vm;
+	int ret;
+
+	for (int pgtype = 0; pgtype <= KVM_SEV_SNP_PAGE_TYPE_CPUID + 1; pgtype++) {
+		vm = vm_create_type(type, 1);
+		snp_vm_launch_start(vm, policy);
+		ret = __snp_vm_launch_update(vm, pgtype);
+
+		switch (pgtype) {
+		case KVM_SEV_SNP_PAGE_TYPE_NORMAL:
+		case KVM_SEV_SNP_PAGE_TYPE_ZERO:
+		case KVM_SEV_SNP_PAGE_TYPE_UNMEASURED:
+		case KVM_SEV_SNP_PAGE_TYPE_SECRETS:
+			TEST_ASSERT(!ret,
+				    "KVM_SEV_SNP_LAUNCH_UPDATE should succeed, invalid Page type %d",
+				    pgtype);
+			break;
+		case KVM_SEV_SNP_PAGE_TYPE_CPUID:
+			/*
+			 * Expect failure if performed on random pages of
+			 * guest memory rather than properly formatted CPUID Page
+			 */
+			TEST_ASSERT(ret && errno == EIO,
+				    "KVM_SEV_SNP_LAUNCH_UPDATE should fail,\n"
+				    "CPUID page type only valid for CPUID pages");
+			break;
+		default:
+			TEST_ASSERT(ret && errno == EINVAL,
+				    "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type");
+		}
+
+		kvm_vm_free(vm);
+	}
+}
+
+void test_snp_launch_finish(uint32_t type, uint64_t policy)
+{
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_create_type(type, 1);
+	snp_vm_launch_start(vm, policy);
+	snp_vm_launch_update(vm);
+	/* Flags must be zero for success */
+	snp_vm_launch_finish(vm);
+	kvm_vm_free(vm);
+
+	for (int i = 1; i < 16; i++) {
+		vm = vm_create_type(type, 1);
+		snp_vm_launch_start(vm, policy);
+		snp_vm_launch_update(vm);
+		ret = __snp_vm_launch_finish(vm, BIT(i));
+		TEST_ASSERT(ret && errno == EINVAL,
+			    "KVM_SEV_SNP_LAUNCH_FINISH should fail, invalid flag\n"
+			    "(type: %d policy: 0x%lx, flag: 0x%lx)",
+			    type, policy, BIT(i));
+		kvm_vm_free(vm);
+	}
+}
+
+static void test_snp_ioctl(void *guest_code, uint32_t type, uint64_t policy)
+{
+	test_snp_launch_start(type, policy);
+	test_snp_launch_update(type, policy);
+	test_snp_launch_finish(type, policy);
+}
+
+static void test_sev_ioctl(void *guest_code, uint32_t type, uint64_t policy)
+{
+	test_sev_launch(guest_code, type, policy);
+}
+
 static void test_sev(void *guest_code, uint32_t type, uint64_t policy)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 	struct ucall uc;
 
-	test_sev_launch(guest_code, type, policy);
+	if (type == KVM_X86_SNP_VM)
+		test_snp_ioctl(guest_code, type, policy);
+	else
+		test_sev_ioctl(guest_code, type, policy);
 
 	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
 
-- 
2.34.1

[PATCH v3 6/9] KVM: selftests: SEV-SNP test for KVM_SEV_INIT2

[Pratik R. Sampat]

Add SEV-SNP VM type to exercise the KVM_SEV_INIT2 call.

Also ensure that SNP case is skipped for scenarios where CPUID supports
it but KVM does not so that a failure is not reported for such cases.

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 .../testing/selftests/kvm/x86_64/sev_init2_tests.c  | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c
index 3fb967f40c6a..3f8fb2cc3431 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c
@@ -28,6 +28,7 @@
 int kvm_fd;
 u64 supported_vmsa_features;
 bool have_sev_es;
+bool have_snp;
 
 static int __sev_ioctl(int vm_fd, int cmd_id, void *data)
 {
@@ -83,6 +84,9 @@ void test_vm_types(void)
 	if (have_sev_es)
 		test_init2(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){});
 
+	if (have_snp)
+		test_init2(KVM_X86_SNP_VM, &(struct kvm_sev_init){});
+
 	test_init2_invalid(0, &(struct kvm_sev_init){},
 			   "VM type is KVM_X86_DEFAULT_VM");
 	if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM))
@@ -138,15 +142,24 @@ int main(int argc, char *argv[])
 		    "sev-es: KVM_CAP_VM_TYPES (%x) does not match cpuid (checking %x)",
 		    kvm_check_cap(KVM_CAP_VM_TYPES), 1 << KVM_X86_SEV_ES_VM);
 
+	have_snp = kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SNP_VM);
+	TEST_ASSERT(!have_snp || kvm_cpu_has(X86_FEATURE_SNP),
+		    "sev-snp: KVM_CAP_VM_TYPES (%x) indicates SNP support (bit %d), but CPUID does not",
+		    kvm_check_cap(KVM_CAP_VM_TYPES), KVM_X86_SNP_VM);
+
 	test_vm_types();
 
 	test_flags(KVM_X86_SEV_VM);
 	if (have_sev_es)
 		test_flags(KVM_X86_SEV_ES_VM);
+	if (have_snp)
+		test_flags(KVM_X86_SNP_VM);
 
 	test_features(KVM_X86_SEV_VM, 0);
 	if (have_sev_es)
 		test_features(KVM_X86_SEV_ES_VM, supported_vmsa_features);
+	if (have_snp)
+		test_features(KVM_X86_SNP_VM, supported_vmsa_features);
 
 	return 0;
 }
-- 
2.34.1

[PATCH v3 7/9] KVM: selftests: Add interface to manually flag protected/encrypted ranges

[Pratik R. Sampat]

From: Michael Roth <michael.roth@amd.com>

For SEV and SNP, currently __vm_phy_pages_alloc() handles setting the
region->protected_phy_pages bitmap to mark that the region needs to be
encrypted/measured into the initial guest state prior to
finalizing/starting the guest. It also marks what GPAs need to be mapped
as encrypted in the initial guest page table.

This works when using virtual/physical allocators to manage memory, but
if the test manages allocations/mapping directly then an alternative is
needed to set region->protected_phy_pages directly. Add an interface to
handle that.

Signed-off-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 .../testing/selftests/kvm/include/kvm_util.h  |  2 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 45 +++++++++++++++++--
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index ab213708b551..642740fe1c59 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -394,6 +394,8 @@ static inline void vm_set_memory_attributes(struct kvm_vm *vm, uint64_t gpa,
 	vm_ioctl(vm, KVM_SET_MEMORY_ATTRIBUTES, &attr);
 }
 
+void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot,
+			  vm_paddr_t paddr, size_t num);
 
 static inline void vm_mem_set_private(struct kvm_vm *vm, uint64_t gpa,
 				      uint64_t size)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index bbf90ad224da..d44a37aebcec 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1991,6 +1991,43 @@ const char *exit_reason_str(unsigned int exit_reason)
 	return "Unknown";
 }
 
+/*
+ * Set what guest GFNs need to be encrypted prior to finalizing a CoCo VM.
+ *
+ * Input Args:
+ *   vm - Virtual Machine
+ *   memslot - Memory region to allocate page from
+ *   paddr - Start of physical address to mark as encrypted
+ *   num - number of pages
+ *
+ * Output Args: None
+ *
+ * Return: None
+ *
+ * Generally __vm_phy_pages_alloc() will handle this automatically, but
+ * for cases where the test handles managing the physical allocation and
+ * mapping directly this interface should be used to mark physical pages
+ * that are intended to be encrypted as part of the initial guest state.
+ * This will also affect whether virt_map()/virt_pg_map() will map the
+ * page as encrypted or not in the initial guest page table.
+ *
+ * If the initial guest state has already been finalized, then setting
+ * it as encrypted will essentially be a noop since nothing more can be
+ * encrypted into the initial guest state at that point.
+ */
+void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot,
+			  vm_paddr_t paddr, size_t num)
+{
+	struct userspace_mem_region *region;
+	sparsebit_idx_t pg, base;
+
+	base = paddr >> vm->page_shift;
+	region = memslot2region(vm, memslot);
+
+	for (pg = base; pg < base + num; ++pg)
+		sparsebit_set(region->protected_phy_pages, pg);
+}
+
 /*
  * Physical Contiguous Page Allocator
  *
@@ -2048,11 +2085,11 @@ vm_paddr_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 		abort();
 	}
 
-	for (pg = base; pg < base + num; ++pg) {
+	for (pg = base; pg < base + num; ++pg)
 		sparsebit_clear(region->unused_phy_pages, pg);
-		if (protected)
-			sparsebit_set(region->protected_phy_pages, pg);
-	}
+
+	if (protected)
+		vm_mem_set_protected(vm, memslot, base << vm->page_shift, num);
 
 	return base * vm->page_size;
 }
-- 
2.34.1

Re: [PATCH v3 7/9] KVM: selftests: Add interface to manually flag protected/encrypted ranges

[Sean Christopherson]

On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
> From: Michael Roth <michael.roth@amd.com>
> 
> For SEV and SNP, currently __vm_phy_pages_alloc() handles setting the
> region->protected_phy_pages bitmap to mark that the region needs to be
> encrypted/measured into the initial guest state prior to

Nothing needs to be measured, no?  (because there's no attestation)

> finalizing/starting the guest. It also marks what GPAs need to be mapped
> as encrypted in the initial guest page table.

...

>  static inline void vm_mem_set_private(struct kvm_vm *vm, uint64_t gpa,
>  				      uint64_t size)
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index bbf90ad224da..d44a37aebcec 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1991,6 +1991,43 @@ const char *exit_reason_str(unsigned int exit_reason)
>  	return "Unknown";
>  }
>  
> +/*
> + * Set what guest GFNs need to be encrypted prior to finalizing a CoCo VM.
> + *
> + * Input Args:
> + *   vm - Virtual Machine
> + *   memslot - Memory region to allocate page from
> + *   paddr - Start of physical address to mark as encrypted
> + *   num - number of pages
> + *
> + * Output Args: None
> + *
> + * Return: None
> + *
> + * Generally __vm_phy_pages_alloc() will handle this automatically, but
> + * for cases where the test handles managing the physical allocation and
> + * mapping directly this interface should be used to mark physical pages
> + * that are intended to be encrypted as part of the initial guest state.
> + * This will also affect whether virt_map()/virt_pg_map() will map the
> + * page as encrypted or not in the initial guest page table.
> + *
> + * If the initial guest state has already been finalized, then setting
> + * it as encrypted will essentially be a noop since nothing more can be
> + * encrypted into the initial guest state at that point.
> + */
> +void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot,
> +			  vm_paddr_t paddr, size_t num)
> +{
> +	struct userspace_mem_region *region;
> +	sparsebit_idx_t pg, base;
> +
> +	base = paddr >> vm->page_shift;
> +	region = memslot2region(vm, memslot);

Please no, doing a memslot lookup in a helper like this is only going to encourage
proliferation of bad code.  vm_mem_add() really should be able to mark the region
as protected.

E.g. practically speaking, the only code that will be able to use this helper is
code that is marking the entire memslot as protection.  And ability to _clear_
the protected_phy_pages bit is conspicuously missing.

> +
> +	for (pg = base; pg < base + num; ++pg)
> +		sparsebit_set(region->protected_phy_pages, pg);
> +}

Re: [PATCH v3 7/9] KVM: selftests: Add interface to manually flag protected/encrypted ranges

[Pratik R. Sampat]

Hi Sean,

On 10/14/2024 5:58 PM, Sean Christopherson wrote:
> On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
>> From: Michael Roth <michael.roth@amd.com>
>>
>> For SEV and SNP, currently __vm_phy_pages_alloc() handles setting the
>> region->protected_phy_pages bitmap to mark that the region needs to be
>> encrypted/measured into the initial guest state prior to
> 
> Nothing needs to be measured, no?  (because there's no attestation)
> 

Right.

>> finalizing/starting the guest. It also marks what GPAs need to be mapped
>> as encrypted in the initial guest page table.
> 
> ...
> 
>>  static inline void vm_mem_set_private(struct kvm_vm *vm, uint64_t gpa,
>>  				      uint64_t size)
>> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
>> index bbf90ad224da..d44a37aebcec 100644
>> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
>> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
>> @@ -1991,6 +1991,43 @@ const char *exit_reason_str(unsigned int exit_reason)
>>  	return "Unknown";
>>  }
>>  
>> +/*
>> + * Set what guest GFNs need to be encrypted prior to finalizing a CoCo VM.
>> + *
>> + * Input Args:
>> + *   vm - Virtual Machine
>> + *   memslot - Memory region to allocate page from
>> + *   paddr - Start of physical address to mark as encrypted
>> + *   num - number of pages
>> + *
>> + * Output Args: None
>> + *
>> + * Return: None
>> + *
>> + * Generally __vm_phy_pages_alloc() will handle this automatically, but
>> + * for cases where the test handles managing the physical allocation and
>> + * mapping directly this interface should be used to mark physical pages
>> + * that are intended to be encrypted as part of the initial guest state.
>> + * This will also affect whether virt_map()/virt_pg_map() will map the
>> + * page as encrypted or not in the initial guest page table.
>> + *
>> + * If the initial guest state has already been finalized, then setting
>> + * it as encrypted will essentially be a noop since nothing more can be
>> + * encrypted into the initial guest state at that point.
>> + */
>> +void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot,
>> +			  vm_paddr_t paddr, size_t num)
>> +{
>> +	struct userspace_mem_region *region;
>> +	sparsebit_idx_t pg, base;
>> +
>> +	base = paddr >> vm->page_shift;
>> +	region = memslot2region(vm, memslot);
> 
> Please no, doing a memslot lookup in a helper like this is only going to encourage
> proliferation of bad code.  vm_mem_add() really should be able to mark the region
> as protected.
> 
> E.g. practically speaking, the only code that will be able to use this helper is
> code that is marking the entire memslot as protection.  And ability to _clear_
> the protected_phy_pages bit is conspicuously missing.
> 

From my understanding, vm_mem_add() only allocates the protected
sparsebits but does not populate them. For that maybe a better path
would be to go through, vm_phy_pages_alloc() - something similar to how
the set_memory_region_test.c does? I think we avoided doing that because
vm_phys_pages_alloc() takes a paddr_min rather than guaranteeing the
specific paddr and even then would need the similar virt_map() logic to
stay. If this is a cleaner approach though, I'm happy to redo this code
that way.

Thanks!
Pratik

>> +
>> +	for (pg = base; pg < base + num; ++pg)
>> +		sparsebit_set(region->protected_phy_pages, pg);
>> +}

[PATCH v3 8/9] KVM: selftests: Add a CoCo-specific test for KVM_PRE_FAULT_MEMORY

[Pratik R. Sampat]

From: Michael Roth <michael.roth@amd.com>

SEV, SEV-ES, and SNP have a few corner cases where there is potential
for KVM_PRE_FAULT_MEMORY to behave differently depending on when it is
issued during initial guest setup. Exercising these various paths
requires a bit more fine-grained control over when the
KVM_PRE_FAULT_MEMORY requests are issued while setting up the guests.

Since these CoCo-specific events are likely to be architecture-specific
KST helpers, take the existing generic test in pre_fault_memory_test.c
as a starting template, and then introduce an x86-specific version of
it with expanded coverage for SEV, SEV-ES, and SNP.

Since there's a reasonable chance that TDX could extend this for similar
testing of TDX, give it a "coco-" prefix rather than an SEV-specific
one.

Signed-off-by: Michael Roth <michael.roth@amd.com>
Co-developed-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../kvm/x86_64/coco_pre_fault_memory_test.c   | 314 ++++++++++++++++++
 2 files changed, 315 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 45cb70c048bb..7b97750a7d71 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -129,6 +129,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/amx_test
 TEST_GEN_PROGS_x86_64 += x86_64/max_vcpuid_cap_test
 TEST_GEN_PROGS_x86_64 += x86_64/triple_fault_event_test
 TEST_GEN_PROGS_x86_64 += x86_64/recalc_apic_map_test
+TEST_GEN_PROGS_x86_64 += x86_64/coco_pre_fault_memory_test
 TEST_GEN_PROGS_x86_64 += access_tracking_perf_test
 TEST_GEN_PROGS_x86_64 += coalesced_io_test
 TEST_GEN_PROGS_x86_64 += demand_paging_test
diff --git a/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c b/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c
new file mode 100644
index 000000000000..c31a5f9e18f4
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/sizes.h>
+
+#include <test_util.h>
+#include <kvm_util.h>
+#include <processor.h>
+#include "sev.h"
+
+/* Arbitrarily chosen values */
+#define TEST_SIZE		(SZ_2M + PAGE_SIZE)
+#define TEST_NPAGES		(TEST_SIZE / PAGE_SIZE)
+#define TEST_SLOT		10
+#define TEST_GPA		0x100000000ul
+#define TEST_GVA		0x100000000ul
+
+enum prefault_snp_test_type {
+	/* Skip pre-faulting tests. */
+	NO_PREFAULT_TYPE,
+	/*
+	 * Issue KVM_PRE_FAULT_MEMORY for GFNs mapping non-private memory
+	 * before finalizing the initial guest contents (e.g. via
+	 * KVM_SEV_SNP_LAUNCH_FINISH for SNP guests).
+	 *
+	 * This should result in failure since KVM explicitly disallows
+	 * KVM_PRE_FAULT_MEMORY from being issued prior to finalizing the
+	 * initial guest contents.
+	 */
+	PREFAULT_SHARED_BEFORE_FINALIZING,
+	/*
+	 * Issue KVM_PRE_FAULT_MEMORY for GFNs mapping private memory
+	 * before finalizing the initial guest contents (e.g. via
+	 * KVM_SEV_SNP_LAUNCH_FINISH for SNP guests).
+	 *
+	 * This should result in failure since KVM explicitly disallows
+	 * KVM_PRE_FAULT_MEMORY from being issued prior to finalizing the
+	 * initial guest contents.
+	 */
+	PREFAULT_PRIVATE_BEFORE_FINALIZING,
+	/*
+	 * Issue KVM_PRE_FAULT_MEMORY for GFNs mapping shared/private
+	 * memory after finalizing the initial guest contents
+	 * (e.g. via * KVM_SEV_SNP_LAUNCH_FINISH for SNP guests).
+	 *
+	 * This should succeed since pre-faulting is supported for both
+	 * non-private/private memory once the guest contents are finalized.
+	 */
+	PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING
+};
+
+static void guest_code_sev(void)
+{
+	int i;
+
+	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED);
+
+	for (i = 0; i < TEST_NPAGES; i++) {
+		uint64_t *src = (uint64_t *)(TEST_GVA + i * PAGE_SIZE);
+		uint64_t val = *src;
+
+		/* Validate the data stored in the pages */
+		if ((i < TEST_NPAGES / 2 && val != i + 1) ||
+		    (i >= TEST_NPAGES / 2 && val != 0)) {
+			GUEST_FAIL("Inconsistent view of memory values in guest");
+		}
+	}
+
+	if (rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED) {
+		wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ);
+		__asm__ __volatile__("rep; vmmcall");
+		GUEST_FAIL("This should be unreachable.");
+	}
+
+	GUEST_DONE();
+}
+
+static void __pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, u64 size,
+			       u64 left, bool expect_fail)
+{
+	struct kvm_pre_fault_memory range = {
+		.gpa = gpa,
+		.size = size,
+		.flags = 0,
+	};
+	int ret, save_errno;
+	u64 prev;
+
+	do {
+		prev = range.size;
+		ret = __vcpu_ioctl(vcpu, KVM_PRE_FAULT_MEMORY, &range);
+		save_errno = errno;
+		TEST_ASSERT((range.size < prev) ^ (ret < 0),
+			    "%sexpecting range.size to change on %s",
+			    ret < 0 ? "not " : "",
+			    ret < 0 ? "failure" : "success");
+	} while (ret >= 0 ? range.size : save_errno == EINTR);
+
+	TEST_ASSERT(expect_fail ? !(range.size == left) : (range.size == left),
+		    "[EXPECT %s] completed with %lld bytes left, expected %" PRId64,
+		    expect_fail ? "FAIL" : "PASS",
+		    range.size, left);
+
+	if (left == 0) {
+		TEST_ASSERT(expect_fail ? ret : !ret,
+			    "[EXPECT %s] KVM_PRE_FAULT_MEMORY",
+			    expect_fail ? "FAIL" : "PASS");
+	} else {
+		/*
+		 * For shared memory, no memory slot causes RET_PF_EMULATE. It
+		 * results in -ENOENT.
+		 *
+		 * For private memory, no memory slot is an error case returning
+		 * -EFAULT. However, it is also possible that only the GPA
+		 *  ranges backed by a slot are marked as private, in which case
+		 *  the noslot range will also result in -ENOENT.
+		 *
+		 *  So allow both errors for now, but in the future it would be
+		 *  good to distinguish between these cases to tighten up the
+		 *  error-checking.
+		 */
+		TEST_ASSERT(expect_fail ? !ret :
+			    (ret && (save_errno == EFAULT || save_errno == ENOENT)),
+			    "[EXPECT %s] KVM_PRE_FAULT_MEMORY",
+			    expect_fail ? "FAIL" : "PASS");
+	}
+}
+
+static void pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa,
+			     u64 size, u64 left)
+{
+	__pre_fault_memory(vcpu, gpa, size, left, false);
+}
+
+static void pre_fault_memory_negative(struct kvm_vcpu *vcpu, u64 gpa,
+				      u64 size, u64 left)
+{
+	__pre_fault_memory(vcpu, gpa, size, left, true);
+}
+
+static void pre_fault_memory_snp(struct kvm_vcpu *vcpu, struct kvm_vm *vm,
+				 bool private, enum prefault_snp_test_type p_type)
+{
+	if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING)
+		pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
+
+	snp_vm_launch_start(vm, SNP_POLICY);
+
+	if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING)
+		pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
+
+	if (private) {
+		/*
+		 * Make sure when pages are pre-faulted later after
+		 * finalization they are treated the same as a private
+		 * access by the guest so that the expected gmem
+		 * backing pages are used.
+		 */
+		vm_mem_set_private(vm, TEST_GPA, TEST_SIZE);
+		if (p_type == PREFAULT_PRIVATE_BEFORE_FINALIZING)
+			pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
+	} else {
+		if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING)
+			pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
+	}
+
+	snp_vm_launch_update(vm);
+
+	if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING)
+		pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
+
+	snp_vm_launch_finish(vm);
+
+	/*
+	 * After finalization, pre-faulting either private or shared
+	 * ranges should work regardless of whether the pages were
+	 * encrypted as part of setting up initial guest state.
+	 */
+	if (p_type == PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING) {
+		pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0);
+		pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE);
+		pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE);
+	}
+}
+
+static void pre_fault_memory_sev(unsigned long vm_type, struct kvm_vcpu *vcpu,
+				 struct kvm_vm *vm)
+{
+	uint32_t policy = (vm_type == KVM_X86_SEV_ES_VM) ? SEV_POLICY_ES : 0;
+
+	pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0);
+	pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE);
+	pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE);
+
+	sev_vm_launch(vm, policy);
+
+	pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0);
+	pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE);
+	pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE);
+
+	sev_vm_launch_measure(vm, alloca(256));
+
+	pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0);
+	pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE);
+	pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE);
+
+	sev_vm_launch_finish(vm);
+
+	pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0);
+	pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE);
+	pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE);
+}
+
+static void test_pre_fault_memory_sev(unsigned long vm_type, bool private,
+				      enum prefault_snp_test_type p_type)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	struct ucall uc;
+	int i;
+
+	vm = vm_sev_create_with_one_vcpu(vm_type, guest_code_sev, &vcpu);
+
+	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
+				    TEST_GPA, TEST_SLOT, TEST_NPAGES,
+				    (vm_type == KVM_X86_SNP_VM) ? KVM_MEM_GUEST_MEMFD : 0);
+
+	/*
+	 * Make sure guest page table is in agreement with what pages will be
+	 * initially encrypted by the ASP.
+	 */
+	if (private)
+		vm_mem_set_protected(vm, TEST_SLOT, TEST_GPA, TEST_NPAGES);
+
+	virt_map(vm, TEST_GVA, TEST_GPA, TEST_NPAGES);
+
+	/*
+	 * Populate the pages to compare data consistency in the guest
+	 * Fill the first half with data and second half with zeros
+	 */
+	for (i = 0; i < TEST_NPAGES; i++) {
+		uint64_t *hva = addr_gva2hva(vm, TEST_GVA + i * PAGE_SIZE);
+
+		if (i < TEST_NPAGES / 2)
+			*hva = i + 1;
+		else
+			*hva = 0;
+	}
+
+	if (vm_type == KVM_X86_SNP_VM)
+		pre_fault_memory_snp(vcpu, vm, private, p_type);
+	else
+		pre_fault_memory_sev(vm_type, vcpu, vm);
+
+	vcpu_run(vcpu);
+
+	if (vm->type == KVM_X86_SEV_ES_VM || vm->type == KVM_X86_SNP_VM) {
+		TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT,
+			    "Wanted SYSTEM_EVENT, got %s",
+			    exit_reason_str(vcpu->run->exit_reason));
+		TEST_ASSERT_EQ(vcpu->run->system_event.type, KVM_SYSTEM_EVENT_SEV_TERM);
+		TEST_ASSERT_EQ(vcpu->run->system_event.ndata, 1);
+		TEST_ASSERT_EQ(vcpu->run->system_event.data[0], GHCB_MSR_TERM_REQ);
+		goto out;
+	}
+
+	switch (get_ucall(vcpu, &uc)) {
+	case UCALL_DONE:
+		break;
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+	default:
+		TEST_FAIL("Unexpected exit: %s",
+			  exit_reason_str(vcpu->run->exit_reason));
+	}
+
+out:
+	kvm_vm_free(vm);
+}
+
+static void test_pre_fault_memory(unsigned long vm_type, bool private)
+{
+	int pt;
+
+	if (vm_type && !(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(vm_type))) {
+		pr_info("Skipping tests for vm_type 0x%lx\n", vm_type);
+		return;
+	}
+
+	switch (vm_type) {
+	case KVM_X86_SEV_VM:
+	case KVM_X86_SEV_ES_VM:
+		test_pre_fault_memory_sev(vm_type, private, NO_PREFAULT_TYPE);
+		break;
+	case KVM_X86_SNP_VM:
+		for (pt = 0; pt <= PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING; pt++)
+			test_pre_fault_memory_sev(vm_type, private, pt);
+		break;
+	default:
+		abort();
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	TEST_REQUIRE(kvm_check_cap(KVM_CAP_PRE_FAULT_MEMORY));
+
+	test_pre_fault_memory(KVM_X86_SEV_VM, false);
+	test_pre_fault_memory(KVM_X86_SEV_VM, true);
+	test_pre_fault_memory(KVM_X86_SEV_ES_VM, false);
+	test_pre_fault_memory(KVM_X86_SEV_ES_VM, true);
+	test_pre_fault_memory(KVM_X86_SNP_VM, false);
+	test_pre_fault_memory(KVM_X86_SNP_VM, true);
+
+	return 0;
+}
-- 
2.34.1

[PATCH v3 9/9] KVM: selftests: Interleave fallocate for KVM_PRE_FAULT_MEMORY

[Pratik R. Sampat]

fallocate triggers gmem_prepare(), and KVM_PRE_FAULT_MEMORY can cause
guest page faults at unexpected points. Therefore, introduce several
test cases to interleave fallocate, hole punching through various
parts of the SNP launch lifecycle, and observe both positive and
negative vcpcu_run exit statuses.

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
Tested-by: Peter Gonda <pgonda@google.com>
Tested-by: Srikanth Aithal <sraithal@amd.com>
---
 .../kvm/x86_64/coco_pre_fault_memory_test.c   | 121 +++++++++++++++++-
 1 file changed, 114 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c b/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c
index c31a5f9e18f4..e9757ba3234c 100644
--- a/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c
+++ b/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c
@@ -47,6 +47,31 @@ enum prefault_snp_test_type {
 	PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING
 };
 
+enum falloc_snp_test_type {
+	/* Skip alloc tests. */
+	NO_ALLOC_TYPE,
+	/*
+	 * Allocate and/or deallocate a region of guest memfd before
+	 * memory regions are updated to be protected and encrypted
+	 *
+	 * This should succeed since allocation and deallocation is
+	 * supported before the memory is finalized.
+	 */
+	ALLOC_BEFORE_UPDATE,
+	ALLOC_AFTER_UPDATE,
+	DEALLOC_BEFORE_UPDATE,
+	ALLOC_DEALLOC_BEFORE_UPDATE,
+	/*
+	 * Allocate and/or deallocate a region of guest memfd after
+	 * memory regions are updated to be protected and encrypted
+	 *
+	 * This should fail since dealloc will nuke the pages that
+	 * contain the initial code that the guest will run.
+	 */
+	DEALLOC_AFTER_UPDATE,
+	ALLOC_DEALLOC_AFTER_UPDATE
+};
+
 static void guest_code_sev(void)
 {
 	int i;
@@ -73,6 +98,29 @@ static void guest_code_sev(void)
 	GUEST_DONE();
 }
 
+static void __falloc_region(struct kvm_vm *vm, bool punch_hole)
+{
+	int ctr, ret, flags = FALLOC_FL_KEEP_SIZE;
+	struct userspace_mem_region *region;
+
+	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) {
+		if (punch_hole)
+			flags |= FALLOC_FL_PUNCH_HOLE;
+		ret = fallocate(region->region.guest_memfd, flags, 0, PAGE_SIZE * TEST_NPAGES);
+		TEST_ASSERT(!ret, "fallocate should succeed.");
+	}
+}
+
+static void gmemfd_alloc(struct kvm_vm *vm)
+{
+	__falloc_region(vm, false);
+}
+
+static void gmemfd_dealloc(struct kvm_vm *vm)
+{
+	__falloc_region(vm, true);
+}
+
 static void __pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, u64 size,
 			       u64 left, bool expect_fail)
 {
@@ -137,13 +185,34 @@ static void pre_fault_memory_negative(struct kvm_vcpu *vcpu, u64 gpa,
 }
 
 static void pre_fault_memory_snp(struct kvm_vcpu *vcpu, struct kvm_vm *vm,
-				 bool private, enum prefault_snp_test_type p_type)
+				 bool private, enum prefault_snp_test_type p_type,
+				 enum falloc_snp_test_type f_type)
 {
+	if (f_type == ALLOC_BEFORE_UPDATE ||
+	    f_type == ALLOC_DEALLOC_BEFORE_UPDATE) {
+		gmemfd_alloc(vm);
+	}
+
+	if (f_type == DEALLOC_BEFORE_UPDATE ||
+	    f_type == ALLOC_DEALLOC_BEFORE_UPDATE) {
+		gmemfd_dealloc(vm);
+	}
+
 	if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING)
 		pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
 
 	snp_vm_launch_start(vm, SNP_POLICY);
 
+	if (f_type == ALLOC_BEFORE_UPDATE ||
+	    f_type == ALLOC_DEALLOC_BEFORE_UPDATE) {
+		gmemfd_alloc(vm);
+	}
+
+	if (f_type == DEALLOC_BEFORE_UPDATE ||
+	    f_type == ALLOC_DEALLOC_BEFORE_UPDATE) {
+		gmemfd_dealloc(vm);
+	}
+
 	if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING)
 		pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
 
@@ -164,11 +233,36 @@ static void pre_fault_memory_snp(struct kvm_vcpu *vcpu, struct kvm_vm *vm,
 
 	snp_vm_launch_update(vm);
 
+	if (f_type == ALLOC_AFTER_UPDATE ||
+	    f_type == ALLOC_DEALLOC_AFTER_UPDATE) {
+		gmemfd_alloc(vm);
+	}
+
+	/*
+	 * Hole-punch after SNP LAUNCH UPDATE is not expected to fail
+	 * immediately, rather its affects are observed on vcpu_run()
+	 * as the pages that contain the initial code is nuked.
+	 */
+	if (f_type == DEALLOC_AFTER_UPDATE ||
+	    f_type == ALLOC_DEALLOC_AFTER_UPDATE) {
+		gmemfd_dealloc(vm);
+	}
+
 	if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING)
 		pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
 
 	snp_vm_launch_finish(vm);
 
+	if (f_type == ALLOC_AFTER_UPDATE ||
+	    f_type == ALLOC_DEALLOC_AFTER_UPDATE) {
+		gmemfd_alloc(vm);
+	}
+
+	if (f_type == DEALLOC_AFTER_UPDATE ||
+	    f_type == ALLOC_DEALLOC_AFTER_UPDATE) {
+		gmemfd_dealloc(vm);
+	}
+
 	/*
 	 * After finalization, pre-faulting either private or shared
 	 * ranges should work regardless of whether the pages were
@@ -210,7 +304,8 @@ static void pre_fault_memory_sev(unsigned long vm_type, struct kvm_vcpu *vcpu,
 }
 
 static void test_pre_fault_memory_sev(unsigned long vm_type, bool private,
-				      enum prefault_snp_test_type p_type)
+				      enum prefault_snp_test_type p_type,
+				      enum falloc_snp_test_type f_type)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
@@ -246,12 +341,22 @@ static void test_pre_fault_memory_sev(unsigned long vm_type, bool private,
 	}
 
 	if (vm_type == KVM_X86_SNP_VM)
-		pre_fault_memory_snp(vcpu, vm, private, p_type);
+		pre_fault_memory_snp(vcpu, vm, private, p_type, f_type);
 	else
 		pre_fault_memory_sev(vm_type, vcpu, vm);
 
 	vcpu_run(vcpu);
 
+	/* Expect SHUTDOWN when we falloc using PUNCH_HOLE after SNP_UPDATE */
+	if (vm->type == KVM_X86_SNP_VM &&
+	    (f_type == DEALLOC_AFTER_UPDATE ||
+	    f_type == ALLOC_DEALLOC_AFTER_UPDATE)) {
+		TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SHUTDOWN,
+			    "Wanted SYSTEM_EVENT, got %s",
+			    exit_reason_str(vcpu->run->exit_reason));
+		goto out;
+	}
+
 	if (vm->type == KVM_X86_SEV_ES_VM || vm->type == KVM_X86_SNP_VM) {
 		TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT,
 			    "Wanted SYSTEM_EVENT, got %s",
@@ -278,7 +383,7 @@ static void test_pre_fault_memory_sev(unsigned long vm_type, bool private,
 
 static void test_pre_fault_memory(unsigned long vm_type, bool private)
 {
-	int pt;
+	int pt, ft;
 
 	if (vm_type && !(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(vm_type))) {
 		pr_info("Skipping tests for vm_type 0x%lx\n", vm_type);
@@ -288,11 +393,13 @@ static void test_pre_fault_memory(unsigned long vm_type, bool private)
 	switch (vm_type) {
 	case KVM_X86_SEV_VM:
 	case KVM_X86_SEV_ES_VM:
-		test_pre_fault_memory_sev(vm_type, private, NO_PREFAULT_TYPE);
+		test_pre_fault_memory_sev(vm_type, private, NO_PREFAULT_TYPE, NO_ALLOC_TYPE);
 		break;
 	case KVM_X86_SNP_VM:
-		for (pt = 0; pt <= PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING; pt++)
-			test_pre_fault_memory_sev(vm_type, private, pt);
+		for (pt = 0; pt <= PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING; pt++) {
+			for (ft = 0; ft <= ALLOC_DEALLOC_AFTER_UPDATE; ft++)
+				test_pre_fault_memory_sev(vm_type, private, pt, ft);
+		}
 		break;
 	default:
 		abort();
-- 
2.34.1

Re: [PATCH v3 0/9] SEV Kernel Selftests

[Sean Christopherson]

On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
> This series primarily introduces SEV-SNP test for the kernel selftest
> framework. It tests boot, ioctl, pre fault, and fallocate in various
> combinations to exercise both positive and negative launch flow paths.
> 
> Patch 1 - Adds a wrapper for the ioctl calls that decouple ioctl and
> asserts, which enables the use of negative test cases. No functional
> change intended.
> Patch 2 - Extend the sev smoke tests to use the SNP specific ioctl
> calls and sets up memory to boot a SNP guest VM
> Patch 3 - Adds SNP to shutdown testing
> Patch 4, 5 - Tests the ioctl path for SEV, SEV-ES and SNP
> Patch 6 - Adds support for SNP in KVM_SEV_INIT2 tests
> Patch 7,8,9 - Enable Prefault tests for SEV, SEV-ES and SNP

There are three separate series here:

 1. Smoke test support for SNP
 2. Negative tests for SEV+
 3. Prefault tests for SEV+

#3 likely has a dependency on #1, and probably on #2 as well (for style if nothing
else).  But that's really just an argument for focuing on #1 first, and the moving
onto the others once that's ready to go.

Re: [PATCH v3 0/9] SEV Kernel Selftests

[Pratik R. Sampat]

Hi Sean,

On 10/14/2024 5:23 PM, Sean Christopherson wrote:
> On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
>> This series primarily introduces SEV-SNP test for the kernel selftest
>> framework. It tests boot, ioctl, pre fault, and fallocate in various
>> combinations to exercise both positive and negative launch flow paths.
>>
>> Patch 1 - Adds a wrapper for the ioctl calls that decouple ioctl and
>> asserts, which enables the use of negative test cases. No functional
>> change intended.
>> Patch 2 - Extend the sev smoke tests to use the SNP specific ioctl
>> calls and sets up memory to boot a SNP guest VM
>> Patch 3 - Adds SNP to shutdown testing
>> Patch 4, 5 - Tests the ioctl path for SEV, SEV-ES and SNP
>> Patch 6 - Adds support for SNP in KVM_SEV_INIT2 tests
>> Patch 7,8,9 - Enable Prefault tests for SEV, SEV-ES and SNP
> 
> There are three separate series here:
> 
>  1. Smoke test support for SNP
>  2. Negative tests for SEV+
>  3. Prefault tests for SEV+
> 
> #3 likely has a dependency on #1, and probably on #2 as well (for style if nothing
> else).  But that's really just an argument for focuing on #1 first, and the moving
> onto the others once that's ready to go.

Based on your feedback on the rest of this patchset, this makes sense to
me. I will first prep for the changes for patchset #1 and once we lock
that down I can introduce patchset #2 and #3 based on that design.

Thank you again for your feedback!
Pratik