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

[Shuah Khan <skhan@linuxfoundation.org>]

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