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@xxxxxxxxxx
> +}
> +
> +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,
> + });
> + }
> +}
