Messages in this thread | | | Subject | Re: [PATCH 2/2] selftests/x86/amx: Fix the test to avoid failure when AMX is unavailable | From | Shuah Khan <> | Date | Thu, 16 Jun 2022 16:54:26 -0600 |
| |
On 4/1/22 4:10 PM, Chang S. Bae wrote: > When a CPU does not have AMX, the test fails. But this is wrong as it > should be runnable regardless. Skip the test instead. > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > Fixes: 6a3e0651b4a ("selftests/x86/amx: Add test cases for AMX state management") > Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com> > Cc: linux-kselftest@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > tools/testing/selftests/x86/amx.c | 42 +++++++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 11 deletions(-) > > diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c > index 3615ef4a48bb..14abb6072a7d 100644 > --- a/tools/testing/selftests/x86/amx.c > +++ b/tools/testing/selftests/x86/amx.c > @@ -106,6 +106,12 @@ static void clearhandler(int sig) > > #define CPUID_LEAF1_ECX_XSAVE_MASK (1 << 26) > #define CPUID_LEAF1_ECX_OSXSAVE_MASK (1 << 27) > + > +static struct { > + unsigned xsave: 1; > + unsigned osxsave: 1; > +} cpuinfo; > +
Why is this needed? Also naming this cpuinfo is confuing.
> 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?
> } > > 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()
> /* 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.
> > init_stashed_xsave(); > sethandler(SIGILL, handle_noperm, 0); >
thanks, -- Shuah
| |