diff mbox series

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

Message ID 20220401221014.13556-3-chang.seok.bae@intel.com
State New
Headers show
Series selftests/x86: AMX-related test improvements | expand

Commit Message

Chang S. Bae April 1, 2022, 10:10 p.m. UTC
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(-)

Comments

Shuah Khan June 16, 2022, 10:54 p.m. UTC | #1
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
Chang S. Bae June 17, 2022, 10:21 p.m. UTC | #2
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/
Shuah Khan June 17, 2022, 10:29 p.m. UTC | #3
On 6/17/22 4:21 PM, Chang S. Bae wrote:
> On 6/16/2022 3:54 PM, Shuah Khan wrote:
>> On 4/1/22 4:10 PM, Chang S. Bae wrote:
>>>

>>
>> 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:
> 

Simplifying is always good. Send me v2 and I will review it.

thanks,
-- Shuah
diff mbox series

Patch

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;
+
 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);
 }
 
 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;
+}
+
 /* 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;
 
 	init_stashed_xsave();
 	sethandler(SIGILL, handle_noperm, 0);