Received-SPF: pass (google.com: domain of linux-kernel+bounces-100021-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <21f8dbe3-9de2-41ad-a8bd-61d66cb38e90@collabora.com>
Date: Tue, 12 Mar 2024 14:26:28 +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>
 <c3362840-365e-40cb-80fe-895aa2d979ec@collabora.com>
 <1cacbd08-1131-4be4-b318-58c05afda2de@intel.com>
Content-Language: en-US
From: Muhammad Usama Anjum <usama.anjum@collabora.com>
In-Reply-To: <1cacbd08-1131-4be4-b318-58c05afda2de@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 3/11/24 10:39 PM, Chang S. Bae wrote:
> On 3/11/2024 10:02 AM, Muhammad Usama Anjum wrote:
>> On 3/9/24 6:06 AM, Chang S. Bae wrote:
>>> On 3/7/2024 10:37 AM, Muhammad Usama Anjum wrote:
>>>
>>>> -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.
> 
> You intention seems to skip the test when AMX is not available. 
Yes,

> But this
> function should only be invoked when AMX is actually available, not as part
> of the feature availability check. 
How can we check if AMX is available or not?

> Therefore, I think this change is not
> relevant. Also, if we encounter invalid TILEDATA CPUID, it should be a
> reason to *fail* the test, rather than calling out a skip, right?
I see. But once we check if AMX is available.

> 
> Thanks,
> Chang
> 

-- 
BR,
Muhammad Usama Anjum