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(-)
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); } }
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.
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
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
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
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); }
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
+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.
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?
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).
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
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
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
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. */
+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?
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!
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. */
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. */
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.
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
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);
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
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
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.
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
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; }
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
linux-kselftest-mirror@lists.linaro.org
-
Peter Gonda -
Pratik R. Sampat -
Sampat, Pratik Rajesh
-
Sean Christopherson -
Tom Lendacky