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

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

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