Re: [PATCH 2/2] selftests/x86/amx: Fix the test to avoid failure when AMX is unavailable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx/




[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux