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