On 5/31/21 12:27 PM, Varad Gautam wrote:
> Some vmexits on a SEV-ES guest need special handling within the guest
> before exiting to the hypervisor. This must happen within the guest's
> \#VC exception handler, triggered on every non automatic exit.
>
> Add a KUnit based test to validate Linux's VC handling. The test:
> 1. installs a kretprobe on the #VC handler (sev_es_ghcb_hv_call, to
> access GHCB before/after the resulting VMGEXIT).
> 2. tiggers an NAE.
> 3. checks that the kretprobe was hit with the right exit_code available
> in GHCB.
>
> Since relying on kprobes, the test does not cover NMI contexts.
I'm not very familiar with these types of tests, so just general feedback
below.
>
> Signed-off-by: Varad Gautam <varad.gautam@xxxxxxxx>
> ---
> v2: Add a testcase for MMIO read/write.
>
> arch/x86/Kconfig | 9 ++
> arch/x86/kernel/Makefile | 5 ++
> arch/x86/kernel/sev-test-vc.c | 155 ++++++++++++++++++++++++++++++++++
> 3 files changed, 169 insertions(+)
> create mode 100644 arch/x86/kernel/sev-test-vc.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 0045e1b441902..0a3c3f31813f1 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1543,6 +1543,15 @@ config AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT
> If set to N, then the encryption of system memory can be
> activated with the mem_encrypt=on command line option.
>
> +config AMD_MEM_ENCRYPT_TEST_VC
> + bool "Test for AMD Secure Memory Encryption (SME) support"
I would change this to indicate that this is specifically for testing
SEV-ES support. We messed up and didn't update the AMD_MEM_ENCRYPT entry
when the SEV support was submitted.
Thanks,
Tom
> + depends on AMD_MEM_ENCRYPT
> + select KUNIT
> + select FUNCTION_TRACER
> + help
> + Enable KUnit-based testing for the encryption of system memory
> + using AMD SEV-ES. Currently only tests #VC handling.
> +
> # Common NUMA Features
> config NUMA
> bool "NUMA Memory Allocation and Scheduler Support"
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 0f66682ac02a6..360c5d33580ca 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -23,6 +23,10 @@ CFLAGS_REMOVE_head64.o = -pg
> CFLAGS_REMOVE_sev.o = -pg
> endif
>
> +ifdef CONFIG_AMD_MEM_ENCRYPT_TEST_VC
> +CFLAGS_sev.o += -fno-ipa-sra
> +endif
Maybe add something in the commit message as to why this is needed.
> +
> KASAN_SANITIZE_head$(BITS).o := n
> KASAN_SANITIZE_dumpstack.o := n
> KASAN_SANITIZE_dumpstack_$(BITS).o := n
> @@ -149,6 +153,7 @@ obj-$(CONFIG_UNWINDER_FRAME_POINTER) += unwind_frame.o
> obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>
> obj-$(CONFIG_AMD_MEM_ENCRYPT) += sev.o
> +obj-$(CONFIG_AMD_MEM_ENCRYPT_TEST_VC) += sev-test-vc.o
> ###
> # 64 bit specific files
> ifeq ($(CONFIG_X86_64),y)
> diff --git a/arch/x86/kernel/sev-test-vc.c b/arch/x86/kernel/sev-test-vc.c
> new file mode 100644
> index 0000000000000..2475270b844e8
> --- /dev/null
> +++ b/arch/x86/kernel/sev-test-vc.c
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 SUSE
> + *
> + * Author: Varad Gautam <varad.gautam@xxxxxxxx>
> + */
> +
> +#include <asm/cpufeature.h>
> +#include <asm/debugreg.h>
> +#include <asm/io.h>
> +#include <asm/sev-common.h>
> +#include <asm/svm.h>
> +#include <kunit/test.h>
> +#include <linux/kprobes.h>
> +
> +static struct kretprobe hv_call_krp;
> +
> +static int hv_call_krp_entry(struct kretprobe_instance *krpi,
> + struct pt_regs *regs)
Align with the argument above.
> +{
> + unsigned long ghcb_vaddr = regs_get_kernel_argument(regs, 0);
> + *((unsigned long *) krpi->data) = ghcb_vaddr;
> +
> + return 0;
> +}
> +
> +static int hv_call_krp_ret(struct kretprobe_instance *krpi,
> + struct pt_regs *regs)
Align with the argument above.
> +{
> + unsigned long ghcb_vaddr = *((unsigned long *) krpi->data);
> + struct ghcb *ghcb = (struct ghcb *) ghcb_vaddr;
> + struct kunit *test = current->kunit_test;
> +
> + if (test && strstr(test->name, "sev_es_") && test->priv)
> + cmpxchg((unsigned long *) test->priv, ghcb->save.sw_exit_code, 1);
> +
> + return 0;
> +}
> +
> +int sev_test_vc_init(struct kunit *test)
> +{
> + int ret;
> +
> + if (!sev_es_active()) {
> + pr_err("Not a SEV-ES guest. Skipping.");
Should this be a pr_info vs a pr_err?
Should you define a pr_fmt above for the pr_ messages?
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + memset(&hv_call_krp, 0, sizeof(hv_call_krp));
> + hv_call_krp.entry_handler = hv_call_krp_entry;
> + hv_call_krp.handler = hv_call_krp_ret;
> + hv_call_krp.maxactive = 100;
> + hv_call_krp.data_size = sizeof(unsigned long);
> + hv_call_krp.kp.symbol_name = "sev_es_ghcb_hv_call";
> + hv_call_krp.kp.addr = 0;
> +
> + ret = register_kretprobe(&hv_call_krp);
> + if (ret < 0) {
Should this just be "if (ret) {"? Can a positive number be returned and if
so, what does it mean?
> + pr_err("Could not register kretprobe. Skipping.");
> + goto out;
> + }
> +
> + test->priv = kunit_kzalloc(test, sizeof(unsigned long), GFP_KERNEL);
> + if (!test->priv) {
> + ret = -ENOMEM;
> + pr_err("Could not allocate. Skipping.");
> + goto out;
> + }
> +
> +out:
> + return ret;
> +}
> +
> +void sev_test_vc_exit(struct kunit *test)
> +{
> + if (test->priv)
> + kunit_kfree(test, test->priv);
> +
> + if (hv_call_krp.kp.addr)
> + unregister_kretprobe(&hv_call_krp);
> +}
> +
> +#define guarded_op(kt, ec, op) \
> +do { \
> + struct kunit *t = (struct kunit *) kt; \
> + smp_store_release((typeof(ec) *) t->priv, ec); \
> + op; \
> + KUNIT_EXPECT_EQ(t, (typeof(ec)) 1, \
> + (typeof(ec)) smp_load_acquire((typeof(ec) *) t->priv)); \
I usually like seeing all the '\' characters lined up, rather than having
just the one hanging out.
Thanks,
Tom
> +} while(0)
> +
> +static void sev_es_nae_cpuid(struct kunit *test)
> +{
> + unsigned int cpuid_fn = 0x8000001f;
> +
> + guarded_op(test, SVM_EXIT_CPUID, native_cpuid_eax(cpuid_fn));
> +}
> +
> +static void sev_es_nae_wbinvd(struct kunit *test)
> +{
> + guarded_op(test, SVM_EXIT_WBINVD, wbinvd());
> +}
> +
> +static void sev_es_nae_msr(struct kunit *test)
> +{
> + guarded_op(test, SVM_EXIT_MSR, __rdmsr(MSR_IA32_TSC));
> +}
> +
> +static void sev_es_nae_dr7_rw(struct kunit *test)
> +{
> + guarded_op(test, SVM_EXIT_WRITE_DR7,
> + native_set_debugreg(7, native_get_debugreg(7)));
> +}
> +
> +static void sev_es_nae_ioio(struct kunit *test)
> +{
> + unsigned long port = 0x80;
> + char val = 0;
> +
> + guarded_op(test, SVM_EXIT_IOIO, val = inb(port));
> + guarded_op(test, SVM_EXIT_IOIO, outb(val, port));
> + guarded_op(test, SVM_EXIT_IOIO, insb(port, &val, sizeof(val)));
> + guarded_op(test, SVM_EXIT_IOIO, outsb(port, &val, sizeof(val)));
> +}
> +
> +static void sev_es_nae_mmio(struct kunit *test)
> +{
> + unsigned long lapic_ver_pa = 0xfee00030; /* APIC_DEFAULT_PHYS_BASE + APIC_LVR */
> + unsigned __iomem *lapic = ioremap(lapic_ver_pa, 0x4);
> + unsigned lapic_version = 0;
> +
> + guarded_op(test, SVM_VMGEXIT_MMIO_READ, lapic_version = *lapic);
> + guarded_op(test, SVM_VMGEXIT_MMIO_WRITE, *lapic = lapic_version);
> +
> + iounmap(lapic);
> +}
> +
> +static struct kunit_case sev_test_vc_testcases[] = {
> + KUNIT_CASE(sev_es_nae_cpuid),
> + KUNIT_CASE(sev_es_nae_wbinvd),
> + KUNIT_CASE(sev_es_nae_msr),
> + KUNIT_CASE(sev_es_nae_dr7_rw),
> + KUNIT_CASE(sev_es_nae_ioio),
> + KUNIT_CASE(sev_es_nae_mmio),
> + {}
> +};
> +
> +static struct kunit_suite sev_vc_test_suite = {
> + .name = "sev_test_vc",
> + .init = sev_test_vc_init,
> + .exit = sev_test_vc_exit,
> + .test_cases = sev_test_vc_testcases,
> +};
> +kunit_test_suite(sev_vc_test_suite);
>
