Re: [PATCH v8 1/1] selftests/x86/xstate: Add xstate test cases for XSAVE feature

[Dave Hansen]

On 3/16/22 05:40, Pengfei Xu wrote:
> +#ifndef __cpuid_count
> +#define __cpuid_count(level, count, a, b, c, d) ({	\
> +	__asm__ __volatile__ ("cpuid\n\t"	\
> +			: "=a" (a), "=b" (b), "=c" (c), "=d" (d)	\
> +			: "0" (level), "2" (count));	\
> +})
> +#endif


By the time you post the next revision, I hope these are merged:

> https://lore.kernel.org/all/cover.1647360971.git.reinette.chatre@xxxxxxxxx/

Could you rebase on top of those to avoid duplication, please?

> +/*
> + * While this function prototype is in the stdlib.h, the header file cannot be
> + * included with the -mno-sse option.
> + */
> +void *aligned_alloc(size_t alignment, size_t size);

That is worrisome.  I would serioulsy suspect that if the header can't
be included that the code that it references has issues as well.

Or, is this just working around a compiler bug?  Just googling:

	https://www.google.com/search?q=stdlib.h+no-sse

points to this:

	https://sourceware.org/bugzilla/show_bug.cgi?id=27600

> +enum supportability {
> +	NOT_SUPPORT,
> +	SUPPORT,
> +};
> +
> +/* It's from arch/x86/kernel/fpu/xstate.c. */
> +static const char * const xfeature_names[] = {
> +	"x87 floating point registers",
> +	"SSE registers",
> +	"AVX registers",
> +	"MPX bounds registers",
> +	"MPX CSR",
> +	"AVX-512 opmask",
> +	"AVX-512 Hi256",
> +	"AVX-512 ZMM_Hi256",
> +	"Processor Trace (unused)",
> +	"Protection Keys User registers",
> +	"PASID state",
> +	"unknown xstate feature",
> +	"unknown xstate feature",
> +	"unknown xstate feature",
> +	"unknown xstate feature",
> +	"unknown xstate feature",
> +	"unknown xstate feature",
> +	"AMX Tile config",
> +	"AMX Tile data",
> +	"unknown xstate feature",
> +};
> +
> +/* 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,
> +	XFEATURE_RSRVD_COMP_16,
> +	XFEATURE_XTILE_CFG,
> +	XFEATURE_XTILE_DATA,
> +	XFEATURE_MAX,
> +};
> +
> +struct xsave_buffer {
> +	union {
> +		struct {
> +			char legacy[XSAVE_HDR_OFFSET];
> +			char header[XSAVE_HDR_SIZE];
> +			char extended[0];
> +		};
> +		char bytes[0];
> +	};
> +};
> +
> +static struct {
> +	uint64_t mask;
> +	uint32_t size[XFEATURE_MAX];
> +	uint32_t offset[XFEATURE_MAX];
> +} xstate_info;
> +
> +static inline void check_cpuid_xsave_availability(void)
> +{
> +	uint32_t eax, ebx, ecx, edx;
> +
> +	/*
> +	 * CPUID.1:ECX.XSAVE[bit 26] enumerates general
> +	 * support for the XSAVE feature set, including
> +	 * XGETBV.
> +	 */
> +	__cpuid_count(1, 0, eax, ebx, ecx, edx);
> +	if (!(ecx & CPUID_LEAF1_ECX_XSAVE_MASK))
> +		fatal_error("cpuid: no CPU xsave support");
> +	if (!(ecx & CPUID_LEAF1_ECX_OSXSAVE_MASK))
> +		fatal_error("cpuid: no OS xsave support");
> +}

Could you please use the standard selftest macros for these fatal
errors?  Also, do these indicate that the test failed, or that it just
was not able to run?

> +static inline bool xstate_tested(int xfeature_num)
> +{
> +	return !!(xstate_info.mask & (1 << xfeature_num));
> +}
> +
> +static inline int cpu_has_avx2(void)
> +{
> +	unsigned int eax, ebx, ecx, edx;
> +
> +	/* CPUID.7.0:EBX.AVX2[bit 5]: the support for AVX2 instructions */
> +	__cpuid_count(7, 0, eax, ebx, ecx, edx);
> +
> +	return !!(ebx & CPUID_LEAF7_EBX_AVX2_MASK);
> +}
> +
> +static inline int cpu_has_avx512f(void)
> +{
> +	unsigned int eax, ebx, ecx, edx;
> +
> +	/* CPUID.7.0:EBX.AVX512F[bit 16]: the support for AVX512F instructions */
> +	__cpuid_count(7, 0, eax, ebx, ecx, edx);
> +
> +	return !!(ebx & CPUID_LEAF7_EBX_AVX512F_MASK);
> +}
> +
> +static inline int cpu_has_pkeys(void)
> +{
> +	unsigned int eax, ebx, ecx, edx;
> +
> +	/* CPUID.7.0:ECX.PKU[bit 3]: the support for PKRU instructions */
> +	__cpuid_count(7, 0, eax, ebx, ecx, edx);
> +	if (!(ecx & CPUID_LEAF7_ECX_PKU_MASK))
> +		return NOT_SUPPORT;
> +	/* CPUID.7.0:ECX.OSPKE[bit 4]: the support for OS set CR4.PKE */
> +	if (!(ecx & CPUID_LEAF7_ECX_OSPKE_MASK))
> +		return NOT_SUPPORT;
> +
> +	return SUPPORT;
> +}

You don't need that CPUID_LEAF7_ECX_PKU_MASK check.  The OSPKE one is
sufficient.

> +static uint32_t get_xstate_size(void)
> +{
> +	uint32_t eax, ebx, ecx, edx;
> +
> +	__cpuid_count(CPUID_LEAF_XSTATE, CPUID_SUBLEAF_XSTATE_USER, eax, ebx,
> +		      ecx, edx);
> +	/*
> +	 * EBX enumerates the size (in bytes) required by the XSAVE
> +	 * instruction for an XSAVE area containing all the user state
> +	 * components corresponding to bits currently set in XCR0.
> +	 */
> +	return ebx;
> +}
> +
> +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)
> +		fatal_error("aligned_alloc()");
> +
> +	return xbuf;
> +}
> +
> +static inline void __xsave(struct xsave_buffer *xbuf, uint64_t rfbm)
> +{
> +	uint32_t rfbm_lo = rfbm;
> +	uint32_t rfbm_hi = rfbm >> 32;
> +
> +	asm volatile("xsave (%%rdi)"
> +		     : : "D" (xbuf), "a" (rfbm_lo), "d" (rfbm_hi)
> +		     : "memory");
> +}
> +
> +static inline void __xrstor(struct xsave_buffer *xbuf, uint64_t rfbm)
> +{
> +	uint32_t rfbm_lo = rfbm;
> +	uint32_t rfbm_hi = rfbm >> 32;
> +
> +	asm volatile("xrstor (%%rdi)"
> +		     : : "D" (xbuf), "a" (rfbm_lo), "d" (rfbm_hi));
> +}

Could you please move all of these generic functions into a common
header?  Maybe the same one with __cpuid()?

> +static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
> +		       int flags)
> +{
> +	struct sigaction sa;
> +
> +	memset(&sa, 0, sizeof(sa));
> +	sa.sa_sigaction = handler;
> +	sa.sa_flags = SA_SIGINFO | flags;
> +	sigemptyset(&sa.sa_mask);
> +	if (sigaction(sig, &sa, 0))
> +		fatal_error("sigaction");
> +}
> +
> +static void clearhandler(int sig)
> +{
> +	struct sigaction sa;
> +
> +	memset(&sa, 0, sizeof(sa));
> +	sa.sa_handler = SIG_DFL;
> +	sigemptyset(&sa.sa_mask);
> +	if (sigaction(sig, &sa, 0))
> +		fatal_error("sigaction");
> +}
> +
> +/* Retrieve the offset and size of a specific xstate. */
> +static void retrieve_xstate_size_and_offset(uint32_t xfeature_num)
> +{
> +	uint32_t eax, ebx, ecx, edx;
> +
> +	__cpuid_count(CPUID_LEAF_XSTATE, xfeature_num, eax, ebx, ecx, edx);
> +	/*
> +	 * CPUID.(EAX=0xd, ECX=xfeature_num), and output is as follow:
> +	 * eax: xfeature num state component size
> +	 * ebx: xfeature num state component offset in user buffer
> +	 */
> +	xstate_info.size[xfeature_num] = eax;
> +	xstate_info.offset[xfeature_num] = ebx;
> +}
> +
> +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;
> +}
> +
> +static void check_cpuid_xstate_info(void)
> +{
> +	/* CPUs that support XSAVE could support FP and SSE by default. */
> +	xstate_info.mask = XFEATURE_MASK_FP | XFEATURE_MASK_SSE;
> +
> +	xstate_size = get_xstate_size();
> +	if (cpu_has_avx2()) {
> +		xstate_info.mask |= XFEATURE_MASK_YMM;
> +		retrieve_xstate_size_and_offset(XFEATURE_YMM);
> +	}
> +
> +	if (cpu_has_avx512f()) {
> +		xstate_info.mask |= XFEATURE_MASK_OPMASK | XFEATURE_MASK_ZMM_Hi256 |
> +				    XFEATURE_MASK_Hi16_ZMM;
> +		retrieve_xstate_size_and_offset(XFEATURE_OPMASK);
> +		retrieve_xstate_size_and_offset(XFEATURE_ZMM_Hi256);
> +		retrieve_xstate_size_and_offset(XFEATURE_Hi16_ZMM);
> +	}
> +
> +	if (cpu_has_pkeys()) {
> +		xstate_info.mask |= XFEATURE_MASK_PKRU;
> +		retrieve_xstate_size_and_offset(XFEATURE_PKRU);
> +	}
> +}
> +
> +static void fill_xstate_buf(uint8_t test_byte, unsigned char *buf,
> +			    int xfeature_num)
> +{
> +	uint32_t i;
> +
> +	for (i = 0; i < xstate_info.size[xfeature_num]; i++)
> +		buf[xstate_info.offset[xfeature_num] + i] = test_byte;
> +}
> +
> +/* Fill FP/XMM/YMM/OPMASK and PKRU xstates into buffer. */
> +static void fill_xstates_buf(struct xsave_buffer *buf, uint32_t xsave_mask)
> +{
> +	uint32_t i;
> +	/* The data of FP x87 state are as follows. */

OK, but what *is* this?  Random data?  Or did you put some data in that
means something?

> +	unsigned char fp_data[160] = {
> +		0x7f, 0x03, 0x00, 0x00, 0xff, 0x00, 0x00, 0x00,
> +		0xf0, 0x19, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0x00, 0x00,
> +		0x00, 0x78, 0xfa, 0x79, 0xf9, 0x78, 0xfa, 0xf9,
> +		0xf2, 0x3d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x78, 0xfa, 0x79, 0xf9, 0x78, 0xfa, 0xf9,
> +		0xf2, 0x3d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x78, 0xfa, 0x79, 0xf9, 0x78, 0xfa, 0xf9,
> +		0xf2, 0x3d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x78, 0xfa, 0x79, 0xf9, 0x78, 0xfa, 0xf9,
> +		0xf2, 0x3d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x78, 0xfa, 0x79, 0xf9, 0x78, 0xfa, 0xf9,
> +		0xf2, 0x3d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x78, 0xfa, 0x79, 0xf9, 0x78, 0xfa, 0xf9,
> +		0xf2, 0x3d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x78, 0xfa, 0x79, 0xf9, 0x78, 0xfa, 0xf9,
> +		0xf2, 0x3d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x78, 0xfa, 0x79, 0xf9, 0x78, 0xfa, 0xf9,
> +		0xf2, 0x3d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
> +
> +	/* Clean the buffer with all 0 first. */
> +	memset(buf, 0, xstate_size);
> +
> +	/* Fill fp x87 state: MXCSR and MXCSR_MASK data(0-159 bytes) into buffer. */
> +	memcpy(buf, fp_data, FP_SIZE);
> +
> +	/*
> +	 * Fill test byte value into XMM xstate buffer(160-415 bytes).
> +	 * xstate 416-511 bytes are reserved as 0.
> +	 */
> +	for (i = 0; i < XMM_SIZE; i++)
> +		*((unsigned char *)buf + XMM_OFFSET + i) = XSTATE_TESTBYTE;

Isn't this just memset()?

> +	/*
> +	 * Fill xstate-component bitmap(512-519 bytes) into xstate header.
> +	 * xstate header range is 512-575 bytes.
> +	 */
> +	set_xstatebv(buf, xsave_mask);
> +
> +	/* Fill test byte value into YMM xstate buffer(YMM offset/size). */
> +	if (xstate_tested(XFEATURE_YMM))
> +		fill_xstate_buf(XSTATE_TESTBYTE, (unsigned char *)buf, XFEATURE_YMM);
> +
> +	/*
> +	 * Fill test byte value into AVX512 OPMASK/ZMM xstates buffer
> +	 * (AVX512_OPMASK/ZMM_Hi256/Hi16_ZMM offset/size).
> +	 */
> +	if (xstate_tested(XFEATURE_OPMASK))
> +		fill_xstate_buf(XSTATE_TESTBYTE, (unsigned char *)buf, XFEATURE_OPMASK);
> +	if (xstate_tested(XFEATURE_ZMM_Hi256)) {
> +		fill_xstate_buf(XSTATE_TESTBYTE, (unsigned char *)buf,
> +				XFEATURE_ZMM_Hi256);
> +	}
> +	if (xstate_tested(XFEATURE_Hi16_ZMM)) {
> +		fill_xstate_buf(XSTATE_TESTBYTE, (unsigned char *)buf,
> +				XFEATURE_Hi16_ZMM);
> +	}
> +
> +	if (xstate_tested(XFEATURE_PKRU)) {
> +		/* Only 0-3 bytes of pkru xstates are allowed to be written. */
> +		memset((unsigned char *)buf + xstate_info.offset[XFEATURE_PKRU],
> +			PKRU_TESTBYTE, sizeof(uint32_t));
> +	}
> +}

I have to wonder if we can do this in a bit more structured way:

struct xstate_test
{
	bool (*init)(void);
	bool (*fill_state)(struct xsave_buffer *buf);
	...
}

Yes, that means indirect function calls, but that's OK since we know it
will all be compiled with the "no-sse" flag (and friends).

Then fill_xstates_buf() just becomes a loop over an xstate_tests[] array.

> +/*
> + * Because xstate like XMM, YMM registers are not preserved across function
> + * calls, so use inline function with assembly code only for fork syscall.
> + */
> +static inline long long __fork(void)
> +{
> +	long long ret, nr = SYS_fork;

Are those the right types?  'long long' is 64-bit on 32-bit systems, iirc.

> +	asm volatile("syscall"
> +		 : "=a" (ret)
> +		 : "a" (nr), "b" (nr)
> +		 : "rcx", "r11", "memory", "cc");
> +
> +	return ret;
> +}
>
> +/*
> + * Because xstate like XMM, YMM registers are not preserved across function
> + * calls, so use inline function with assembly code only to raise signal.
> + */
> +static inline long long __raise(long long pid_num, long long sig_num)
> +{
> +	long long ret, nr = SYS_kill;
> +
> +	register long long arg1 asm("rdi") = pid_num;
> +	register long long arg2 asm("rsi") = sig_num;

I really don't like register variables.  They're very fragile.  Imagine
if someone put a printf() right here.  Why don't you just do this with
inline assembly directives?

> +	asm volatile("syscall"
> +		 : "=a" (ret)
> +		 : "a" (nr), "b" (nr), "r" (arg1), "r" (arg2)
> +		 : "rcx", "r11", "memory", "cc");
> +
> +	return ret;
> +}
> +
> +static void sigusr1_handler(int signum, siginfo_t *info, void *__ctxp)
> +{
> +	sigusr1_done = true;
> +}
> +
> +static void test_xstate_sig_handle(void)
> +{
> +	pid_t process_pid;
> +
> +	sethandler(SIGUSR1, sigusr1_handler, 0);
> +	printf("[RUN]\tCheck xstate around signal handling test.\n");
> +	process_pid = getpid();
> +
> +	/*
> +	 * Xrstor the valid_xbuf and call syscall assembly instruction, then
> +	 * save the xstate to compared_xbuf after signal handling for comparison.
> +	 */
> +	__xrstor(valid_xbuf, xstate_info.mask);
> +	__raise(process_pid, SIGUSR1);
> +	__xsave(compared_xbuf, xstate_info.mask);
> +	if (sigusr1_done == true)
> +		printf("[NOTE]\tSIGUSR1 handling is done.\n");
> +	else
> +		fatal_error("Didn't access SIGUSR1 handling after raised SIGUSR1");
> +
> +	if (memcmp(&valid_xbuf->bytes[0], &compared_xbuf->bytes[0], xstate_size))
> +		printf("[FAIL]\tProcess xstate is not same after signal handling\n");
> +	else
> +		printf("[PASS]\tProcess xstate is same after signal handling.\n");
> +
> +	clearhandler(SIGUSR1);
> +}
> +
> +static void test_xstate_fork(void)
> +{
> +	pid_t child;
> +	int status;
> +
> +	printf("[RUN]\tParent pid:%d check xstate around fork test.\n", getpid());
> +	memset(compared_xbuf, 0, xstate_size);
> +
> +	/*
> +	 * Xrstor the valid_xbuf and call syscall assembly instruction, then
> +	 * save the xstate to compared_xbuf in child process for comparison.
> +	 */
> +	__xrstor(valid_xbuf, xstate_info.mask);
> +	child = __fork();
> +	if (child < 0) {
> +		/* Fork syscall failed */
> +		fatal_error("fork failed");
> +	} else if (child == 0) {
> +		/* Fork syscall succeeded, now in the child. */
> +		__xsave(compared_xbuf, xstate_info.mask);
> +		if (memcmp(&valid_xbuf->bytes[0], &compared_xbuf->bytes[0],
> +			xstate_size)) {
> +			printf("[FAIL]\tXstate of child process:%d is not same as xstate of parent\n",
> +				getpid());
> +		} else {
> +			printf("[PASS]\tXstate of child process:%d is same as xstate of parent.\n",
> +				getpid());
> +		}
> +	} else {
> +		if (waitpid(child, &status, 0) != child || !WIFEXITED(status))
> +			fatal_error("Child exit with error status");

It also couldn't hurt to check the XSAVE state in the parent.