| Message ID | 20220829171021.701198-7-pgonda@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Add simple SEV test | expand |
On Mon, Aug 29, 2022, Peter Gonda wrote: > Add interfaces to allow tests to create/manage SEV guests. The > additional state associated with these guests is encapsulated in a new > struct sev_vm, which is a light wrapper around struct kvm_vm. These > VMs will use vm_set_memory_encryption() and vm_get_encrypted_phy_pages() > under the covers to configure and sync up with the core kvm_util > library on what should/shouldn't be treated as encrypted memory. > > Originally-by: Michael Roth <michael.roth@amd.com> > Signed-off-by: Peter Gonda <pgonda@google.com> > --- > tools/testing/selftests/kvm/Makefile | 1 + > .../selftests/kvm/include/kvm_util_base.h | 3 + > .../selftests/kvm/include/x86_64/sev.h | 47 ++++ > tools/testing/selftests/kvm/lib/x86_64/sev.c | 232 ++++++++++++++++++ > 4 files changed, 283 insertions(+) > create mode 100644 tools/testing/selftests/kvm/include/x86_64/sev.h > create mode 100644 tools/testing/selftests/kvm/lib/x86_64/sev.c > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > index 23649c5d42fc..0a70e50f0498 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -56,6 +56,7 @@ LIBKVM_x86_64 += lib/x86_64/processor.c > LIBKVM_x86_64 += lib/x86_64/svm.c > LIBKVM_x86_64 += lib/x86_64/ucall.c > LIBKVM_x86_64 += lib/x86_64/vmx.c > +LIBKVM_x86_64 += lib/x86_64/sev.c > > LIBKVM_aarch64 += lib/aarch64/gic.c > LIBKVM_aarch64 += lib/aarch64/gic_v3.c > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index 489e8c833e5f..0927e262623d 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -69,6 +69,7 @@ struct userspace_mem_regions { > /* Memory encryption policy/configuration. */ > struct vm_memcrypt { > bool enabled; > + bool encrypted; > int8_t enc_by_default; > bool has_enc_bit; > int8_t enc_bit; > @@ -831,6 +832,8 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva); > > static inline vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva) > { > + TEST_ASSERT(!vm->memcrypt.encrypted, vm->protected > + "Encrypted guests have their page tables encrypted so gva2gpa conversions are not possible."); Unnecessarily verbose, e.g. "Protected VMs have private, inaccessible page tables"); > +#define CPUID_MEM_ENC_LEAF 0x8000001f > +#define CPUID_EBX_CBIT_MASK 0x3f > + > +/* Common SEV helpers/accessors. */ Please drop this comment and the "Local helpers" and "SEV VM implementation" comments below. There's 0% chance these comments will stay fresh as code is added and moved around. They also add no value IMO, e.g. "static" makes it quite obvious it's a local function, and sev_* vs. sev_es_*. vs. sev_snp_* namespacing takes care of the rest. > +void sev_ioctl(int sev_fd, int cmd, void *data) > +{ > + int ret; > + struct sev_issue_cmd arg; > + > + arg.cmd = cmd; > + arg.data = (unsigned long)data; > + ret = ioctl(sev_fd, SEV_ISSUE_CMD, &arg); > + TEST_ASSERT(ret == 0, > + "SEV ioctl %d failed, error: %d, fw_error: %d", > + cmd, ret, arg.error); > +} > + > +void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data) > +{ > + struct kvm_sev_cmd arg = {0}; > + int ret; > + > + arg.id = cmd; > + arg.sev_fd = sev->fd; > + arg.data = (__u64)data; > + > + ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_OP, &arg); > + TEST_ASSERT(ret == 0, > + "SEV KVM ioctl %d failed, rc: %i errno: %i (%s), fw_error: %d", > + cmd, ret, errno, strerror(errno), arg.error); > +} > + > +/* Local helpers. */ > + > +static void sev_register_user_region(struct sev_vm *sev, void *hva, uint64_t size) > +{ > + struct kvm_enc_region range = {0}; > + int ret; > + > + pr_debug("%s: hva: %p, size: %lu\n", __func__, hva, size); > + > + range.addr = (__u64)hva; > + range.size = size; > + > + ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_REG_REGION, &range); > + TEST_ASSERT(ret == 0, "failed to register user range, errno: %i\n", errno); > +} > + > +static void sev_encrypt_phy_range(struct sev_vm *sev, vm_paddr_t gpa, uint64_t size) > +{ > + struct kvm_sev_launch_update_data ksev_update_data = {0}; > + > + pr_debug("%s: addr: 0x%lx, size: %lu\n", __func__, gpa, size); > + > + ksev_update_data.uaddr = (__u64)addr_gpa2hva(sev->vm, gpa); > + ksev_update_data.len = size; > + > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_UPDATE_DATA, &ksev_update_data); > +} > + > +static void sev_encrypt(struct sev_vm *sev) > +{ > + const struct sparsebit *enc_phy_pages; > + struct kvm_vm *vm = sev->vm; > + sparsebit_idx_t pg = 0; > + vm_paddr_t gpa_start; > + uint64_t memory_size; > + int ctr; > + struct userspace_mem_region *region; > + > + hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) { > + enc_phy_pages = vm_get_encrypted_phy_pages( Please don't wrap after the opening paranthesis unless it's really, really necessary. More for future reference since I think vm_get_encrypted_phy_pages() should be open coded here. E.g. in this case, the "enc_phy_" prefix doesn't add much value, and dropping that makes the code easier to read overall. pages = vm_get_encrypted_phy_pages(sev->vm, region->region.slot, &gpa_start, &memory_size); > + sev->vm, region->region.slot, &gpa_start, &memory_size); > + TEST_ASSERT(enc_phy_pages, > + "Unable to retrieve encrypted pages bitmap"); > + while (pg < (memory_size / vm->page_size)) { > + sparsebit_idx_t pg_cnt; s/pg_cnt/nr_pages > + > + if (sparsebit_is_clear(enc_phy_pages, pg)) { > + pg = sparsebit_next_set(enc_phy_pages, pg); > + if (!pg) > + break; > + } > + > + pg_cnt = sparsebit_next_clear(enc_phy_pages, pg) - pg; > + if (pg_cnt <= 0) > + pg_cnt = 1; > + > + sev_encrypt_phy_range(sev, > + gpa_start + pg * vm->page_size, > + pg_cnt * vm->page_size); > + pg += pg_cnt; > + } > + } > + > + sev->vm->memcrypt.encrypted = true; > +} > + > +/* SEV VM implementation. */ > + > +static struct sev_vm *sev_vm_alloc(struct kvm_vm *vm) > +{ > + struct sev_user_data_status sev_status; > + uint32_t eax, ebx, ecx, edx; > + struct sev_vm *sev; > + int sev_fd; > + > + sev_fd = open(SEV_DEV_PATH, O_RDWR); > + if (sev_fd < 0) { > + pr_info("Failed to open SEV device, path: %s, error: %d, skipping test.\n", > + SEV_DEV_PATH, sev_fd); > + return NULL; Printing "skipping test" is wrong as there's no guarantee the caller is going to skip the test. E.g. the sole user in this series asserts, i.e. fails the test. I also think that waiting until VM allocation to perform these sanity checks is flawed. Rather do these checks every time, add helpers to query SEV and SEV-ES support, and then use TEST_REQUIRE() to actually skip tests that require support, e.g. TEST_REQUIRE(kvm_is_sev_supported()); or TEST_REQUIRE(kvm_is_sev_es_supported()); Then this helper can simply assert that opening SEV_DEV_PATH succeeds. > + } > + > + sev_ioctl(sev_fd, SEV_PLATFORM_STATUS, &sev_status); > + > + if (!(sev_status.api_major > SEV_FW_REQ_VER_MAJOR || > + (sev_status.api_major == SEV_FW_REQ_VER_MAJOR && > + sev_status.api_minor >= SEV_FW_REQ_VER_MINOR))) { > + pr_info("SEV FW version too old. Have API %d.%d (build: %d), need %d.%d, skipping test.\n", > + sev_status.api_major, sev_status.api_minor, sev_status.build, > + SEV_FW_REQ_VER_MAJOR, SEV_FW_REQ_VER_MINOR); > + return NULL; > + } > + > + sev = calloc(1, sizeof(*sev)); TEST_ASSERT(sev, ...) > + sev->fd = sev_fd; > + sev->vm = vm; > + > + /* Get encryption bit via CPUID. */ > + cpuid(CPUID_MEM_ENC_LEAF, &eax, &ebx, &ecx, &edx); > + sev->enc_bit = ebx & CPUID_EBX_CBIT_MASK; Oh hey, another series of mine[*] that you can leverage :-) [*] https://lore.kernel.org/all/20221006005125.680782-1-seanjc@google.com > + > + return sev; > +} > + > +void sev_vm_free(struct sev_vm *sev) > +{ > + kvm_vm_free(sev->vm); > + close(sev->fd); > + free(sev); > +} > + > +struct sev_vm *sev_vm_create(uint32_t policy, uint64_t npages) The migration test already defines sev_vm_create(). That conflict needs to be resolved. > +{ > + struct sev_vm *sev; > + struct kvm_vm *vm; > + > + /* Need to handle memslots after init, and after setting memcrypt. */ > + vm = vm_create_barebones(); Do not use vm_create_barebones(). That API is only to be used for tests that do not intend to run vCPUs. > + sev = sev_vm_alloc(vm); > + if (!sev) > + return NULL; > + sev->sev_policy = policy; > + > + kvm_sev_ioctl(sev, KVM_SEV_INIT, NULL); > + > + vm->vpages_mapped = sparsebit_alloc(); This is unnecessary and leaks memory, vm->vpages_mapped is allocated by ____vm_create(). > + vm_set_memory_encryption(vm, true, true, sev->enc_bit); > + pr_info("SEV cbit: %d\n", sev->enc_bit); > + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, npages, 0); > + sev_register_user_region(sev, addr_gpa2hva(vm, 0), > + npages * vm->page_size); Burying sev_register_user_region() in here is not going to be maintainble. I think the best away to handle this is to add an arch hook in vm_userspace_mem_region_add() and automatically register regions when they're created. And with that, I believe sev_vm_create() can go away entirely and the SEV encryption stuff can be handled via a new vm_guest_mode. ____vm_create() already has a gross __x86_64__ hook that we can tweak, e.g. diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 54b8d8825f5d..2d6cbca2c01a 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -238,9 +238,10 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode, uint64_t nr_pages) case VM_MODE_P36V47_16K: vm->pgtable_levels = 3; break; + case VM_MODE_PXXV48_4K_SEV: case VM_MODE_PXXV48_4K: #ifdef __x86_64__ - kvm_get_cpu_address_width(&vm->pa_bits, &vm->va_bits); + kvm_init_vm_address_properties(vm); /* * Ignore KVM support for 5-level paging (vm->va_bits == 57), * it doesn't take effect unless a CR4.LA57 is set, which it Then kvm_init_vm_address_properties() can pivot on vm->mode to deal with SEV specific stuff. > + > + pr_info("SEV guest created, policy: 0x%x, size: %lu KB\n", > + sev->sev_policy, npages * vm->page_size / 1024); > + > + return sev; > +} > + > +void sev_vm_launch(struct sev_vm *sev) > +{ > + struct kvm_sev_launch_start ksev_launch_start = {0}; > + struct kvm_sev_guest_status ksev_status; > + > + ksev_launch_start.policy = sev->sev_policy; > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_START, &ksev_launch_start); > + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); > + TEST_ASSERT(ksev_status.policy == sev->sev_policy, "Incorrect guest policy."); > + TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE, > + "Unexpected guest state: %d", ksev_status.state); > + > + ucall_init(sev->vm, 0); > + > + sev_encrypt(sev); > +} > + > +void sev_vm_launch_measure(struct sev_vm *sev, uint8_t *measurement) > +{ > + struct kvm_sev_launch_measure ksev_launch_measure; > + struct kvm_sev_guest_status ksev_guest_status; > + > + ksev_launch_measure.len = 256; > + ksev_launch_measure.uaddr = (__u64)measurement; > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_MEASURE, &ksev_launch_measure); > + > + /* Measurement causes a state transition, check that. */ > + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_guest_status); > + TEST_ASSERT(ksev_guest_status.state == SEV_GSTATE_LSECRET, > + "Unexpected guest state: %d", ksev_guest_status.state); > +} > + > +void sev_vm_launch_finish(struct sev_vm *sev) > +{ > + struct kvm_sev_guest_status ksev_status; > + > + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); > + TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE || > + ksev_status.state == SEV_GSTATE_LSECRET, > + "Unexpected guest state: %d", ksev_status.state); > + > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_FINISH, NULL); > + > + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); > + TEST_ASSERT(ksev_status.state == SEV_GSTATE_RUNNING, > + "Unexpected guest state: %d", ksev_status.state); > +} Rather than force each test to invoke these via something like setup_test_common(), add the same kvm_arch_vm_post_create() hook that Vishal is likely going to add, and then automatically do all of the launch+measure+finish stuff for non-barebones VMs. That will let SEV/SEV-ES tests use __vm_create_with_vcpus() and __vm_create(). And it'd be a little gross, but I think it'd be wortwhile to add another layer to the "one_vcpu" helpers to make things even more convenient, e.g. struct kvm_vm *____vm_create_with_one_vcpu(enum vm_guest_mode mode, struct kvm_vcpu **vcpu, uint64_t extra_mem_pages, void *guest_code) { struct kvm_vcpu *vcpus[1]; struct kvm_vm *vm; vm = __vm_create_with_vcpus(mode, 1, extra_mem_pages, guest_code, vcpus); *vcpu = vcpus[0]; return vm; } static inline struct kvm_vm *__vm_create_with_one_vcpu(struct kvm_vcpu **vcpu, uint64_t extra_mem_pages, void *guest_code) { return ____vm_create_with_one_vcpu(VM_MODE_DEFAULT, vcpu, extra_mem_pages, guest_code); } static inline struct kvm_vm *____vm_create_with_one_vcpu(enum vm_guest_mode mode, struct kvm_vcpu **vcpu, uint64_t extra_mem_pages, void *guest_code) ____vm_create_with_one_vcpu diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index dafe4471a6c7..593dfadb662e 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -298,9 +298,8 @@ struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint32_t nr_runnable_vcpus, kvm_vm_elf_load(vm, program_invocation_name); -#ifdef __x86_64__ - vm_create_irqchip(vm); -#endif + kvm_arch_vm_post_create(vm); + return vm; } [*] https://lore.kernel.org/all/YzsC4ibDqGh5qaP9@google.com
On Thu, Oct 6, 2022 at 12:25 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Aug 29, 2022, Peter Gonda wrote: > > Add interfaces to allow tests to create/manage SEV guests. The > > additional state associated with these guests is encapsulated in a new > > struct sev_vm, which is a light wrapper around struct kvm_vm. These > > VMs will use vm_set_memory_encryption() and vm_get_encrypted_phy_pages() > > under the covers to configure and sync up with the core kvm_util > > library on what should/shouldn't be treated as encrypted memory. > > > > Originally-by: Michael Roth <michael.roth@amd.com> > > Signed-off-by: Peter Gonda <pgonda@google.com> > > --- > > tools/testing/selftests/kvm/Makefile | 1 + > > .../selftests/kvm/include/kvm_util_base.h | 3 + > > .../selftests/kvm/include/x86_64/sev.h | 47 ++++ > > tools/testing/selftests/kvm/lib/x86_64/sev.c | 232 ++++++++++++++++++ > > 4 files changed, 283 insertions(+) > > create mode 100644 tools/testing/selftests/kvm/include/x86_64/sev.h > > create mode 100644 tools/testing/selftests/kvm/lib/x86_64/sev.c > > > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > > index 23649c5d42fc..0a70e50f0498 100644 > > --- a/tools/testing/selftests/kvm/Makefile > > +++ b/tools/testing/selftests/kvm/Makefile > > @@ -56,6 +56,7 @@ LIBKVM_x86_64 += lib/x86_64/processor.c > > LIBKVM_x86_64 += lib/x86_64/svm.c > > LIBKVM_x86_64 += lib/x86_64/ucall.c > > LIBKVM_x86_64 += lib/x86_64/vmx.c > > +LIBKVM_x86_64 += lib/x86_64/sev.c > > > > LIBKVM_aarch64 += lib/aarch64/gic.c > > LIBKVM_aarch64 += lib/aarch64/gic_v3.c > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > index 489e8c833e5f..0927e262623d 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > @@ -69,6 +69,7 @@ struct userspace_mem_regions { > > /* Memory encryption policy/configuration. */ > > struct vm_memcrypt { > > bool enabled; > > + bool encrypted; > > int8_t enc_by_default; > > bool has_enc_bit; > > int8_t enc_bit; > > @@ -831,6 +832,8 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva); > > > > static inline vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva) > > { > > + TEST_ASSERT(!vm->memcrypt.encrypted, > > vm->protected > > > + "Encrypted guests have their page tables encrypted so gva2gpa conversions are not possible."); > > Unnecessarily verbose, e.g. > > "Protected VMs have private, inaccessible page tables"); > > > +#define CPUID_MEM_ENC_LEAF 0x8000001f > > +#define CPUID_EBX_CBIT_MASK 0x3f > > + > > +/* Common SEV helpers/accessors. */ > > Please drop this comment and the "Local helpers" and "SEV VM implementation" comments > below. There's 0% chance these comments will stay fresh as code is added and moved > around. They also add no value IMO, e.g. "static" makes it quite obvious it's a > local function, and sev_* vs. sev_es_*. vs. sev_snp_* namespacing takes care of the > rest. > > > +void sev_ioctl(int sev_fd, int cmd, void *data) > > +{ > > + int ret; > > + struct sev_issue_cmd arg; > > + > > + arg.cmd = cmd; > > + arg.data = (unsigned long)data; > > + ret = ioctl(sev_fd, SEV_ISSUE_CMD, &arg); > > + TEST_ASSERT(ret == 0, > > + "SEV ioctl %d failed, error: %d, fw_error: %d", > > + cmd, ret, arg.error); > > +} > > + > > +void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data) > > +{ > > + struct kvm_sev_cmd arg = {0}; > > + int ret; > > + > > + arg.id = cmd; > > + arg.sev_fd = sev->fd; > > + arg.data = (__u64)data; > > + > > + ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_OP, &arg); > > + TEST_ASSERT(ret == 0, > > + "SEV KVM ioctl %d failed, rc: %i errno: %i (%s), fw_error: %d", > > + cmd, ret, errno, strerror(errno), arg.error); > > +} > > + > > +/* Local helpers. */ > > + > > +static void sev_register_user_region(struct sev_vm *sev, void *hva, uint64_t size) > > +{ > > + struct kvm_enc_region range = {0}; > > + int ret; > > + > > + pr_debug("%s: hva: %p, size: %lu\n", __func__, hva, size); > > + > > + range.addr = (__u64)hva; > > + range.size = size; > > + > > + ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_REG_REGION, &range); > > + TEST_ASSERT(ret == 0, "failed to register user range, errno: %i\n", errno); > > +} > > + > > +static void sev_encrypt_phy_range(struct sev_vm *sev, vm_paddr_t gpa, uint64_t size) > > +{ > > + struct kvm_sev_launch_update_data ksev_update_data = {0}; > > + > > + pr_debug("%s: addr: 0x%lx, size: %lu\n", __func__, gpa, size); > > + > > + ksev_update_data.uaddr = (__u64)addr_gpa2hva(sev->vm, gpa); > > + ksev_update_data.len = size; > > + > > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_UPDATE_DATA, &ksev_update_data); > > +} > > + > > +static void sev_encrypt(struct sev_vm *sev) > > +{ > > + const struct sparsebit *enc_phy_pages; > > + struct kvm_vm *vm = sev->vm; > > + sparsebit_idx_t pg = 0; > > + vm_paddr_t gpa_start; > > + uint64_t memory_size; > > + int ctr; > > + struct userspace_mem_region *region; > > + > > + hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) { > > + enc_phy_pages = vm_get_encrypted_phy_pages( > > Please don't wrap after the opening paranthesis unless it's really, really necessary. > More for future reference since I think vm_get_encrypted_phy_pages() should be open > coded here. E.g. in this case, the "enc_phy_" prefix doesn't add much value, and > dropping that makes the code easier to read overall. > > pages = vm_get_encrypted_phy_pages(sev->vm, region->region.slot, > &gpa_start, &memory_size); > > > + sev->vm, region->region.slot, &gpa_start, &memory_size); > > + TEST_ASSERT(enc_phy_pages, > > + "Unable to retrieve encrypted pages bitmap"); > > + while (pg < (memory_size / vm->page_size)) { > > + sparsebit_idx_t pg_cnt; > > s/pg_cnt/nr_pages > > > + > > + if (sparsebit_is_clear(enc_phy_pages, pg)) { > > + pg = sparsebit_next_set(enc_phy_pages, pg); > > + if (!pg) > > + break; > > + } > > + > > + pg_cnt = sparsebit_next_clear(enc_phy_pages, pg) - pg; > > + if (pg_cnt <= 0) > > + pg_cnt = 1; > > + > > + sev_encrypt_phy_range(sev, > > + gpa_start + pg * vm->page_size, > > + pg_cnt * vm->page_size); > > + pg += pg_cnt; > > + } > > + } > > + > > + sev->vm->memcrypt.encrypted = true; > > +} > > + > > +/* SEV VM implementation. */ > > + > > +static struct sev_vm *sev_vm_alloc(struct kvm_vm *vm) > > +{ > > + struct sev_user_data_status sev_status; > > + uint32_t eax, ebx, ecx, edx; > > + struct sev_vm *sev; > > + int sev_fd; > > + > > + sev_fd = open(SEV_DEV_PATH, O_RDWR); > > + if (sev_fd < 0) { > > + pr_info("Failed to open SEV device, path: %s, error: %d, skipping test.\n", > > + SEV_DEV_PATH, sev_fd); > > + return NULL; > > Printing "skipping test" is wrong as there's no guarantee the caller is going to > skip the test. E.g. the sole user in this series asserts, i.e. fails the test. > > I also think that waiting until VM allocation to perform these sanity checks is > flawed. Rather do these checks every time, add helpers to query SEV and SEV-ES > support, and then use TEST_REQUIRE() to actually skip tests that require support, > e.g. > > TEST_REQUIRE(kvm_is_sev_supported()); > > or > > TEST_REQUIRE(kvm_is_sev_es_supported()); > > Then this helper can simply assert that opening SEV_DEV_PATH succeeds. > > > + } > > + > > + sev_ioctl(sev_fd, SEV_PLATFORM_STATUS, &sev_status); > > + > > + if (!(sev_status.api_major > SEV_FW_REQ_VER_MAJOR || > > + (sev_status.api_major == SEV_FW_REQ_VER_MAJOR && > > + sev_status.api_minor >= SEV_FW_REQ_VER_MINOR))) { > > + pr_info("SEV FW version too old. Have API %d.%d (build: %d), need %d.%d, skipping test.\n", > > + sev_status.api_major, sev_status.api_minor, sev_status.build, > > + SEV_FW_REQ_VER_MAJOR, SEV_FW_REQ_VER_MINOR); > > + return NULL; > > + } > > + > > + sev = calloc(1, sizeof(*sev)); > > TEST_ASSERT(sev, ...) > > > + sev->fd = sev_fd; > > + sev->vm = vm; > > + > > + /* Get encryption bit via CPUID. */ > > + cpuid(CPUID_MEM_ENC_LEAF, &eax, &ebx, &ecx, &edx); > > + sev->enc_bit = ebx & CPUID_EBX_CBIT_MASK; > > Oh hey, another series of mine[*] that you can leverage :-) > > [*] https://lore.kernel.org/all/20221006005125.680782-1-seanjc@google.com > > > + > > + return sev; > > +} > > + > > +void sev_vm_free(struct sev_vm *sev) > > +{ > > + kvm_vm_free(sev->vm); > > + close(sev->fd); > > + free(sev); > > +} > > + > > +struct sev_vm *sev_vm_create(uint32_t policy, uint64_t npages) > > The migration test already defines sev_vm_create(). That conflict needs to be > resolved. > > > +{ > > + struct sev_vm *sev; > > + struct kvm_vm *vm; > > + > > + /* Need to handle memslots after init, and after setting memcrypt. */ > > + vm = vm_create_barebones(); > > Do not use vm_create_barebones(). That API is only to be used for tests that do > not intend to run vCPUs. > > > > > + sev = sev_vm_alloc(vm); > > + if (!sev) > > + return NULL; > > + sev->sev_policy = policy; > > + > > + kvm_sev_ioctl(sev, KVM_SEV_INIT, NULL); > > + > > + vm->vpages_mapped = sparsebit_alloc(); > > This is unnecessary and leaks memory, vm->vpages_mapped is allocated by > ____vm_create(). > > > + vm_set_memory_encryption(vm, true, true, sev->enc_bit); > > + pr_info("SEV cbit: %d\n", sev->enc_bit); > > + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, npages, 0); > > + sev_register_user_region(sev, addr_gpa2hva(vm, 0), > > + npages * vm->page_size); > > Burying sev_register_user_region() in here is not going to be maintainble. I > think the best away to handle this is to add an arch hook in vm_userspace_mem_region_add() > and automatically register regions when they're created. > > And with that, I believe sev_vm_create() can go away entirely and the SEV encryption > stuff can be handled via a new vm_guest_mode. ____vm_create() already has a gross > __x86_64__ hook that we can tweak, e.g. > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index 54b8d8825f5d..2d6cbca2c01a 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -238,9 +238,10 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode, uint64_t nr_pages) > case VM_MODE_P36V47_16K: > vm->pgtable_levels = 3; > break; > + case VM_MODE_PXXV48_4K_SEV: > case VM_MODE_PXXV48_4K: > #ifdef __x86_64__ > - kvm_get_cpu_address_width(&vm->pa_bits, &vm->va_bits); > + kvm_init_vm_address_properties(vm); > /* > * Ignore KVM support for 5-level paging (vm->va_bits == 57), > * it doesn't take effect unless a CR4.LA57 is set, which it > > Then kvm_init_vm_address_properties() can pivot on vm->mode to deal with SEV > specific stuff. > > > + > > + pr_info("SEV guest created, policy: 0x%x, size: %lu KB\n", > > + sev->sev_policy, npages * vm->page_size / 1024); > > + > > + return sev; > > +} > > + > > +void sev_vm_launch(struct sev_vm *sev) > > +{ > > + struct kvm_sev_launch_start ksev_launch_start = {0}; > > + struct kvm_sev_guest_status ksev_status; > > + > > + ksev_launch_start.policy = sev->sev_policy; > > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_START, &ksev_launch_start); > > + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); > > + TEST_ASSERT(ksev_status.policy == sev->sev_policy, "Incorrect guest policy."); > > + TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE, > > + "Unexpected guest state: %d", ksev_status.state); > > + > > + ucall_init(sev->vm, 0); > > + > > + sev_encrypt(sev); > > +} > > + > > +void sev_vm_launch_measure(struct sev_vm *sev, uint8_t *measurement) > > +{ > > + struct kvm_sev_launch_measure ksev_launch_measure; > > + struct kvm_sev_guest_status ksev_guest_status; > > + > > + ksev_launch_measure.len = 256; > > + ksev_launch_measure.uaddr = (__u64)measurement; > > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_MEASURE, &ksev_launch_measure); > > + > > + /* Measurement causes a state transition, check that. */ > > + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_guest_status); > > + TEST_ASSERT(ksev_guest_status.state == SEV_GSTATE_LSECRET, > > + "Unexpected guest state: %d", ksev_guest_status.state); > > +} > > + > > +void sev_vm_launch_finish(struct sev_vm *sev) > > +{ > > + struct kvm_sev_guest_status ksev_status; > > + > > + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); > > + TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE || > > + ksev_status.state == SEV_GSTATE_LSECRET, > > + "Unexpected guest state: %d", ksev_status.state); > > + > > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_FINISH, NULL); > > + > > + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); > > + TEST_ASSERT(ksev_status.state == SEV_GSTATE_RUNNING, > > + "Unexpected guest state: %d", ksev_status.state); > > +} > > Rather than force each test to invoke these via something like setup_test_common(), > add the same kvm_arch_vm_post_create() hook that Vishal is likely going to add, > and then automatically do all of the launch+measure+finish stuff for non-barebones > VMs. That will let SEV/SEV-ES tests use __vm_create_with_vcpus() and > __vm_create(). > > And it'd be a little gross, but I think it'd be wortwhile to add another layer > to the "one_vcpu" helpers to make things even more convenient, e.g. > > struct kvm_vm *____vm_create_with_one_vcpu(enum vm_guest_mode mode, > struct kvm_vcpu **vcpu, > uint64_t extra_mem_pages, > void *guest_code) > { > struct kvm_vcpu *vcpus[1]; > struct kvm_vm *vm; > > vm = __vm_create_with_vcpus(mode, 1, extra_mem_pages, guest_code, vcpus); > > *vcpu = vcpus[0]; > return vm; > } > > static inline struct kvm_vm *__vm_create_with_one_vcpu(struct kvm_vcpu **vcpu, > uint64_t extra_mem_pages, > void *guest_code) > { > return ____vm_create_with_one_vcpu(VM_MODE_DEFAULT, vcpu, > extra_mem_pages, guest_code); > } > > static inline struct kvm_vm *____vm_create_with_one_vcpu(enum vm_guest_mode mode, > struct kvm_vcpu **vcpu, > uint64_t extra_mem_pages, > void *guest_code) > ____vm_create_with_one_vcpu > > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index dafe4471a6c7..593dfadb662e 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -298,9 +298,8 @@ struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint32_t nr_runnable_vcpus, > > kvm_vm_elf_load(vm, program_invocation_name); > > -#ifdef __x86_64__ > - vm_create_irqchip(vm); > -#endif > + kvm_arch_vm_post_create(vm); > + > return vm; > } > > > [*] https://lore.kernel.org/all/YzsC4ibDqGh5qaP9@google.com This refactor sounds good, working on this with a few changes. Instead of kvm_init_vm_address_properties() as you suggested I've added this: @@ -272,6 +275,8 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode, uint64_t nr_pages) vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits); #endif + kvm_init_vm_arch(vm); + vm_open(vm); /* Limit to VA-bit canonical virtual addresses. */ And I need to put kvm_arch_vm_post_create() after the vCPUs are created because the ordering we need is: KVM_SEV_INIT -> Create vCPUS -> KVM_SEV_LAUNCH_FINISH.
On Mon, Oct 17, 2022, Peter Gonda wrote: > On Thu, Oct 6, 2022 at 12:25 PM Sean Christopherson <seanjc@google.com> wrote: > > And with that, I believe sev_vm_create() can go away entirely and the SEV encryption > > stuff can be handled via a new vm_guest_mode. ____vm_create() already has a gross > > __x86_64__ hook that we can tweak, e.g. > > > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > > index 54b8d8825f5d..2d6cbca2c01a 100644 > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > > @@ -238,9 +238,10 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode, uint64_t nr_pages) > > case VM_MODE_P36V47_16K: > > vm->pgtable_levels = 3; > > break; > > + case VM_MODE_PXXV48_4K_SEV: > > case VM_MODE_PXXV48_4K: > > #ifdef __x86_64__ > > - kvm_get_cpu_address_width(&vm->pa_bits, &vm->va_bits); > > + kvm_init_vm_address_properties(vm); > > /* > > * Ignore KVM support for 5-level paging (vm->va_bits == 57), > > * it doesn't take effect unless a CR4.LA57 is set, which it > > > > Then kvm_init_vm_address_properties() can pivot on vm->mode to deal with SEV > > specific stuff. ... > This refactor sounds good, working on this with a few changes. > > Instead of kvm_init_vm_address_properties() as you suggested I've added this: > > @@ -272,6 +275,8 @@ struct kvm_vm *____vm_create(enum vm_guest_mode > mode, uint64_t nr_pages) > vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits); > #endif > > + kvm_init_vm_arch(vm); Why? I'm not necessarily opposed to adding kvm_init_vm_arch(), but since x86 "needs" a dedicated hook to unpack the mode, why not piggyback that one? > + > vm_open(vm); > > /* Limit to VA-bit canonical virtual addresses. */ > > And I need to put kvm_arch_vm_post_create() after the vCPUs are > created because the ordering we need is: KVM_SEV_INIT -> Create vCPUS > -> KVM_SEV_LAUNCH_FINISH. Hrm, that's annoying. Please don't use kvm_arch_vm_post_create() as the name, that's a better fit for what Vishal is doing since the "vm_post_create()" implies that it's called for "all" VM creation paths, where "all" means "everything except barebones VMs". E.g. in Vishal's series, kvm_arch_vm_post_create() can be used to drop the vm_create_irqchip() call in common code. In your case, IIUC the hook will be invoked from __vm_create_with_vcpus(). I'm a little hesitant to have an arch hook for this case since it can't be all-or-nothing (again, ignoring barebones VMs). If a "finalize" arch hook is added, then arguably tests that do __vm_create() and manually add vCPUs should call the arch hook, i.e. we'd be adding maintenance burden to tests that in all likelihood don't care about SEV and never will. It's somewhat unfortunate, but dedicated vm_sev_create_with_one_vcpu() and and vm_sev_create_with_vcpus() wrappers is probably the least awful solution.
On Mon, Oct 17, 2022 at 12:04 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > On Thu, Oct 6, 2022 at 12:25 PM Sean Christopherson <seanjc@google.com> wrote: > > > And with that, I believe sev_vm_create() can go away entirely and the SEV encryption > > > stuff can be handled via a new vm_guest_mode. ____vm_create() already has a gross > > > __x86_64__ hook that we can tweak, e.g. > > > > > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > > > index 54b8d8825f5d..2d6cbca2c01a 100644 > > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > > > @@ -238,9 +238,10 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode, uint64_t nr_pages) > > > case VM_MODE_P36V47_16K: > > > vm->pgtable_levels = 3; > > > break; > > > + case VM_MODE_PXXV48_4K_SEV: > > > case VM_MODE_PXXV48_4K: > > > #ifdef __x86_64__ > > > - kvm_get_cpu_address_width(&vm->pa_bits, &vm->va_bits); > > > + kvm_init_vm_address_properties(vm); > > > /* > > > * Ignore KVM support for 5-level paging (vm->va_bits == 57), > > > * it doesn't take effect unless a CR4.LA57 is set, which it > > > > > > Then kvm_init_vm_address_properties() can pivot on vm->mode to deal with SEV > > > specific stuff. > > ... > > > This refactor sounds good, working on this with a few changes. > > > > Instead of kvm_init_vm_address_properties() as you suggested I've added this: > > > > @@ -272,6 +275,8 @@ struct kvm_vm *____vm_create(enum vm_guest_mode > > mode, uint64_t nr_pages) > > vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits); > > #endif > > > > + kvm_init_vm_arch(vm); > > Why? I'm not necessarily opposed to adding kvm_init_vm_arch(), but since x86 > "needs" a dedicated hook to unpack the mode, why not piggyback that one? > Well I since I need to do more than just kvm_init_vm_address_properties() I thought the more generic name would be better. We need to allocate kvm_vm_arch, find the c-bit, and call KVM_SEV_INIT. I can put it back in that switch case if thats better, thoughts? > > + > > vm_open(vm); > > > > /* Limit to VA-bit canonical virtual addresses. */ > > > > And I need to put kvm_arch_vm_post_create() after the vCPUs are > > created because the ordering we need is: KVM_SEV_INIT -> Create vCPUS > > -> KVM_SEV_LAUNCH_FINISH. > > Hrm, that's annoying. Please don't use kvm_arch_vm_post_create() as the name, > that's a better fit for what Vishal is doing since the "vm_post_create()" implies > that it's called for "all" VM creation paths, where "all" means "everything > except barebones VMs". E.g. in Vishal's series, kvm_arch_vm_post_create() can > be used to drop the vm_create_irqchip() call in common code. In your case, IIUC > the hook will be invoked from __vm_create_with_vcpus(). > > I'm a little hesitant to have an arch hook for this case since it can't be > all-or-nothing (again, ignoring barebones VMs). If a "finalize" arch hook is added, > then arguably tests that do __vm_create() and manually add vCPUs should call the > arch hook, i.e. we'd be adding maintenance burden to tests that in all likelihood > don't care about SEV and never will. > > It's somewhat unfortunate, but dedicated vm_sev_create_with_one_vcpu() and > and vm_sev_create_with_vcpus() wrappers is probably the least awful solution. Make sense. I think we can go back to your suggestion of kvm_init_vm_address_properties() above since we can now do all the KVM_SEV_* stuff. I think this means we don't need to add VM_MODE_PXXV48_4K_SEV since we can set up the c-bit from inside of vm_sev_create_*(), thoughts?
On Mon, Oct 17, 2022, Peter Gonda wrote: > On Mon, Oct 17, 2022 at 12:04 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > > This refactor sounds good, working on this with a few changes. > > > > > > Instead of kvm_init_vm_address_properties() as you suggested I've added this: > > > > > > @@ -272,6 +275,8 @@ struct kvm_vm *____vm_create(enum vm_guest_mode > > > mode, uint64_t nr_pages) > > > vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits); > > > #endif > > > > > > + kvm_init_vm_arch(vm); > > > > Why? I'm not necessarily opposed to adding kvm_init_vm_arch(), but since x86 > > "needs" a dedicated hook to unpack the mode, why not piggyback that one? > > > > Well I since I need to do more than just > kvm_init_vm_address_properties() I thought the more generic name would > be better. We need to allocate kvm_vm_arch, find the c-bit, and call > KVM_SEV_INIT. I can put it back in that switch case if thats better, > thoughts? > > > > + > > > vm_open(vm); > > > > > > /* Limit to VA-bit canonical virtual addresses. */ > > > > > > And I need to put kvm_arch_vm_post_create() after the vCPUs are > > > created because the ordering we need is: KVM_SEV_INIT -> Create vCPUS > > > -> KVM_SEV_LAUNCH_FINISH. > > > > Hrm, that's annoying. Please don't use kvm_arch_vm_post_create() as the name, > > that's a better fit for what Vishal is doing since the "vm_post_create()" implies > > that it's called for "all" VM creation paths, where "all" means "everything > > except barebones VMs". E.g. in Vishal's series, kvm_arch_vm_post_create() can > > be used to drop the vm_create_irqchip() call in common code. In your case, IIUC > > the hook will be invoked from __vm_create_with_vcpus(). > > > > I'm a little hesitant to have an arch hook for this case since it can't be > > all-or-nothing (again, ignoring barebones VMs). If a "finalize" arch hook is added, > > then arguably tests that do __vm_create() and manually add vCPUs should call the > > arch hook, i.e. we'd be adding maintenance burden to tests that in all likelihood > > don't care about SEV and never will. > > > > It's somewhat unfortunate, but dedicated vm_sev_create_with_one_vcpu() and > > and vm_sev_create_with_vcpus() wrappers is probably the least awful solution. > > Make sense. I think we can go back to your suggestion of > kvm_init_vm_address_properties() above since we can now do all the > KVM_SEV_* stuff. I think this means we don't need to add > VM_MODE_PXXV48_4K_SEV since we can set up the c-bit from inside of > vm_sev_create_*(), thoughts? Configuring the C-bit inside vm_sev_create_*() won't work (at least not well). The C-bit needs to be known before kvm_vm_elf_load(), i.e. can't be handled after __vm_create(), and needs to be tracked inside the VM, i.e. can't be handled before __vm_create(). The proposed kvm_init_vm_address_properties() seems like the best fit since the C-bit (and TDX's S-bit) is stolen from GPA space, i.e. directly affects the other values computed in that path. As for the kvm_vm_arch allocation ugliness, when we talked off-list I didn't consider the need to allocate in kvm_init_vm_address_properties(). That's quite gross, especially since the pointer will be larger than the thing being allocated. With that in mind, adding .../include/<arch>/kvm_util.h so that "struct kvm_vm_arch" can be defined and referenced directly doesn't seem so bad. Having to stub in the struct for the other architectures is annoying, but not the end of the world.
On Mon, Oct 17, 2022 at 2:34 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > On Mon, Oct 17, 2022 at 12:04 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > > > This refactor sounds good, working on this with a few changes. > > > > > > > > Instead of kvm_init_vm_address_properties() as you suggested I've added this: > > > > > > > > @@ -272,6 +275,8 @@ struct kvm_vm *____vm_create(enum vm_guest_mode > > > > mode, uint64_t nr_pages) > > > > vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits); > > > > #endif > > > > > > > > + kvm_init_vm_arch(vm); > > > > > > Why? I'm not necessarily opposed to adding kvm_init_vm_arch(), but since x86 > > > "needs" a dedicated hook to unpack the mode, why not piggyback that one? > > > > > > > Well I since I need to do more than just > > kvm_init_vm_address_properties() I thought the more generic name would > > be better. We need to allocate kvm_vm_arch, find the c-bit, and call > > KVM_SEV_INIT. I can put it back in that switch case if thats better, > > thoughts? > > > > > > + > > > > vm_open(vm); > > > > > > > > /* Limit to VA-bit canonical virtual addresses. */ > > > > > > > > And I need to put kvm_arch_vm_post_create() after the vCPUs are > > > > created because the ordering we need is: KVM_SEV_INIT -> Create vCPUS > > > > -> KVM_SEV_LAUNCH_FINISH. > > > > > > Hrm, that's annoying. Please don't use kvm_arch_vm_post_create() as the name, > > > that's a better fit for what Vishal is doing since the "vm_post_create()" implies > > > that it's called for "all" VM creation paths, where "all" means "everything > > > except barebones VMs". E.g. in Vishal's series, kvm_arch_vm_post_create() can > > > be used to drop the vm_create_irqchip() call in common code. In your case, IIUC > > > the hook will be invoked from __vm_create_with_vcpus(). > > > > > > I'm a little hesitant to have an arch hook for this case since it can't be > > > all-or-nothing (again, ignoring barebones VMs). If a "finalize" arch hook is added, > > > then arguably tests that do __vm_create() and manually add vCPUs should call the > > > arch hook, i.e. we'd be adding maintenance burden to tests that in all likelihood > > > don't care about SEV and never will. > > > > > > It's somewhat unfortunate, but dedicated vm_sev_create_with_one_vcpu() and > > > and vm_sev_create_with_vcpus() wrappers is probably the least awful solution. > > > > Make sense. I think we can go back to your suggestion of > > kvm_init_vm_address_properties() above since we can now do all the > > KVM_SEV_* stuff. I think this means we don't need to add > > VM_MODE_PXXV48_4K_SEV since we can set up the c-bit from inside of > > vm_sev_create_*(), thoughts? > > Configuring the C-bit inside vm_sev_create_*() won't work (at least not well). > The C-bit needs to be known before kvm_vm_elf_load(), i.e. can't be handled after > __vm_create(), and needs to be tracked inside the VM, i.e. can't be handled before > __vm_create(). > > The proposed kvm_init_vm_address_properties() seems like the best fit since the > C-bit (and TDX's S-bit) is stolen from GPA space, i.e. directly affects the other > values computed in that path. > > As for the kvm_vm_arch allocation ugliness, when we talked off-list I didn't > consider the need to allocate in kvm_init_vm_address_properties(). That's quite > gross, especially since the pointer will be larger than the thing being allocated. > > With that in mind, adding .../include/<arch>/kvm_util.h so that "struct kvm_vm_arch" > can be defined and referenced directly doesn't seem so bad. Having to stub in the > struct for the other architectures is annoying, but not the end of the world. I'll make "struct kvm_vm_arch" a non pointer member, so adding /include/<arch>/kvm_util.h files. But I think we do not need VM_MODE_PXXV48_4K_SEV, see: struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t policy, void *guest_code, struct kvm_vcpu **cpu) { enum vm_guest_mode mode = VM_MODE_PXXV48_4K; uint64_t nr_pages = vm_nr_pages_required(mode, 1, 0); struct kvm_vm *vm; uint8_t measurement[512]; int i; vm = ____vm_create(mode, nr_pages); kvm_sev_ioctl(vm, KVM_SEV_INIT, NULL); configure_sev_pte_masks(vm); *cpu = vm_vcpu_add(vm, 0, guest_code); kvm_vm_elf_load(vm, program_invocation_name); sev_vm_launch(vm, policy); /* Dump the initial measurement. A test to actually verify it would be nice. */ sev_vm_launch_measure(vm, measurement); pr_info("guest measurement: "); for (i = 0; i < 32; ++i) pr_info("%02x", measurement[i]); pr_info("\n"); sev_vm_launch_finish(vm); return vm; }
On Tue, Oct 18, 2022, Peter Gonda wrote: > On Mon, Oct 17, 2022 at 2:34 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > > I think this means we don't need to add VM_MODE_PXXV48_4K_SEV since we > > > can set up the c-bit from inside of vm_sev_create_*(), thoughts? > > > > Configuring the C-bit inside vm_sev_create_*() won't work (at least not well). > > The C-bit needs to be known before kvm_vm_elf_load(), i.e. can't be handled after > > __vm_create(), and needs to be tracked inside the VM, i.e. can't be handled before > > __vm_create(). > > > > The proposed kvm_init_vm_address_properties() seems like the best fit since the > > C-bit (and TDX's S-bit) is stolen from GPA space, i.e. directly affects the other > > values computed in that path. > > > > As for the kvm_vm_arch allocation ugliness, when we talked off-list I didn't > > consider the need to allocate in kvm_init_vm_address_properties(). That's quite > > gross, especially since the pointer will be larger than the thing being allocated. > > > > With that in mind, adding .../include/<arch>/kvm_util.h so that "struct kvm_vm_arch" > > can be defined and referenced directly doesn't seem so bad. Having to stub in the > > struct for the other architectures is annoying, but not the end of the world. > > I'll make "struct kvm_vm_arch" a non pointer member, so adding > /include/<arch>/kvm_util.h files. > > But I think we do not need VM_MODE_PXXV48_4K_SEV, see: I really don't want to open code __vm_create() with a slight tweak. E.g. the below code will be broken by Ricardo's series to add memslot0 is moved out of ____vm_create()[1], and kinda sorta be broken again by Vishal's series to add an arch hook to __vm_create()[2]. AFAICT, there is no requirement that KVM_SEV_INIT be called before computing the C-Bit, the only requirement is that KVM_SEV_INIT is called before adding vCPUs. [1] https://lore.kernel.org/all/20221017195834.2295901-8-ricarkol@google.com [2] https://lore.kernel.org/all/YzsC4ibDqGh5qaP9@google.com > struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t policy, void *guest_code, > struct kvm_vcpu **cpu) > { > enum vm_guest_mode mode = VM_MODE_PXXV48_4K; > uint64_t nr_pages = vm_nr_pages_required(mode, 1, 0); > struct kvm_vm *vm; > uint8_t measurement[512]; > int i; > > vm = ____vm_create(mode, nr_pages); > > kvm_sev_ioctl(vm, KVM_SEV_INIT, NULL); > > configure_sev_pte_masks(vm); > > *cpu = vm_vcpu_add(vm, 0, guest_code); > kvm_vm_elf_load(vm, program_invocation_name); > > sev_vm_launch(vm, policy); > > /* Dump the initial measurement. A test to actually verify it > would be nice. */ > sev_vm_launch_measure(vm, measurement); > pr_info("guest measurement: "); > for (i = 0; i < 32; ++i) > pr_info("%02x", measurement[i]); > pr_info("\n"); > > sev_vm_launch_finish(vm); > > return vm; > }
On Wed, Oct 19, 2022 at 10:34 AM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Oct 18, 2022, Peter Gonda wrote: > > On Mon, Oct 17, 2022 at 2:34 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > > > I think this means we don't need to add VM_MODE_PXXV48_4K_SEV since we > > > > can set up the c-bit from inside of vm_sev_create_*(), thoughts? > > > > > > Configuring the C-bit inside vm_sev_create_*() won't work (at least not well). > > > The C-bit needs to be known before kvm_vm_elf_load(), i.e. can't be handled after > > > __vm_create(), and needs to be tracked inside the VM, i.e. can't be handled before > > > __vm_create(). > > > > > > The proposed kvm_init_vm_address_properties() seems like the best fit since the > > > C-bit (and TDX's S-bit) is stolen from GPA space, i.e. directly affects the other > > > values computed in that path. > > > > > > As for the kvm_vm_arch allocation ugliness, when we talked off-list I didn't > > > consider the need to allocate in kvm_init_vm_address_properties(). That's quite > > > gross, especially since the pointer will be larger than the thing being allocated. > > > > > > With that in mind, adding .../include/<arch>/kvm_util.h so that "struct kvm_vm_arch" > > > can be defined and referenced directly doesn't seem so bad. Having to stub in the > > > struct for the other architectures is annoying, but not the end of the world. > > > > I'll make "struct kvm_vm_arch" a non pointer member, so adding > > /include/<arch>/kvm_util.h files. > > > > But I think we do not need VM_MODE_PXXV48_4K_SEV, see: > > I really don't want to open code __vm_create() with a slight tweak. E.g. the > below code will be broken by Ricardo's series to add memslot0 is moved out of > ____vm_create()[1], and kinda sorta be broken again by Vishal's series to add an > arch hook to __vm_create()[2]. > > AFAICT, there is no requirement that KVM_SEV_INIT be called before computing the > C-Bit, the only requirement is that KVM_SEV_INIT is called before adding vCPUs. > > [1] https://lore.kernel.org/all/20221017195834.2295901-8-ricarkol@google.com > [2] https://lore.kernel.org/all/YzsC4ibDqGh5qaP9@google.com Oh I misunderstood your suggestion above. I should make KVM_SEV_INIT happen from kvm_arch_vm_post_create(). Add VM_MODE_PXXV48_4K_SEV for c-bit setting inside of kvm_init_vm_address_properties(). Inside of vm_sev_create_with_one_vcpu() I use __vm_create_with_vcpus(), then call KVM_SEV_LAUNCH_FINISH. Is that correct? > > > struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t policy, void *guest_code, > > struct kvm_vcpu **cpu) > > { > > enum vm_guest_mode mode = VM_MODE_PXXV48_4K; > > uint64_t nr_pages = vm_nr_pages_required(mode, 1, 0); > > struct kvm_vm *vm; > > uint8_t measurement[512]; > > int i; > > > > vm = ____vm_create(mode, nr_pages); > > > > kvm_sev_ioctl(vm, KVM_SEV_INIT, NULL); > > > > configure_sev_pte_masks(vm); > > > > *cpu = vm_vcpu_add(vm, 0, guest_code); > > kvm_vm_elf_load(vm, program_invocation_name); > > > > sev_vm_launch(vm, policy); > > > > /* Dump the initial measurement. A test to actually verify it > > would be nice. */ > > sev_vm_launch_measure(vm, measurement); > > pr_info("guest measurement: "); > > for (i = 0; i < 32; ++i) > > pr_info("%02x", measurement[i]); > > pr_info("\n"); > > > > sev_vm_launch_finish(vm); > > > > return vm; > > }
On Thu, Oct 27, 2022, Peter Gonda wrote: > On Wed, Oct 19, 2022 at 10:34 AM Sean Christopherson <seanjc@google.com> wrote: > > > > On Tue, Oct 18, 2022, Peter Gonda wrote: > > > On Mon, Oct 17, 2022 at 2:34 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > > > > I think this means we don't need to add VM_MODE_PXXV48_4K_SEV since we > > > > > can set up the c-bit from inside of vm_sev_create_*(), thoughts? > > > > > > > > Configuring the C-bit inside vm_sev_create_*() won't work (at least not well). > > > > The C-bit needs to be known before kvm_vm_elf_load(), i.e. can't be handled after > > > > __vm_create(), and needs to be tracked inside the VM, i.e. can't be handled before > > > > __vm_create(). > > > > > > > > The proposed kvm_init_vm_address_properties() seems like the best fit since the > > > > C-bit (and TDX's S-bit) is stolen from GPA space, i.e. directly affects the other > > > > values computed in that path. > > > > > > > > As for the kvm_vm_arch allocation ugliness, when we talked off-list I didn't > > > > consider the need to allocate in kvm_init_vm_address_properties(). That's quite > > > > gross, especially since the pointer will be larger than the thing being allocated. > > > > > > > > With that in mind, adding .../include/<arch>/kvm_util.h so that "struct kvm_vm_arch" > > > > can be defined and referenced directly doesn't seem so bad. Having to stub in the > > > > struct for the other architectures is annoying, but not the end of the world. > > > > > > I'll make "struct kvm_vm_arch" a non pointer member, so adding > > > /include/<arch>/kvm_util.h files. > > > > > > But I think we do not need VM_MODE_PXXV48_4K_SEV, see: > > > > I really don't want to open code __vm_create() with a slight tweak. E.g. the > > below code will be broken by Ricardo's series to add memslot0 is moved out of > > ____vm_create()[1], and kinda sorta be broken again by Vishal's series to add an > > arch hook to __vm_create()[2]. > > > > AFAICT, there is no requirement that KVM_SEV_INIT be called before computing the > > C-Bit, the only requirement is that KVM_SEV_INIT is called before adding vCPUs. > > > > [1] https://lore.kernel.org/all/20221017195834.2295901-8-ricarkol@google.com > > [2] https://lore.kernel.org/all/YzsC4ibDqGh5qaP9@google.com > > Oh I misunderstood your suggestion above. > > I should make KVM_SEV_INIT happen from kvm_arch_vm_post_create(). Add > VM_MODE_PXXV48_4K_SEV for c-bit setting inside of > kvm_init_vm_address_properties(). > > Inside of vm_sev_create_with_one_vcpu() I use > __vm_create_with_vcpus(), then call KVM_SEV_LAUNCH_FINISH. > > Is that correct? Yep, I'm pretty sure that was what I was thinking.
On Thu, Oct 27, 2022 at 11:59 AM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Oct 27, 2022, Peter Gonda wrote: > > On Wed, Oct 19, 2022 at 10:34 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Tue, Oct 18, 2022, Peter Gonda wrote: > > > > On Mon, Oct 17, 2022 at 2:34 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > > > > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > > > > > I think this means we don't need to add VM_MODE_PXXV48_4K_SEV since we > > > > > > can set up the c-bit from inside of vm_sev_create_*(), thoughts? > > > > > > > > > > Configuring the C-bit inside vm_sev_create_*() won't work (at least not well). > > > > > The C-bit needs to be known before kvm_vm_elf_load(), i.e. can't be handled after > > > > > __vm_create(), and needs to be tracked inside the VM, i.e. can't be handled before > > > > > __vm_create(). > > > > > > > > > > The proposed kvm_init_vm_address_properties() seems like the best fit since the > > > > > C-bit (and TDX's S-bit) is stolen from GPA space, i.e. directly affects the other > > > > > values computed in that path. > > > > > > > > > > As for the kvm_vm_arch allocation ugliness, when we talked off-list I didn't > > > > > consider the need to allocate in kvm_init_vm_address_properties(). That's quite > > > > > gross, especially since the pointer will be larger than the thing being allocated. > > > > > > > > > > With that in mind, adding .../include/<arch>/kvm_util.h so that "struct kvm_vm_arch" > > > > > can be defined and referenced directly doesn't seem so bad. Having to stub in the > > > > > struct for the other architectures is annoying, but not the end of the world. > > > > > > > > I'll make "struct kvm_vm_arch" a non pointer member, so adding > > > > /include/<arch>/kvm_util.h files. > > > > > > > > But I think we do not need VM_MODE_PXXV48_4K_SEV, see: > > > > > > I really don't want to open code __vm_create() with a slight tweak. E.g. the > > > below code will be broken by Ricardo's series to add memslot0 is moved out of > > > ____vm_create()[1], and kinda sorta be broken again by Vishal's series to add an > > > arch hook to __vm_create()[2]. > > > > > > AFAICT, there is no requirement that KVM_SEV_INIT be called before computing the > > > C-Bit, the only requirement is that KVM_SEV_INIT is called before adding vCPUs. > > > > > > [1] https://lore.kernel.org/all/20221017195834.2295901-8-ricarkol@google.com > > > [2] https://lore.kernel.org/all/YzsC4ibDqGh5qaP9@google.com > > > > Oh I misunderstood your suggestion above. > > > > I should make KVM_SEV_INIT happen from kvm_arch_vm_post_create(). Add > > VM_MODE_PXXV48_4K_SEV for c-bit setting inside of > > kvm_init_vm_address_properties(). > > > > Inside of vm_sev_create_with_one_vcpu() I use > > __vm_create_with_vcpus(), then call KVM_SEV_LAUNCH_FINISH. > > > > Is that correct? > > Yep, I'm pretty sure that was what I was thinking. Duh! Should have realized that at first. You said you were going to rebase your selftest series. I'll follow up with a V6 ontop of that rebase.
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 23649c5d42fc..0a70e50f0498 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -56,6 +56,7 @@ LIBKVM_x86_64 += lib/x86_64/processor.c LIBKVM_x86_64 += lib/x86_64/svm.c LIBKVM_x86_64 += lib/x86_64/ucall.c LIBKVM_x86_64 += lib/x86_64/vmx.c +LIBKVM_x86_64 += lib/x86_64/sev.c LIBKVM_aarch64 += lib/aarch64/gic.c LIBKVM_aarch64 += lib/aarch64/gic_v3.c diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 489e8c833e5f..0927e262623d 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -69,6 +69,7 @@ struct userspace_mem_regions { /* Memory encryption policy/configuration. */ struct vm_memcrypt { bool enabled; + bool encrypted; int8_t enc_by_default; bool has_enc_bit; int8_t enc_bit; @@ -831,6 +832,8 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva); static inline vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva) { + TEST_ASSERT(!vm->memcrypt.encrypted, + "Encrypted guests have their page tables encrypted so gva2gpa conversions are not possible."); return addr_arch_gva2gpa(vm, gva); } diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h new file mode 100644 index 000000000000..810d36377e8c --- /dev/null +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h @@ -0,0 +1,47 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Helpers used for SEV guests + * + * Copyright (C) 2021 Advanced Micro Devices + */ +#ifndef SELFTEST_KVM_SEV_H +#define SELFTEST_KVM_SEV_H + +#include <stdint.h> +#include <stdbool.h> +#include "kvm_util.h" + +/* Makefile might set this separately for user-overrides */ +#ifndef SEV_DEV_PATH +#define SEV_DEV_PATH "/dev/sev" +#endif + +#define SEV_FW_REQ_VER_MAJOR 0 +#define SEV_FW_REQ_VER_MINOR 17 + +#define SEV_POLICY_NO_DBG (1UL << 0) +#define SEV_POLICY_ES (1UL << 2) + +enum sev_guest_state { + SEV_GSTATE_UNINIT = 0, + SEV_GSTATE_LUPDATE, + SEV_GSTATE_LSECRET, + SEV_GSTATE_RUNNING, +}; + +struct sev_vm { + struct kvm_vm *vm; + int fd; + int enc_bit; + uint32_t sev_policy; +}; + +void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data); + +struct sev_vm *sev_vm_create(uint32_t policy, uint64_t npages); +void sev_vm_free(struct sev_vm *sev); +void sev_vm_launch(struct sev_vm *sev); +void sev_vm_launch_measure(struct sev_vm *sev, uint8_t *measurement); +void sev_vm_launch_finish(struct sev_vm *sev); + +#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 new file mode 100644 index 000000000000..dd8d708fd01a --- /dev/null +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c @@ -0,0 +1,232 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Helpers used for SEV guests + * + * Copyright (C) 2021 Advanced Micro Devices + */ + +#include <stdint.h> +#include <stdbool.h> +#include "kvm_util.h" +#include "linux/psp-sev.h" +#include "processor.h" +#include "sev.h" + +#define CPUID_MEM_ENC_LEAF 0x8000001f +#define CPUID_EBX_CBIT_MASK 0x3f + +/* Common SEV helpers/accessors. */ + +void sev_ioctl(int sev_fd, int cmd, void *data) +{ + int ret; + struct sev_issue_cmd arg; + + arg.cmd = cmd; + arg.data = (unsigned long)data; + ret = ioctl(sev_fd, SEV_ISSUE_CMD, &arg); + TEST_ASSERT(ret == 0, + "SEV ioctl %d failed, error: %d, fw_error: %d", + cmd, ret, arg.error); +} + +void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data) +{ + struct kvm_sev_cmd arg = {0}; + int ret; + + arg.id = cmd; + arg.sev_fd = sev->fd; + arg.data = (__u64)data; + + ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_OP, &arg); + TEST_ASSERT(ret == 0, + "SEV KVM ioctl %d failed, rc: %i errno: %i (%s), fw_error: %d", + cmd, ret, errno, strerror(errno), arg.error); +} + +/* Local helpers. */ + +static void sev_register_user_region(struct sev_vm *sev, void *hva, uint64_t size) +{ + struct kvm_enc_region range = {0}; + int ret; + + pr_debug("%s: hva: %p, size: %lu\n", __func__, hva, size); + + range.addr = (__u64)hva; + range.size = size; + + ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_REG_REGION, &range); + TEST_ASSERT(ret == 0, "failed to register user range, errno: %i\n", errno); +} + +static void sev_encrypt_phy_range(struct sev_vm *sev, vm_paddr_t gpa, uint64_t size) +{ + struct kvm_sev_launch_update_data ksev_update_data = {0}; + + pr_debug("%s: addr: 0x%lx, size: %lu\n", __func__, gpa, size); + + ksev_update_data.uaddr = (__u64)addr_gpa2hva(sev->vm, gpa); + ksev_update_data.len = size; + + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_UPDATE_DATA, &ksev_update_data); +} + +static void sev_encrypt(struct sev_vm *sev) +{ + const struct sparsebit *enc_phy_pages; + struct kvm_vm *vm = sev->vm; + sparsebit_idx_t pg = 0; + vm_paddr_t gpa_start; + uint64_t memory_size; + int ctr; + struct userspace_mem_region *region; + + hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) { + enc_phy_pages = vm_get_encrypted_phy_pages( + sev->vm, region->region.slot, &gpa_start, &memory_size); + TEST_ASSERT(enc_phy_pages, + "Unable to retrieve encrypted pages bitmap"); + while (pg < (memory_size / vm->page_size)) { + sparsebit_idx_t pg_cnt; + + if (sparsebit_is_clear(enc_phy_pages, pg)) { + pg = sparsebit_next_set(enc_phy_pages, pg); + if (!pg) + break; + } + + pg_cnt = sparsebit_next_clear(enc_phy_pages, pg) - pg; + if (pg_cnt <= 0) + pg_cnt = 1; + + sev_encrypt_phy_range(sev, + gpa_start + pg * vm->page_size, + pg_cnt * vm->page_size); + pg += pg_cnt; + } + } + + sev->vm->memcrypt.encrypted = true; +} + +/* SEV VM implementation. */ + +static struct sev_vm *sev_vm_alloc(struct kvm_vm *vm) +{ + struct sev_user_data_status sev_status; + uint32_t eax, ebx, ecx, edx; + struct sev_vm *sev; + int sev_fd; + + sev_fd = open(SEV_DEV_PATH, O_RDWR); + if (sev_fd < 0) { + pr_info("Failed to open SEV device, path: %s, error: %d, skipping test.\n", + SEV_DEV_PATH, sev_fd); + return NULL; + } + + sev_ioctl(sev_fd, SEV_PLATFORM_STATUS, &sev_status); + + if (!(sev_status.api_major > SEV_FW_REQ_VER_MAJOR || + (sev_status.api_major == SEV_FW_REQ_VER_MAJOR && + sev_status.api_minor >= SEV_FW_REQ_VER_MINOR))) { + pr_info("SEV FW version too old. Have API %d.%d (build: %d), need %d.%d, skipping test.\n", + sev_status.api_major, sev_status.api_minor, sev_status.build, + SEV_FW_REQ_VER_MAJOR, SEV_FW_REQ_VER_MINOR); + return NULL; + } + + sev = calloc(1, sizeof(*sev)); + sev->fd = sev_fd; + sev->vm = vm; + + /* Get encryption bit via CPUID. */ + cpuid(CPUID_MEM_ENC_LEAF, &eax, &ebx, &ecx, &edx); + sev->enc_bit = ebx & CPUID_EBX_CBIT_MASK; + + return sev; +} + +void sev_vm_free(struct sev_vm *sev) +{ + kvm_vm_free(sev->vm); + close(sev->fd); + free(sev); +} + +struct sev_vm *sev_vm_create(uint32_t policy, uint64_t npages) +{ + struct sev_vm *sev; + struct kvm_vm *vm; + + /* Need to handle memslots after init, and after setting memcrypt. */ + vm = vm_create_barebones(); + sev = sev_vm_alloc(vm); + if (!sev) + return NULL; + sev->sev_policy = policy; + + kvm_sev_ioctl(sev, KVM_SEV_INIT, NULL); + + vm->vpages_mapped = sparsebit_alloc(); + vm_set_memory_encryption(vm, true, true, sev->enc_bit); + pr_info("SEV cbit: %d\n", sev->enc_bit); + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, npages, 0); + sev_register_user_region(sev, addr_gpa2hva(vm, 0), + npages * vm->page_size); + + pr_info("SEV guest created, policy: 0x%x, size: %lu KB\n", + sev->sev_policy, npages * vm->page_size / 1024); + + return sev; +} + +void sev_vm_launch(struct sev_vm *sev) +{ + struct kvm_sev_launch_start ksev_launch_start = {0}; + struct kvm_sev_guest_status ksev_status; + + ksev_launch_start.policy = sev->sev_policy; + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_START, &ksev_launch_start); + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); + TEST_ASSERT(ksev_status.policy == sev->sev_policy, "Incorrect guest policy."); + TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE, + "Unexpected guest state: %d", ksev_status.state); + + ucall_init(sev->vm, 0); + + sev_encrypt(sev); +} + +void sev_vm_launch_measure(struct sev_vm *sev, uint8_t *measurement) +{ + struct kvm_sev_launch_measure ksev_launch_measure; + struct kvm_sev_guest_status ksev_guest_status; + + ksev_launch_measure.len = 256; + ksev_launch_measure.uaddr = (__u64)measurement; + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_MEASURE, &ksev_launch_measure); + + /* Measurement causes a state transition, check that. */ + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_guest_status); + TEST_ASSERT(ksev_guest_status.state == SEV_GSTATE_LSECRET, + "Unexpected guest state: %d", ksev_guest_status.state); +} + +void sev_vm_launch_finish(struct sev_vm *sev) +{ + struct kvm_sev_guest_status ksev_status; + + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); + TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE || + ksev_status.state == SEV_GSTATE_LSECRET, + "Unexpected guest state: %d", ksev_status.state); + + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_FINISH, NULL); + + kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status); + TEST_ASSERT(ksev_status.state == SEV_GSTATE_RUNNING, + "Unexpected guest state: %d", ksev_status.state); +}
Add interfaces to allow tests to create/manage SEV guests. The additional state associated with these guests is encapsulated in a new struct sev_vm, which is a light wrapper around struct kvm_vm. These VMs will use vm_set_memory_encryption() and vm_get_encrypted_phy_pages() under the covers to configure and sync up with the core kvm_util library on what should/shouldn't be treated as encrypted memory. Originally-by: Michael Roth <michael.roth@amd.com> Signed-off-by: Peter Gonda <pgonda@google.com> --- tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/include/kvm_util_base.h | 3 + .../selftests/kvm/include/x86_64/sev.h | 47 ++++ tools/testing/selftests/kvm/lib/x86_64/sev.c | 232 ++++++++++++++++++ 4 files changed, 283 insertions(+) create mode 100644 tools/testing/selftests/kvm/include/x86_64/sev.h create mode 100644 tools/testing/selftests/kvm/lib/x86_64/sev.c