Re: [PATCH V7 6/8] KVM: selftests: add library for creating/interacting with SEV guests

[Sean Christopherson <seanjc@google.com>]

+TDX folks

On Mon, Dec 18, 2023, Peter Gonda wrote:
> Add interfaces to allow tests to create SEV guests. The additional
> requirements for SEV guests PTs and other state is encapsulated by the
> new vm_sev_create_with_one_vcpu() function. This can future be
> generalized for more vCPUs but the first set of SEV selftests in this
> series only uses a single vCPU.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Vishal Annapurve <vannapurve@google.com>
> Cc: Ackerly Tng <ackerleytng@google.com>
> cc: Andrew Jones <andrew.jones@linux.dev>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Originally-by: Michael Roth <michael.roth@amd.com>
> Co-developed-by: Ackerly Tng <ackerleytng@google.com>

Needs Ackerly's SoB.

> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>  include/uapi/linux/kvm.h                      |   2 +-
>  tools/arch/x86/include/asm/kvm_host.h         |   2 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../testing/selftests/kvm/include/sparsebit.h |  22 ++
>  .../selftests/kvm/include/x86_64/processor.h  |   2 +
>  .../selftests/kvm/include/x86_64/sev.h        |  27 +++
>  tools/testing/selftests/kvm/lib/kvm_util.c    |   1 +
>  .../selftests/kvm/lib/x86_64/processor.c      |  16 ++
>  tools/testing/selftests/kvm/lib/x86_64/sev.c  | 202 ++++++++++++++++++
>  9 files changed, 274 insertions(+), 1 deletion(-)
>  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/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 13065dd96132..251f422bcaa7 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1660,7 +1660,7 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_S390_GET_CMMA_BITS      _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
>  /* Memory Encryption Commands */
> -#define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
> +#define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, struct kvm_sev_cmd)

*sigh*

This cost me an hour of debug, partly because I was looking at tools/'s "good"
version, but mostly because I didn't expect a selftests patch to clobber KVM's uapi.

<rant>
This is an incredibly frustrating violating of basic patch principles.  Don't
include unrelated/unnecessary changes without good cause, especially not for uapi
headers.  If you do include unrelated changes, _document_ it in the changelog.
And if you don't understand _why_ something is weird, ask!

Even worse is that this series has sat on the lists for over a month, and NO ONE
tested it.  I can kinda sorta see how Peter missed this, e.g. if the host kernel
was build from the same source as the selftests.  But I have a very, very hard time
believing that every other person that peeked at a _selftests_ series rebuilt and
rebooted their hosts using that series.

If this were some obscure series that was touching an area of KVM that few devs
care about, I wouldn't react so strongly.  But there are how many developers working
on SNP and TDX?  10?  15?  20?

I have made it _abundantly_ clear, over, and over, that tests are a hard requirement
for new features.  Yet I see *zero* review/testing activity on this series or Sagi's
TDX selftests series.  Some of the flaws in this series are design-ish problems,
i.e. not things I would expect everyone to be able to independently identify and/or
address, but there are also a number of glaring flaws that anyone giving this more
than a cursory glance would pick out.  E.g. this patch adds an SEV library, but then
the series doesn't bother to use it to dedup the existing code in sev_migrate_test.c.

If y'all want SNP or TDX support to go anywhere but the backburner, cycles need
to be redirected to getting selftests written, healthy, and *maintainable*.  I have
*very* limited cycles for SNP/TDX for the foreseeable future, and I care a hell of a
lot more about having healthy, robust selftests than I do about getting SNP or TDX
merged by some arbitrary deadline.
</rant>

> @@ -66,6 +66,28 @@ void sparsebit_dump(FILE *stream, const struct sparsebit *sbit,
>  		    unsigned int indent);
>  void sparsebit_validate_internal(const struct sparsebit *sbit);
>  
> +/*
> + * Iterate over set ranges within sparsebit @s. In each iteration,
> + * @range_begin and @range_end will take the beginning and end of the set
> + * range, which are of type sparsebit_idx_t.
> + *
> + * For example, if the range [3, 7] (inclusive) is set, within the
> + * iteration,@range_begin will take the value 3 and @range_end will take
> + * the value 7.
> + *
> + * Ensure that there is at least one bit set before using this macro with
> + * sparsebit_any_set(), because sparsebit_first_set() will abort if none
> + * are set.
> + */
> +#define sparsebit_for_each_set_range(s, range_begin, range_end)         \
> +	for (range_begin = sparsebit_first_set(s),                      \
> +	     range_end =						\

Unnecessary newline.

> +	     sparsebit_next_clear(s, range_begin) - 1;		\
> +	     range_begin && range_end;                                  \
> +	     range_begin = sparsebit_next_set(s, range_end),            \
> +	     range_end =						\
> +	     sparsebit_next_clear(s, range_begin) - 1)
> +

It probably makes sense to split this to a separate patch.  Adding APIs without
users is generally frowned upon, but I think in this case it's worth isolating
the SEV changes.

>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 4fd042112526..67cc32b1a29a 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -266,6 +266,7 @@ struct kvm_x86_cpu_property {
>  #define X86_PROPERTY_MAX_PHY_ADDR		KVM_X86_CPU_PROPERTY(0x80000008, 0, EAX, 0, 7)
>  #define X86_PROPERTY_MAX_VIRT_ADDR		KVM_X86_CPU_PROPERTY(0x80000008, 0, EAX, 8, 15)
>  #define X86_PROPERTY_PHYS_ADDR_REDUCTION	KVM_X86_CPU_PROPERTY(0x8000001F, 0, EBX, 6, 11)
> +#define X86_PROPERTY_SEV_C_BIT KVM_X86_CPU_PROPERTY(0x8000001F, 0, EBX, 0, 5)

Put this above X86_PROPERTY_PHYS_ADDR_REDUCTION so that they are sorted in
ascending order.

> 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..f2bac717cac1
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#define _GNU_SOURCE /* for program_invocation_short_name */
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +#include "kvm_util.h"
> +#include "svm_util.h"
> +#include "linux/psp-sev.h"
> +#include "processor.h"
> +#include "sev.h"
> +
> +#define SEV_FW_REQ_VER_MAJOR 0
> +#define SEV_FW_REQ_VER_MINOR 17
> +
> +enum sev_guest_state {
> +	SEV_GSTATE_UNINIT = 0,
> +	SEV_GSTATE_LUPDATE,
> +	SEV_GSTATE_LSECRET,
> +	SEV_GSTATE_RUNNING,

Spell these out, saving a few keystrokes is not worth inscrutable names.  These
enums/define also belong in sev.h

> +};
> +
> +static void sev_ioctl(int cmd, void *data)
> +{
> +	int sev_fd = open_sev_dev_path_or_exit();
> +	struct sev_issue_cmd arg = {
> +		.cmd = cmd,
> +		.data = (unsigned long)data,
> +	};
> +
> +	kvm_ioctl(sev_fd, SEV_ISSUE_CMD, &arg);
> +	close(sev_fd);
> +}
> +
> +static void kvm_sev_ioctl(struct kvm_vm *vm, int cmd, void *data)
> +{
> +	struct kvm_sev_cmd sev_cmd = {
> +		.id = cmd,
> +		.sev_fd = vm->arch.sev_fd,
> +		.data = (unsigned long)data,
> +	};
> +
> +	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_OP, &sev_cmd);
> +}
> +
> +static void sev_register_encrypted_memory(struct kvm_vm *vm,
> +					  struct userspace_mem_region *region)
> +{
> +	struct kvm_enc_region range = {
> +		.addr = region->region.userspace_addr,
> +		.size = region->region.memory_size,
> +	};
> +
> +	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
> +}
> +
> +static void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> +				   uint64_t size)
> +{
> +	struct kvm_sev_launch_update_data update_data = {
> +		.uaddr = (unsigned long)addr_gpa2hva(vm, gpa),
> +		.len = size,
> +	};
> +
> +	kvm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
> +}
> +

The APIs that are effectively wrappers to KVM ioctls() should be globally visible,
e.g. to allow mixing in match them in negative tests that want to do "dumb" things
on a half-baked VM.

> +static void sev_vm_launch(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;
> +
> +	kvm_sev_ioctl(vm, KVM_SEV_LAUNCH_START, &launch_start);
> +	kvm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status);
> +
> +	TEST_ASSERT(status.policy == policy, "Expected policy %d, got %d",
> +		    policy, status.policy);

	TEST_ASSERT_EQ() will do the heavy lifting for you.

> +static void sev_vm_measure(struct kvm_vm *vm)
> +{
> +	uint8_t measurement[512];
> +	int i;
> +
> +	sev_vm_launch_measure(vm, measurement);
> +
> +	/* TODO: Validate the measurement is as expected. */
> +	pr_debug("guest measurement: ");
> +	for (i = 0; i < 32; ++i)
> +		pr_debug("%02x", measurement[i]);
> +	pr_debug("\n");
> +}

Meh, this isn't helpful for a test, e.g. the average user isn't never going to do
anything useful with the measurement info.