Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 2/2] selftests/x86/amx: Fix the test to avoid failure when
 AMX is unavailable
To:     "Chang S. Bae" <chang.seok.bae@intel.com>,
        linux-kselftest@vger.kernel.org, shuah@kernel.org,
        linux-kernel@vger.kernel.org
Cc:     dave.hansen@linux.intel.com, tglx@linutronix.de, bp@suse.de,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20220401221014.13556-1-chang.seok.bae@intel.com>
 <20220401221014.13556-3-chang.seok.bae@intel.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <aaab50d2-592c-69e4-58a6-0a0926669de3@linuxfoundation.org>
Date:   Thu, 16 Jun 2022 16:54:26 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20220401221014.13556-3-chang.seok.bae@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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