Re: [PATCH] selftests: x86: skip the tests if prerequisites aren't fulfilled

[Chang S. Bae]

On 3/7/2024 10:37 AM, Muhammad Usama Anjum wrote:
> diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c
> index d884fd69dd510..5d1ca0bbaaae7 100644
> --- a/tools/testing/selftests/x86/amx.c
> +++ b/tools/testing/selftests/x86/amx.c
> @@ -103,9 +103,10 @@ static void clearhandler(int sig)
>   
>   #define CPUID_LEAF1_ECX_XSAVE_MASK	(1 << 26)
>   #define CPUID_LEAF1_ECX_OSXSAVE_MASK	(1 << 27)
> -static inline void check_cpuid_xsave(void)
> +static inline int check_cpuid_xsave(void)
>   {
>   	uint32_t eax, ebx, ecx, edx;
> +	int ret = 0;
>   
>   	/*
>   	 * CPUID.1:ECX.XSAVE[bit 26] enumerates general
> @@ -113,10 +114,16 @@ static inline void check_cpuid_xsave(void)
>   	 * XGETBV.
>   	 */
>   	__cpuid_count(1, 0, 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");
> +	if (!(ecx & CPUID_LEAF1_ECX_XSAVE_MASK)) {
> +		ksft_print_msg("cpuid: no CPU xsave support\n");
> +		ret = -1;
> +	}
> +	if (!(ecx & CPUID_LEAF1_ECX_OSXSAVE_MASK)) {
> +		ksft_print_msg("cpuid: no OS xsave support\n");
> +		ret = -1;
> +	}
> +
> +	return ret;
>   }

I thought check_cpuid_xsave() can go away [1] by simplifying the 
availability check through arch_prctl():

+#define ARCH_GET_XCOMP_SUPP    0x1021
 #define ARCH_GET_XCOMP_PERM    0x1022
 #define ARCH_REQ_XCOMP_PERM    0x1023

@@ -928,8 +911,15 @@ static void test_ptrace(void)

 int main(void)
 {
-       /* Check hardware availability at first */
-       check_cpuid_xsave();
+       unsigned long features;
+       long rc;
+
+       rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_SUPP, &features);
+       if (rc || (features & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE) {
+               ksft_print_msg("no AMX support\n");
+               return KSFT_SKIP;
+       }

> -static void check_cpuid_xtiledata(void)
> +static int check_cpuid_xtiledata(void)
>   {
>   	uint32_t eax, ebx, ecx, edx;
>   
> @@ -153,12 +160,16 @@ 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);
> +	if (!eax || !ebx) {
> +		ksft_print_msg("xstate cpuid: invalid tile data size/offset: %d/%d\n",
> +			       eax, ebx);
> +		return -1;
> +	}
>   
>   	xtiledata.size	      = eax;
>   	xtiledata.xbuf_offset = ebx;
> +
> +	return 0;
>   }

I don't think it is okay to silently skip the test here. If the feature 
is available, the tile data size and offset should not be zero.

Thanks,
Chang

[1] 
https://lore.kernel.org/lkml/327cde12-daea-84ba-4b24-64fe12e89dea@xxxxxxxxx/