| Message ID | 20220318094532.7023-3-vkarasulli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/tests: Add tests for AMD SEV-ES #VC handling | expand |
The shortlog and changelog are all messed up. Ditto for the other patches in this series. On Fri, Mar 18, 2022, Vasant Karasulli wrote: > Signed-off-by: Vasant Karasulli <vkarasulli@suse.de> > --- > arch/x86/tests/Makefile | 2 + > arch/x86/tests/sev-test-vc.c | 114 +++++++++++++++++++++++++++++++++++ > 2 files changed, 116 insertions(+) > create mode 100644 arch/x86/tests/sev-test-vc.c ... > +int sev_es_test_vc_init(struct kunit *test) > +{ > + int ret; > + > + if (!cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) { > + 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(u64), GFP_KERNEL); Allocating 8 bytes and storing the pointer an 8-byte field is rather pointless :-) > + 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 check_op(kt, ec, op) \ > +do { \ > + struct kunit *t = (struct kunit *) kt; \ > + op; \ > + KUNIT_EXPECT_EQ(t, (typeof(ec)) ec, \ > + *((typeof(ec) *)(t->priv))); \ > +} while (0) > + > +static void sev_es_nae_cpuid(struct kunit *test) > +{ > + unsigned int cpuid_fn = 0x8000001f; > + > + check_op(test, SVM_EXIT_CPUID, native_cpuid_eax(cpuid_fn)); Are there plans to go beyond basic checks? Neat idea, but it seems like it will be prone to bitrot since it requires a somewhat esoteric setup and an opt-in config. And odds are very good that if the kernel can make it this far as an SEV-ES guest, it's gotten the basics right.
On Mi 06-04-22 01:22:55, Sean Christopherson wrote: > The shortlog and changelog are all messed up. Ditto for the other patches in this > series. I am really sorry about that. I had sent another mail with the same patch version with subject line corrected. https://lore.kernel.org/kvm/20220318104646.8313-1-vkarasulli@suse.de/T/#t > > On Fri, Mar 18, 2022, Vasant Karasulli wrote: > > Signed-off-by: Vasant Karasulli <vkarasulli@suse.de> > > --- > > arch/x86/tests/Makefile | 2 + > > arch/x86/tests/sev-test-vc.c | 114 +++++++++++++++++++++++++++++++++++ > > 2 files changed, 116 insertions(+) > > create mode 100644 arch/x86/tests/sev-test-vc.c > > ... > > > +int sev_es_test_vc_init(struct kunit *test) > > +{ > > + int ret; > > + > > + if (!cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) { > > + 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(u64), GFP_KERNEL); > > Allocating 8 bytes and storing the pointer an 8-byte field is rather pointless :-) Yes, I will remove this in the next version. > > > + 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 check_op(kt, ec, op) \ > > +do { \ > > + struct kunit *t = (struct kunit *) kt; \ > > + op; \ > > + KUNIT_EXPECT_EQ(t, (typeof(ec)) ec, \ > > + *((typeof(ec) *)(t->priv))); \ > > +} while (0) > > + > > +static void sev_es_nae_cpuid(struct kunit *test) > > +{ > > + unsigned int cpuid_fn = 0x8000001f; > > + > > + check_op(test, SVM_EXIT_CPUID, native_cpuid_eax(cpuid_fn)); > > Are there plans to go beyond basic checks? Neat idea, but it seems like it will > be prone to bitrot since it requires a somewhat esoteric setup and an opt-in config. > And odds are very good that if the kernel can make it this far as an SEV-ES guest, > it's gotten the basics right. I will definitely think about adding more checks and performing these checks early enough in the guest run. Thanks for your feedback. Thanks, Vasant Karasulli Kernel generalist www.suse.com<http://www.suse.com> [https://www.suse.com/assets/img/social-platforms-suse-logo.png]<http://www.suse.com/> SUSE - Open Source Solutions for Enterprise Servers & Cloud<http://www.suse.com/> Modernize your infrastructure with SUSE Linux Enterprise servers, cloud technology for IaaS, and SUSE's software-defined storage. www.suse.com
On Mi 06-04-22 01:22:55, Sean Christopherson wrote: > > + if (ret) { > > + kunit_info(test, "Could not register kretprobe. Skipping."); > > + goto out; > > + } > > + > > + test->priv = kunit_kzalloc(test, sizeof(u64), GFP_KERNEL); > > Allocating 8 bytes and storing the pointer an 8-byte field is rather pointless :-) > Actually it's necessary to allocate memory to test->priv before using according to https://www.kernel.org/doc/html/latest/dev-tools/kunit/tips.html > > + if (!test->priv) { > > + ret = -ENOMEM; > > + kunit_info(test, "Could not allocate. Skipping."); > > + goto out; > > + } > > + -- Vasant Karasulli Kernel generalist www.suse.com<http://www.suse.com> [https://www.suse.com/assets/img/social-platforms-suse-logo.png]<http://www.suse.com/> SUSE - Open Source Solutions for Enterprise Servers & Cloud<http://www.suse.com/> Modernize your infrastructure with SUSE Linux Enterprise servers, cloud technology for IaaS, and SUSE's software-defined storage. www.suse.com
On Wed, Jun 08, 2022, Vasant Karasulli wrote: > On Mi 06-04-22 01:22:55, Sean Christopherson wrote: > > > + if (ret) { > > > + kunit_info(test, "Could not register kretprobe. Skipping."); > > > + goto out; > > > + } > > > + > > > + test->priv = kunit_kzalloc(test, sizeof(u64), GFP_KERNEL); > > > > Allocating 8 bytes and storing the pointer an 8-byte field is rather pointless :-) > > > > Actually it's necessary to allocate memory to test->priv before using according to > https://www.kernel.org/doc/html/latest/dev-tools/kunit/tips.html If priv points at structure of some form, sure, but you're storing a simple value.
On Mi 08-06-22 14:35:55, Sean Christopherson wrote: > On Wed, Jun 08, 2022, Vasant Karasulli wrote: > > On Mi 06-04-22 01:22:55, Sean Christopherson wrote: > > > > + if (ret) { > > > > + kunit_info(test, "Could not register kretprobe. Skipping."); > > > > + goto out; > > > > + } > > > > + > > > > + test->priv = kunit_kzalloc(test, sizeof(u64), GFP_KERNEL); > > > > > > Allocating 8 bytes and storing the pointer an 8-byte field is rather pointless :-) > > > > > > > Actually it's necessary to allocate memory to test->priv before using according to > > https://www.kernel.org/doc/html/latest/dev-tools/kunit/tips.html > > If priv points at structure of some form, sure, but you're storing a simple value. Yes, I agree. The reason it was done this way I guess is that type of priv is a void pointer and storing a u64 value results in a compiler warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]. Thanks, Vasant Karasulli Kernel generalist www.suse.com<http://www.suse.com> [https://www.suse.com/assets/img/social-platforms-suse-logo.png]<http://www.suse.com/> SUSE - Open Source Solutions for Enterprise Servers & Cloud<http://www.suse.com/> Modernize your infrastructure with SUSE Linux Enterprise servers, cloud technology for IaaS, and SUSE's software-defined storage. www.suse.com
On Wed, Jun 08, 2022, Vasant Karasulli wrote: > On Mi 08-06-22 14:35:55, Sean Christopherson wrote: > > On Wed, Jun 08, 2022, Vasant Karasulli wrote: > > > On Mi 06-04-22 01:22:55, Sean Christopherson wrote: > > > > > + if (ret) { > > > > > + kunit_info(test, "Could not register kretprobe. Skipping."); > > > > > + goto out; > > > > > + } > > > > > + > > > > > + test->priv = kunit_kzalloc(test, sizeof(u64), GFP_KERNEL); > > > > > > > > Allocating 8 bytes and storing the pointer an 8-byte field is rather pointless :-) > > > > > > > > > > Actually it's necessary to allocate memory to test->priv before using according to > > > https://www.kernel.org/doc/html/latest/dev-tools/kunit/tips.html > > > > If priv points at structure of some form, sure, but you're storing a simple value. > > Yes, I agree. The reason it was done this way I guess is that type of priv is a > void pointer and storing a u64 value results in a compiler warning: > cast from pointer to integer of different size [-Wpointer-to-int-cast]. An intermediate cast to "unsigned long" should make that go away.
On Mi 08-06-22 19:57:45, Sean Christopherson wrote: > On Wed, Jun 08, 2022, Vasant Karasulli wrote: > > On Mi 08-06-22 14:35:55, Sean Christopherson wrote: > > > On Wed, Jun 08, 2022, Vasant Karasulli wrote: > > > > On Mi 06-04-22 01:22:55, Sean Christopherson wrote: > > > > > > + if (ret) { > > > > > > + kunit_info(test, "Could not register kretprobe. Skipping."); > > > > > > + goto out; > > > > > > + } > > > > > > + > > > > > > + test->priv = kunit_kzalloc(test, sizeof(u64), GFP_KERNEL); > > > > > > > > > > Allocating 8 bytes and storing the pointer an 8-byte field is rather pointless :-) > > > > > > > > > > > > > Actually it's necessary to allocate memory to test->priv before using according to > > > > https://www.kernel.org/doc/html/latest/dev-tools/kunit/tips.html > > > > > > If priv points at structure of some form, sure, but you're storing a simple value. > > > > Yes, I agree. The reason it was done this way I guess is that type of priv is a > > void pointer and storing a u64 value results in a compiler warning: > > cast from pointer to integer of different size [-Wpointer-to-int-cast]. > > An intermediate cast to "unsigned long" should make that go away. Yes, intermediate cast satisfied the compiler. Thanks for the tip. -- Vasant Karasulli Kernel generalist www.suse.com<http://www.suse.com> [https://www.suse.com/assets/img/social-platforms-suse-logo.png]<http://www.suse.com/> SUSE - Open Source Solutions for Enterprise Servers & Cloud<http://www.suse.com/> Modernize your infrastructure with SUSE Linux Enterprise servers, cloud technology for IaaS, and SUSE's software-defined storage. www.suse.com
diff --git a/arch/x86/tests/Makefile b/arch/x86/tests/Makefile index f66554cd5c45..4beca64bd2aa 100644 --- a/arch/x86/tests/Makefile +++ b/arch/x86/tests/Makefile @@ -1 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 + +obj-$(CONFIG_AMD_SEV_TEST_VC) += sev-test-vc.o diff --git a/arch/x86/tests/sev-test-vc.c b/arch/x86/tests/sev-test-vc.c new file mode 100644 index 000000000000..9d415b9708dc --- /dev/null +++ b/arch/x86/tests/sev-test-vc.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2021 SUSE + * + * Author: Varad Gautam <varad.gautam@suse.com> + */ + +#include <asm/cpufeature.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) + *((u64 *)test->priv) = ghcb->save.sw_exit_code; + + return 0; +} + +int sev_es_test_vc_init(struct kunit *test) +{ + int ret; + + if (!cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) { + 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(u64), 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 check_op(kt, ec, op) \ +do { \ + struct kunit *t = (struct kunit *) kt; \ + op; \ + KUNIT_EXPECT_EQ(t, (typeof(ec)) ec, \ + *((typeof(ec) *)(t->priv))); \ +} while (0) + +static void sev_es_nae_cpuid(struct kunit *test) +{ + unsigned int cpuid_fn = 0x8000001f; + + check_op(test, SVM_EXIT_CPUID, native_cpuid_eax(cpuid_fn)); +} + +static void sev_es_nae_wbinvd(struct kunit *test) +{ + check_op(test, SVM_EXIT_WBINVD, wbinvd()); +} + +static struct kunit_case sev_es_vc_testcases[] = { + KUNIT_CASE(sev_es_nae_cpuid), + KUNIT_CASE(sev_es_nae_wbinvd), + {} +}; + +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);
Signed-off-by: Vasant Karasulli <vkarasulli@suse.de> --- arch/x86/tests/Makefile | 2 + arch/x86/tests/sev-test-vc.c | 114 +++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 arch/x86/tests/sev-test-vc.c -- 2.32.0