[RFC 0/5] SEV Kernel Selftests

[RFC 0/5] SEV Kernel Selftests

[Pratik R. Sampat]

This patchset introduces SEV-SNP test to the kernel selftest framework.
It also adds negative testing of SEV, ES and SNP VM types.

Patch 1 - Extend the sev smoke tests to use the SNP specific ioctl
calls and sets up memory to boot a SNP guest VM
Patch 2 - Cleanup patch that decouples the ioctl calls from the sev
selftest library with its test assert and status counterparts. No
functional change introduced
Patch 3 - Introduce ioctl test for SEV, ES
Patch 4 - Introduce positive and negative ioctl test for SEV-SNP
Patch 5 - Adds the X86_FEATURE_SEV_SNP vm type test for KVM_SEV_INIT2

Any feedback/review on the approach and design is highly appreciated!

Pratik R. Sampat (5):
  selftests: KVM: Add a basic SNP smoke test
  selftests: KVM: Decouple SEV ioctls from asserts
  selftests: KVM: SEV IOCTL test
  selftests: KVM: SNP IOCTL test
  selftests: KVM: SEV-SNP test for KVM_SEV_INIT2

 .../selftests/kvm/include/x86_64/processor.h  |   1 +
 .../selftests/kvm/include/x86_64/sev.h        |  39 ++-
 tools/testing/selftests/kvm/lib/kvm_util.c    |   7 +-
 .../selftests/kvm/lib/x86_64/processor.c      |   6 +-
 tools/testing/selftests/kvm/lib/x86_64/sev.c  | 181 +++++++++++---
 .../selftests/kvm/x86_64/sev_init2_tests.c    |  13 +
 .../selftests/kvm/x86_64/sev_smoke_test.c     | 223 +++++++++++++++++-
 7 files changed, 418 insertions(+), 52 deletions(-)

-- 
2.34.1

[RFC 1/5] selftests: KVM: 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 it's 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>
---
 .../selftests/kvm/include/x86_64/processor.h  |  1 +
 .../selftests/kvm/include/x86_64/sev.h        | 29 ++++++++
 tools/testing/selftests/kvm/lib/kvm_util.c    |  7 +-
 .../selftests/kvm/lib/x86_64/processor.c      |  6 +-
 tools/testing/selftests/kvm/lib/x86_64/sev.c  | 70 ++++++++++++++++++-
 .../selftests/kvm/x86_64/sev_smoke_test.c     | 51 ++++++++++----
 6 files changed, 146 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 8eb57de0b587..5683fc9794e4 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -194,6 +194,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 82c11c81a956..43b6c52831b2 100644
--- a/tools/testing/selftests/kvm/include/x86_64/sev.h
+++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
@@ -22,8 +22,17 @@ 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 SEV_POLICY_NO_DBG	(1UL << 0)
 #define SEV_POLICY_ES		(1UL << 2)
+#define SNP_POLICY_ABI_MINOR	(1ULL << 0)
+#define SNP_POLICY_ABI_MAJOR	(1ULL << 8)
+#define SNP_POLICY_SMT		(1ULL << 16)
+#define SNP_POLICY_RSVD_MBO	(1ULL << 17)
+#define SNP_POLICY_DBG		(1ULL << 19)
 
 #define GHCB_MSR_TERM_REQ	0x100
 
@@ -31,6 +40,12 @@ 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);
 
+bool is_kvm_snp_supported(void);
+
+void snp_vm_launch(struct kvm_vm *vm, uint32_t policy);
+void snp_vm_launch_update(struct kvm_vm *vm);
+void snp_vm_launch_finish(struct kvm_vm *vm);
+
 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);
@@ -70,6 +85,7 @@ kvm_static_assert(SEV_RET_SUCCESS == 0);
 
 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)
@@ -82,6 +98,19 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
 	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
 }
 
+static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
+					   uint64_t size, uint8_t type)
+{
+	struct kvm_sev_snp_launch_update update_data = {
+		.uaddr = (unsigned long)addr_gpa2hva(vm, gpa),
+		.gfn_start = gpa >> PAGE_SHIFT,
+		.len = size,
+		.type = type,
+	};
+
+	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 size)
 {
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index ad00e4761886..4c00a96f9b80 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -412,14 +412,17 @@ 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 c664e446136b..d1ea030f6be0 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -623,7 +623,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);
@@ -1127,7 +1128,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 e9535ee20b7f..90231c578aca 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
@@ -24,12 +24,19 @@ static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *regio
 	if (!sparsebit_any_set(protected_phy_pages))
 		return;
 
-	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);
+			snp_launch_update_data(vm, gpa_base + offset, size,
+					       KVM_SEV_SNP_PAGE_TYPE_NORMAL);
+			continue;
+		}
 		sev_launch_update_data(vm, gpa_base + offset, size);
 	}
 }
@@ -60,6 +67,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);
+}
+
 void sev_vm_launch(struct kvm_vm *vm, uint32_t policy)
 {
 	struct kvm_sev_launch_start launch_start = {
@@ -112,6 +127,51 @@ void sev_vm_launch_finish(struct kvm_vm *vm)
 	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING);
 }
 
+void snp_vm_launch(struct kvm_vm *vm, uint32_t policy)
+{
+	struct kvm_sev_snp_launch_start launch_start = {
+		.policy = policy,
+	};
+
+	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_START, &launch_start);
+}
+
+void snp_vm_launch_update(struct kvm_vm *vm)
+{
+	struct userspace_mem_region *region;
+	int ctr;
+
+	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node)
+		encrypt_region(vm, region);
+
+	vm->arch.is_pt_protected = true;
+}
+
+void snp_vm_launch_finish(struct kvm_vm *vm)
+{
+	struct kvm_sev_snp_launch_finish launch_finish = { 0 };
+
+	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_FINISH, &launch_finish);
+}
+
+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)
 {
@@ -130,6 +190,14 @@ struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
 
 void vm_sev_launch(struct kvm_vm *vm, uint32_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(vm, policy);
+		snp_vm_launch_update(vm);
+		snp_vm_launch_finish(vm);
+		return;
+	}
+
 	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 7c70c0da4fb7..1a50a280173c 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,16 @@
 
 #define XFEATURE_MASK_X87_AVX (XFEATURE_MASK_FP | XFEATURE_MASK_SSE | XFEATURE_MASK_YMM)
 
+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 +71,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, uint32_t policy)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
@@ -77,7 +87,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 +112,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 +131,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 +145,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));
@@ -164,17 +175,31 @@ 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);
+
+		if (kvm_has_cap(KVM_CAP_XCRS) &&
+		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
+			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()) {
+		test_sev(guest_snp_code, KVM_X86_SNP_VM, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO);
+		/* Test minimum firmware level */
+		test_sev(guest_snp_code, KVM_X86_SNP_VM,
+			 SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO |
+			 (SNP_FW_REQ_VER_MAJOR * SNP_POLICY_ABI_MAJOR) |
+			 (SNP_FW_REQ_VER_MINOR * SNP_POLICY_ABI_MINOR));
 
 		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_SNP_VM, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO);
 		}
 	}
 
-- 
2.34.1

Re: [RFC 1/5] selftests: KVM: Add a basic SNP smoke test

[Peter Gonda]

On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
<pratikrajesh.sampat@amd.com> 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 it's 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>

>
> -       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);
> +
> +               if (kvm_has_cap(KVM_CAP_XCRS) &&
> +                   (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
> +                       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()) {
> +               test_sev(guest_snp_code, KVM_X86_SNP_VM, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO);

I'd guess most systems have SMT enabled, but is there a way we can
check and toggle the SNP_POLICY_SMT policy bit programmatically?

Also should we have a base SNP policy so we don't have to read
`SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO` every time? Not sure I think
selftests prefer more verbosity.

Re: [RFC 1/5] selftests: KVM: Add a basic SNP smoke test

[Sampat, Pratik Rajesh]

Hi Peter,

Thank you for your review!

On 7/11/2024 10:16 AM, Peter Gonda wrote:
> On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
> <pratikrajesh.sampat@amd.com> 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 it's 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>
> 
>>
>> -       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);
>> +
>> +               if (kvm_has_cap(KVM_CAP_XCRS) &&
>> +                   (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
>> +                       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()) {
>> +               test_sev(guest_snp_code, KVM_X86_SNP_VM, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO);
> 
> I'd guess most systems have SMT enabled, but is there a way we can
> check and toggle the SNP_POLICY_SMT policy bit programmatically?
> 

We could do that by making a check to /sys/devices/system/cpu/smt/active
maybe?

> Also should we have a base SNP policy so we don't have to read
> `SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO` every time? Not sure I think
> selftests prefer more verbosity.

Sure, that makes sense. I can also include the following to save us a
few keystrokes and help read easier.
#define SNP_POLICY	SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO

Thanks!
Pratik

Re: [RFC 1/5] selftests: KVM: Add a basic SNP smoke test

[Tom Lendacky]

On 7/10/24 17:05, 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 it's 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>
> ---
>  .../selftests/kvm/include/x86_64/processor.h  |  1 +
>  .../selftests/kvm/include/x86_64/sev.h        | 29 ++++++++
>  tools/testing/selftests/kvm/lib/kvm_util.c    |  7 +-
>  .../selftests/kvm/lib/x86_64/processor.c      |  6 +-
>  tools/testing/selftests/kvm/lib/x86_64/sev.c  | 70 ++++++++++++++++++-
>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 51 ++++++++++----
>  6 files changed, 146 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 8eb57de0b587..5683fc9794e4 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h

> +
> +	if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
> +		test_sev(guest_snp_code, KVM_X86_SNP_VM, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO);
> +		/* Test minimum firmware level */
> +		test_sev(guest_snp_code, KVM_X86_SNP_VM,
> +			 SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO |
> +			 (SNP_FW_REQ_VER_MAJOR * SNP_POLICY_ABI_MAJOR) |
> +			 (SNP_FW_REQ_VER_MINOR * SNP_POLICY_ABI_MINOR));

This seems an odd way of setting these fields. Maybe, instead, use a
couple of macros that take the values and shift appropriately and ensure
that they don't exceed the 8-bits each field occupies.

Thanks,
Tom

>  

Re: [RFC 1/5] selftests: KVM: Add a basic SNP smoke test

[Sampat, Pratik Rajesh]

Hi Tom,

On 7/11/2024 10:56 AM, Tom Lendacky wrote:
> On 7/10/24 17:05, 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 it's 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>
>> ---
>>  .../selftests/kvm/include/x86_64/processor.h  |  1 +
>>  .../selftests/kvm/include/x86_64/sev.h        | 29 ++++++++
>>  tools/testing/selftests/kvm/lib/kvm_util.c    |  7 +-
>>  .../selftests/kvm/lib/x86_64/processor.c      |  6 +-
>>  tools/testing/selftests/kvm/lib/x86_64/sev.c  | 70 ++++++++++++++++++-
>>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 51 ++++++++++----
>>  6 files changed, 146 insertions(+), 18 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
>> index 8eb57de0b587..5683fc9794e4 100644
>> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
>> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> 
>> +
>> +	if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
>> +		test_sev(guest_snp_code, KVM_X86_SNP_VM, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO);
>> +		/* Test minimum firmware level */
>> +		test_sev(guest_snp_code, KVM_X86_SNP_VM,
>> +			 SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO |
>> +			 (SNP_FW_REQ_VER_MAJOR * SNP_POLICY_ABI_MAJOR) |
>> +			 (SNP_FW_REQ_VER_MINOR * SNP_POLICY_ABI_MINOR));
> 
> This seems an odd way of setting these fields. Maybe, instead, use a
> couple of macros that take the values and shift appropriately and ensure
> that they don't exceed the 8-bits each field occupies.
> 

Sure, I will clean this up and ensure the flags are set up more elegantly.

> Thanks,
> Tom
> 
>>  

[RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

[Pratik R. Sampat]

This commit separates the SEV, SEV-ES, SEV-SNP ioctl calls from its
positive test asserts. This is done so that negative tests can be
introduced and both kinds of testing can be performed independently
using the same base helpers of the ioctl.

This commit also adds additional parameters such as flags to improve
testing coverage for the ioctls.

Cleanups performed with no functional change intended.

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
---
 .../selftests/kvm/include/x86_64/sev.h        |  20 +--
 tools/testing/selftests/kvm/lib/x86_64/sev.c  | 145 ++++++++++++------
 2 files changed, 108 insertions(+), 57 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
index 43b6c52831b2..ef99151e13a7 100644
--- a/tools/testing/selftests/kvm/include/x86_64/sev.h
+++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
@@ -37,14 +37,16 @@ enum sev_guest_state {
 #define GHCB_MSR_TERM_REQ	0x100
 
 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);
+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);
 
 bool is_kvm_snp_supported(void);
 
-void snp_vm_launch(struct kvm_vm *vm, uint32_t policy);
-void snp_vm_launch_update(struct kvm_vm *vm);
-void snp_vm_launch_finish(struct kvm_vm *vm);
+int snp_vm_launch(struct kvm_vm *vm, uint32_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);
 
 struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
 					   struct kvm_vcpu **cpu);
@@ -98,7 +100,7 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
 	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
 }
 
-static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
+static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
 					   uint64_t size, uint8_t type)
 {
 	struct kvm_sev_snp_launch_update update_data = {
@@ -108,10 +110,10 @@ static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
 		.type = type,
 	};
 
-	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
+	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,
+static inline int sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
 					  uint64_t size)
 {
 	struct kvm_sev_launch_update_data update_data = {
@@ -119,7 +121,7 @@ static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
 		.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);
 }
 
 #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 90231c578aca..a931a321968f 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
@@ -14,15 +14,18 @@
  * 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,
+			  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;
 	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;
 
 	if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM)
 		sev_register_encrypted_memory(vm, region);
@@ -33,12 +36,18 @@ static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *regio
 
 		if (vm->type == KVM_X86_SNP_VM) {
 			vm_mem_set_private(vm, gpa_base + offset, size);
-			snp_launch_update_data(vm, gpa_base + offset, size,
-					       KVM_SEV_SNP_PAGE_TYPE_NORMAL);
+			ret = snp_launch_update_data(vm, gpa_base + offset, size,
+						     page_type);
+			if (ret)
+				return ret;
 			continue;
 		}
-		sev_launch_update_data(vm, gpa_base + offset, size);
+		ret = sev_launch_update_data(vm, gpa_base + offset, size);
+		if (ret)
+			return ret;
 	}
+
+	return 0;
 }
 
 void sev_vm_init(struct kvm_vm *vm)
@@ -75,83 +84,97 @@ void snp_vm_init(struct kvm_vm *vm)
 	vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
 }
 
-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,
 	};
-	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);
 
-	TEST_ASSERT_EQ(status.policy, policy);
-	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_LAUNCH_UPDATE);
+	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_START, &launch_start);
+}
 
-	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node)
-		encrypt_region(vm, region);
+int sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy)
+{
+	struct userspace_mem_region *region;
+	int ctr, ret;
 
+	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) {
+		ret = encrypt_region(vm, region, 0);
+		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)
+void sev_vm_launch(struct kvm_vm *vm, uint32_t policy)
 {
-	struct kvm_sev_launch_measure launch_measure;
-	struct kvm_sev_guest_status guest_status;
+	struct kvm_sev_guest_status status;
+	int ret;
 
-	launch_measure.len = 256;
-	launch_measure.uaddr = (__u64)measurement;
-	vm_sev_ioctl(vm, KVM_SEV_LAUNCH_MEASURE, &launch_measure);
+	ret = sev_vm_launch_start(vm, policy);
+	TEST_ASSERT(!ret, KVM_IOCTL_ERROR(KVM_SEV_SNP_LAUNCH_START, ret));
+
+	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);
 
-	vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &guest_status);
-	TEST_ASSERT_EQ(guest_status.state, SEV_GUEST_STATE_LAUNCH_SECRET);
+	ret = sev_vm_launch_update(vm, policy);
+	TEST_ASSERT(!ret, KVM_IOCTL_ERROR(KVM_SEV_LAUNCH_UPDATE_DATA, ret));
 }
 
-void sev_vm_launch_finish(struct kvm_vm *vm)
+int sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement)
 {
-	struct kvm_sev_guest_status status;
+	struct kvm_sev_launch_measure launch_measure;
 
-	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);
+	launch_measure.len = 256;
+	launch_measure.uaddr = (__u64)measurement;
 
-	vm_sev_ioctl(vm, KVM_SEV_LAUNCH_FINISH, NULL);
+	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_MEASURE, &launch_measure);
+}
 
-	vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
-	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING);
+int sev_vm_launch_finish(struct kvm_vm *vm)
+{
+	return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_FINISH, NULL);
 }
 
-void snp_vm_launch(struct kvm_vm *vm, uint32_t policy)
+int snp_vm_launch(struct kvm_vm *vm, uint32_t policy, uint8_t flags)
 {
 	struct kvm_sev_snp_launch_start launch_start = {
 		.policy = policy,
+		.flags = flags,
 	};
 
-	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_START, &launch_start);
+	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_START, &launch_start);
 }
 
-void snp_vm_launch_update(struct kvm_vm *vm)
+int snp_vm_launch_update(struct kvm_vm *vm, uint8_t page_type)
 {
 	struct userspace_mem_region *region;
-	int ctr;
+	int ctr, ret;
 
-	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node)
-		encrypt_region(vm, region);
+	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;
 }
 
-void snp_vm_launch_finish(struct kvm_vm *vm)
+int snp_vm_launch_finish(struct kvm_vm *vm, uint16_t flags)
 {
-	struct kvm_sev_snp_launch_finish launch_finish = { 0 };
+	struct kvm_sev_snp_launch_finish launch_finish = {
+		.flags = flags,
+	};
 
-	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_FINISH, &launch_finish);
+	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_FINISH, &launch_finish);
 }
 
 bool is_kvm_snp_supported(void)
@@ -190,20 +213,46 @@ struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
 
 void vm_sev_launch(struct kvm_vm *vm, uint32_t policy, uint8_t *measurement)
 {
+	struct kvm_sev_guest_status status;
+	int ret;
+
 	if (vm->type == KVM_X86_SNP_VM) {
 		vm_enable_cap(vm, KVM_CAP_EXIT_HYPERCALL, (1 << KVM_HC_MAP_GPA_RANGE));
-		snp_vm_launch(vm, policy);
-		snp_vm_launch_update(vm);
-		snp_vm_launch_finish(vm);
+		ret = snp_vm_launch(vm, policy, 0);
+		TEST_ASSERT(!ret, KVM_IOCTL_ERROR(KVM_SEV_SNP_LAUNCH_START, ret));
+
+		ret = snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
+		TEST_ASSERT(!ret, KVM_IOCTL_ERROR(KVM_SEV_SNP_LAUNCH_UPDATE, ret));
+
+		ret = snp_vm_launch_finish(vm, 0);
+		TEST_ASSERT(!ret, KVM_IOCTL_ERROR(KVM_SEV_SNP_LAUNCH_FINISH, ret));
 		return;
 	}
 
-	sev_vm_launch(vm, policy);
+	ret = sev_vm_launch_start(vm, policy);
+	TEST_ASSERT(!ret, KVM_IOCTL_ERROR(KVM_SEV_LAUNCH_START, ret));
+
+	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(!ret, KVM_IOCTL_ERROR(KVM_SEV_LAUNCH_UPDATE_DATA, ret));
 
 	if (!measurement)
 		measurement = alloca(256);
 
-	sev_vm_launch_measure(vm, measurement);
+	ret = sev_vm_launch_measure(vm, measurement);
+	TEST_ASSERT(!ret, KVM_IOCTL_ERROR(KVM_SEV_LAUNCH_MEASURE, ret));
 
-	sev_vm_launch_finish(vm);
+	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);
+
+	ret = sev_vm_launch_finish(vm);
+	TEST_ASSERT(!ret, KVM_IOCTL_ERROR(KVM_SEV_LAUNCH_FINISH, ret));
+
+	vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
+	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING);
 }
-- 
2.34.1

Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

[Peter Gonda]

On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
<pratikrajesh.sampat@amd.com> wrote:
>
> This commit separates the SEV, SEV-ES, SEV-SNP ioctl calls from its
> positive test asserts. This is done so that negative tests can be
> introduced and both kinds of testing can be performed independently
> using the same base helpers of the ioctl.
>
> This commit also adds additional parameters such as flags to improve
> testing coverage for the ioctls.
>
> Cleanups performed with no functional change intended.
>
> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>

Tested-by: Peter Gonda <pgonda@google.com>

Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

[Peter Gonda]

> +int sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy)
> +{
> +       struct userspace_mem_region *region;
> +       int ctr, ret;
>
> +       hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) {
> +               ret = encrypt_region(vm, region, 0);
> +               if (ret)
> +                       return ret;
> +       }
>         if (policy & SEV_POLICY_ES)
>                 vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);

Adding the sev-es policy bit for negative testing is a bit confusing,
but I guess it works. For negative testing should we be more explicit?
Ditto for other usages of `policy` simply to toggle sev-es features.

Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

[Sampat, Pratik Rajesh]

On 7/11/2024 11:11 AM, Peter Gonda wrote:
>> +int sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy)
>> +{
>> +       struct userspace_mem_region *region;
>> +       int ctr, ret;
>>
>> +       hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) {
>> +               ret = encrypt_region(vm, region, 0);
>> +               if (ret)
>> +                       return ret;
>> +       }
>>         if (policy & SEV_POLICY_ES)
>>                 vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
> 
> Adding the sev-es policy bit for negative testing is a bit confusing,
> but I guess it works. For negative testing should we be more explicit?
> Ditto for other usages of `policy` simply to toggle sev-es features.

You're right. Although it works because the way we want for negative
testing it does go by exercising a different path meant for a different
policy.

Maybe I can refactor the old code to all test for type instead like I
have done with the rest of the patchset just so that we are more
explicit. Would that fare any better?

Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

[Sean Christopherson]

On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
> This commit separates the SEV, SEV-ES, SEV-SNP ioctl calls from its

Don't start with "This commit".  Please read Documentation/process/maintainer-kvm-x86.rst,
and by extension, Documentation/process/maintainer-tip.rst.

> positive test asserts. This is done so that negative tests can be
> introduced and both kinds of testing can be performed independently
> using the same base helpers of the ioctl.
> 
> This commit also adds additional parameters such as flags to improve
> testing coverage for the ioctls.
> 
> Cleanups performed with no functional change intended.
> 
> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
> ---
>  .../selftests/kvm/include/x86_64/sev.h        |  20 +--
>  tools/testing/selftests/kvm/lib/x86_64/sev.c  | 145 ++++++++++++------
>  2 files changed, 108 insertions(+), 57 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
> index 43b6c52831b2..ef99151e13a7 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/sev.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
> @@ -37,14 +37,16 @@ enum sev_guest_state {
>  #define GHCB_MSR_TERM_REQ	0x100
>  
>  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);
> +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);
>  
>  bool is_kvm_snp_supported(void);
>  
> -void snp_vm_launch(struct kvm_vm *vm, uint32_t policy);
> -void snp_vm_launch_update(struct kvm_vm *vm);
> -void snp_vm_launch_finish(struct kvm_vm *vm);
> +int snp_vm_launch(struct kvm_vm *vm, uint32_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);
>  
>  struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
>  					   struct kvm_vcpu **cpu);
> @@ -98,7 +100,7 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
>  	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
>  }
>  
> -static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> +static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>  					   uint64_t size, uint8_t type)
>  {
>  	struct kvm_sev_snp_launch_update update_data = {
> @@ -108,10 +110,10 @@ static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>  		.type = type,
>  	};
>  
> -	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
> +	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);

Don't introduce APIs and then immediately rewrite all of the users.  If you want
to rework similar APIs, do the rework, then add the new APIs.  Doing things in
this order adds a pile of pointless churn.

But that's a moot point, because it's far easier to just add __snp_launch_update_data().
And if you look through other APIs in kvm_util.h, you'll see that the strong
preference is to let vm_ioctl(), or in this case vm_sev_ioctl(), do the heavy
lifting.  Yeah, it requires copy+pasting marshalling parameters into the struct,
but that's relatively uninteresting code, _and_ piggybacking the "good" version
means you can't do things like pass in a garbage virtual address (because the
"good" version always guarantees a good virtual address).

Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

[Pratik R. Sampat]

Hi Sean,

Thanks for your review.

On 8/9/2024 10:40 AM, Sean Christopherson wrote:
> On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
>> This commit separates the SEV, SEV-ES, SEV-SNP ioctl calls from its
> 
> Don't start with "This commit".  Please read Documentation/process/maintainer-kvm-x86.rst,
> and by extension, Documentation/process/maintainer-tip.rst.

Sure, I will frame the message better.

> 
>> positive test asserts. This is done so that negative tests can be
>> introduced and both kinds of testing can be performed independently
>> using the same base helpers of the ioctl.
>>
>> This commit also adds additional parameters such as flags to improve
>> testing coverage for the ioctls.
>>
>> Cleanups performed with no functional change intended.
>>
>> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
>> ---
>>  .../selftests/kvm/include/x86_64/sev.h        |  20 +--
>>  tools/testing/selftests/kvm/lib/x86_64/sev.c  | 145 ++++++++++++------
>>  2 files changed, 108 insertions(+), 57 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
>> index 43b6c52831b2..ef99151e13a7 100644
>> --- a/tools/testing/selftests/kvm/include/x86_64/sev.h
>> +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
>> @@ -37,14 +37,16 @@ enum sev_guest_state {
>>  #define GHCB_MSR_TERM_REQ	0x100
>>  
>>  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);
>> +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);
>>  
>>  bool is_kvm_snp_supported(void);
>>  
>> -void snp_vm_launch(struct kvm_vm *vm, uint32_t policy);
>> -void snp_vm_launch_update(struct kvm_vm *vm);
>> -void snp_vm_launch_finish(struct kvm_vm *vm);
>> +int snp_vm_launch(struct kvm_vm *vm, uint32_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);
>>  
>>  struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
>>  					   struct kvm_vcpu **cpu);
>> @@ -98,7 +100,7 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
>>  	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
>>  }
>>  
>> -static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>> +static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>>  					   uint64_t size, uint8_t type)
>>  {
>>  	struct kvm_sev_snp_launch_update update_data = {
>> @@ -108,10 +110,10 @@ static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>>  		.type = type,
>>  	};
>>  
>> -	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
>> +	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
> 
> Don't introduce APIs and then immediately rewrite all of the users.  If you want
> to rework similar APIs, do the rework, then add the new APIs.  Doing things in
> this order adds a pile of pointless churn.
> 
> But that's a moot point, because it's far easier to just add __snp_launch_update_data().
> And if you look through other APIs in kvm_util.h, you'll see that the strong
> preference is to let vm_ioctl(), or in this case vm_sev_ioctl(), do the heavy
> lifting.  Yeah, it requires copy+pasting marshalling parameters into the struct,
> but that's relatively uninteresting code, _and_ piggybacking the "good" version
> means you can't do things like pass in a garbage virtual address (because the
> "good" version always guarantees a good virtual address).

I am a little confused by this.

Are you suggesting that I leave the original functions intact with using
vm_sev_ioctl() and have an additional variant such as
__snp_launch_update_data() which calls into __vm_sev_ioctl() to decouple
the ioctl from the assert for negative asserts?

Or, do you suggest that I alter vm_sev_ioctl() to handle both positive
and negative asserts?

Thanks!
-Pratik

Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

[Sean Christopherson]

On Tue, Aug 13, 2024, Pratik R. Sampat wrote:
> On 8/9/2024 10:40 AM, Sean Christopherson wrote:
> > On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
> >> @@ -98,7 +100,7 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
> >>  	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
> >>  }
> >>  
> >> -static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> >> +static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> >>  					   uint64_t size, uint8_t type)
> >>  {
> >>  	struct kvm_sev_snp_launch_update update_data = {
> >> @@ -108,10 +110,10 @@ static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> >>  		.type = type,
> >>  	};
> >>  
> >> -	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
> >> +	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
> > 
> > Don't introduce APIs and then immediately rewrite all of the users.  If you want
> > to rework similar APIs, do the rework, then add the new APIs.  Doing things in
> > this order adds a pile of pointless churn.
> > 
> > But that's a moot point, because it's far easier to just add __snp_launch_update_data().
> > And if you look through other APIs in kvm_util.h, you'll see that the strong
> > preference is to let vm_ioctl(), or in this case vm_sev_ioctl(), do the heavy
> > lifting.  Yeah, it requires copy+pasting marshalling parameters into the struct,
> > but that's relatively uninteresting code, _and_ piggybacking the "good" version
> > means you can't do things like pass in a garbage virtual address (because the
> > "good" version always guarantees a good virtual address).
> 
> I am a little confused by this.
> 
> Are you suggesting that I leave the original functions intact with using
> vm_sev_ioctl() and have an additional variant such as
> __snp_launch_update_data() which calls into __vm_sev_ioctl() to decouple
> the ioctl from the assert for negative asserts?

Yes, this one.

> Or, do you suggest that I alter vm_sev_ioctl() to handle both positive
> and negative asserts?
> 
> Thanks!
> -Pratik
> 

Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

[Pratik R. Sampat]

On 8/13/2024 10:27 AM, Sean Christopherson wrote:
> On Tue, Aug 13, 2024, Pratik R. Sampat wrote:
>> On 8/9/2024 10:40 AM, Sean Christopherson wrote:
>>> On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
>>>> @@ -98,7 +100,7 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
>>>>  	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
>>>>  }
>>>>  
>>>> -static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>>>> +static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>>>>  					   uint64_t size, uint8_t type)
>>>>  {
>>>>  	struct kvm_sev_snp_launch_update update_data = {
>>>> @@ -108,10 +110,10 @@ static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>>>>  		.type = type,
>>>>  	};
>>>>  
>>>> -	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
>>>> +	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
>>>
>>> Don't introduce APIs and then immediately rewrite all of the users.  If you want
>>> to rework similar APIs, do the rework, then add the new APIs.  Doing things in
>>> this order adds a pile of pointless churn.
>>>
>>> But that's a moot point, because it's far easier to just add __snp_launch_update_data().
>>> And if you look through other APIs in kvm_util.h, you'll see that the strong
>>> preference is to let vm_ioctl(), or in this case vm_sev_ioctl(), do the heavy
>>> lifting.  Yeah, it requires copy+pasting marshalling parameters into the struct,
>>> but that's relatively uninteresting code, _and_ piggybacking the "good" version
>>> means you can't do things like pass in a garbage virtual address (because the
>>> "good" version always guarantees a good virtual address).
>>
>> I am a little confused by this.
>>
>> Are you suggesting that I leave the original functions intact with using
>> vm_sev_ioctl() and have an additional variant such as
>> __snp_launch_update_data() which calls into __vm_sev_ioctl() to decouple
>> the ioctl from the assert for negative asserts?
> 
> Yes, this one.

Got it. Thanks a lot!

> 
>> Or, do you suggest that I alter vm_sev_ioctl() to handle both positive
>> and negative asserts?
>>
>> Thanks!
>> -Pratik
>>

[RFC 3/5] selftests: KVM: 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>
---
 .../selftests/kvm/x86_64/sev_smoke_test.c     | 57 +++++++++++++++++++
 1 file changed, 57 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 1a50a280173c..500c67b3793b 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
@@ -131,12 +131,69 @@ static void test_sync_vmsa(uint32_t type, uint32_t policy)
 	kvm_vm_free(vm);
 }
 
+static void sev_guest_status_assert(struct kvm_vm *vm, uint32_t type)
+{
+	struct kvm_sev_guest_status status;
+	bool cond;
+	int ret;
+
+	ret = __vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
+	cond = type == KVM_X86_SEV_VM ? !ret : ret;
+	TEST_ASSERT(cond,
+		    "KVM_SEV_GUEST_STATUS should fail, invalid VM Type.");
+}
+
+static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	struct ucall uc;
+	bool cond;
+	int ret;
+
+	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
+	ret = sev_vm_launch_start(vm, 0);
+	cond = type == KVM_X86_SEV_VM ? !ret : ret;
+	TEST_ASSERT(cond,
+		    "KVM_SEV_LAUNCH_START should fail, invalid policy.");
+
+	ret = sev_vm_launch_update(vm, policy);
+	cond = type == KVM_X86_SEV_VM ? !ret : ret;
+	TEST_ASSERT(cond,
+		    "KVM_SEV_LAUNCH_UPDATE should fail, invalid policy.");
+	sev_guest_status_assert(vm, type);
+
+	ret = sev_vm_launch_measure(vm, alloca(256));
+	cond = type == KVM_X86_SEV_VM ? !ret : ret;
+	TEST_ASSERT(cond,
+		    "KVM_SEV_LAUNCH_MEASURE should fail, invalid policy.");
+	sev_guest_status_assert(vm, type);
+
+	ret = sev_vm_launch_finish(vm);
+	cond = type == KVM_X86_SEV_VM ? !ret : ret;
+	TEST_ASSERT(cond,
+		    "KVM_SEV_LAUNCH_FINISH should fail, invalid policy.");
+	sev_guest_status_assert(vm, type);
+
+	vcpu_run(vcpu);
+	get_ucall(vcpu, &uc);
+	cond = type == KVM_X86_SEV_VM ?
+		vcpu->run->exit_reason == KVM_EXIT_IO :
+		vcpu->run->exit_reason == KVM_EXIT_FAIL_ENTRY;
+	TEST_ASSERT(cond,
+		    "vcpu_run should fail, invalid policy.");
+
+	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

Re: [RFC 3/5] selftests: KVM: SEV IOCTL test

[Peter Gonda]

> +
> +static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
> +{
> +       struct kvm_vcpu *vcpu;
> +       struct kvm_vm *vm;
> +       struct ucall uc;
> +       bool cond;
> +       int ret;
> +
> +       vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
> +       ret = sev_vm_launch_start(vm, 0);
> +       cond = type == KVM_X86_SEV_VM ? !ret : ret;
> +       TEST_ASSERT(cond,
> +                   "KVM_SEV_LAUNCH_START should fail, invalid policy.");
> +
> +       ret = sev_vm_launch_update(vm, policy);
> +       cond = type == KVM_X86_SEV_VM ? !ret : ret;
> +       TEST_ASSERT(cond,
> +                   "KVM_SEV_LAUNCH_UPDATE should fail, invalid policy.");

Isn't the reason we expect all other calls to fail here because we
have not successfully called `sev_vm_launch_start()`?

> +       sev_guest_status_assert(vm, type);
> +
> +       ret = sev_vm_launch_measure(vm, alloca(256));

Should we free this buffer?

Re: [RFC 3/5] selftests: KVM: SEV IOCTL test

[Sampat, Pratik Rajesh]

On 7/11/2024 10:23 AM, Peter Gonda wrote:
>> +
>> +static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>> +{
>> +       struct kvm_vcpu *vcpu;
>> +       struct kvm_vm *vm;
>> +       struct ucall uc;
>> +       bool cond;
>> +       int ret;
>> +
>> +       vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
>> +       ret = sev_vm_launch_start(vm, 0);
>> +       cond = type == KVM_X86_SEV_VM ? !ret : ret;
>> +       TEST_ASSERT(cond,
>> +                   "KVM_SEV_LAUNCH_START should fail, invalid policy.");
>> +
>> +       ret = sev_vm_launch_update(vm, policy);
>> +       cond = type == KVM_X86_SEV_VM ? !ret : ret;
>> +       TEST_ASSERT(cond,
>> +                   "KVM_SEV_LAUNCH_UPDATE should fail, invalid policy.");
> 
> Isn't the reason we expect all other calls to fail here because we
> have not successfully called `sev_vm_launch_start()`?
> 

Yes you're right. The idea is that none of the consequent "good" ioctl
calls should succeed if the vm_launch_start was faulty.

>> +       sev_guest_status_assert(vm, type);
>> +
>> +       ret = sev_vm_launch_measure(vm, alloca(256));
> 
> Should we free this buffer?

Sure, I should store this into a pointer and free it.

I guess this also happens within vm_sev_launch() where we should include
a free as well.

Thanks for catching this!

Re: [RFC 3/5] selftests: KVM: SEV IOCTL test

[Tom Lendacky]

On 7/10/24 17:05, Pratik R. Sampat wrote:
> 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>
> ---
>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 57 +++++++++++++++++++
>  1 file changed, 57 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 1a50a280173c..500c67b3793b 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> @@ -131,12 +131,69 @@ static void test_sync_vmsa(uint32_t type, uint32_t policy)
>  	kvm_vm_free(vm);
>  }
>  
> +static void sev_guest_status_assert(struct kvm_vm *vm, uint32_t type)
> +{
> +	struct kvm_sev_guest_status status;
> +	bool cond;
> +	int ret;
> +
> +	ret = __vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
> +	TEST_ASSERT(cond,
> +		    "KVM_SEV_GUEST_STATUS should fail, invalid VM Type.");
> +}
> +
> +static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +	bool cond;
> +	int ret;
> +

Maybe a block comment here indicating what you're actually doing would
be good, because I'm a bit confused.

A policy value of 0 is valid for SEV, so you expect each call to
succeed, right? And, actually, for SEV-ES the launch start will succeed,
too, but the launch update will fail because LAUNCH_UPDATE_VMSA is not
valid for SEV, but then the launch measure should succeed. Is that
right? What about the other calls?

Thanks,
Tom

> +	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
> +	ret = sev_vm_launch_start(vm, 0);
> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
> +	TEST_ASSERT(cond,
> +		    "KVM_SEV_LAUNCH_START should fail, invalid policy.");
> +
> +	ret = sev_vm_launch_update(vm, policy);
> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
> +	TEST_ASSERT(cond,
> +		    "KVM_SEV_LAUNCH_UPDATE should fail, invalid policy.");
> +	sev_guest_status_assert(vm, type);
> +
> +	ret = sev_vm_launch_measure(vm, alloca(256));
> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
> +	TEST_ASSERT(cond,
> +		    "KVM_SEV_LAUNCH_MEASURE should fail, invalid policy.");
> +	sev_guest_status_assert(vm, type);
> +
> +	ret = sev_vm_launch_finish(vm);
> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
> +	TEST_ASSERT(cond,
> +		    "KVM_SEV_LAUNCH_FINISH should fail, invalid policy.");
> +	sev_guest_status_assert(vm, type);
> +
> +	vcpu_run(vcpu);
> +	get_ucall(vcpu, &uc);
> +	cond = type == KVM_X86_SEV_VM ?
> +		vcpu->run->exit_reason == KVM_EXIT_IO :
> +		vcpu->run->exit_reason == KVM_EXIT_FAIL_ENTRY;
> +	TEST_ASSERT(cond,
> +		    "vcpu_run should fail, invalid policy.");
> +
> +	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. */

Re: [RFC 3/5] selftests: KVM: SEV IOCTL test

[Sampat, Pratik Rajesh]

Hi Tom

On 7/11/2024 1:34 PM, Tom Lendacky wrote:
> On 7/10/24 17:05, Pratik R. Sampat wrote:
>> 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>
>> ---
>>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 57 +++++++++++++++++++
>>  1 file changed, 57 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 1a50a280173c..500c67b3793b 100644
>> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> @@ -131,12 +131,69 @@ static void test_sync_vmsa(uint32_t type, uint32_t policy)
>>  	kvm_vm_free(vm);
>>  }
>>  
>> +static void sev_guest_status_assert(struct kvm_vm *vm, uint32_t type)
>> +{
>> +	struct kvm_sev_guest_status status;
>> +	bool cond;
>> +	int ret;
>> +
>> +	ret = __vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
>> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
>> +	TEST_ASSERT(cond,
>> +		    "KVM_SEV_GUEST_STATUS should fail, invalid VM Type.");
>> +}
>> +
>> +static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	struct kvm_vm *vm;
>> +	struct ucall uc;
>> +	bool cond;
>> +	int ret;
>> +
> 
> Maybe a block comment here indicating what you're actually doing would
> be good, because I'm a bit confused.
> 
> A policy value of 0 is valid for SEV, so you expect each call to
> succeed, right? And, actually, for SEV-ES the launch start will succeed,
> too, but the launch update will fail because LAUNCH_UPDATE_VMSA is not
> valid for SEV, but then the launch measure should succeed. Is that
> right? What about the other calls?
> 

Sure, I can do that.
Yes for SEV, the policy value of 0 succeeds for everything except when
we try to run and we see a KVM_EXIT_IO.

For SEV-ES, with the policy value of 0 - we don't see launch_start
succeed. It fails with EIO in this case. Post that all the calls for
SEV-ES also fail subsequent to that. I guess the core idea behind this
test is to ensure that once the first bad case of launch_start fails, we
should see a cascading list of failures.

Thank you!
Pratik

> Thanks,
> Tom
> 
>> +	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
>> +	ret = sev_vm_launch_start(vm, 0);
>> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
>> +	TEST_ASSERT(cond,
>> +		    "KVM_SEV_LAUNCH_START should fail, invalid policy.");
>> +
>> +	ret = sev_vm_launch_update(vm, policy);
>> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
>> +	TEST_ASSERT(cond,
>> +		    "KVM_SEV_LAUNCH_UPDATE should fail, invalid policy.");
>> +	sev_guest_status_assert(vm, type);
>> +
>> +	ret = sev_vm_launch_measure(vm, alloca(256));
>> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
>> +	TEST_ASSERT(cond,
>> +		    "KVM_SEV_LAUNCH_MEASURE should fail, invalid policy.");
>> +	sev_guest_status_assert(vm, type);
>> +
>> +	ret = sev_vm_launch_finish(vm);
>> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
>> +	TEST_ASSERT(cond,
>> +		    "KVM_SEV_LAUNCH_FINISH should fail, invalid policy.");
>> +	sev_guest_status_assert(vm, type);
>> +
>> +	vcpu_run(vcpu);
>> +	get_ucall(vcpu, &uc);
>> +	cond = type == KVM_X86_SEV_VM ?
>> +		vcpu->run->exit_reason == KVM_EXIT_IO :
>> +		vcpu->run->exit_reason == KVM_EXIT_FAIL_ENTRY;
>> +	TEST_ASSERT(cond,
>> +		    "vcpu_run should fail, invalid policy.");
>> +
>> +	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. */

Re: [RFC 3/5] selftests: KVM: SEV IOCTL test

[Sean Christopherson]

On Thu, Jul 11, 2024, Pratik Rajesh Sampat wrote:
> >> +static void sev_guest_status_assert(struct kvm_vm *vm, uint32_t type)
> >> +{
> >> +	struct kvm_sev_guest_status status;
> >> +	bool cond;
> >> +	int ret;
> >> +
> >> +	ret = __vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
> >> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
> >> +	TEST_ASSERT(cond,
> >> +		    "KVM_SEV_GUEST_STATUS should fail, invalid VM Type.");
> >> +}
> >> +
> >> +static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
> >> +{
> >> +	struct kvm_vcpu *vcpu;
> >> +	struct kvm_vm *vm;
> >> +	struct ucall uc;
> >> +	bool cond;
> >> +	int ret;
> >> +
> > 
> > Maybe a block comment here indicating what you're actually doing would
> > be good, because I'm a bit confused.
> > 
> > A policy value of 0 is valid for SEV, so you expect each call to
> > succeed, right? And, actually, for SEV-ES the launch start will succeed,
> > too, but the launch update will fail because LAUNCH_UPDATE_VMSA is not
> > valid for SEV, but then the launch measure should succeed. Is that
> > right? What about the other calls?
> > 
> 
> Sure, I can do that.
> Yes for SEV, the policy value of 0 succeeds for everything except when
> we try to run and we see a KVM_EXIT_IO.
> 
> For SEV-ES, with the policy value of 0 - we don't see launch_start
> succeed. It fails with EIO in this case. Post that all the calls for
> SEV-ES also fail subsequent to that. I guess the core idea behind this
> test is to ensure that once the first bad case of launch_start fails, we
> should see a cascading list of failures.
>
> >> +	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
> >> +	ret = sev_vm_launch_start(vm, 0);
> >> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
> >> +	TEST_ASSERT(cond,

Don't bury the result in a local boolean.  It's confusing, and _worse_ for debug
as it makes it impossible to see what actually failed (the assert message will
simply print "cond", which is useless).


> >> +		    "KVM_SEV_LAUNCH_START should fail, invalid policy.");

This is a blatant lie, because the KVM_X86_SEV_VM case apparently expects success.
Similar to Tom's comments about explaing what this code is doing, these assert
messages need to explain what the actually expected result it, provide a hint as
to _why_ that result is expected, and print the result.  As is, this will be
unnecessarily difficult to debug if/when it fails.

Re: [RFC 3/5] selftests: KVM: SEV IOCTL test

[Pratik R. Sampat]

On 8/9/2024 10:45 AM, Sean Christopherson wrote:
> On Thu, Jul 11, 2024, Pratik Rajesh Sampat wrote:
>>>> +static void sev_guest_status_assert(struct kvm_vm *vm, uint32_t type)
>>>> +{
>>>> +	struct kvm_sev_guest_status status;
>>>> +	bool cond;
>>>> +	int ret;
>>>> +
>>>> +	ret = __vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
>>>> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
>>>> +	TEST_ASSERT(cond,
>>>> +		    "KVM_SEV_GUEST_STATUS should fail, invalid VM Type.");
>>>> +}
>>>> +
>>>> +static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>>>> +{
>>>> +	struct kvm_vcpu *vcpu;
>>>> +	struct kvm_vm *vm;
>>>> +	struct ucall uc;
>>>> +	bool cond;
>>>> +	int ret;
>>>> +
>>>
>>> Maybe a block comment here indicating what you're actually doing would
>>> be good, because I'm a bit confused.
>>>
>>> A policy value of 0 is valid for SEV, so you expect each call to
>>> succeed, right? And, actually, for SEV-ES the launch start will succeed,
>>> too, but the launch update will fail because LAUNCH_UPDATE_VMSA is not
>>> valid for SEV, but then the launch measure should succeed. Is that
>>> right? What about the other calls?
>>>
>>
>> Sure, I can do that.
>> Yes for SEV, the policy value of 0 succeeds for everything except when
>> we try to run and we see a KVM_EXIT_IO.
>>
>> For SEV-ES, with the policy value of 0 - we don't see launch_start
>> succeed. It fails with EIO in this case. Post that all the calls for
>> SEV-ES also fail subsequent to that. I guess the core idea behind this
>> test is to ensure that once the first bad case of launch_start fails, we
>> should see a cascading list of failures.
>>
>>>> +	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
>>>> +	ret = sev_vm_launch_start(vm, 0);
>>>> +	cond = type == KVM_X86_SEV_VM ? !ret : ret;
>>>> +	TEST_ASSERT(cond,
> 
> Don't bury the result in a local boolean.  It's confusing, and _worse_ for debug
> as it makes it impossible to see what actually failed (the assert message will
> simply print "cond", which is useless).
> 

Ack, I will make sure all the other occurrences of using similar boolean
are also removed and the conditions themselves are passed into the assert.

> 
>>>> +		    "KVM_SEV_LAUNCH_START should fail, invalid policy.");
> 
> This is a blatant lie, because the KVM_X86_SEV_VM case apparently expects success.
> Similar to Tom's comments about explaing what this code is doing, these assert
> messages need to explain what the actually expected result it, provide a hint as
> to _why_ that result is expected, and print the result.  As is, this will be
> unnecessarily difficult to debug if/when it fails.

Right. I'll make the error messages more reflective of what they are as
well as have an explanation to why we expect this behavior.

Thanks!
- Pratik

[RFC 4/5] selftests: KVM: SNP IOCTL test

[Pratik R. Sampat]

Introduce testing of SNP ioctl calls. This patch includes both positive
and negative tests of various parameters such as flags, page types and
policies.

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
---
 .../selftests/kvm/x86_64/sev_smoke_test.c     | 119 +++++++++++++++++-
 1 file changed, 118 insertions(+), 1 deletion(-)

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 500c67b3793b..1d5c275c11b3 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
@@ -186,13 +186,130 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
 	kvm_vm_free(vm);
 }
 
+static int spawn_snp_launch_start(uint32_t type, uint64_t policy, uint8_t flags)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
+	ret = snp_vm_launch(vm, policy, flags);
+	kvm_vm_free(vm);
+
+	return ret;
+}
+
+static void test_snp_launch_start(uint32_t type, uint64_t policy)
+{
+	uint8_t i;
+	int ret;
+
+	ret = spawn_snp_launch_start(type, policy, 0);
+	TEST_ASSERT(!ret,
+		    "KVM_SEV_SNP_LAUNCH_START should not fail, invalid flag.");
+
+	for (i = 1; i < 8; i++) {
+		ret = spawn_snp_launch_start(type, policy, BIT(i));
+		TEST_ASSERT(ret && errno == EINVAL,
+			    "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag.");
+	}
+
+	ret = spawn_snp_launch_start(type, 0, 0);
+	TEST_ASSERT(ret && errno == EINVAL,
+		    "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
+
+	ret = spawn_snp_launch_start(type, SNP_POLICY_SMT, 0);
+	TEST_ASSERT(ret && errno == EINVAL,
+		    "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
+
+	ret = spawn_snp_launch_start(type, SNP_POLICY_RSVD_MBO, 0);
+	TEST_ASSERT(ret && errno == EINVAL,
+		    "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
+
+	ret = spawn_snp_launch_start(type, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO |
+				     (255 * SNP_POLICY_ABI_MAJOR) |
+				     (255 * SNP_POLICY_ABI_MINOR), 0);
+	TEST_ASSERT(ret && errno == EIO,
+		    "KVM_SEV_SNP_LAUNCH_START should fail, invalid version.");
+}
+
+static void test_snp_launch_update(uint32_t type, uint64_t policy)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	for (int pgtype = 0; pgtype <= KVM_SEV_SNP_PAGE_TYPE_CPUID; pgtype++) {
+		vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
+		snp_vm_launch(vm, policy, 0);
+		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 not fail, invalid Page type.");
+			break;
+		case KVM_SEV_SNP_PAGE_TYPE_CPUID:
+			TEST_ASSERT(ret && errno == EIO,
+				    "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type.");
+			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_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
+	snp_vm_launch(vm, policy, 0);
+	snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
+	ret = snp_vm_launch_finish(vm, 0);
+	TEST_ASSERT(!ret,
+		    "KVM_SEV_SNP_LAUNCH_FINISH should not fail, invalid flag.");
+	kvm_vm_free(vm);
+
+	for (int i = 1; i < 16; i++) {
+		vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
+		snp_vm_launch(vm, policy, 0);
+		snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
+		ret = snp_vm_launch_finish(vm, BIT(i));
+		TEST_ASSERT(ret && errno == EINVAL,
+			    "KVM_SEV_SNP_LAUNCH_FINISH should fail, invalid flag.");
+		kvm_vm_free(vm);
+	}
+}
+
+static void test_sev_ioctl(void *guest_code, uint32_t type, uint64_t policy)
+{
+	if (type == KVM_X86_SNP_VM) {
+		test_snp_launch_start(type, policy);
+		test_snp_launch_update(type, policy);
+		test_snp_launch_finish(type, policy);
+
+		return;
+	}
+
+	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);
+	test_sev_ioctl(guest_code, type, policy);
 
 	vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
 
-- 
2.34.1

Re: [RFC 4/5] selftests: KVM: SNP IOCTL test

[Peter Gonda]

On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
<pratikrajesh.sampat@amd.com> wrote:
>
> Introduce testing of SNP ioctl calls. This patch includes both positive
> and negative tests of various parameters such as flags, page types and
> policies.
>
> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>

Tested-by: Peter Gonda <pgonda@google.com>

> ---
>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 119 +++++++++++++++++-
>  1 file changed, 118 insertions(+), 1 deletion(-)
>
> 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 500c67b3793b..1d5c275c11b3 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> @@ -186,13 +186,130 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>         kvm_vm_free(vm);
>  }
>
> +static int spawn_snp_launch_start(uint32_t type, uint64_t policy, uint8_t flags)
> +{
> +       struct kvm_vcpu *vcpu;
> +       struct kvm_vm *vm;
> +       int ret;
> +
> +       vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> +       ret = snp_vm_launch(vm, policy, flags);
> +       kvm_vm_free(vm);
> +
> +       return ret;
> +}
> +
> +static void test_snp_launch_start(uint32_t type, uint64_t policy)
> +{
> +       uint8_t i;
> +       int ret;
> +
> +       ret = spawn_snp_launch_start(type, policy, 0);
> +       TEST_ASSERT(!ret,
> +                   "KVM_SEV_SNP_LAUNCH_START should not fail, invalid flag.");
> +
> +       for (i = 1; i < 8; i++) {
> +               ret = spawn_snp_launch_start(type, policy, BIT(i));
> +               TEST_ASSERT(ret && errno == EINVAL,
> +                           "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag.");
> +       }

To save readers sometime do we want to comment that flags must be zero?

> +
> +       ret = spawn_snp_launch_start(type, 0, 0);
> +       TEST_ASSERT(ret && errno == EINVAL,
> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
> +
> +       ret = spawn_snp_launch_start(type, SNP_POLICY_SMT, 0);
> +       TEST_ASSERT(ret && errno == EINVAL,
> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
> +
> +       ret = spawn_snp_launch_start(type, SNP_POLICY_RSVD_MBO, 0);
> +       TEST_ASSERT(ret && errno == EINVAL,
> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");

Ditto on SMT comment, this could pass if SMT was disabled right?

> +
> +       ret = spawn_snp_launch_start(type, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO |
> +                                    (255 * SNP_POLICY_ABI_MAJOR) |
> +                                    (255 * SNP_POLICY_ABI_MINOR), 0);
> +       TEST_ASSERT(ret && errno == EIO,
> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid version.");
> +}
> +
> +static void test_snp_launch_update(uint32_t type, uint64_t policy)
> +{
> +       struct kvm_vcpu *vcpu;
> +       struct kvm_vm *vm;
> +       int ret;
> +
> +       for (int pgtype = 0; pgtype <= KVM_SEV_SNP_PAGE_TYPE_CPUID; pgtype++) {

Do we want to test KVM_SEV_SNP_PAGE_TYPE_CPUID+1 to make sure that fails?

> +               vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> +               snp_vm_launch(vm, policy, 0);
> +               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 not fail, invalid Page type.");

Double negative maybe: "KVM_SEV_SNP_LAUNCH_UPDATE should succeed..."

> +                       break;
> +               case KVM_SEV_SNP_PAGE_TYPE_CPUID:
> +                       TEST_ASSERT(ret && errno == EIO,
> +                                   "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type.");

This is a valid page type right? But I think the error is from the ASP
due to the page being malformed for a CPUID page.

> +                       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_vcpu *vcpu;
> +       struct kvm_vm *vm;
> +       int ret;
> +
> +       vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> +       snp_vm_launch(vm, policy, 0);
> +       snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
> +       ret = snp_vm_launch_finish(vm, 0);
> +       TEST_ASSERT(!ret,
> +                   "KVM_SEV_SNP_LAUNCH_FINISH should not fail, invalid flag.");

Comment is wrong, maybe: "KVM_SEV_SNP_LAUNCH_FINISH should not fail."

> +       kvm_vm_free(vm);
> +
> +       for (int i = 1; i < 16; i++) {
> +               vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> +               snp_vm_launch(vm, policy, 0);
> +               snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
> +               ret = snp_vm_launch_finish(vm, BIT(i));
> +               TEST_ASSERT(ret && errno == EINVAL,
> +                           "KVM_SEV_SNP_LAUNCH_FINISH should fail, invalid flag.");
> +               kvm_vm_free(vm);

To save readers sometime do we want to comment that flags must be zero?

> +       }
> +}
> +
> +static void test_sev_ioctl(void *guest_code, uint32_t type, uint64_t policy)
> +{
> +       if (type == KVM_X86_SNP_VM) {
> +               test_snp_launch_start(type, policy);
> +               test_snp_launch_update(type, policy);
> +               test_snp_launch_finish(type, policy);
> +
> +               return;
> +       }
> +
> +       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);
> +       test_sev_ioctl(guest_code, type, policy);
>
>         vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
>
> --
> 2.34.1
>

Re: [RFC 4/5] selftests: KVM: SNP IOCTL test

[Sampat, Pratik Rajesh]

On 7/11/2024 10:57 AM, Peter Gonda wrote:
> On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
> <pratikrajesh.sampat@amd.com> wrote:
>>
>> Introduce testing of SNP ioctl calls. This patch includes both positive
>> and negative tests of various parameters such as flags, page types and
>> policies.
>>
>> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
> 
> Tested-by: Peter Gonda <pgonda@google.com>
> 
>> ---
>>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 119 +++++++++++++++++-
>>  1 file changed, 118 insertions(+), 1 deletion(-)
>>
>> 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 500c67b3793b..1d5c275c11b3 100644
>> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> @@ -186,13 +186,130 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>>         kvm_vm_free(vm);
>>  }
>>
>> +static int spawn_snp_launch_start(uint32_t type, uint64_t policy, uint8_t flags)
>> +{
>> +       struct kvm_vcpu *vcpu;
>> +       struct kvm_vm *vm;
>> +       int ret;
>> +
>> +       vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
>> +       ret = snp_vm_launch(vm, policy, flags);
>> +       kvm_vm_free(vm);
>> +
>> +       return ret;
>> +}
>> +
>> +static void test_snp_launch_start(uint32_t type, uint64_t policy)
>> +{
>> +       uint8_t i;
>> +       int ret;
>> +
>> +       ret = spawn_snp_launch_start(type, policy, 0);
>> +       TEST_ASSERT(!ret,
>> +                   "KVM_SEV_SNP_LAUNCH_START should not fail, invalid flag.");
>> +
>> +       for (i = 1; i < 8; i++) {
>> +               ret = spawn_snp_launch_start(type, policy, BIT(i));
>> +               TEST_ASSERT(ret && errno == EINVAL,
>> +                           "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag.");
>> +       }
> 
> To save readers sometime do we want to comment that flags must be zero?
> 

Ack. I can add that comment.

>> +
>> +       ret = spawn_snp_launch_start(type, 0, 0);
>> +       TEST_ASSERT(ret && errno == EINVAL,
>> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
>> +
>> +       ret = spawn_snp_launch_start(type, SNP_POLICY_SMT, 0);
>> +       TEST_ASSERT(ret && errno == EINVAL,
>> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
>> +
>> +       ret = spawn_snp_launch_start(type, SNP_POLICY_RSVD_MBO, 0);
>> +       TEST_ASSERT(ret && errno == EINVAL,
>> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
> 
> Ditto on SMT comment, this could pass if SMT was disabled right?
> 

Ack.
Yes, it could. Maybe the check I was speaking about earlier about SMT
can be made here as well and based on that we decide if this should fail
or pass.

>> +
>> +       ret = spawn_snp_launch_start(type, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO |
>> +                                    (255 * SNP_POLICY_ABI_MAJOR) |
>> +                                    (255 * SNP_POLICY_ABI_MINOR), 0);
>> +       TEST_ASSERT(ret && errno == EIO,
>> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid version.");
>> +}
>> +
>> +static void test_snp_launch_update(uint32_t type, uint64_t policy)
>> +{
>> +       struct kvm_vcpu *vcpu;
>> +       struct kvm_vm *vm;
>> +       int ret;
>> +
>> +       for (int pgtype = 0; pgtype <= KVM_SEV_SNP_PAGE_TYPE_CPUID; pgtype++) {
> 
> Do we want to test KVM_SEV_SNP_PAGE_TYPE_CPUID+1 to make sure that fails?
> 

We could. Looking at loop however, we also go through 0x2 which is
undefined so I thought we were already taking care of the negative test
case here. Having said that, I have no issues in adding one more case
that fails.

>> +               vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
>> +               snp_vm_launch(vm, policy, 0);
>> +               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 not fail, invalid Page type.");
> 
> Double negative maybe: "KVM_SEV_SNP_LAUNCH_UPDATE should succeed..."
> 

Ack. This double negative is used in a couple of more places. Will clean
them up too.

>> +                       break;
>> +               case KVM_SEV_SNP_PAGE_TYPE_CPUID:
>> +                       TEST_ASSERT(ret && errno == EIO,
>> +                                   "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type.");
> 
> This is a valid page type right? But I think the error is from the ASP
> due to the page being malformed for a CPUID page.
> 

Yes you're absolutely right. It's technically a correct page type just
not set up correctly to be used this way so we should see it fail.

>> +                       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_vcpu *vcpu;
>> +       struct kvm_vm *vm;
>> +       int ret;
>> +
>> +       vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
>> +       snp_vm_launch(vm, policy, 0);
>> +       snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
>> +       ret = snp_vm_launch_finish(vm, 0);
>> +       TEST_ASSERT(!ret,
>> +                   "KVM_SEV_SNP_LAUNCH_FINISH should not fail, invalid flag.");
> 
> Comment is wrong, maybe: "KVM_SEV_SNP_LAUNCH_FINISH should not fail."
> 

Thanks for catching this. Will fix the comment.

>> +       kvm_vm_free(vm);
>> +
>> +       for (int i = 1; i < 16; i++) {
>> +               vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
>> +               snp_vm_launch(vm, policy, 0);
>> +               snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
>> +               ret = snp_vm_launch_finish(vm, BIT(i));
>> +               TEST_ASSERT(ret && errno == EINVAL,
>> +                           "KVM_SEV_SNP_LAUNCH_FINISH should fail, invalid flag.");
>> +               kvm_vm_free(vm);
> 
> To save readers sometime do we want to comment that flags must be zero?
> 

Ack.

Thanks again for the review

Re: [RFC 4/5] selftests: KVM: SNP IOCTL test

[Sean Christopherson]

On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
> Introduce testing of SNP ioctl calls. This patch includes both positive
> and negative tests of various parameters such as flags, page types and
> policies.
> 
> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
> ---
>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 119 +++++++++++++++++-
>  1 file changed, 118 insertions(+), 1 deletion(-)
> 
> 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 500c67b3793b..1d5c275c11b3 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> @@ -186,13 +186,130 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>  	kvm_vm_free(vm);
>  }
>  
> +static int spawn_snp_launch_start(uint32_t type, uint64_t policy, uint8_t flags)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	int ret;
> +
> +	vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);

Is a vCPU actually necessary/interesting?

> +	ret = snp_vm_launch(vm, policy, flags);
> +	kvm_vm_free(vm);
> +
> +	return ret;
> +}
> +
> +static void test_snp_launch_start(uint32_t type, uint64_t policy)
> +{
> +	uint8_t i;
> +	int ret;
> +
> +	ret = spawn_snp_launch_start(type, policy, 0);

s/spawn/__test, because "spawn" implies there's something living after this.

> +	TEST_ASSERT(!ret,
> +		    "KVM_SEV_SNP_LAUNCH_START should not fail, invalid flag.");

This should go away once vm_sev_ioctl() handles the assertion, but this assert
message is bad (there's no invalid flag).

> +
> +	for (i = 1; i < 8; i++) {
> +		ret = spawn_snp_launch_start(type, policy, BIT(i));
> +		TEST_ASSERT(ret && errno == EINVAL,
> +			    "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag.");

Print the flag, type, and policy.  In general, please think about what information
would be helpful if this fails.

Re: [RFC 4/5] selftests: KVM: SNP IOCTL test

[Pratik R. Sampat]

On 8/9/2024 10:48 AM, Sean Christopherson wrote:
> On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
>> Introduce testing of SNP ioctl calls. This patch includes both positive
>> and negative tests of various parameters such as flags, page types and
>> policies.
>>
>> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
>> ---
>>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 119 +++++++++++++++++-
>>  1 file changed, 118 insertions(+), 1 deletion(-)
>>
>> 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 500c67b3793b..1d5c275c11b3 100644
>> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> @@ -186,13 +186,130 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>>  	kvm_vm_free(vm);
>>  }
>>  
>> +static int spawn_snp_launch_start(uint32_t type, uint64_t policy, uint8_t flags)
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	struct kvm_vm *vm;
>> +	int ret;
>> +
>> +	vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> 
> Is a vCPU actually necessary/interesting?
> 

Yes, vcpu is not really needed here.
I could get away with a simple vm_create variant where I can pass the type.

>> +	ret = snp_vm_launch(vm, policy, flags);
>> +	kvm_vm_free(vm);
>> +
>> +	return ret;
>> +}
>> +
>> +static void test_snp_launch_start(uint32_t type, uint64_t policy)
>> +{
>> +	uint8_t i;
>> +	int ret;
>> +
>> +	ret = spawn_snp_launch_start(type, policy, 0);
> 
> s/spawn/__test, because "spawn" implies there's something living after this.
> 

Ack. Changed.

>> +	TEST_ASSERT(!ret,
>> +		    "KVM_SEV_SNP_LAUNCH_START should not fail, invalid flag.");
> 
> This should go away once vm_sev_ioctl() handles the assertion, but this assert
> message is bad (there's no invalid flag).
> 
>> +
>> +	for (i = 1; i < 8; i++) {
>> +		ret = spawn_snp_launch_start(type, policy, BIT(i));
>> +		TEST_ASSERT(ret && errno == EINVAL,
>> +			    "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag.");
> 
> Print the flag, type, and policy.  In general, please think about what information
> would be helpful if this fails.

Right, I'll be more mindful of the error messages shown and try to
display more useful information from them for the overall series.

Thanks!
Pratik

[RFC 5/5] selftests: KVM: SEV-SNP test for KVM_SEV_INIT2

[Pratik R. Sampat]

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

Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@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 7a4a61be119b..68f7edaa5526 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_cpu_has(X86_FEATURE_SNP);
+	TEST_ASSERT(have_snp == !!(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SNP_VM)),
+		    "sev-snp: KVM_CAP_VM_TYPES (%x) does not match cpuid (checking %x)",
+		    kvm_check_cap(KVM_CAP_VM_TYPES), 1 << 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

Re: [RFC 5/5] selftests: KVM: SEV-SNP test for KVM_SEV_INIT2

[Peter Gonda]

On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
<pratikrajesh.sampat@amd.com> wrote:
>
> Add SEV-SNP VM type to exercise the KVM_SEV_INIT2 call.
>
> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>

Tested-by: Peter Gonda <pgonda@google.com>