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@xxxxxxxxx>
> > Signed-off-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> > ---
> > 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
