+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@xxxxxxxxxx>
> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> Cc: Vishal Annapurve <vannapurve@xxxxxxxxxx>
> Cc: Ackerly Tng <ackerleytng@xxxxxxxxxx>
> cc: Andrew Jones <andrew.jones@xxxxxxxxx>
> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Cc: Michael Roth <michael.roth@xxxxxxx>
> Originally-by: Michael Roth <michael.roth@xxxxxxx>
> Co-developed-by: Ackerly Tng <ackerleytng@xxxxxxxxxx>
Needs Ackerly's SoB.
> Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx>
> ---
> 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.
