Re: [RFC PATCH v6 1/2] selftests/x86: add xsave test related to nested signal handling

From: Pengfei Xu
Date: Wed Dec 22 2021 - 01:47:01 EST

Next message: Chen-Yu Tsai: "Re: [PATCH] scsi: ufs: ufs-mediatek : Fix NULL vs IS_ERR checking in ufs_mtk_init_va09_pwr_ctrl"
Previous message: Kalle Valo: "Re: [PATCH] ath10k: enable threaded napi on ath10k driver"
In reply to: Shuah Khan: "Re: [RFC PATCH v6 1/2] selftests/x86: add xsave test related to nested signal handling"
Next in thread: Pengfei Xu: "[RFC PATCH v6 2/2] selftests/x86: add xsave test related to process switching"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2021-12-21 at 10:50:08 -0700, Shuah Khan wrote:
> On 12/20/21 8:22 PM, Pengfei Xu wrote:
> > In order to ensure that content of FPU, SSE(XMM), AVX2(YMM), AVX512 opmask and
> > PKRU xstates in the same process is not affected by signal handling, this case
> > tests that:
> > 1. In nested signal processing, the signal handling will use each signal's
> > own xstates, and the xstates of the signal handling under test should
> > not be changed after previous nested signal handling is completed;
> > 2. The xstates content of the process should not change after the entire
> > nested signal handling.
> >
> > Process will change above mentioned xstates before signal handling test to
> > ensure that these xstates have been tested.
> >
> > Signed-off-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx> # compile issues during review
> > ---
> > tools/testing/selftests/x86/Makefile | 3 +-
> > tools/testing/selftests/x86/xsave_common.h | 380 ++++++++++++++++++
> > .../selftests/x86/xsave_signal_handle.c | 151 +++++++
> > 3 files changed, 533 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/x86/xsave_common.h
> > create mode 100644 tools/testing/selftests/x86/xsave_signal_handle.c
> >
> > +#include <unistd.h>
> > +#include <cpuid.h>
> > +
> > +#define XSAVE_MASK_FP (1 << XFEATURE_FP)
> > +#define XSAVE_MASK_XMM (1 << XFEATURE_SSE)
> > +#define XSAVE_MASK_YMM (1 << XFEATURE_YMM)
> > +#define XSAVE_MASK_OPMASK (1 << XFEATURE_OPMASK)
> > +#define XSAVE_MASK_PKRU (1 << XFEATURE_PKRU)
> > +#define XSAVE_TEST_MASK (XSAVE_MASK_FP | XSAVE_MASK_XMM | XSAVE_MASK_YMM \
> > + | XSAVE_MASK_OPMASK | XSAVE_MASK_PKRU)
> > +#define RESULT_PASS 0
> > +#define RESULT_FAIL 1
> > +#define RESULT_ERROR 3
> > +#define CHANGE 10
> > +#define NO_CHANGE 11
> > +
> > +/* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */
> > +#define X86_FEATURE_AVX2 (1U << 5) /* AVX2 instructions */
> > +#define X86_FEATURE_AVX512F (1U << 16) /* AVX-512 Foundation */
> > +
> > +/* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx) */
> > +#define X86_FEATURE_PKU (1U << 3) /* Protection Keys for Userspace */
> > +#define X86_FEATURE_OSPKE (1U << 4) /* OS Protection Keys Enable */
> > +
> > +#define XSTATE_CPUID (0xd)
>
> Match the define in arch/x86/include/asm/fpu/xstate.h
> and add a comment that this also defined in the above
> include.
>
> Same comment for any other duplicate defines in this file.
>
Thanks a lot for taking a look. I will add comments in next version.

> > +
> > +static int err_num, case_num, pass_num;
> > +
> > +/*
> > + * List of XSAVE features Linux knows about:
> > + */
> > +enum xfeature {
> > + XFEATURE_FP,
> > + XFEATURE_SSE,
> > + /*
> > + * Values above here are "legacy states".
> > + * Those below are "extended states".
> > + */
> > + XFEATURE_YMM,
> > + XFEATURE_BNDREGS,
> > + XFEATURE_BNDCSR,
> > + XFEATURE_OPMASK,
> > + XFEATURE_ZMM_Hi256,
> > + XFEATURE_Hi16_ZMM,
> > + XFEATURE_PT_UNIMPLEMENTED_SO_FAR,
> > + XFEATURE_PKRU,
> > + XFEATURE_PASID,
> > + XFEATURE_RSRVD_COMP_11,
> > + XFEATURE_RSRVD_COMP_12,
> > + XFEATURE_RSRVD_COMP_13,
> > + XFEATURE_RSRVD_COMP_14,
> > + XFEATURE_LBR,
> > +
>
> Remove the black line.
>
Thanks, will fix it.

> > + XFEATURE_MAX,
> > +};
> > +
> > +
..
> > +int compare_xsave_buf(unsigned char *buf0, unsigned char *buf1,
> > + int size, const char *test_name, int change)
> > +{
> > + int result_buf = RESULT_ERROR, result_resv_header = RESULT_ERROR;
> > +
> > + switch (change) {
> > + case CHANGE:
> > + if (show_buf_diff(buf0, buf1, size) == CHANGE)
> > + result_buf = RESULT_PASS;
> > + else {
> > + printf("%s FAIL: xsave content was same\n", test_name);
> > + result_buf = RESULT_FAIL;
> > + }
> > + break;
> > + case NO_CHANGE:
> > + if (show_buf_diff(buf0, buf1, size) == CHANGE) {
> > + printf("%s FAIL: xsave content changed\n", test_name);
> > + show_buf_diff(buf0, buf1, size);
> > + result_buf = RESULT_FAIL;
> > + } else
> > + result_buf = RESULT_PASS;
> > + break;
> > + default:
> > + printf("%s ERROR: invalid change:%d\n", test_name,
> > + change);
> > + break;
> > + }
> > +
> > + result_resv_header = check_xsave_reserved_header(buf0, buf1, size,
> > + test_name);
> > +
> > + return (result_buf || result_resv_header);
> > +}
> > +
>
> Please explain why all these 3 routines are needed?
> compare_xsave_buf(), show_buf_diff() and show_part_buf()
>
compare_xsave_buf() will compare xstate buf1 and buf2 difference, and check
both buf1 and buf2 xstate reserved header 63:16 should be all 00.
SDM mentioned that:
"Software should ensure that bytes 63:16 of the XSAVE header are 0"

If there is difference between buf1 and buf2, show_buf_diff() will be called
by compare_xsave_buf(), and find the offsets in all the different places.
show_part_buf() called by show_buf_diff() will print all the different
places between buf1 and buf2.

For example, change ymm0 and ymm1 xstates:
"
uint32_t ui32=0xf1f2;
xsave(xsave_buf0, XSAVE_TEST_MASK);
asm volatile ("vbroadcastss %0, %%ymm0" : : "m" (ui32));
asm volatile ("vbroadcastss %0, %%ymm1" : : "m" (ui32));
xsave(xsave_buf1, XSAVE_TEST_MASK);
compare_xsave_buf(xsave_buf0, xsave_buf1, xsave_size,
test_name1, NO_CHANGE);
"
It will only show different places as below:
00a0: 4f 3f 2f 1f 4f 3f 2f 1f 4f 3f 2f 1f 4f 3f 2f 1f -> f2 f1 00 00 f2 f1 00 00 f2 f1 00 00 f2 f1 00 00
00b0: 4f 3f 2f 1f 4f 3f 2f 1f 4f 3f 2f 1f 4f 3f 2f 1f -> f2 f1 00 00 f2 f1 00 00 f2 f1 00 00 f2 f1 00 00
0240: 4f 3f 2f 1f 4f 3f 2f 1f 4f 3f 2f 1f 4f 3f 2f 1f -> f2 f1 00 00 f2 f1 00 00 f2 f1 00 00 f2 f1 00 00
0250: 4f 3f 2f 1f 4f 3f 2f 1f 4f 3f 2f 1f 4f 3f 2f 1f -> f2 f1 00 00 f2 f1 00 00 f2 f1 00 00 f2 f1 00 00

> > +void check_result(int result, const char *test_name)
> > +{
> > + switch (result) {
> > + case RESULT_PASS:
> > + printf("[PASS]:%s\n", test_name);
> > + pass_num++;
> > + break;
> > + case RESULT_FAIL:
> > + printf("[FAIL]:%s\n", test_name);
> > + break;
> > + case RESULT_ERROR:
> > + printf("[ERROR]:%s\n", test_name);
> > + err_num++;
> > + break;
> > + default:
> > + printf("[ERROR]:%s:invalid result:%c\n",
> > + test_name, result);
> > + err_num++;
> > + break;
> > + }
> > +}
> > +
...
> > +
> > +/* Initialize FPU and push different values onto FPU register stack */
> > +static void set_fpu_reg(void)
> > +{
> > + uint32_t ui32;
> > + uint64_t ui64;
> > + float a = 0.12, b = 0.23;
> > +
> > + ui32 = 1;
> > + ui64 = 0xBAB00500FAB7;
> > +
>
> Just intialze at the time of define up above. Also add
> some commeng indicating what 0xBAB00500FAB7 is for.
>
Thanks for suggestion, yes, I will add comments for it.
It will fill 0xBAB00500FAB7 and some different values into FP xstate ST0/MM0-
ST7/MM7.

> > + asm volatile("finit");
> > + asm volatile("fldl %0" : : "m" (ui64));
> > + asm volatile("flds %0" : : "m" (ui32));
> > + ui64 += 0x93ABE13;
> > + asm volatile("fldl %0" : : "m" (ui64));
> > + ui64 += 0x93;
> > + asm volatile("fldl %0" : : "m" (ui64));
> > + asm volatile("flds %0" : : "m" (ui32));
> > + asm volatile("fldl %0" : : "m" (ui64));
> > + ui64 -= 0x21;
> > + asm volatile("fldl %0" : : "m" (ui64));
> > + asm volatile("flds %0" : : "m" (ui32));
> > + asm volatile("fldl %0" : : "m" (ui64));
> > + a = a * b;
> > +}
> > +
> > +void change_xstate(uint32_t ui32)
> > +{
> > + uint32_t pkey_change = 0xffffff00, ecx = 0, edx = 0;
>
> What does thsi value of pkey_change mean? What are you testing for?
>
pkey_change means PKRU(Memory Protection Keys Register for Userspace) xstate
will change to 0x00000000ffffff00.

At beginning of test, populate_xstate_regs() -> set_pkru_xstate(pkey) will
set PKRU xstate to 0x00000000fffffffc.

When signal handling test, signal handling function will use it's xstate,
and it changes PKRU xstate to 0x00000000ffffff00. After signal handling is
finished, process xstate should be resumed by xsaves/xrstors kernel schedule,
process xstate should be 0x00000000fffffffc, it's same as
populate_xstate_regs() -> set_pkru_xstate(pkey) pkru xstate.

> > +
> > + /* Push value onto FP stack */
> > + asm volatile("flds %0" : : "m" (ui32));
> > +
> > + /* change ymm0 to ymm15 with ui32 in 64bit kernel */
> > + if (cpu_has_avx2())
> > + set_avx2_ymm(ui32);
> > +
> > + /* change avx512 opmask k0 to k7 with ff */
> > + if (cpu_has_avx512f()) {
> > + asm volatile("kxnorb %%k0, %%k0, %%k0;" : : : "rcx");
> > + asm volatile("kxnorb %%k1, %%k1, %%k1;" : : : "rcx");
> > + asm volatile("kxnorb %%k2, %%k2, %%k2;" : : : "rcx");
> > + asm volatile("kxnorb %%k3, %%k3, %%k3;" : : : "rcx");
> > + asm volatile("kxnorb %%k4, %%k4, %%k4;" : : : "rcx");
> > + asm volatile("kxnorb %%k5, %%k5, %%k5;" : : : "rcx");
> > + asm volatile("kxnorb %%k6, %%k6, %%k6;" : : : "rcx");
> > + asm volatile("kxnorb %%k7, %%k7, %%k7;" : : : "rcx");
> > + }
> > +
> > + /* Change PKRU xstate */
> > + if (cpu_has_pkeys())
> > + asm volatile(".byte 0x0f,0x01,0xef\n\t"
> > + : : "a" (pkey_change), "c" (ecx), "d" (edx));
> > +}
> > +
> > +void check_cpu_capability(void)
> > +{
> > + if (!cpu_has_avx2())
> > + printf("[SKIP] No avx2 capability, skip avx2 part xstate.\n");
> > +
> > + if (!cpu_has_avx512f())
> > + printf("[SKIP] No avx512f capability, skip avx512f part xstate.\n");
> > +
> > + if (!cpu_has_pkeys())
> > + printf("[SKIP] No pkeys capability, skip pkru part xstate.\n");
> > +}
> > +
> > +void populate_xstate_regs(void)
> > +{
> > + uint32_t ui32_ymm_value = 0x1F2F3F4F, pkey = 0xfffffffc;
>
> Same comment here. What does this test do?
>
populate_xstate_regs() will populate FP, XMM(SSE), YMM(AVX2), AVX512_opmask
and PKRU xstates with values at the begining of the test. If there are all 00
in above xstates, we could not confirm kernel xsaves and xrstors the xstates
works well after signal handling and process switching.

> > +
> > + set_fpu_reg();
> > + set_avx2_ymm(ui32_ymm_value);
> > + set_avx512_opmask();
> > + set_pkru_xstate(pkey);
> > +}
> > diff --git a/tools/testing/selftests/x86/xsave_signal_handle.c b/tools/testing/selftests/x86/xsave_signal_handle.c
> > new file mode 100644
> > index 000000000000..e1754b279d00
> > --- /dev/null
> > +++ b/tools/testing/selftests/x86/xsave_signal_handle.c
> > @@ -0,0 +1,151 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * It's used for XSAVE test with signal handling.
> > + */
> > +
> > +#define _GNU_SOURCE
> > +
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <string.h>
> > +#include <signal.h>
> > +#include <unistd.h>
> > +#include <sched.h>
> > +#include <sys/wait.h>
> > +#include <sys/syscall.h>
> > +
> > +#include "xsave_common.h"
> > +
> > +void *aligned_alloc(size_t alignment, size_t size);
> > +static unsigned char *xsave_buf0, *xsave_buf1, *xsave_buf2, *xsave_buf3;
> > +static int result[2], xsave_size;
> > +
> > +static void usr1_handler(int signum, siginfo_t *info, void *__ctxp)
> > +{
> > + uint32_t ui32;
> > + const char *usr1_name = "USR1 signal xstates should same after USR2 signal";
> > +
> > + /* Xstate change in signal handling USR1 will not affect process xstates */
> > + ui32 = 0x1f2fffff;
> > + if (signum == SIGUSR1) {
> > + printf("SIGUSR1:0x%x changed fld & ymm0-15:0x%x\n",
> > + SIGUSR1, ui32);
> > + change_xstate(ui32);
> > + }
> > + /*
> > + * SIGUSR1 handler has independent XSAVE content, which is not affected by
> > + * the SIGUSR2 handler
> > + */
> > + xsave(xsave_buf2, XSAVE_TEST_MASK);
> > + kill(getpid(), SIGUSR2);
> > + xsave(xsave_buf3, XSAVE_TEST_MASK);
> > + result[0] = compare_xsave_buf(xsave_buf2, xsave_buf3, xsave_size, usr1_name,
> > + NO_CHANGE);
> > +}
> > +
> > +static void usr2_handler(int signum, siginfo_t *info, void *__ctxp)
> > +{
> > + uint32_t ui32;
> > +
> > + ui32 = 0x3f4fffff;
>
> Same here.
>
In nested signal handling for SIGUSR2, it changes signal handling own ymm0-15
xstates to 0x3f4fffff.
Process has set ymm0-15 to 0x1F2F3F4F at begining of test
populate_xstate_regs().
Process ymm xstate should not change after signal handling test.

> > + if (signum == SIGUSR2) {
> > + printf("SIGUSR2:0x%x changed fld & ymm0-15:0x%x\n",
> > + SIGUSR2, ui32);
> > + change_xstate(ui32);
> > + }
> > +}
> > +
...
> > +
> > +int main(void)
> > +{
> > + case_num = 2;
> > +
> > + prepare_environment();
> > + test_xsave_sig_handle();
> > +
> > + return 0;
> > +}
> >
>
> Please add information on what these tests are doing?
>
Thanks, yes, I will add test information to the head comments of the test
code in next version.

Thanks very much!
BR.

> thanks,
> -- Shuah
>

Next message: Chen-Yu Tsai: "Re: [PATCH] scsi: ufs: ufs-mediatek : Fix NULL vs IS_ERR checking in ufs_mtk_init_va09_pwr_ctrl"
Previous message: Kalle Valo: "Re: [PATCH] ath10k: enable threaded napi on ath10k driver"
In reply to: Shuah Khan: "Re: [RFC PATCH v6 1/2] selftests/x86: add xsave test related to nested signal handling"
Next in thread: Pengfei Xu: "[RFC PATCH v6 2/2] selftests/x86: add xsave test related to process switching"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]