Messages in this thread | | | Subject | Re: [PATCH v9 1/2] selftests/x86/xstate: Add xstate signal handling test for XSAVE feature | From | Shuah Khan <> | Date | Thu, 16 Jun 2022 17:24:20 -0600 |
| |
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.
> +} > + > +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.
> +static void allocate_xbuf(void) > +{ > + valid_xbuf = alloc_xbuf(xstate_size); > + compared_xbuf = alloc_xbuf(xstate_size); > +} > +
Probably another case of unnecessary function?
> +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.
thanks, -- Shuah
| |