[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <327cde12-daea-84ba-4b24-64fe12e89dea@intel.com>
Date: Fri, 17 Jun 2022 15:21:18 -0700
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: Shuah Khan <skhan@...uxfoundation.org>,
<linux-kselftest@...r.kernel.org>, <shuah@...nel.org>,
<linux-kernel@...r.kernel.org>
CC: <dave.hansen@...ux.intel.com>, <tglx@...utronix.de>, <bp@...e.de>
Subject: Re: [PATCH 2/2] selftests/x86/amx: Fix the test to avoid failure when
AMX is unavailable
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/
Powered by blists - more mailing lists