diff mbox series

[v4] x86: Add a test for AMD SEV-ES #VC handling

Message ID 20210616091538.15321-1-varad.gautam@suse.com (mailing list archive)
State New, archived
Headers show
Series [v4] x86: Add a test for AMD SEV-ES #VC handling | expand

Commit Message

Varad Gautam June 16, 2021, 9:15 a.m. UTC 
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, and introduce
a new CONFIG_X86_TESTS to cover such tests. 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.

Signed-off-by: Varad Gautam <varad.gautam@suse.com>
---
v4: Move this test to arch/x86/tests/, enabled by CONFIG_X86_TESTS.

 arch/x86/Kbuild                 |   2 +
 arch/x86/Kconfig.debug          |  15 ++++
 arch/x86/kernel/Makefile        |   7 ++
 arch/x86/tests/Makefile         |   3 +
 arch/x86/tests/sev-es-test-vc.c | 155 ++++++++++++++++++++++++++++++++
 5 files changed, 182 insertions(+)
 create mode 100644 arch/x86/tests/Makefile
 create mode 100644 arch/x86/tests/sev-es-test-vc.c

Comments

Borislav Petkov June 24, 2021, 10:36 a.m. UTC | #1 
On Wed, Jun 16, 2021 at 11:15:38AM +0200, Varad Gautam wrote:

Subject: Re: [PATCH v4] x86: Add a test for AMD SEV-ES #VC handling

Change your subject prefix to "x86/test: ... "

> 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, and introduce
> a new CONFIG_X86_TESTS to cover such tests. 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.
> 
> Signed-off-by: Varad Gautam <varad.gautam@suse.com>
> ---
> v4: Move this test to arch/x86/tests/, enabled by CONFIG_X86_TESTS.
> 
>  arch/x86/Kbuild                 |   2 +
>  arch/x86/Kconfig.debug          |  15 ++++
>  arch/x86/kernel/Makefile        |   7 ++
>  arch/x86/tests/Makefile         |   3 +
>  arch/x86/tests/sev-es-test-vc.c | 155 ++++++++++++++++++++++++++++++++
>  5 files changed, 182 insertions(+)
>  create mode 100644 arch/x86/tests/Makefile
>  create mode 100644 arch/x86/tests/sev-es-test-vc.c

Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.

There's a bunch of things to fix I've pasted at the end of this mail.

> diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
> index 30dec019756b9..965cfcbd12f67 100644
> --- a/arch/x86/Kbuild
> +++ b/arch/x86/Kbuild
> @@ -25,3 +25,5 @@ obj-y += platform/
>  obj-y += net/
>  
>  obj-$(CONFIG_KEXEC_FILE) += purgatory/
> +
> +obj-$(CONFIG_X86_TESTS) += tests/
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 80b57e7f49477..6f63069fff972 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -282,3 +282,18 @@ endchoice
>  config FRAME_POINTER
>  	depends on !UNWINDER_ORC && !UNWINDER_GUESS
>  	bool
> +
> +config X86_TESTS
> +	bool "Tests for x86"
> +	help
> +	    This enables building the tests under arch/x86/tests.
> +

All those tests should probably be in a

if X86_TESTS

> +config AMD_SEV_ES_TEST_VC
> +	bool "Test for AMD SEV-ES VC exception handling"
> +	depends on AMD_MEM_ENCRYPT
> +	depends on X86_TESTS
> +	select FUNCTION_TRACER
> +	select KPROBES
> +	select KUNIT
> +	help
> +	  Enable KUnit-based testing for AMD SEV-ES #VC exception handling.

endif # X86_TESTS

so that you don't have the X86_TESTS dependency in each test explicitly.

> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 0f66682ac02a6..bf1c4dc525ac6 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -23,6 +23,13 @@ CFLAGS_REMOVE_head64.o = -pg
>  CFLAGS_REMOVE_sev.o = -pg
>  endif
>  
> +# AMD_SEV_ES_TEST_VC registers a kprobe by function name. IPA-SRA creates
> +# function copies and renames them to have an .isra suffix, which breaks kprobes'
> +# lookup. Build with -fno-ipa-sra for the test.
> +ifdef CONFIG_AMD_SEV_ES_TEST_VC
> +CFLAGS_sev.o	+= -fno-ipa-sra
> +endif
> +
>  KASAN_SANITIZE_head$(BITS).o				:= n
>  KASAN_SANITIZE_dumpstack.o				:= n
>  KASAN_SANITIZE_dumpstack_$(BITS).o			:= n
> diff --git a/arch/x86/tests/Makefile b/arch/x86/tests/Makefile
> new file mode 100644
> index 0000000000000..fa79c435d7843
> --- /dev/null
> +++ b/arch/x86/tests/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_AMD_SEV_ES_TEST_VC)	+= sev-es-test-vc.o
> diff --git a/arch/x86/tests/sev-es-test-vc.c b/arch/x86/tests/sev-es-test-vc.c
> new file mode 100644
> index 0000000000000..98dc38572ed5d
> --- /dev/null
> +++ b/arch/x86/tests/sev-es-test-vc.c

Can this be called sev-tests.c and simply collect all SEV-related tests
in it?

> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 SUSE
> + *
> + * Author: Varad Gautam <varad.gautam@suse.com>
> + */
> +
> +#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)
> +{
> +	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)
> +{
> +	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);

Why is this needed? Normal assignment won't do?

Also, that "1" is magic as it is used in KUNIT_EXPECT_EQ below so maybe
have it defined with a nice telling name.

IOW, why are those memory barriers needed?

...

Some checkpatch issues:

WARNING: 'tiggers' may be misspelled - perhaps 'triggers'?
#74: 
2. tiggers an NAE.
   ^^^^^^^

WARNING: please write a paragraph that describes the config symbol fully
#112: FILE: arch/x86/Kconfig.debug:286:
+config X86_TESTS

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#145: 
new file mode 100644

WARNING: memory barrier without comment
#245: FILE: arch/x86/tests/sev-es-test-vc.c:87:
+	smp_store_release((typeof(ec) *) t->priv, ec);			\

WARNING: please, no space before tabs
#247: FILE: arch/x86/tests/sev-es-test-vc.c:89:
+^IKUNIT_EXPECT_EQ(t, (typeof(ec)) 1, ^I^I^I^I\$

WARNING: memory barrier without comment
#248: FILE: arch/x86/tests/sev-es-test-vc.c:90:
+		(typeof(ec)) smp_load_acquire((typeof(ec) *) t->priv));	\

ERROR: space required before the open parenthesis '('
#249: FILE: arch/x86/tests/sev-es-test-vc.c:91:
+} while(0)

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#289: FILE: arch/x86/tests/sev-es-test-vc.c:131:
+	unsigned lapic_version = 0;

total: 1 errors, 7 warnings, 194 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH v4] x86: Add a test for AMD SEV-ES #VC handling" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.
/tmp/varad.01 has problems, exiting...
diff mbox series

Patch

diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
index 30dec019756b9..965cfcbd12f67 100644
--- a/arch/x86/Kbuild
+++ b/arch/x86/Kbuild
@@ -25,3 +25,5 @@  obj-y += platform/
 obj-y += net/
 
 obj-$(CONFIG_KEXEC_FILE) += purgatory/
+
+obj-$(CONFIG_X86_TESTS) += tests/
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 80b57e7f49477..6f63069fff972 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -282,3 +282,18 @@  endchoice
 config FRAME_POINTER
 	depends on !UNWINDER_ORC && !UNWINDER_GUESS
 	bool
+
+config X86_TESTS
+	bool "Tests for x86"
+	help
+	    This enables building the tests under arch/x86/tests.
+
+config AMD_SEV_ES_TEST_VC
+	bool "Test for AMD SEV-ES VC exception handling"
+	depends on AMD_MEM_ENCRYPT
+	depends on X86_TESTS
+	select FUNCTION_TRACER
+	select KPROBES
+	select KUNIT
+	help
+	  Enable KUnit-based testing for AMD SEV-ES #VC exception handling.
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 0f66682ac02a6..bf1c4dc525ac6 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -23,6 +23,13 @@  CFLAGS_REMOVE_head64.o = -pg
 CFLAGS_REMOVE_sev.o = -pg
 endif
 
+# AMD_SEV_ES_TEST_VC registers a kprobe by function name. IPA-SRA creates
+# function copies and renames them to have an .isra suffix, which breaks kprobes'
+# lookup. Build with -fno-ipa-sra for the test.
+ifdef CONFIG_AMD_SEV_ES_TEST_VC
+CFLAGS_sev.o	+= -fno-ipa-sra
+endif
+
 KASAN_SANITIZE_head$(BITS).o				:= n
 KASAN_SANITIZE_dumpstack.o				:= n
 KASAN_SANITIZE_dumpstack_$(BITS).o			:= n
diff --git a/arch/x86/tests/Makefile b/arch/x86/tests/Makefile
new file mode 100644
index 0000000000000..fa79c435d7843
--- /dev/null
+++ b/arch/x86/tests/Makefile
@@ -0,0 +1,3 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_AMD_SEV_ES_TEST_VC)	+= sev-es-test-vc.o
diff --git a/arch/x86/tests/sev-es-test-vc.c b/arch/x86/tests/sev-es-test-vc.c
new file mode 100644
index 0000000000000..98dc38572ed5d
--- /dev/null
+++ b/arch/x86/tests/sev-es-test-vc.c
@@ -0,0 +1,155 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 SUSE
+ *
+ * Author: Varad Gautam <varad.gautam@suse.com>
+ */
+
+#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)
+{
+	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)
+{
+	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_es_test_vc_init(struct kunit *test)
+{
+	int ret;
+
+	if (!sev_es_active()) {
+		kunit_info(test, "Not a SEV-ES guest. Skipping.");
+		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) {
+		kunit_info(test, "Could not register kretprobe. Skipping.");
+		goto out;
+	}
+
+	test->priv = kunit_kzalloc(test, sizeof(unsigned long), GFP_KERNEL);
+	if (!test->priv) {
+		ret = -ENOMEM;
+		kunit_info(test, "Could not allocate. Skipping.");
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+void sev_es_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));	\
+} 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_es_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_es_vc_test_suite = {
+	.name = "sev_es_test_vc",
+	.init = sev_es_test_vc_init,
+	.exit = sev_es_test_vc_exit,
+	.test_cases = sev_es_vc_testcases,
+};
+kunit_test_suite(sev_es_vc_test_suite);