On 6/16/2022 3:54 PM, Shuah Khan wrote:
On 4/1/22 4:10 PM, Chang S. Bae wrote:
+static struct { + unsigned xsave: 1; + unsigned osxsave: 1; +} cpuinfo;
Why is this needed? Also naming this cpuinfo is confuing.
This came from the below CPUID check which seems to be moot.
static inline void check_cpuid_xsave(void) { uint32_t eax, ebx, ecx, edx; @@ -118,10 +124,8 @@ static inline void check_cpuid_xsave(void) eax = 1; ecx = 0; cpuid(&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"); + cpuinfo.xsave = !!(ecx & CPUID_LEAF1_ECX_XSAVE_MASK); + cpuinfo.osxsave = !!(ecx & CPUID_LEAF1_ECX_OSXSAVE_MASK);
Why add this complexity. Why not just Skip here?
I think these CPUID checks can go away with ARCH_GET_XCOMP_SUPP.
} static uint32_t xbuf_size; @@ -161,14 +165,31 @@ static void check_cpuid_xtiledata(void) * eax: XTILEDATA state component size * ebx: XTILEDATA state component offset in user buffer */ - if (!eax || !ebx) - fatal_error("xstate cpuid: invalid tile data size/offset: %d/%d", - eax, ebx);
xtiledata.size = eax; xtiledata.xbuf_offset = ebx; } +static bool amx_available(void) +{ + check_cpuid_xsave(); + if (!cpuinfo.xsave) { + printf("[SKIP]\tcpuid: no CPU xsave support\n"); + return false; + } else if (!cpuinfo.osxsave) { + printf("[SKIP]\tcpuid: no OS xsave support\n"); + return false; + }
+ check_cpuid_xtiledata(); + if (!xtiledata.size || !xtiledata.xbuf_offset) { + printf("[SKIP]\txstate cpuid: no tile data (size/offset: %d/%d)\n", + xtiledata.size, xtiledata.xbuf_offset); + return false; + }
+ return true; +}
I am not seeing any value in adding this layer of abstraction. Keep it simple and do the handling in main()
Sure.
/* The helpers for managing XSAVE buffer and tile states: */ struct xsave_buffer *alloc_xbuf(void) @@ -826,9 +847,8 @@ static void test_context_switch(void) int main(void) { - /* Check hardware availability at first */ - check_cpuid_xsave(); - check_cpuid_xtiledata(); + if (!amx_available()) + return 0;
This should KSFT_SKIP for this to be reported as a skip. Returning 0 will be reported as a Pass.
I think that's a good point, thanks.
Now, along with the on-going documentation [1], this test code can be simplified like the below changes, instead of having those cpuid functions:
diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c index 625e42901237..83705c472a5c 100644 --- a/tools/testing/selftests/x86/amx.c +++ b/tools/testing/selftests/x86/amx.c @@ -348,6 +348,7 @@ enum expected_result { FAIL_EXPECTED, SUCCESS_EXPECTED };
/* arch_prctl() and sigaltstack() test */
+#define ARCH_GET_XCOMP_SUPP 0x1021 #define ARCH_GET_XCOMP_PERM 0x1022 #define ARCH_REQ_XCOMP_PERM 0x1023
@@ -828,9 +829,14 @@ static void test_context_switch(void)
int main(void) { - /* Check hardware availability at first */ - check_cpuid_xsave(); - check_cpuid_xtiledata(); + unsigned long features; + long rc; + + rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_SUPP, &features); + if (rc || (features & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE) { + printf("[SKIP]\tno AMX support\n"); + exit(KSFT_FAIL); + }
init_stashed_xsave(); sethandler(SIGILL, handle_noperm, 0);
Thanks, Chang
[1] https://lore.kernel.org/lkml/86952726-53e6-17a9-dbe0-3e970c565044@intel.com/