Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 17 Jun 2022 16:52:07 +0800
From:   Pengfei Xu <pengfei.xu@intel.com>
To:     Shuah Khan <skhan@linuxfoundation.org>
Cc:     linux-kselftest <linux-kselftest@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Heng Su <heng.su@intel.com>,
        Hansen Dave <dave.hansen@intel.com>,
        Luck Tony <tony.luck@intel.com>,
        Mehta Sohil <sohil.mehta@intel.com>,
        Chen Yu C <yu.c.chen@intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Borislav Petkov <bp@suse.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Bae Chang Seok <chang.seok.bae@intel.com>
Subject: Re: [PATCH v9 1/2] selftests/x86/xstate: Add xstate signal handling
 test for XSAVE feature
Message-ID: <YqxAt9flTQH5TYQh@xpf.sh.intel.com>
References: <cover.1655218544.git.pengfei.xu@intel.com>
 <f750fb183cf13e83cb2a10befacd3707879851ec.1655218544.git.pengfei.xu@intel.com>
 <86f9d913-1041-b52d-0221-3585473239d2@linuxfoundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <86f9d913-1041-b52d-0221-3585473239d2@linuxfoundation.org>
Precedence: bulk

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