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

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

[Varad Gautam]

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 it relies on kprobes, the test does not cover NMI contexts.

Signed-off-by: Varad Gautam <varad.gautam@suse.com>
---
 arch/x86/Kconfig              |   9 +++
 arch/x86/kernel/Makefile      |   5 ++
 arch/x86/kernel/sev-test-vc.c | 142 ++++++++++++++++++++++++++++++++++
 3 files changed, 156 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"
+	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
+
 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..abb00a44ff25d
--- /dev/null
+++ b/arch/x86/kernel/sev-test-vc.c
@@ -0,0 +1,142 @@
+// 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_test_vc_init(struct kunit *test)
+{
+	int ret;
+
+	if (!sev_es_active()) {
+		pr_err("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 < 0) {
+		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));	\
+} while(0)
+
+static void sev_es_nae_cpuid(struct kunit *test)
+{
+	unsigned int cpuid_fn = 0x8000001f;
+
+	guarded_op(test, (unsigned long) SVM_EXIT_CPUID, native_cpuid_eax(cpuid_fn));
+}
+
+static void sev_es_nae_wbinvd(struct kunit *test)
+{
+	guarded_op(test, (unsigned long) SVM_EXIT_WBINVD, wbinvd());
+}
+
+static void sev_es_nae_msr(struct kunit *test)
+{
+	guarded_op(test, (unsigned long) SVM_EXIT_MSR, __rdmsr(MSR_IA32_TSC));
+}
+
+static void sev_es_nae_dr7_rw(struct kunit *test)
+{
+	guarded_op(test, (unsigned long) 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 = 0x1;
+
+	guarded_op(test, (unsigned long) SVM_EXIT_IOIO, inb(port));
+	guarded_op(test, (unsigned long) SVM_EXIT_IOIO, outb(val, port));
+	guarded_op(test, (unsigned long) SVM_EXIT_IOIO, insb(port, &val, sizeof(val)));
+	guarded_op(test, (unsigned long) SVM_EXIT_IOIO, outsb(port, &val, sizeof(val)));
+}
+
+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),
+	{}
+};
+
+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);
-- 
2.30.2

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

[Varad Gautam]

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.

Signed-off-by: Varad Gautam <varad.gautam@suse.com>
---
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"
+	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
+
 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@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_test_vc_init(struct kunit *test)
+{
+	int ret;
+
+	if (!sev_es_active()) {
+		pr_err("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 < 0) {
+		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));	\
+} 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);
-- 
2.30.2

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

[Tom Lendacky]

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@suse.com>
> ---
> 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@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)

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);
> 

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

[Borislav Petkov]

On Tue, Jun 01, 2021 at 11:41:31AM -0500, Tom Lendacky wrote:
> > +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.

Or even better - in a comment above it. Commit messages are harder to
find when time passes and other commits touch this line and you have to
go and do git archeology just to figure out why this was done...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v3] x86: Add a test for AMD SEV-ES guest #VC handling

[Varad Gautam]

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.

Signed-off-by: Varad Gautam <varad.gautam@suse.com>
---
 arch/x86/Kconfig                 |   9 ++
 arch/x86/kernel/Makefile         |   8 ++
 arch/x86/kernel/sev-es-test-vc.c | 155 +++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)
 create mode 100644 arch/x86/kernel/sev-es-test-vc.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0045e1b441902..85b8ac450ba56 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_SEV_ES_TEST_VC
+	bool "Test for AMD SEV-ES VC exception handling."
+	depends on AMD_MEM_ENCRYPT
+	select FUNCTION_TRACER
+	select KPROBES
+	select KUNIT
+	help
+	  Enable KUnit-based testing for AMD SEV-ES #VC exception 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..1632d6156be6e 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
@@ -149,6 +156,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_SEV_ES_TEST_VC)	+= sev-es-test-vc.o
 ###
 # 64 bit specific files
 ifeq ($(CONFIG_X86_64),y)
diff --git a/arch/x86/kernel/sev-es-test-vc.c b/arch/x86/kernel/sev-es-test-vc.c
new file mode 100644
index 0000000000000..98dc38572ed5d
--- /dev/null
+++ b/arch/x86/kernel/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);
-- 
2.30.2

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

[Varad Gautam]

On 6/1/21 6:41 PM, Tom Lendacky wrote:
> 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@suse.com>
>> ---
>> 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.
> 

Ack, changed naming everywhere to indicate that this is specific to SEV-ES.

> 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.
> 

I added a comment here, ipa-sra adds an .isra.* suffix to function names,
which breaks kprobes' lookup for "sev_es_ghcb_hv_call". The alternative here
is to do an inexact search within kallsyms, but -fno-ipa-sra is simpler and
assures that register_kprobes finds the right address.

>> +
>>  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@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)
> 
> 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?
> 

Changed the prints to kunit_info(), which appends a per-test prefix.

>> +		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?
> 

Yes, "if (ret) {" is better. Changed.

Thanks,
Varad

>> +		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);
>>
> 

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany

HRB 36809, AG Nürnberg
Geschäftsführer: Felix Imendörffer

[PATCH v3] x86: Add a test for AMD SEV-ES guest #VC handling

[Varad Gautam]

From: Varad Gautam <varad.gautam@suse.com>

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.

Signed-off-by: Varad Gautam <varad.gautam@suse.com>
---
 arch/x86/Kconfig                 |   9 ++
 arch/x86/kernel/Makefile         |   8 ++
 arch/x86/kernel/sev-es-test-vc.c | 155 +++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)
 create mode 100644 arch/x86/kernel/sev-es-test-vc.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0045e1b441902..85b8ac450ba56 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_SEV_ES_TEST_VC
+	bool "Test for AMD SEV-ES VC exception handling."
+	depends on AMD_MEM_ENCRYPT
+	select FUNCTION_TRACER
+	select KPROBES
+	select KUNIT
+	help
+	  Enable KUnit-based testing for AMD SEV-ES #VC exception 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..1632d6156be6e 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
@@ -149,6 +156,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_SEV_ES_TEST_VC)	+= sev-es-test-vc.o
 ###
 # 64 bit specific files
 ifeq ($(CONFIG_X86_64),y)
diff --git a/arch/x86/kernel/sev-es-test-vc.c b/arch/x86/kernel/sev-es-test-vc.c
new file mode 100644
index 0000000000000..98dc38572ed5d
--- /dev/null
+++ b/arch/x86/kernel/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);
-- 
2.30.2

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

[Joerg Roedel]

On Wed, Jun 02, 2021 at 04:14:47PM +0200, Varad Gautam wrote:
> From: Varad Gautam <varad.gautam@suse.com>
> 
> 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.
> 
> Signed-off-by: Varad Gautam <varad.gautam@suse.com>
> ---
>  arch/x86/Kconfig                 |   9 ++
>  arch/x86/kernel/Makefile         |   8 ++
>  arch/x86/kernel/sev-es-test-vc.c | 155 +++++++++++++++++++++++++++++++

This looks good to me except for the small comment below, thanks Varad.
I ran it in an SEV-ES guest and I am seeing the test results in dmesg.
Only thing I am missing is a 'rep movs' test for MMIO, but that can be
added later, so

Tested-by: Joerg Roedel <jroedel@suse.de>

Btw, should we create a separate directory for such tests like
/arch/x86/tests/ or something along those lines?

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 0045e1b441902..85b8ac450ba56 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_SEV_ES_TEST_VC
> +	bool "Test for AMD SEV-ES VC exception handling."
> +	depends on AMD_MEM_ENCRYPT
> +	select FUNCTION_TRACER
> +	select KPROBES
> +	select KUNIT
> +	help
> +	  Enable KUnit-based testing for AMD SEV-ES #VC exception handling.
> +

I think this should be in arch/x86/Kconfig.debug.

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

[Varad Gautam]

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

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);
-- 
2.30.2

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

[Varad Gautam]

On 6/9/21 4:50 PM, Joerg Roedel wrote:
> On Wed, Jun 02, 2021 at 04:14:47PM +0200, Varad Gautam wrote:
>> From: Varad Gautam <varad.gautam@suse.com>
>>
>> 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.
>>
>> Signed-off-by: Varad Gautam <varad.gautam@suse.com>
>> ---
>>  arch/x86/Kconfig                 |   9 ++
>>  arch/x86/kernel/Makefile         |   8 ++
>>  arch/x86/kernel/sev-es-test-vc.c | 155 +++++++++++++++++++++++++++++++
> 
> This looks good to me except for the small comment below, thanks Varad.
> I ran it in an SEV-ES guest and I am seeing the test results in dmesg.
> Only thing I am missing is a 'rep movs' test for MMIO, but that can be
> added later, so
> 
> Tested-by: Joerg Roedel <jroedel@suse.de>
> 
> Btw, should we create a separate directory for such tests like
> /arch/x86/tests/ or something along those lines?
> 

Thanks, I've sent out a v4 with the test moved to arch/x86/tests. This is
now hidden behind a CONFIG_X86_TESTS, where more stuff can be plugged in.

>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 0045e1b441902..85b8ac450ba56 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_SEV_ES_TEST_VC
>> +	bool "Test for AMD SEV-ES VC exception handling."
>> +	depends on AMD_MEM_ENCRYPT
>> +	select FUNCTION_TRACER
>> +	select KPROBES
>> +	select KUNIT
>> +	help
>> +	  Enable KUnit-based testing for AMD SEV-ES #VC exception handling.
>> +
> 
> I think this should be in arch/x86/Kconfig.debug.
> 
Ack, also moved in v4.

Varad

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany

HRB 36809, AG Nürnberg
Geschäftsführer: Felix Imendörffer

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

[Borislav Petkov]

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...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette