| Message ID | 20240223104009.632194-12-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SEV: allow customizing VMSA features | expand |
On Fri, Feb 23, 2024, Paolo Bonzini wrote: > diff --git a/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c > new file mode 100644 > index 000000000000..644fd5757041 > --- /dev/null > +++ b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c > @@ -0,0 +1,146 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +#include <linux/kvm.h> > +#include <linux/psp-sev.h> > +#include <stdio.h> > +#include <sys/ioctl.h> > +#include <stdlib.h> > +#include <errno.h> > +#include <pthread.h> > + > +#include "test_util.h" > +#include "kvm_util.h" > +#include "processor.h" > +#include "svm_util.h" > +#include "kselftest.h" > + > +#define SVM_SEV_FEAT_DEBUG_SWAP 32u > + > +/* > + * Some features may have hidden dependencies, or may only work > + * for certain VM types. Err on the side of safety and don't > + * expect that all supported features can be passed one by one > + * to KVM_SEV_INIT2. > + * > + * (Well, right now there's only one...) > + */ > +#define KNOWN_FEATURES SVM_SEV_FEAT_DEBUG_SWAP > + > +int kvm_fd; > +u64 supported_vmsa_features; > +bool have_sev_es; > + > +static int __sev_ioctl(int vm_fd, int cmd_id, void *data) > +{ > + struct kvm_sev_cmd cmd = { > + .id = cmd_id, > + .data = (uint64_t)data, > + .sev_fd = open_sev_dev_path_or_exit(), > + }; > + int ret; > + > + ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd); > + TEST_ASSERT(ret < 0 || cmd.error == SEV_RET_SUCCESS, > + "%d failed: fw error: %d\n", > + cmd_id, cmd.error); > + > + return ret; If you can hold off on v3 until next week, I'll get the SEV+SEV-ES smoke test series into a branch and thus kvm-x86/next. Then this can take advantage of the library files and functions that are added there. I don't know if it will save much code, but it'll at least provide a better place to land some of the "library" #define and helpers. https://lore.kernel.org/all/20240223004258.3104051-1-seanjc@google.com > +} > + > +static void test_init2(unsigned long vm_type, struct kvm_sev_init *init) > +{ > + struct kvm_vm *vm; > + int ret; > + > + vm = vm_create_barebones_type(vm_type); > + ret = __sev_ioctl(vm->fd, KVM_SEV_INIT2, init); The SEV library provided vm_sev_ioctl() for this. > + TEST_ASSERT(ret == 0, > + "KVM_SEV_INIT2 return code is %d (expected 0), errno: %d", > + ret, errno); TEST > + kvm_vm_free(vm); > +} > + > +static void test_init2_invalid(unsigned long vm_type, struct kvm_sev_init *init) > +{ > + struct kvm_vm *vm; > + int ret; > + > + vm = vm_create_barebones_type(vm_type); > + ret = __sev_ioctl(vm->fd, KVM_SEV_INIT2, init); __vm_sev_ioctl() in the library. > + TEST_ASSERT(ret == -1 && errno == EINVAL, > + "KVM_SEV_INIT2 return code %d, errno: %d (expected EINVAL)", > + ret, errno); TEST_ASSERT() will spit out the errno and it's user-friendly name. I would prefer the assert message to explain why failure was expected. That way readers of the code don't need a comment, and runners of failed tests get more info. Hrm, though that'd require assing in a "const char *msg", which would limit this to constant strings and no formatting. I think that's still a net positive though. TEST_ASSERT(ret == -1 && errno == EINVAL, "KVM_SET_INIT2 should fail, %s.", msg); > + kvm_vm_free(vm); > +} > + > +void test_vm_types(void) > +{ > + test_init2(KVM_X86_SEV_VM, &(struct kvm_sev_init){}); > + > + if (have_sev_es) > + test_init2(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){}); > + else > + test_init2_invalid(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){}); E.g. this could be something like test_init2_invalid(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){}, "SEV-ES unsupported); Though shouldn't vm_create_barebones_type() fail on the unsupported VM type, not KVM_SEV_INIT2? > + > + test_init2_invalid(0, &(struct kvm_sev_init){}); > + if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM)) > + test_init2_invalid(KVM_X86_SW_PROTECTED_VM, &(struct kvm_sev_init){}); > +} > + > +void test_flags(uint32_t vm_type) > +{ > + int i; > + > + for (i = 0; i < 32; i++) > + test_init2_invalid(vm_type, &(struct kvm_sev_init){ > + .flags = 1u << i, BIT() > + }); And I think I'd prefer to have the line run long? test_init2_invalid(vm_type, &(struct kvm_sev_init) { .flags = BIT(i) }); > +} > + > +void test_features(uint32_t vm_type, uint64_t supported_features) > +{ > + int i; > + > + for (i = 0; i < 64; i++) { > + if (!(supported_features & (1u << i))) > + test_init2_invalid(vm_type, &(struct kvm_sev_init){ > + .vmsa_features = 1u << i, > + }); Rather than create &(struct kvm_sev_init) for each path, this? struct kvm_sev_init init = { .vmsa_features = BIT(i); }; if (!(supported_features & BIT(i)) test_init2_invalid(vm_type, &init); else if (KNOWN_FEATURES & (1u << i)) test_init2(vm_type, &init); > + else if (KNOWN_FEATURES & (1u << i)) > + test_init2(vm_type, &(struct kvm_sev_init){ > + .vmsa_features = 1u << i, > + }); > + } > +}
On Fri, Feb 23, 2024 at 6:18 PM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Feb 23, 2024, Paolo Bonzini wrote: > > diff --git a/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c > > new file mode 100644 > > index 000000000000..644fd5757041 > > --- /dev/null > > +++ b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c > > @@ -0,0 +1,146 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +#include <linux/kvm.h> > > +#include <linux/psp-sev.h> > > +#include <stdio.h> > > +#include <sys/ioctl.h> > > +#include <stdlib.h> > > +#include <errno.h> > > +#include <pthread.h> > > + > > +#include "test_util.h" > > +#include "kvm_util.h" > > +#include "processor.h" > > +#include "svm_util.h" > > +#include "kselftest.h" > > + > > +#define SVM_SEV_FEAT_DEBUG_SWAP 32u > > + > > +/* > > + * Some features may have hidden dependencies, or may only work > > + * for certain VM types. Err on the side of safety and don't > > + * expect that all supported features can be passed one by one > > + * to KVM_SEV_INIT2. > > + * > > + * (Well, right now there's only one...) > > + */ > > +#define KNOWN_FEATURES SVM_SEV_FEAT_DEBUG_SWAP > > + > > +int kvm_fd; > > +u64 supported_vmsa_features; > > +bool have_sev_es; > > + > > +static int __sev_ioctl(int vm_fd, int cmd_id, void *data) > > +{ > > + struct kvm_sev_cmd cmd = { > > + .id = cmd_id, > > + .data = (uint64_t)data, > > + .sev_fd = open_sev_dev_path_or_exit(), > > + }; > > + int ret; > > + > > + ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd); > > + TEST_ASSERT(ret < 0 || cmd.error == SEV_RET_SUCCESS, > > + "%d failed: fw error: %d\n", > > + cmd_id, cmd.error); > > + > > + return ret; > > If you can hold off on v3 until next week, I'll get the SEV+SEV-ES smoke test > series into a branch and thus kvm-x86/next. Then this can take advantage of the > library files and functions that are added there. I don't know if it will save > much code, but it'll at least provide a better place to land some of the "library" > #define and helpers. > > https://lore.kernel.org/all/20240223004258.3104051-1-seanjc@google.com I'll post v3 anyway, but hold on actually committing this until I've taken a closer look at kvm-x86/next. > > + TEST_ASSERT(ret == -1 && errno == EINVAL, > > + "KVM_SEV_INIT2 return code %d, errno: %d (expected EINVAL)", > > + ret, errno); > > TEST_ASSERT() will spit out the errno and it's user-friendly name. I would prefer > the assert message to explain why failure was expected. That way readers of the > code don't need a comment, and runners of failed tests get more info. > > Hrm, though that'd require assing in a "const char *msg", which would limit this > to constant strings and no formatting. I think that's still a net positive though. Ok, will do. > TEST_ASSERT(ret == -1 && errno == EINVAL, > "KVM_SET_INIT2 should fail, %s.", msg); > > > + kvm_vm_free(vm); > > +} > > + > > +void test_vm_types(void) > > +{ > > + test_init2(KVM_X86_SEV_VM, &(struct kvm_sev_init){}); > > + > > + if (have_sev_es) > > + test_init2(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){}); > > + else > > + test_init2_invalid(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){}); > > E.g. this could be something like > > test_init2_invalid(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){}, > "SEV-ES unsupported); > > Though shouldn't vm_create_barebones_type() fail on the unsupported VM type, not > KVM_SEV_INIT2? Yes, this test is broken. vm_create_barebones_type() errors out. Paolo
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 492e937fab00..a81a89b1dc2a 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -116,6 +116,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test TEST_GEN_PROGS_x86_64 += x86_64/vmx_pmu_caps_test TEST_GEN_PROGS_x86_64 += x86_64/xen_shinfo_test TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test +TEST_GEN_PROGS_x86_64 += x86_64/sev_init2_tests TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests TEST_GEN_PROGS_x86_64 += x86_64/amx_test TEST_GEN_PROGS_x86_64 += x86_64/max_vcpuid_cap_test diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 9e5afc472c14..44b6ea56a205 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -846,17 +846,15 @@ static inline struct kvm_vm *vm_create_barebones(void) return ____vm_create(VM_SHAPE_DEFAULT); } -#ifdef __x86_64__ -static inline struct kvm_vm *vm_create_barebones_protected_vm(void) +static inline struct kvm_vm *vm_create_barebones_type(unsigned long type) { const struct vm_shape shape = { .mode = VM_MODE_DEFAULT, - .type = KVM_X86_SW_PROTECTED_VM, + .type = type, }; return ____vm_create(shape); } -#endif static inline struct kvm_vm *vm_create(uint32_t nr_runnable_vcpus) { diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c index 075b80dbe237..0ac6c21d7251 100644 --- a/tools/testing/selftests/kvm/set_memory_region_test.c +++ b/tools/testing/selftests/kvm/set_memory_region_test.c @@ -339,7 +339,7 @@ static void test_invalid_memory_region_flags(void) #ifdef __x86_64__ if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM)) - vm = vm_create_barebones_protected_vm(); + vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM); else #endif vm = vm_create_barebones(); @@ -452,7 +452,7 @@ static void test_add_private_memory_region(void) pr_info("Testing ADD of KVM_MEM_GUEST_MEMFD memory regions\n"); - vm = vm_create_barebones_protected_vm(); + vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM); test_invalid_guest_memfd(vm, vm->kvm_fd, 0, "KVM fd should fail"); test_invalid_guest_memfd(vm, vm->fd, 0, "VM's fd should fail"); @@ -461,7 +461,7 @@ static void test_add_private_memory_region(void) test_invalid_guest_memfd(vm, memfd, 0, "Regular memfd() should fail"); close(memfd); - vm2 = vm_create_barebones_protected_vm(); + vm2 = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM); memfd = vm_create_guest_memfd(vm2, MEM_REGION_SIZE, 0); test_invalid_guest_memfd(vm, memfd, 0, "Other VM's guest_memfd() should fail"); @@ -489,7 +489,7 @@ static void test_add_overlapping_private_memory_regions(void) pr_info("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n"); - vm = vm_create_barebones_protected_vm(); + vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM); memfd = vm_create_guest_memfd(vm, MEM_REGION_SIZE * 4, 0); diff --git a/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c new file mode 100644 index 000000000000..644fd5757041 --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <linux/kvm.h> +#include <linux/psp-sev.h> +#include <stdio.h> +#include <sys/ioctl.h> +#include <stdlib.h> +#include <errno.h> +#include <pthread.h> + +#include "test_util.h" +#include "kvm_util.h" +#include "processor.h" +#include "svm_util.h" +#include "kselftest.h" + +#define SVM_SEV_FEAT_DEBUG_SWAP 32u + +/* + * Some features may have hidden dependencies, or may only work + * for certain VM types. Err on the side of safety and don't + * expect that all supported features can be passed one by one + * to KVM_SEV_INIT2. + * + * (Well, right now there's only one...) + */ +#define KNOWN_FEATURES SVM_SEV_FEAT_DEBUG_SWAP + +int kvm_fd; +u64 supported_vmsa_features; +bool have_sev_es; + +static int __sev_ioctl(int vm_fd, int cmd_id, void *data) +{ + struct kvm_sev_cmd cmd = { + .id = cmd_id, + .data = (uint64_t)data, + .sev_fd = open_sev_dev_path_or_exit(), + }; + int ret; + + ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd); + TEST_ASSERT(ret < 0 || cmd.error == SEV_RET_SUCCESS, + "%d failed: fw error: %d\n", + cmd_id, cmd.error); + + return ret; +} + +static void test_init2(unsigned long vm_type, struct kvm_sev_init *init) +{ + struct kvm_vm *vm; + int ret; + + vm = vm_create_barebones_type(vm_type); + ret = __sev_ioctl(vm->fd, KVM_SEV_INIT2, init); + TEST_ASSERT(ret == 0, + "KVM_SEV_INIT2 return code is %d (expected 0), errno: %d", + ret, errno); + kvm_vm_free(vm); +} + +static void test_init2_invalid(unsigned long vm_type, struct kvm_sev_init *init) +{ + struct kvm_vm *vm; + int ret; + + vm = vm_create_barebones_type(vm_type); + ret = __sev_ioctl(vm->fd, KVM_SEV_INIT2, init); + TEST_ASSERT(ret == -1 && errno == EINVAL, + "KVM_SEV_INIT2 return code %d, errno: %d (expected EINVAL)", + ret, errno); + kvm_vm_free(vm); +} + +void test_vm_types(void) +{ + test_init2(KVM_X86_SEV_VM, &(struct kvm_sev_init){}); + + if (have_sev_es) + test_init2(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){}); + else + test_init2_invalid(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){}); + + test_init2_invalid(0, &(struct kvm_sev_init){}); + if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM)) + test_init2_invalid(KVM_X86_SW_PROTECTED_VM, &(struct kvm_sev_init){}); +} + +void test_flags(uint32_t vm_type) +{ + int i; + + for (i = 0; i < 32; i++) + test_init2_invalid(vm_type, &(struct kvm_sev_init){ + .flags = 1u << i, + }); +} + +void test_features(uint32_t vm_type, uint64_t supported_features) +{ + int i; + + for (i = 0; i < 64; i++) { + if (!(supported_features & (1u << i))) + test_init2_invalid(vm_type, &(struct kvm_sev_init){ + .vmsa_features = 1u << i, + }); + else if (KNOWN_FEATURES & (1u << i)) + test_init2(vm_type, &(struct kvm_sev_init){ + .vmsa_features = 1u << i, + }); + } +} + +int main(int argc, char *argv[]) +{ + int kvm_fd = open_kvm_dev_path_or_exit(); + bool have_sev; + + TEST_REQUIRE(__kvm_has_device_attr(kvm_fd, 0, KVM_X86_SEV_VMSA_FEATURES) == 0); + kvm_device_attr_get(kvm_fd, 0, KVM_X86_SEV_VMSA_FEATURES, &supported_vmsa_features); + + have_sev = kvm_cpu_has(X86_FEATURE_SEV); + TEST_ASSERT(have_sev == !!(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_VM)), + "sev: KVM_CAP_VM_TYPES (%x) does not match cpuid (checking %x)", + kvm_check_cap(KVM_CAP_VM_TYPES), 1 << KVM_X86_SEV_VM); + + TEST_REQUIRE(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_VM)); + have_sev_es = kvm_cpu_has(X86_FEATURE_SEV_ES); + + TEST_ASSERT(have_sev_es == !!(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_ES_VM)), + "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); + + test_vm_types(); + + test_flags(KVM_X86_SEV_VM); + if (have_sev_es) + test_flags(KVM_X86_SEV_ES_VM); + + test_features(KVM_X86_SEV_VM, 0); + if (have_sev_es) + test_features(KVM_X86_SEV_ES_VM, supported_vmsa_features); + + return 0; +}