| 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
| ||
|
Message-ID: <ZB3qtwN2BtSDgU0Y@google.com>
Date: Fri, 24 Mar 2023 11:23:51 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Peter Gonda <pgonda@...gle.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Paolo Bonzini <pbonzini@...hat.com>,
Vishal Annapurve <vannapurve@...gle.com>,
Ackerley Tng <ackerleytng@...gle.com>,
Andrew Jones <andrew.jones@...ux.dev>,
Michael Roth <michael.roth@....com>
Subject: Re: [PATCH V6 7/7] KVM: selftests: Add simple sev vm testing
On Tue, Jan 10, 2023, Peter Gonda wrote:
> A very simple of booting SEV guests that checks related CPUID bits. This
> is a stripped down version of "[PATCH v2 08/13] KVM: selftests: add SEV
> boot tests" from Michael but much simpler.
>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Sean Christopherson <seanjc@...gle.com>
> Cc: Vishal Annapurve <vannapurve@...gle.com>
> Cc: Ackerley Tng <ackerleytng@...gle.com>
> cc: Andrew Jones <andrew.jones@...ux.dev>
> Suggested-by: Michael Roth <michael.roth@....com>
> Signed-off-by: Peter Gonda <pgonda@...gle.com>
> ---
> tools/testing/selftests/kvm/.gitignore | 84 +++++++++++++++++++
> tools/testing/selftests/kvm/Makefile | 1 +
> .../selftests/kvm/x86_64/sev_all_boot_test.c | 84 +++++++++++++++++++
> 3 files changed, 169 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
>
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 6d9381d60172..6d826957c6ae 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,7 +1,91 @@
> # SPDX-License-Identifier: GPL-2.0-only
> +<<<<<<< HEAD
> *
> !/**/
> !*.c
> !*.h
> !*.S
> !*.sh
> +=======
For the love of all that is holy, please take the time to actually look at what
you're posting.
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index b7cfb15712d1..66d7ab3da990 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -111,6 +111,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
> TEST_GEN_PROGS_x86_64 += x86_64/vmx_pmu_caps_test
> TEST_GEN_PROGS_x86_64 += x86_64/xen_shinfo_test
> TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test
> +TEST_GEN_PROGS_x86_64 += x86_64/sev_all_boot_test
> TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests
> TEST_GEN_PROGS_x86_64 += x86_64/amx_test
> TEST_GEN_PROGS_x86_64 += x86_64/max_vcpuid_cap_test
> 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..e9e4d7305bc1
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
"all_boot" is kinda odd. Maybe just sev_basic_test?
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Basic SEV boot tests.
> + *
> + */
Meh, I'd leave off the comment, the testname should be pretty self-explanatory.
> +#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 NR_SYNCS 1
> +
> +#define MSR_AMD64_SEV_BIT 1
msr-index.h exists in tools, use the defines from there.
> +
> +static void guest_run_loop(struct kvm_vcpu *vcpu)
Another pointless helper with one caller. _If_ more subtests come along _and_
need this separate loop, then by all means. But right now, it's just noise.
> +{
> + struct ucall uc;
> + int i;
> +
> + for (i = 0; i <= NR_SYNCS; ++i) {
There's zero reason to cap the number of loops, and it can lead to false passes,
e.g. if the guest gets stuck on GUEST_SYNC() then this will pass. Just do
for (;;) {
> + vcpu_run(vcpu);
> + switch (get_ucall(vcpu, &uc)) {
> + case UCALL_SYNC:
> + continue;
> + case UCALL_DONE:
> + return;
> + case UCALL_ABORT:
> + REPORT_GUEST_ASSERT(uc);
> + default:
> + TEST_FAIL("Unexpected exit: %s",
> + exit_reason_str(vcpu->run->exit_reason));
> + }
> + }
> +}
> +
> +static void is_sev_enabled(void)
> +{
> + uint64_t sev_status;
> +
> + GUEST_ASSERT(this_cpu_has(X86_FEATURE_SEV));
> +
> + sev_status = rdmsr(MSR_AMD64_SEV);
> + GUEST_ASSERT(sev_status & 0x1);
> +}
> +
> +static void guest_sev_code(void)
> +{
> + GUEST_SYNC(1);
> +
> + is_sev_enabled();
Again, completely unnecessary helper. And is_blah_enabled() _strongly_ suggests
a pure function that returns true/false.
All in all, this can be slimmed down to:
// SPDX-License-Identifier: GPL-2.0-only
#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"
static void guest_sev_code(void)
{
GUEST_ASSERT(this_cpu_has(X86_FEATURE_SEV));
GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED);
GUEST_DONE();
}
static void test_sev(void *guest_code, uint64_t policy)
{
struct kvm_vcpu *vcpu;
struct kvm_vm *vm;
struct ucall uc;
int i;
vm = vm_sev_create_with_one_vcpu(policy, guest_code, &vcpu);
for (;;) {
vcpu_run(vcpu);
switch (get_ucall(vcpu, &uc)) {
case UCALL_SYNC:
continue;
case UCALL_DONE:
return;
case UCALL_ABORT:
REPORT_GUEST_ASSERT(uc);
default:
TEST_FAIL("Unexpected exit: %s",
exit_reason_str(vcpu->run->exit_reason));
}
}
kvm_vm_free(vm);
}
int main(int argc, char *argv[])
{
TEST_REQUIRE(is_kvm_sev_supported());
test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
test_sev(guest_sev_code, 0);
return 0;
}
Powered by blists - more mailing lists