Re: [PATCH v9 1/2] selftests/x86/xstate: Add xstate signal handling test for XSAVE feature

Hi Shuah,

On 2022-06-16 at 17:24:20 -0600, Shuah Khan wrote:

On 6/14/22 9:11 AM, Pengfei Xu wrote:

...

The XSAVE feature set supports the saving and restoring of xstate components.

In order to ensure that XSAVE works correctly, add XSAVE most basic signal handling test for XSAVE architecture functionality, this patch tests "FP, SSE(XMM), AVX2(YMM), AVX512_OPMASK/AVX512_ZMM_Hi256/AVX512_Hi16_ZMM and PKRU" xstates with the following: The contents of these xstates in the process should not change after the signal handling.

[ Dave Hansen; Chang S. Bae: bunches of cleanups ]

Reviewed-by: Chang S. Bae chang.seok.bae@intel.com Signed-off-by: Pengfei Xu pengfei.xu@intel.com

---

tools/testing/selftests/x86/.gitignore | 1 + tools/testing/selftests/x86/Makefile | 11 +- tools/testing/selftests/x86/xstate.c | 215 +++++++++++++++ tools/testing/selftests/x86/xstate.h | 266 +++++++++++++++++++ tools/testing/selftests/x86/xstate_helpers.c | 160 +++++++++++ tools/testing/selftests/x86/xstate_helpers.h | 8 + 6 files changed, 659 insertions(+), 2 deletions(-) create mode 100644 tools/testing/selftests/x86/xstate.c create mode 100644 tools/testing/selftests/x86/xstate.h create mode 100644 tools/testing/selftests/x86/xstate_helpers.c create mode 100644 tools/testing/selftests/x86/xstate_helpers.h

diff --git a/tools/testing/selftests/x86/.gitignore b/tools/testing/selftests/x86/.gitignore index 1aaef5bf119a..68951ceefe30 100644 --- a/tools/testing/selftests/x86/.gitignore +++ b/tools/testing/selftests/x86/.gitignore @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only *_32 *_64 +*.o single_step_syscall sysret_ss_attrs syscall_nt diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile index 0388c4d60af0..49a6d78e0831 100644 --- a/tools/testing/selftests/x86/Makefile +++ b/tools/testing/selftests/x86/Makefile @@ -18,7 +18,7 @@ TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \ test_FCMOV test_FCOMI test_FISTTP \ vdso_restorer TARGETS_C_64BIT_ONLY := fsgsbase sysret_rip syscall_numbering \

• 	corrupt_xstate_header amx
  

• 	corrupt_xstate_header amx xstate
  

  # Some selftests require 32bit support enabled also on 64bit systems TARGETS_C_32BIT_NEEDED := ldt_gdt ptrace_syscall

@@ -69,7 +69,7 @@ all_32: $(BINARIES_32) all_64: $(BINARIES_64) -EXTRA_CLEAN := $(BINARIES_32) $(BINARIES_64) +EXTRA_CLEAN := $(BINARIES_32) $(BINARIES_64) *.o $(BINARIES_32): $(OUTPUT)/%_32: %.c helpers.h $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl -lm @@ -109,3 +109,10 @@ $(OUTPUT)/test_syscall_vdso_32: thunks_32.S # state. $(OUTPUT)/check_initial_reg_state_32: CFLAGS += -Wl,-ereal_start -static $(OUTPUT)/check_initial_reg_state_64: CFLAGS += -Wl,-ereal_start -static

+# xstate_64 is special: it needs xstate_helpers.o to prevent GCC from +# generating any FP code by mistake and stdlib.h can't be used due to +# "-mno-sse" parameter, so compile xstate_64 with the code file xstate.c +# which can use stdlib.h and xstate_helpers.o which cannot use stdlib.h +xstate_helpers.o: CFLAGS += -mno-sse -mno-mmx -mno-sse2 -mno-avx -mno-pku +$(OUTPUT)/xstate_64: xstate_helpers.o diff --git a/tools/testing/selftests/x86/xstate.c b/tools/testing/selftests/x86/xstate.c new file mode 100644 index 000000000000..05dabb4733a0 --- /dev/null +++ b/tools/testing/selftests/x86/xstate.c @@ -0,0 +1,215 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • xstate.c - tests XSAVE feature with fork and signal handling.
• • The XSAVE feature set supports the saving and restoring of state components.
• • It tests "FP, SSE(XMM), AVX2(YMM), AVX512_OPMASK/AVX512_ZMM_Hi256/
• • AVX512_Hi16_ZMM and PKRU parts" xstates with following cases:
• • 1. The contents of these xstates in the process should not change after the
• • signal handling.
• • 2. The contents of these xstates in the child process should be the same as
• • the contents of the xstate in the parent process after the fork syscall.
• • 3. The contents of xstates in the parent process should not change after
• • the context switch.
• • The regions and reserved bytes of the components tested for XSAVE feature
• • are as follows:
• • x87(FP)/SSE (0 - 159 bytes)
• • SSE(XMM part) (160-415 bytes)
• • Reserved (416-511 bytes)
• • Header_used (512-527 bytes; XSTATE BV(bitmap vector) mask:512-519 bytes)
• • Header_reserved(528-575 bytes must be 00)
• • YMM (Offset:CPUID.(EAX=0D,ECX=2).EBX Size:CPUID(EAX=0D,ECX=2).EAX)
• • AVX512_OPMASK (Offset:CPUID.(EAX=0D,ECX=5).EBX Size:CPUID(EAX=0D,ECX=5).EAX)
• • ZMM_Hi256 (Offset:CPUID.(EAX=0D,ECX=6).EBX Size:CPUID(EAX=0D,ECX=6).EAX)
• • Hi16_ZMM (Offset:CPUID.(EAX=0D,ECX=7).EBX Size:CPUID(EAX=0D,ECX=7).EAX)
• • PKRU (Offset:CPUID.(EAX=0D,ECX=9).EBX Size:CPUID(EAX=0D,ECX=9).EAX)
• */

+#define _GNU_SOURCE +#include <err.h> +#include <stdio.h> +#include <stdint.h> +#include <string.h> +#include <signal.h> +#include <unistd.h> +#include <sched.h> +#include <stdbool.h> +#include <sys/wait.h> +#include <sys/syscall.h> +#include <cpuid.h> +#include <malloc.h> +#include <stdlib.h>

+#include "xstate.h" +#include "xstate_helpers.h" +#include "../kselftest.h"

+#define NUM_TESTS 1 +#define xstate_test_array_init(idx, init_opt, fill_opt) \

• do { \

• xstate_tests[idx].init = init_opt;	\
  

• xstate_tests[idx].fill_xbuf = fill_opt;	\
  

• } while (0)

+static struct xsave_buffer *valid_xbuf, *compared_xbuf; +static struct xstate_test xstate_tests[XFEATURE_MAX]; +static uint32_t xstate_size;

+static bool xstate_in_test(int xfeature_num) +{

• return !!(xstate_info.mask & (1 << xfeature_num));

This is used just one. Why do you need a function for this? Also please don't use !! - it is just very hard to read.

Ok, this function is only used once, will remove it. Thanks!

...

+}

+static struct xsave_buffer *alloc_xbuf(uint32_t buf_size) +{

• struct xsave_buffer *xbuf;
• /* XSAVE buffer should be 64B-aligned. */
• xbuf = aligned_alloc(64, buf_size);
• if (!xbuf)

• ksft_exit_fail_msg("aligned_alloc() failed.\n");
  

• return xbuf;

+}

+static void free_xbuf(void) +{

• free(valid_xbuf);
• free(compared_xbuf);

+}

Again this is called just one. WHy do you need a speacial function for this. Please don't fragment code without a good reason.

Will fix it, thanks!

...

+static void allocate_xbuf(void) +{

• valid_xbuf = alloc_xbuf(xstate_size);
• compared_xbuf = alloc_xbuf(xstate_size);

+}

Probably another case of unnecessary function?

Will fix it, thanks!

...

+static void show_test_xfeatures(void) +{

• uint32_t i;
• const char *feature_name;
• ksft_print_msg("[NOTE] Test following xstates with mask:%lx.\n",

•        xstate_info.mask);
  

• for (i = 0; i < XFEATURE_MAX; i++) {

• if (!xstate_in_test(i))
  

• 	continue;
  

• feature_name = xfeature_names[i];
  

• ksft_print_msg("[NOTE] XSAVE feature num %02d: '%s'.\n", i,
  

• 	       feature_name);
  

• }

+}

+static inline void set_xstatebv(struct xsave_buffer *buffer, uint64_t bv) +{

• /* XSTATE_BV is at the beginning of xstate header. */
• *(uint64_t *)(&buffer->header) = bv;

+}

Okay - if you have a function - I want to see it called at least 2 times. Having so many little function breaks up the code for no good reason.

Let's fix these first and in both patches.

Sure, I will collect all the comments and fix them in next version.

Thanks! -- Pengfei

thanks, -- Shuah