lkml.org 
[lkml]   [2022]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v9 1/2] selftests/x86/xstate: Add xstate signal handling test for XSAVE feature
From
Date
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

\
 
 \ /
  Last update: 2022-06-17 01:25    [W:4.127 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site