Received-SPF: pass (google.com: domain of linux-kernel+bounces-99282-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <c3362840-365e-40cb-80fe-895aa2d979ec@collabora.com>
Date: Mon, 11 Mar 2024 22:02:27 +0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>, kernel@collabora.com,
 linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] selftests: x86: skip the tests if prerequisites aren't
 fulfilled
To: "Chang S. Bae" <chang.seok.bae@intel.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>
References: <20240307183730.2858264-1-usama.anjum@collabora.com>
 <dc8d122a-22b7-4d17-abd9-66262af0b058@intel.com>
Content-Language: en-US
From: Muhammad Usama Anjum <usama.anjum@collabora.com>
In-Reply-To: <dc8d122a-22b7-4d17-abd9-66262af0b058@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 3/9/24 6:06 AM, Chang S. Bae wrote:
> 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():
In this patch, I'm just focusing on skip login on existing code. I'll make
this change when I'll transform the entire test to TAP.

> 
> +#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.
We are logging that data size/offset are invalid if either eax or ebx are
invalid and then we are skipping. Not sure what you are asking me to change.

> 
> Thanks,
> Chang
> 
> [1]
> https://lore.kernel.org/lkml/327cde12-daea-84ba-4b24-64fe12e89dea@intel.com/
> 

-- 
BR,
Muhammad Usama Anjum