Received-SPF: pass (google.com: domain of linux-kernel+bounces-97716-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <dc8d122a-22b7-4d17-abd9-66262af0b058@intel.com>
Date: Fri, 8 Mar 2024 17:06:29 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] selftests: x86: skip the tests if prerequisites aren't
 fulfilled
To: Muhammad Usama Anjum <usama.anjum@collabora.com>, Shuah Khan
	<shuah@kernel.org>, Dave Hansen <dave.hansen@linux.intel.com>, "Kirill A.
 Shutemov" <kirill.shutemov@linux.intel.com>, "Peter Zijlstra (Intel)"
	<peterz@infradead.org>, Weihong Zhang <weihong.zhang@intel.com>, Binbin Wu
	<binbin.wu@linux.intel.com>, angquan yu <angquan21@gmail.com>
CC: <kernel@collabora.com>, <linux-kselftest@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
References: <20240307183730.2858264-1-usama.anjum@collabora.com>
Content-Language: en-US
From: "Chang S. Bae" <chang.seok.bae@intel.com>
In-Reply-To: <20240307183730.2858264-1-usama.anjum@collabora.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eUJqNS90NW9NMW9BNmk0ZUM2OERPdldpYjRMUkhqNVhnSkpGSDZaTzI5VFp4?=
 =?utf-8?B?aDM1azM2ekVEdElqbFhrVys1dUtCZ2RhcjRNdWxWZWoxNnNSL2hZMGdRc3da?=
 =?utf-8?B?c2pScDhsbWZLSCtQQzNXandWMk9Da2psMldJNStJTHh5RDFzUkZNSUZob0Ji?=
 =?utf-8?B?bzllYklDcFNiS2dZalArSGJTSjFaZ2drbHBWZ0pvMWZKZzZUeUxzWkd4cWdh?=
 =?utf-8?B?RmQ1NXV5UUQ2Z3VsOUxGVEUzekVaSjVYaEs5Z2pwSmpTTGZJbUk1SmdaTE95?=
 =?utf-8?B?cDVaMlFaRDB6NFNiYTdBWllJVFdzRjFUZmRTTGdYbGZ3VTlLclQwQ0NxTzJS?=
 =?utf-8?B?NUZRMWszWUNvWlpXaUdGSytrVktTcW0weDBqb281LzVGYkxrU3NyazVmVlg3?=
 =?utf-8?B?RWFBdmRXV2I5N00vTVlOQTdCRENGZGlrc2kvM0FvMGxQeExnREQyaXNWRkFG?=
 =?utf-8?B?U0kwQk5uUG9BZVBIZWd4cS84Q3NEbEQ4VlN1R2ZMeEJGVjNXdzVkc3VQQkNm?=
 =?utf-8?B?cE91UE9qekx1czJWRXFleWJ2UkY2S0hYdUE4VHR4U0p2eEFGODltUGRQNitm?=
 =?utf-8?B?QU1YSjM5N2Y4M1grbHpZUlVTU09hM214dFpNdDZYcGZJWlRUNVVnNWszZ0g2?=
 =?utf-8?B?S0ZUUW5Ebm5WQ2RrbFRYaVJsMHNoTmpONEFuNnBOamY2MkFIK1JMY05WenlQ?=
 =?utf-8?B?SGlQRXFNeE5qa1VxSTZHN2s5cmxMWmJFVEE4ai82cmg3ejc0em15K1Q1WFZ5?=
 =?utf-8?B?N0FsNnBvTlYrak44c3Ntckk3eHI3TkxTellSbVMreVpzdXlwc3h4aysxbzNp?=
 =?utf-8?B?T2p1NnBYbS9Wa1VheWR5eVMwa25qSTJXcG1GcTVXVWNDZ0NEQ05zb0pMakVp?=
 =?utf-8?B?bWdKNlZzRlYyNURabnFoQXR4NE82OWN3d1laNjFReHVUU1JDaXk3OXhQMzN4?=
 =?utf-8?B?SDk1QjFHamtxMEcrOGZyUDd0b0lrNjVPTlJCVm9YVGZCbDU0ZnkydHlkT0pv?=
 =?utf-8?B?TWl4eVR1eVZPcTdYL2hKZjUrNDk3VG1nU2g0bXlVamhYQkloem5hZGJMaURq?=
 =?utf-8?B?bzdvanA3SFRhVWlDVXpyMXF1OFY3REt2SEFoVnhHRnl3YmNydUtTek5TdVBG?=
 =?utf-8?B?RnMvbUtBNjNQVVk3SVQ1SXRMSkN3cVovdkVIVWFibWZoWTFoMVdnc1ZyV3ps?=
 =?utf-8?B?RUpNNEJKOFVXbklRTGlXMlB5SExhQU55aHRHZEtwdWpWL0ZGakJPN1hUd0JX?=
 =?utf-8?B?Z3c1bVppSVIxcGtjN0x5VysvdFN3U3E2Mlh3azIydnFBNDVMSmUzbjlRb3Mr?=
 =?utf-8?B?L09nbzdoKzhzZXZNRCtuYkFKM3p6WWlidGJXSUp4cWtnSXQzbXg4elUwcTkv?=
 =?utf-8?B?T3Q2aHNHcXg4R0VHaWIvV0Y0MjduUlBuWXB5YUtKSGZkcVJNeGNYZ0lUZ05W?=
 =?utf-8?B?OFNjYk5PSmE5L0xJQ2lPRm4wQ3gyM0pZT2J2V0VPMDlkbW5LbmtlSWkzMXJF?=
 =?utf-8?B?S1FDZzdLYTQweFpEVkgybUM2Mko0Q2ZLTktOWDMyaWV0QXdxQm5MVFJqUTZ5?=
 =?utf-8?B?NzVlWTI2TUhUWHBEUklnQ280ODZFWDhXZ25ZYlJxdGZxZS8vT0t6aklSODlL?=
 =?utf-8?B?dWRWMVVFbkJoMk9wcmkzQlpiRDFtYUpteWY2VmtQeUJXME1LVlQ0SnJzVUI0?=
 =?utf-8?B?QmYrZW45Mmp5UUQveVZHTzU5N3VDbUZzSytOd3QwcGJ5TXgwOFFML3hLT25P?=
 =?utf-8?B?a1orbUtsdmFnWWtYOW82SEhOWFpOaEtmbW5lMHAwd1JyYVllTFN4N0NxOTQ2?=
 =?utf-8?B?VGNUYlBRaW0vSWhCYmJKamFwSDFNVk1ZaGovMWNBanlBNncrVVJQK09yNkZF?=
 =?utf-8?B?eXY2NGx3NkdXS3hqNTJDeGNrQnVDRncvZU5CSnlvaDRyWGx0TVQycGFyQ0Jk?=
 =?utf-8?B?b056c0NOL3F2SEFoN2xBQ3hwK1RVRTVRbllReUlNK3BmRlluc08vbUorOUMx?=
 =?utf-8?B?UjlnNVN4SVFLdzRhZkJZUHM2ZnBTMW5IZ2dtamVCQVZzN2c0UXEvdDhwYXMr?=
 =?utf-8?B?LzBEVVpSMms3UHRjVzlISmtXNXYzbFBlUGpWZFpiV3RFQmU4cXpTcGtHVFRt?=
 =?utf-8?B?VXZyQW9Fd0tCR1R2MGVMbUo5UXB1eW13SzFFOExac1hVdWhUOVRFdm1IYW5B?=
 =?utf-8?B?YkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fdd94c70-f3c1-480b-709d-08dc3fd5285b
X-MS-Exchange-CrossTenant-AuthSource: MW4PR11MB7164.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2024 01:06:31.9113
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Qx8R97P22EZIolsYkdDsSXhB3mumvX4EptutOdva9WZI1IJmKbAo1IMEK5iVeT1oPC/oPspaG0v/6jcHu2LEBDtA4qV4Bk8QObZ086YRvEw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6437
X-OriginatorOrg: intel.com

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@intel.com/