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@xxxxxxxxx/
>
--
BR,
Muhammad Usama Anjum
