Received-SPF: pass (google.com: domain of linux-kernel+bounces-78817-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 23 Feb 2024 09:18:04 -0800
In-Reply-To: <20240223104009.632194-12-pbonzini@redhat.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240223104009.632194-1-pbonzini@redhat.com> <20240223104009.632194-12-pbonzini@redhat.com>
Message-ID: <ZdjTTK1TgN8B64zO@google.com>
Subject: Re: [PATCH v2 11/11] selftests: kvm: add tests for KVM_SEV_INIT2
From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, michael.roth@amd.com, 
	aik@amd.com
Content-Type: text/plain; charset="us-ascii"

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