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;
Message-ID: <327cde12-daea-84ba-4b24-64fe12e89dea@intel.com>
Date:   Fri, 17 Jun 2022 15:21:18 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH 2/2] selftests/x86/amx: Fix the test to avoid failure when
 AMX is unavailable
Content-Language: en-CA
To:     Shuah Khan <skhan@linuxfoundation.org>,
        <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>
References: <20220401221014.13556-1-chang.seok.bae@intel.com>
 <20220401221014.13556-3-chang.seok.bae@intel.com>
 <aaab50d2-592c-69e4-58a6-0a0926669de3@linuxfoundation.org>
From:   "Chang S. Bae" <chang.seok.bae@intel.com>
In-Reply-To: <aaab50d2-592c-69e4-58a6-0a0926669de3@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YmVlZ1NQN3BxaVU2MXA3ODhCTmtOaUhDUFFCV0JVOTN5WVZxYzJPd3lDZUtB?=
 =?utf-8?B?bVl3QW5helZ0MGZPUHhneWJERmNQbDFxUVlMemNGKzU3dzE4Q2ZYYkZlNE5S?=
 =?utf-8?B?aDhmYStGRWh4ZzFESWYxVUFER1BHRTVWMUJkelBXakkrSmZGM0ExYzhjYjZU?=
 =?utf-8?B?VUNaYm00UGdRakIxU3ViMEdSbmY1SnBtUUw5cnBSTEtzdFpmN0ZsamIxSHBE?=
 =?utf-8?B?OGVVWU1kRFNSZU5kbzBvdTFWK3NpaWFCd2VLakxaT1lPOTlDa3VucFFvUE03?=
 =?utf-8?B?V0dFeFlkVU9qV1lObCtLWnQ0YjdKUzFwR2JBZUphaTdFWjNPU2VXK0FLelFQ?=
 =?utf-8?B?dS9LdWx3QWxFYWlvZ2hoQ2hYdWNZbmZkUDJNRU9rd3hxQm4zRlJ4R1lzbkxI?=
 =?utf-8?B?amhBc3BKTG16ZndiWFlnVWozakNub1ZPSHdVcnlFOHNwNjJ6di84c05ieEw4?=
 =?utf-8?B?YVZ5YXd5djVRRGc2MTYrNjlSaXNyUkRHRHpKcXRKbk9KdjFwNFhSLzdqc3Np?=
 =?utf-8?B?UHczUkwzRHArMVNpNUxTQ3hwYUl2TW03eUp3SHh0RHFVTU9HTi91WWltYWNh?=
 =?utf-8?B?aDdQVGRSVmZBVmdYMG01M0ljZkcrY1NJQXgzdWg0YXREN2ZkVDltQTJFU0Vl?=
 =?utf-8?B?ZFltT1IzSDMyenhtU2dSbkgrQmNTMTZWZlJHRFNia3FhMGpwNk1UeDA5TVBi?=
 =?utf-8?B?WlBpVS92YXB3cklhb1dabWpZaEJzcWpjcEwyeEJnWlpZMCtHZEVCVUNwRitr?=
 =?utf-8?B?RFQ4Rnl0RWRTRThNQU9xUHVtUTIwaFAyeElGa3oxNjVtYlBuakMvWkVGeDZa?=
 =?utf-8?B?ZUV0d1VHQWVTaUdKOGhMSHFVdW5aUmFLY0lvV01RaEVzSVE5a0dodVBtSHVP?=
 =?utf-8?B?RHhZbHI4a1RjMUxTZjZHS0FGNmFVV1o0bTVoYlYzR2tiRmd5dE5hemFJb2Z5?=
 =?utf-8?B?eS9OakJhbHF5U0tOOGIxYyt0aVY1YVp5eHgxN3ppNmxXanIrTFBFSUVmNnFM?=
 =?utf-8?B?b1gxTEs1SDJtTDBEMTgwL0VlaTZaWjkzelJSNUtWVFNyRTdYemVNakJpS0NW?=
 =?utf-8?B?MG1jRTAxZHhQeFNGTmZPL1JxQzZxM0NnM2RnOWs3TnlpVmJCcnpiei9yRnMw?=
 =?utf-8?B?ZHQ4M1E4MEMxMTdmRXcrSCs0bkYxczFCMjg2V0ZQcGtEeERjcm5kTzZkUVZm?=
 =?utf-8?B?ZGlyVzNMNkpORUp4QU55RnE4UWJNMk4rSm5iSDk2a2FXZitVUlpFenFLaFlo?=
 =?utf-8?B?aWo4MUlGMUZ6Q0dmbjhpckxKTndGZjBhS3RhR3NRT0ljRWMwQy84QXFVVU1W?=
 =?utf-8?B?YVZKWnJTNHlQUHp4WGNHc3UvTWVsRDN0ODBQYUNFY3VqSFRKYWZyazJwMlBL?=
 =?utf-8?B?Y1FvZUNCNWNjdDVkbCtBSVd6cC92YmdxbENYbFN4QXFya1ZvMGE2ZDA2V2Nk?=
 =?utf-8?B?UDBWOWhJM2JERjNTM3hDT2YvdzYxa004ZmJsRk5NZE9ORnJzbEE0bnlYWEM4?=
 =?utf-8?B?R3M5bWZVM0FFM0pqZ1pKZ0UvNmZQcExNeWM4aS9uNm1uTXdZNEludEFKaHB3?=
 =?utf-8?B?WlpBVzUvZzVDM2pjaUhOTE5mK0trMFNNS1I3dkxReTNFUEtUR1lKRGhSSmZw?=
 =?utf-8?B?QjhCRG9yVURuTWhNYWpiMnNOR0RCVnZTV3dFb2RTTm1mTjJkM2lzVEVWWHhu?=
 =?utf-8?B?cmttMnNTSnBYQU9xRDFSWGZ2T2E1cWdRbWU5TVptSVhpakdHMG1NZ3lVMkxz?=
 =?utf-8?B?ak1EcExJOGdMZTloaDltV0dSSWVWT3paWGcvUE5CMDJQNDI2bkRQZ2Rneldi?=
 =?utf-8?B?SVZLZU81UTluVEx3NDJWRDFzMHE0R1pMYXJYRGlQMWs2UEVrU1g0STkxK3RM?=
 =?utf-8?B?bGEydUx0czl2UFNDVVQ0SlFyVFdwTGNRTUVjSTUvMWJuK1RjYnlVTWJocmtp?=
 =?utf-8?B?N1NoUVBzVXU3SE5EeVc0Y0ZsNUZVMTVXdkczUmFzR0RBaXR0K2FodGw5c0d1?=
 =?utf-8?B?RTlocksvckdkd0kzRGM0R2hJOUZlMkE3bldXUHo0ZzhPVGdUbzRrM0cvL1F0?=
 =?utf-8?B?N1UvakM1WFI4bWFGWGhBWGU1Mm5TZ01UU1dpR2RXdGRwYXRvUlVPdDBna2R5?=
 =?utf-8?B?M2J0bUlSNUlrZFA4d0ZGZ0JiZzBvT3RQOUhBdllSSkkxK1hVOU1Nb2dVZ1Nw?=
 =?utf-8?B?eVRNVy9kZnF2WHpoUHR3aGRzTXNsTG9lbVNCZWlRdU1Qb0dzN05mTU5URGt3?=
 =?utf-8?B?ZEhhRi9hMCs5VUtIbjdJTkFCOEVOSS9BcDdKK2VnTENLQkxyWGxiL1RnVlhP?=
 =?utf-8?B?TG5RZmcrSUlJRmFpWklhb1FMTE0vQVgrSnFDNUUwYVl2di9YL3paek5iN2pm?=
 =?utf-8?Q?6F48cmtAm5oRH85k=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c9cedbad-abbf-421b-8c6c-08da50afb4e7
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4849.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jun 2022 22:21:28.2092
 (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: GtKYpB0fwrasczJZ9Wz17v4+VaKBpMi97q0kGYnCMOfTUM3FnoErethlzsMrl2JVDGPjDWt5lpr7EPaIc8lJpXlBgnUM8nqvyzQ8DpEnse8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB3614
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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/