Messages in this thread Patch in this message | | | Date | Fri, 17 Jun 2022 15:21:18 -0700 | Subject | Re: [PATCH 2/2] selftests/x86/amx: Fix the test to avoid failure when AMX is unavailable | From | "Chang S. Bae" <> |
| |
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/
| |