diff mbox series

[v6,1/4] x86/tests: Add tests for AMD SEV-ES #VC handling

Message ID 20220318104646.8313-2-vkarasulli@suse.de (mailing list archive)
State New, archived
Headers show
Series x86/tests: Add tests for AMD SEV-ES #VC handling | expand

Commit Message

Vasant Karasulli March 18, 2022, 10:46 a.m. UTC 
Add Kconfig options for testing AMD SEV
 related features.

Signed-off-by: Vasant Karasulli <vkarasulli@suse.de>
---
 arch/x86/Kbuild          |  2 ++
 arch/x86/Kconfig.debug   | 16 ++++++++++++++++
 arch/x86/kernel/Makefile |  7 +++++++
 arch/x86/tests/Makefile  |  1 +
 4 files changed, 26 insertions(+)
 create mode 100644 arch/x86/tests/Makefile

--
2.32.0

Comments

Borislav Petkov April 8, 2022, 1:44 p.m. UTC | #1 
> Subject: Re: [PATCH v6 1/4] x86/tests: Add tests for AMD SEV-ES #VC handling

Your subject need to summarize each patch and not be the same for each
patch.

On Fri, Mar 18, 2022 at 11:46:43AM +0100, Vasant Karasulli wrote:
>  Add Kconfig options for testing AMD SEV
>  related features.
> 
> Signed-off-by: Vasant Karasulli <vkarasulli@suse.de>
> ---
>  arch/x86/Kbuild          |  2 ++
>  arch/x86/Kconfig.debug   | 16 ++++++++++++++++
>  arch/x86/kernel/Makefile |  7 +++++++
>  arch/x86/tests/Makefile  |  1 +
>  4 files changed, 26 insertions(+)
>  create mode 100644 arch/x86/tests/Makefile
> 
> diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
> index f384cb1a4f7a..90470c76866a 100644
> --- a/arch/x86/Kbuild
> +++ b/arch/x86/Kbuild
> @@ -26,5 +26,7 @@ obj-y += net/
> 
>  obj-$(CONFIG_KEXEC_FILE) += purgatory/
> 
> +obj-y += tests/
> +
>  # for cleaning
>  subdir- += boot tools
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index d3a6f74a94bd..e4f61af66816 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -279,3 +279,19 @@ endchoice
>  config FRAME_POINTER
>  	depends on !UNWINDER_ORC && !UNWINDER_GUESS
>  	bool
> +
> +config X86_TESTS
> +	bool "Tests for x86"

"x86 unit tests"

or so.

> +	help
> +	    This enables building the tests under arch/x86/tests.

This needs to explain more what "the tests" are and how people should
run them or at least there should be a pointer to a doc how. Running
tests should be trivial to mount so that everyone can run them. You want
as many people testing stuff as possible so the testing infra needs to
be easy to use.

For example, I have no clue how to run those tests.

Also, I have no clue why those tests are in arch/x86/tests/ and not
somewhere in tools/testing/selftests/x86/ or so.

All this stuff needs to be explained in the commit message.

Also, you should read

Documentation/process/submitting-patches.rst

first as there it is explained at length how a patch should look like.

> +
> +if X86_TESTS
> +config AMD_SEV_TEST_VC
> +	bool "Test for AMD SEV VC exception handling"
> +	depends on AMD_MEM_ENCRYPT
> +	select FUNCTION_TRACER
> +	select KPROBES
> +	select KUNIT
> +	help
> +	  Enable KUnit-based testing for AMD SEV #VC exception handling.
> +endif # X86_TESTS
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 6aef9ee28a39..69472a576909 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -24,6 +24,13 @@ CFLAGS_REMOVE_sev.o = -pg
>  CFLAGS_REMOVE_cc_platform.o = -pg
>  endif
> 
> +# AMD_SEV_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_TEST_VC

Why ifdef?

I think you want this to be enabled unconditionally since the VC tests
select KRPOBES.

> +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 000000000000..f66554cd5c45
> --- /dev/null
> +++ b/arch/x86/tests/Makefile
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0

Add that file with the next patch - this hunk is just silly as it is.

Thx.
Vasant Karasulli June 7, 2022, 9:45 a.m. UTC | #2 
On Fr 08-04-22 15:44:26, Borislav Petkov wrote:
> > Subject: Re: [PATCH v6 1/4] x86/tests: Add tests for AMD SEV-ES #VC handling
>
> > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> > index 6aef9ee28a39..69472a576909 100644
> > --- a/arch/x86/kernel/Makefile
> > +++ b/arch/x86/kernel/Makefile
> > @@ -24,6 +24,13 @@ CFLAGS_REMOVE_sev.o = -pg
> >  CFLAGS_REMOVE_cc_platform.o = -pg
> >  endif
> >
> > +# AMD_SEV_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_TEST_VC
>
> Why ifdef?
>
> I think you want this to be enabled unconditionally since the VC tests
> select KRPOBES.
>

VC tests added in this patch series depend on the configuration
option CONFIG_AMD_SEV_TEST_VC which in turn selects KPROBES.
I think compiler flag -fno-ipa-sra wouldn't be necessary if
configuration option CONFIG_AMD_SEV_TEST_VC is disabled.

> > +CFLAGS_sev.o	+= -fno-ipa-sra
> > +endif
> > +
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

--
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 mbox series

Patch

diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
index f384cb1a4f7a..90470c76866a 100644
--- a/arch/x86/Kbuild
+++ b/arch/x86/Kbuild
@@ -26,5 +26,7 @@  obj-y += net/

 obj-$(CONFIG_KEXEC_FILE) += purgatory/

+obj-y += tests/
+
 # for cleaning
 subdir- += boot tools
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index d3a6f74a94bd..e4f61af66816 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -279,3 +279,19 @@  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.
+
+if X86_TESTS
+config AMD_SEV_TEST_VC
+	bool "Test for AMD SEV VC exception handling"
+	depends on AMD_MEM_ENCRYPT
+	select FUNCTION_TRACER
+	select KPROBES
+	select KUNIT
+	help
+	  Enable KUnit-based testing for AMD SEV #VC exception handling.
+endif # X86_TESTS
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 6aef9ee28a39..69472a576909 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -24,6 +24,13 @@  CFLAGS_REMOVE_sev.o = -pg
 CFLAGS_REMOVE_cc_platform.o = -pg
 endif

+# AMD_SEV_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_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 000000000000..f66554cd5c45
--- /dev/null
+++ b/arch/x86/tests/Makefile
@@ -0,0 +1 @@ 
+# SPDX-License-Identifier: GPL-2.0