The XSAVE feature set supports the saving and restoring of state components such as FPU, which is used for process context switching. In order to ensure that XSAVE works correctly, add XSAVE basic test for XSAVE architecture functionality.
This patch set tests XSAVE/XRSTOR instructions on x86 platforms and verify if the XSAVE/XRSTOR works correctly during signal handling.
Cases such as signal handling, process creation, other xstate(except FPU) tests for XSAVE check, etc. will be added to the Linux kernel self-test. If appropriate, it is even planned to add the [1] mentioned XSAVE issues reproduce and some XSAVE anomaly tests to the kernel self-test.
[1]: https://lore.kernel.org/lkml/0000000000004c453905c30f8334@google.com/
Pengfei Xu (2): selftests/xsave: test basic XSAVE architecture functionality selftests/xsave: add xsave test during signal handling
tools/testing/selftests/Makefile | 1 + tools/testing/selftests/xsave/.gitignore | 3 + tools/testing/selftests/xsave/Makefile | 6 + tools/testing/selftests/xsave/xsave_common.h | 246 ++++++++++++++++++ .../selftests/xsave/xsave_instruction.c | 83 ++++++ .../selftests/xsave/xsave_signal_handle.c | 184 +++++++++++++ 6 files changed, 523 insertions(+) create mode 100644 tools/testing/selftests/xsave/.gitignore create mode 100644 tools/testing/selftests/xsave/Makefile create mode 100644 tools/testing/selftests/xsave/xsave_common.h create mode 100644 tools/testing/selftests/xsave/xsave_instruction.c create mode 100644 tools/testing/selftests/xsave/xsave_signal_handle.c
The XSAVE feature set supports the saving and restoring of state components such as FPU, which is used for process context switching.
In order to ensure that XSAVE feature works correctly, this case tests the execution of xsave/xrstor in user space: show and tests the content change for xsave; the contents of xrstor and xsave should same on x86 platforms. This case is the basis for XSAVE feature testing.
Signed-off-by: Pengfei Xu pengfei.xu@intel.com Reported-by: kernel test robot lkp@intel.com # compile issues during review --- tools/testing/selftests/Makefile | 1 + tools/testing/selftests/xsave/.gitignore | 2 + tools/testing/selftests/xsave/Makefile | 6 + tools/testing/selftests/xsave/xsave_common.h | 246 ++++++++++++++++++ .../selftests/xsave/xsave_instruction.c | 83 ++++++ 5 files changed, 338 insertions(+) create mode 100644 tools/testing/selftests/xsave/.gitignore create mode 100644 tools/testing/selftests/xsave/Makefile create mode 100644 tools/testing/selftests/xsave/xsave_common.h create mode 100644 tools/testing/selftests/xsave/xsave_instruction.c
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index fb010a35d61a..4c0ddb8228e1 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -73,6 +73,7 @@ TARGETS += user TARGETS += vDSO TARGETS += vm TARGETS += x86 +TARGETS += xsave TARGETS += zram #Please keep the TARGETS list alphabetically sorted # Run "make quicktest=1 run_tests" or diff --git a/tools/testing/selftests/xsave/.gitignore b/tools/testing/selftests/xsave/.gitignore new file mode 100644 index 000000000000..00b9970360c4 --- /dev/null +++ b/tools/testing/selftests/xsave/.gitignore @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0-only +xsave_instruction diff --git a/tools/testing/selftests/xsave/Makefile b/tools/testing/selftests/xsave/Makefile new file mode 100644 index 000000000000..dafdb0abdeb3 --- /dev/null +++ b/tools/testing/selftests/xsave/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0-only +CFLAGS := -g -Wall -mxsave -O2 + +TEST_GEN_PROGS := xsave_instruction + +include ../lib.mk diff --git a/tools/testing/selftests/xsave/xsave_common.h b/tools/testing/selftests/xsave/xsave_common.h new file mode 100644 index 000000000000..c555f9d289a3 --- /dev/null +++ b/tools/testing/selftests/xsave/xsave_common.h @@ -0,0 +1,246 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <stdio.h> +#include <stdint.h> +#include <stdlib.h> +#include <x86intrin.h> +#include <string.h> +#include <signal.h> +#include <unistd.h> +#include "../kselftest.h" + +#ifdef __i386__ +#define XSAVE _xsave +#else +#define XSAVE _xsave64 +#endif + +#ifdef __i386__ +#define XRSTOR _xrstor +#else +#define XRSTOR _xrstor64 +#endif + +#define SAVE_MASK 0xffffffffffffffff +#define RESULT_PASS 0 +#define RESULT_FAIL 1 +#define RESULT_ERROR 3 +#define CHANGE 10 +#define NO_CHANGE 11 + +/* Copied from Linux kernel */ +static inline void native_cpuid(unsigned int *eax, unsigned int *ebx, + unsigned int *ecx, unsigned int *edx) +{ + /* ecx is often an input as well as an output. */ + asm volatile("cpuid" + : "=a" (*eax), + "=b" (*ebx), + "=c" (*ecx), + "=d" (*edx) + : "0" (*eax), "2" (*ecx) + : "memory"); +} + +void execution_failed(char *reason) +{ + ksft_test_result_xfail("%s", reason); + ksft_exit_fail(); +} + +int get_xsave_size(void) +{ + unsigned int eax, ebx, ecx, edx; + + eax = 0x0d; + ebx = 0; + ecx = 0; + edx = 0; + native_cpuid(&eax, &ebx, &ecx, &edx); + + return (int)ecx; +} + +void dump_buffer(unsigned char *buf, int size) +{ + int i, j; + + printf("xsave size = %d (%03xh)\n", size, size); + + for (i = 0; i < size; i += 16) { + printf("%04x: ", i); + + for (j = i; ((j < i + 16) && (j < size)); j++) + printf("%02x ", buf[j]); + printf("\n"); + } +} + +void show_part_buf(unsigned char *buf0, unsigned char *buf1, int start, + int size) +{ + int c; + + printf("%04x: ", start); + for (c = start; ((c < start + 16) && (c < size)); c++) + printf("%02x ", buf0[c]); + printf(" -> "); + for (c = start; ((c < start + 16) && (c < size)); c++) + printf("%02x ", buf1[c]); + printf("\n"); +} + +int show_buf_diff(unsigned char *buf0, unsigned char *buf1, int size) +{ + int a, b, result_buf = RESULT_PASS; + + for (a = 0; a < size; a += 16) { + /* SDM "XSAVE Area": XSAVE feature set does not use bytes 511:416 */ + if ((a >= 416) && (a <= 511)) + continue; + + for (b = a; ((b < a + 16) && (b < size)); b++) { + if (buf0[b] != buf1[b]) { + show_part_buf(buf0, buf1, a, size); + result_buf = RESULT_FAIL; + break; + } + } + } + + return result_buf; +} + +int check_xsave_reserved_header(unsigned char *buf0, + unsigned char *buf1, int size, const char *test_name) +{ + int a, b, result_resv_header = RESULT_PASS; + + /* SDM "Form of XRSTOR": Bytes 63:16 of the XSAVE header should 0 */ + for (a = 528; a < 576 ; a += 16) { + for (b = a; ((b < a + 16) && (b < size)); b++) { + if ((buf0[b] != 0) || (buf1[b] != 0)) { + ksft_print_msg("%s FAIL: buf0[%d]:%d or buf1[%d]:%d not 0\n", + test_name, b, buf0[b], b, buf1[b]); + show_part_buf(buf0, buf1, a, size); + result_resv_header = RESULT_FAIL; + break; + } + } + } + + return result_resv_header; +} + +int check_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)) + result_buf = RESULT_PASS; + else { + ksft_print_msg("%s FAIL: xsave content was same\n", test_name); + result_buf = RESULT_FAIL; + } + break; + case NO_CHANGE: + if (show_buf_diff(buf0, buf1, size)) { + ksft_print_msg("%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: + ksft_test_result_error("%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); +} + +void check_result(int result, const char *test_name) +{ + switch (result) { + case RESULT_PASS: + ksft_test_result_pass("%s PASS\n", test_name); + break; + case RESULT_FAIL: + ksft_test_result_fail("%s FAIL\n", test_name); + break; + case RESULT_ERROR: + ksft_test_result_fail("%s ERROR\n", test_name); + break; + default: + ksft_test_result_error("%s ERROR: invalid result:%c\n", + test_name, result); + break; + } +} + +void populate_fpu_regs(void) +{ + uint32_t ui32; + uint64_t ui64; + + ui32 = 1; + ui64 = 0xBAB00500FAB7; + + /* Initialize FPU and push different values onto FPU register stack: */ + 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)); + + /* Fill each remaining YMM register with a different value: */ + asm volatile ("vbroadcastss %0, %%ymm0" : : "m" (ui32)); + ui32 = 0xFAFBABAF; + asm volatile ("vbroadcastss %0, %%ymm1" : : "m" (ui32)); + ui32 -= 0xA; + asm volatile ("vbroadcastss %0, %%ymm2" : : "m" (ui32)); + ui32 -= 0xB; + asm volatile ("vbroadcastss %0, %%ymm3" : : "m" (ui32)); + ui32 -= 0x3; + asm volatile ("vbroadcastss %0, %%ymm4" : : "m" (ui32)); + ui32 += 0xA; + asm volatile ("vbroadcastss %0, %%ymm5" : : "m" (ui32)); + ui32 -= 0x7; + asm volatile ("vbroadcastss %0, %%ymm6" : : "m" (ui32)); + ui32 -= 0xABABA; + asm volatile ("vbroadcastss %0, %%ymm7" : : "m" (ui32)); + + #ifndef __i386__ + ui32 += 0xF7; + asm volatile ("vbroadcastss %0, %%ymm8" : : "m" (ui32)); + ui32 -= 0x7; + asm volatile ("vbroadcastss %0, %%ymm9" : : "m" (ui32)); + ui32 += 0x2; + asm volatile ("vbroadcastss %0, %%ymm10" : : "m" (ui32)); + ui32 += 0xD; + asm volatile ("vbroadcastss %0, %%ymm11" : : "m" (ui32)); + ui32 -= 0x4; + asm volatile ("vbroadcastss %0, %%ymm12" : : "m" (ui32)); + ui32 -= 0xDD; + asm volatile ("vbroadcastss %0, %%ymm13" : : "m" (ui32)); + ui32 -= 0xABD; + asm volatile ("vbroadcastss %0, %%ymm14" : : "m" (ui32)); + ui32 += 0xBEBABF456; + asm volatile ("vbroadcastss %0, %%ymm15" : : "m" (ui32)); + #endif +} diff --git a/tools/testing/selftests/xsave/xsave_instruction.c b/tools/testing/selftests/xsave/xsave_instruction.c new file mode 100644 index 000000000000..2b610101511d --- /dev/null +++ b/tools/testing/selftests/xsave/xsave_instruction.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Test kernel support for XSAVE-managed features. + */ + +#include <stdio.h> +#include <stdint.h> +#include <stdlib.h> +#include <x86intrin.h> +#include <string.h> +#include <signal.h> +#include <unistd.h> + +#include "../kselftest.h" +#include "xsave_common.h" + +static unsigned char *xsave_buf0, *xsave_buf1; + +static void set_ymm0_reg(uint32_t ui32) +{ + asm volatile ("vbroadcastss %0, %%ymm0" : : "m" (ui32)); +} + +static void dump_xsave_content(int xsave_size) +{ + XSAVE(xsave_buf0, SAVE_MASK); + dump_buffer(xsave_buf0, xsave_size); + ksft_print_msg("Entire contents of XSAVE is as above\n"); +} + +static void test_xsave_ymm_change(int xsave_size) +{ + const char *test_name = "xsave test after ymm change"; + uint32_t ui32_set = 0x1234, ui32_change = 0x5678; + int result = RESULT_ERROR; + + set_ymm0_reg(ui32_set); + XSAVE(xsave_buf0, SAVE_MASK); + set_ymm0_reg(ui32_change); + XSAVE(xsave_buf1, SAVE_MASK); + result = check_xsave_buf(xsave_buf0, xsave_buf1, xsave_size, test_name, + CHANGE); + check_result(result, test_name); +} + +static void test_xsave_xrstor(int xsave_size) +{ + const char *test_name = "xsave after xrstor test"; + int result = RESULT_ERROR; + + XSAVE(xsave_buf0, SAVE_MASK); + XRSTOR(xsave_buf0, SAVE_MASK); + XSAVE(xsave_buf1, SAVE_MASK); + result = check_xsave_buf(xsave_buf0, xsave_buf1, xsave_size, test_name, + NO_CHANGE); + check_result(result, test_name); +} + +int main(void) +{ + int xsave_size; + + ksft_print_header(); + ksft_set_plan(2); + + xsave_size = get_xsave_size(); + /* SDM XSAVE: misalignment to a 64-byte boundary will result in #GP */ + xsave_buf0 = aligned_alloc(64, xsave_size); + if (!xsave_buf0) + execution_failed("aligned_alloc xsave_buf0 failed\n"); + xsave_buf1 = aligned_alloc(64, xsave_size); + if (!xsave_buf1) + execution_failed("aligned_alloc xsave_buf1 failed\n"); + + populate_fpu_regs(); + /* Show the entire contents of xsave for issue debug */ + dump_xsave_content(xsave_size); + + test_xsave_ymm_change(xsave_size); + test_xsave_xrstor(xsave_size); + + ksft_exit(!ksft_get_fail_cnt()); +}
In order to ensure that the XSAVE content is not affected by signal handling, this case tests XSAVE content should not change in nested signal handling, and XSAVE content should same before and after signal handling.
Signed-off-by: Pengfei Xu pengfei.xu@intel.com --- tools/testing/selftests/xsave/.gitignore | 1 + tools/testing/selftests/xsave/Makefile | 2 +- .../selftests/xsave/xsave_signal_handle.c | 184 ++++++++++++++++++ 3 files changed, 186 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/xsave/xsave_signal_handle.c
diff --git a/tools/testing/selftests/xsave/.gitignore b/tools/testing/selftests/xsave/.gitignore index 00b9970360c4..b448d36186f3 100644 --- a/tools/testing/selftests/xsave/.gitignore +++ b/tools/testing/selftests/xsave/.gitignore @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0-only xsave_instruction +xsave_signal_handle diff --git a/tools/testing/selftests/xsave/Makefile b/tools/testing/selftests/xsave/Makefile index dafdb0abdeb3..fedae2778297 100644 --- a/tools/testing/selftests/xsave/Makefile +++ b/tools/testing/selftests/xsave/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only CFLAGS := -g -Wall -mxsave -O2
-TEST_GEN_PROGS := xsave_instruction +TEST_GEN_PROGS := xsave_instruction xsave_signal_handle
include ../lib.mk diff --git a/tools/testing/selftests/xsave/xsave_signal_handle.c b/tools/testing/selftests/xsave/xsave_signal_handle.c new file mode 100644 index 000000000000..0afcba3a1bd5 --- /dev/null +++ b/tools/testing/selftests/xsave/xsave_signal_handle.c @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * It's for xsave/xrstor during signal handling tests + */ + +#include <stdio.h> +#include <stdint.h> +#include <stdlib.h> +#include <x86intrin.h> +#include <string.h> +#include <signal.h> +#include <unistd.h> +#include <sched.h> +#include <sys/wait.h> +#include <time.h> + +#include "../kselftest.h" +#include "xsave_common.h" + +static unsigned char *xsave_buf0, *xsave_buf1, *xsave_buf2, *xsave_buf3; +static int result[2]; + +static void change_fpu_content(uint32_t ui32_random, double flt) +{ + asm volatile ("fldl %0" : : "m" (flt)); + asm volatile ("vbroadcastss %0, %%ymm0" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm1" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm2" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm3" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm4" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm5" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm6" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm7" : : "m" (ui32_random)); + #ifndef __i386__ + asm volatile ("vbroadcastss %0, %%ymm8" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm9" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm10" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm11" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm12" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm13" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm14" : : "m" (ui32_random)); + asm volatile ("vbroadcastss %0, %%ymm15" : : "m" (ui32_random)); + #endif +} + +static void usr1_handler(int signum, siginfo_t *info, void *__ctxp) +{ + uint32_t ui32_random; + double flt; + int xsave_size; + const char *test_name = "Child XSAVE should not change in nested signal"; + + ui32_random = rand(); + flt = ui32_random/10000.0; + if (signum == SIGUSR1) { + ksft_print_msg("SIGUSR1:0x%x changed fld:%f & ymm0-15:0x%x\n", + SIGUSR1, flt, ui32_random); + change_fpu_content(ui32_random, flt); + } + xsave_size = get_xsave_size(); + XSAVE(xsave_buf2, SAVE_MASK); + raise(SIGUSR2); + XSAVE(xsave_buf3, SAVE_MASK); + result[0] = check_xsave_buf(xsave_buf2, xsave_buf2, xsave_size, test_name, + NO_CHANGE); +} + +static void usr2_handler(int signum, siginfo_t *info, void *__ctxp) +{ + uint32_t ui32_random; + double flt; + + ui32_random = rand(); + flt = ui32_random/10000.0; + if (signum == SIGUSR2) { + ksft_print_msg("SIGUSR2:0x%x changed fld:%f & ymm0-15:0x%x\n", + SIGUSR2, flt, ui32_random); + change_fpu_content(ui32_random, flt); + } +} + +static void set_signal_handle(void) +{ + struct sigaction sigact; + + memset(&sigact, 0, sizeof(sigact)); + if (sigemptyset(&sigact.sa_mask)) + execution_failed("FAIL: sigemptyset error\n"); + + sigact.sa_flags = SA_SIGINFO; + + sigact.sa_sigaction = usr1_handler; + if (sigaction(SIGUSR1, &sigact, NULL)) + execution_failed("FAIL: SIGUSR1 handling failed\n"); + + sigact.sa_sigaction = usr2_handler; + if (sigaction(SIGUSR2, &sigact, NULL)) + execution_failed("FAIL: SIGUSR2 handling failed\n"); +} + +static void sig_handle_xsave_test(void) +{ + int i, loop_times = 100, xsave_size; + const char *sig_test_name0 = "Child XSAVE was same in nested signal test"; + const char *sig_test_name1 = "Child XSAVE content was same after signal"; + + xsave_size = get_xsave_size(); + /* SDM XSAVE: misalignment to a 64-byte boundary will result in #GP */ + xsave_buf0 = aligned_alloc(64, xsave_size); + if (!xsave_buf0) + execution_failed("aligned_alloc xsave_buf0 failed\n"); + xsave_buf1 = aligned_alloc(64, xsave_size); + if (!xsave_buf1) + execution_failed("aligned_alloc xsave_buf1 failed\n"); + xsave_buf2 = aligned_alloc(64, xsave_size); + if (!xsave_buf2) + execution_failed("aligned_alloc xsave_buf2 failed\n"); + xsave_buf3 = aligned_alloc(64, xsave_size); + if (!xsave_buf3) + execution_failed("aligned_alloc xsave_buf3 failed\n"); + + srand(time(NULL)); + result[0] = RESULT_PASS; + result[1] = RESULT_PASS; + + XSAVE(xsave_buf0, SAVE_MASK); + for (i = 1; i <= loop_times; i++) { + raise(SIGUSR1); + XSAVE(xsave_buf1, SAVE_MASK); + result[1] = check_xsave_buf(xsave_buf0, xsave_buf1, xsave_size, + sig_test_name1, NO_CHANGE); + if (result[1] != RESULT_PASS) + break; + } + + check_result(result[0], sig_test_name0); + check_result(result[1], sig_test_name1); +} + +static void test_xsave_sig_handle(void) +{ + const char *test_name0 = "xsave in child nested signal handling test"; + const char *test_name1 = "xsave after child signal handling test"; + pid_t child; + int status, fd[2], readbuf[2]; + + set_signal_handle(); + + /* Use pipe to transfer test result of child process to parent process */ + if (pipe(fd) < 0) + execution_failed("FAIL: create pipe failed\n"); + + /* Use child process testing to avoid abnormal blocking the next test */ + child = fork(); + if (child < 0) + execution_failed("FAIL: create child pid failed\n"); + else if (child == 0) { + populate_fpu_regs(); + sig_handle_xsave_test(); + close(fd[0]); + write(fd[1], &result, sizeof(result)); + } else { + if (waitpid(child, &status, 0) != child || + !WIFEXITED(status)) + execution_failed("FAIL: Child died unexpectedly\n"); + else { + close(fd[1]); + read(fd[0], &readbuf, sizeof(readbuf)); + + check_result(readbuf[0], test_name0); + check_result(readbuf[1], test_name1); + } + } +} + +int main(void) +{ + ksft_print_header(); + ksft_set_plan(2); + + test_xsave_sig_handle(); + + ksft_exit(!ksft_get_fail_cnt()); +}
On 7/26/21 8:34 PM, Pengfei Xu wrote:
The XSAVE feature set supports the saving and restoring of state components such as FPU, which is used for process context switching.
This sentence is really awkward. It reads at first as saying that the FPU is used for context switching. Can you rephrase.
In order to ensure that XSAVE works correctly, add XSAVE basic test for XSAVE architecture functionality.
This sentence needs to be start on the same line as the previous one, *or* be in a new paragraph. Please rewrap it.
This patch set tests XSAVE/XRSTOR instructions on x86 platforms and verify if the XSAVE/XRSTOR works correctly during signal handling.
This reads to me like you are going to test the XSAVE/XRSTOR instructions *in* a signal handler, instead of testing the XSAVE/XRSTOR instructions that the kernel uses at signal entry/exit.
Also, the kernel does *NOT* *USE* XSAVE/XRSTOR in many cases to save/restore signal state. The changelog could be read as implying that it does.
Cases such as signal handling, process creation, other xstate(except FPU) tests for XSAVE check, etc. will be added to the Linux kernel self-test. If appropriate, it is even planned to add the [1] mentioned XSAVE issues reproduce and some XSAVE anomaly tests to the kernel self-test.
This is not clear whether it is talking about *this* series int he future tense (will be added) or whether it is talking about future *work*.
Maybe something like this:
This series introduces only the most basic XSAVE tests. In the future, the intention is to continue expanding the scope of these selftests to include more kernel XSAVE-related functionality and XSAVE-managed features like AMX and shadow stacks.
Hi Dave, Thanks for comments!
On 2021-07-27 at 10:46:45 -0700, Dave Hansen wrote:
On 7/26/21 8:34 PM, Pengfei Xu wrote:
The XSAVE feature set supports the saving and restoring of state components such as FPU, which is used for process context switching.
This sentence is really awkward. It reads at first as saying that the FPU is used for context switching. Can you rephrase.
Thanks for advice, I will update it as below if it's appropriate: " The XSAVE feature set supports the saving and restoring of state components, which is used for process context switching. The state components include x87 state for FPU execution environment, SSE state, AVX state and so on. "
In order to ensure that XSAVE works correctly, add XSAVE basic test for XSAVE architecture functionality.
This sentence needs to be start on the same line as the previous one, *or* be in a new paragraph. Please rewrap it.
Will rewrap it.
This patch set tests XSAVE/XRSTOR instructions on x86 platforms and verify if the XSAVE/XRSTOR works correctly during signal handling.
This reads to me like you are going to test the XSAVE/XRSTOR instructions *in* a signal handler, instead of testing the XSAVE/XRSTOR instructions that the kernel uses at signal entry/exit.
Also, the kernel does *NOT* *USE* XSAVE/XRSTOR in many cases to save/restore signal state. The changelog could be read as implying that it does.
Yes, how about updating it as below: " This patch set tests and verifies the basic functions of XSAVE/XRSTOR in user space; during and after signal processing on the x86 platform, the XSAVE contents of the process should not be changed. "
Cases such as signal handling, process creation, other xstate(except FPU) tests for XSAVE check, etc. will be added to the Linux kernel self-test. If appropriate, it is even planned to add the [1] mentioned XSAVE issues reproduce and some XSAVE anomaly tests to the kernel self-test.
This is not clear whether it is talking about *this* series int he future tense (will be added) or whether it is talking about future *work*.
Maybe something like this:
This series introduces only the most basic XSAVE tests. In the future, the intention is to continue expanding the scope of these selftests to include more kernel XSAVE-related functionality and XSAVE-managed features like AMX and shadow stacks.
Thanks for advice, will update it.
-Pengfei BR.
linux-kselftest-mirror@lists.linaro.org
-
Dave Hansen -
Pengfei Xu