| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 29 Aug 2022 09:38:46 -0600
From: Peter Gonda <pgonda@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm list <kvm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Marc Orr <marcorr@...gle.com>,
Michael Roth <michael.roth@....com>,
"Lendacky, Thomas" <thomas.lendacky@....com>,
Joerg Roedel <joro@...tes.org>,
Mingwei Zhang <mizhang@...gle.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Andrew Jones <andrew.jones@...ux.dev>,
Vishal Annapurve <vannapurve@...gle.com>
Subject: Re: [V3 11/11] KVM: selftests: Add simple sev vm testing
On Wed, Aug 17, 2022 at 6:22 PM Sean Christopherson <seanjc@...gle.com> wrote:
>
> /sev_vm_launch_measurOn Wed, Aug 10, 2022, Peter Gonda wrote:
> > diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
> > index 2f7f7c741b12..b6552ea1c716 100644
> > --- a/tools/testing/selftests/kvm/include/x86_64/sev.h
> > +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
> > @@ -22,6 +22,9 @@
> > #define SEV_POLICY_NO_DBG (1UL << 0)
> > #define SEV_POLICY_ES (1UL << 2)
> >
> > +#define CPUID_MEM_ENC_LEAF 0x8000001f
> > +#define CPUID_EBX_CBIT_MASK 0x3f
>
> Ha! I was going to say "put these in processor.h", but I have an even better idea.
> I'll try to a series posted tomorrow (compile tested only at this point), but what
> I'm hoping to do is to allow automagic retrieval of multi-bit CPUID properties, a la
> the existing this_cpu_has() stuff.
>
> E.g.
>
> #define X86_PROPERTY_CBIT_LOCATION KVM_X86_CPU_PROPERTY(0x8000001F, 0, EBX, 0, 5)
>
> and then
>
> sev->enc_bit = this_cpu_property(X86_PROPERTY_CBIT_LOCATION);
>
> LOL, now I see that the defines in sev.c were introduced back in patch 08. That's
> probably fine for your submission so as not to take a dependency on the "property"
> idea. This patch doesn't need to move the CPUID_* defines because it can use
> this_cpu_has(X86_FEATURE_SEV) and avoid referencing CPUID_MEM_ENC_LEAF.
OK I've put these into sev.c instead of the header. I can clean up
after you property patch series goes in.
>
> > enum {
> > SEV_GSTATE_UNINIT = 0,
> > SEV_GSTATE_LUPDATE,
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> > index 3abcf50c0b5d..8f9f55c685a7 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> > @@ -13,8 +13,6 @@
> > #include "sev.h"
> >
> > #define PAGE_SHIFT 12
>
> Already defined in processor.h
Removed
>
> > -#define CPUID_MEM_ENC_LEAF 0x8000001f
> > -#define CPUID_EBX_CBIT_MASK 0x3f
> >
> > struct sev_vm {
> > struct kvm_vm *vm;
> > diff --git a/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> > new file mode 100644
> > index 000000000000..b319d18bdb60
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> > @@ -0,0 +1,131 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Basic SEV boot tests.
> > + *
> > + * Copyright (C) 2021 Advanced Micro Devices
> > + */
> > +#define _GNU_SOURCE /* for program_invocation_short_name */
> > +#include <fcntl.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +
> > +#include "test_util.h"
> > +
> > +#include "kvm_util.h"
> > +#include "processor.h"
> > +#include "svm_util.h"
> > +#include "linux/psp-sev.h"
> > +#include "sev.h"
> > +
> > +#define VCPU_ID 2
>
> Nooooooo. Unless there is a really, REALLY good reason this needs to be '2', just
> pass '0' as a literal to vm_vcpu_add() and delete VCPU_ID.
Not needed, removed.
>
> > +#define PAGE_STRIDE 32
> > +
> > +#define SHARED_PAGES 8192
> > +#define SHARED_VADDR_MIN 0x1000000
> > +
> > +#define PRIVATE_PAGES 2048
> > +#define PRIVATE_VADDR_MIN (SHARED_VADDR_MIN + SHARED_PAGES * PAGE_SIZE)
> > +
> > +#define TOTAL_PAGES (512 + SHARED_PAGES + PRIVATE_PAGES)
> > +
> > +#define NR_SYNCS 1
> > +
> > +static void guest_run_loop(struct kvm_vcpu *vcpu)
> > +{
> > + struct ucall uc;
> > + int i;
> > +
> > + for (i = 0; i <= NR_SYNCS; ++i) {
> > + vcpu_run(vcpu);
> > + switch (get_ucall(vcpu, &uc)) {
> > + case UCALL_SYNC:
> > + continue;
> > + case UCALL_DONE:
> > + return;
> > + case UCALL_ABORT:
> > + TEST_ASSERT(false, "%s at %s:%ld\n\tvalues: %#lx, %#lx",
> > + (const char *)uc.args[0], __FILE__,
> > + uc.args[1], uc.args[2], uc.args[3]);
> > + default:
> > + TEST_ASSERT(
> > + false, "Unexpected exit: %s",
> > + exit_reason_str(vcpu->run->exit_reason));
> > + }
> > + }
> > +}
> > +
> > +static void __attribute__((__flatten__)) guest_sev_code(void)
>
> Is __flatten__ strictly necessary? I don't see this being copied over anything
> that would require it to be a contiguous chunk.
Nope, removed.
>
> > +{
> > + uint32_t eax, ebx, ecx, edx;
> > + uint64_t sev_status;
> > +
> > + GUEST_SYNC(1);
> > +
> > + cpuid(CPUID_MEM_ENC_LEAF, &eax, &ebx, &ecx, &edx);
> > + GUEST_ASSERT(eax & (1 << 1));
>
> GUEST_ASSERT(this_cpu_has(X86_FEATURE_SEV));
Done.
> > +
> > + sev_status = rdmsr(MSR_AMD64_SEV);
> > + GUEST_ASSERT((sev_status & 0x1) == 1);
> > +
> > + GUEST_DONE();
> > +}
> > +
> > +static struct sev_vm *setup_test_common(void *guest_code, uint64_t policy,
> > + struct kvm_vcpu **vcpu)
> > +{
> > + uint8_t measurement[512];
> > + struct sev_vm *sev;
> > + struct kvm_vm *vm;
> > + int i;
> > +
> > + sev = sev_vm_create(policy, TOTAL_PAGES);
>
> TEST_ASSERT(sev, ...) so that this doesn't silently "pass"?
Done.
>
> > + if (!sev)
> > + return NULL;
> > + vm = sev_get_vm(sev);
> > +
> > + /* Set up VCPU and initial guest kernel. */
> > + *vcpu = vm_vcpu_add(vm, VCPU_ID, guest_code);
> > + kvm_vm_elf_load(vm, program_invocation_name);
> > +
> > + /* Allocations/setup done. Encrypt initial guest payload. */
> > + sev_vm_launch(sev);
> > +
> > + /* Dump the initial measurement. A test to actually verify it would be nice. */
> > + sev_vm_launch_measure(sev, measurement);
> > + pr_info("guest measurement: ");
> > + for (i = 0; i < 32; ++i)
> > + pr_info("%02x", measurement[i]);
> > + pr_info("\n");
> > +
> > + sev_vm_launch_finish(sev);
> > +
> > + return sev;
> > +}
> > +
> > +static void test_sev(void *guest_code, uint64_t policy)
> > +{
> > + struct sev_vm *sev;
> > + struct kvm_vcpu *vcpu;
> > +
> > + sev = setup_test_common(guest_code, policy, &vcpu);
> > + if (!sev)
> > + return;
>
> And with an assert above, this return goes away. Or better yet, fold setup_test_common()
> into test_sev(), there's only the one user of the so called "common" function.
Done.
>
> > +
> > + /* Guest is ready to run. Do the tests. */
> > + guest_run_loop(vcpu);
> > +
> > + pr_info("guest ran successfully\n");
> > +
> > + sev_vm_free(sev);
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + /* SEV tests */
> > + test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
> > + test_sev(guest_sev_code, 0);
> > +
> > + return 0;
> > +}
> > --
> > 2.37.1.559.g78731f0fdb-goog
> >
Powered by blists - more mailing lists