linux-kernel - Re: [PATCH] selftests: resctrl: ignore builds for unsupported architectures

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <98d8f119-02f7-49af-a891-cb13bd9f9a2d@linuxfoundation.org>
Date: Tue, 13 Aug 2024 02:25:10 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Muhammad Usama Anjum <Usama.Anjum@...labora.com>
Cc: Fenghua Yu <fenghua.yu@...el.com>,
 Shaopeng Tan <tan.shaopeng@...fujitsu.com>, kernel@...labora.com,
 LKML <linux-kernel@...r.kernel.org>, linux-kselftest@...r.kernel.org,
 Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>,
 Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] selftests: resctrl: ignore builds for unsupported
 architectures

On 8/12/24 18:05, Reinette Chatre wrote:
> Hi Shuah,
> 
> On 8/12/24 3:49 PM, Shuah Khan wrote:
>> On 8/9/24 02:45, Ilpo Järvinen wrote:
>>> Adding Maciej.
>>>
>>> On Fri, 9 Aug 2024, Muhammad Usama Anjum wrote:
>>>> On 8/9/24 12:23 PM, Ilpo Järvinen wrote:
>>>>> On Fri, 9 Aug 2024, Muhammad Usama Anjum wrote:
>>>>>
>>>>>> This test doesn't have support for other architectures. Altough resctrl
>>>>>> is supported on x86 and ARM, but arch_supports_noncont_cat() shows that
>>>>>> only x86 for AMD and Intel are supported by the test.
>>>>>
>>>>> One does not follow from the other. arch_supports_noncont_cat() is only
>>>>> small part of the tests so saying "This test" based on a small subset of
>>>>> all tests is bogus. Also, I don't see any reason why ARCH_ARM could not be
>>>>> added and arch_supports_noncont_cat() adapted accordingly.
>>>> I'm not familiar with resctrl and the architectural part of it. Feel
>>>> free to fix it and ignore this patch.
>>>>
>>>> If more things are missing than just adjusting
>>>> arch_supports_noncont_cat(), the test should be turned off until proper
>>>> support is added to the test.
>>>>
>>>>>> We get build
>>>>>> errors when built for ARM and ARM64.
>>>>>
>>>>> As this seems the real reason, please quote any errors when you use them
>>>>> as justification so it can be reviewed if the reasoning is sound or not.
>>>>
>>>>    CC       resctrl_tests
>>>> In file included from resctrl.h:24,
>>>>                   from cat_test.c:11:
>>>> In function 'arch_supports_noncont_cat',
>>>>      inlined from 'noncont_cat_run_test' at cat_test.c:323:6:
>>>> ../kselftest.h:74:9: error: impossible constraint in 'asm'
>>>>     74 |         __asm__ __volatile__ ("cpuid\n\t"
>>>>         \
>>>>        |         ^~~~~~~
>>>> cat_test.c:301:17: note: in expansion of macro '__cpuid_count'
>>>>    301 |                 __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
>>>>        |                 ^~~~~~~~~~~~~
>>>> ../kselftest.h:74:9: error: impossible constraint in 'asm'
>>>>     74 |         __asm__ __volatile__ ("cpuid\n\t"
>>>>         \
>>>>        |         ^~~~~~~
>>>> cat_test.c:303:17: note: in expansion of macro '__cpuid_count'
>>>>    303 |                 __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
>>>>        |                 ^~~~~~~~~~~~~
>>>
>>> Okay, so it's specific to lack of CPUID. This seems a kselftest common
>>> level problem to me, since __cpuid_count() is provided in kselftest.h.
>>>
>>> Shuah (or others), what is the intended mechanism for selftests to know if
>>> it can be used or not since as is, it's always defined?
>> _cpuid_count() gets defined in ksefltest.h if it can't find it.
>>
>> As the comment says both gcc and cland probide __cpuid_count()
>>
>>    gcc cpuid.h provides __cpuid_count() since v4.4.
>>    Clang/LLVM cpuid.h provides  __cpuid_count() since v3.4.0.
>>
>>>
>>> I see some Makefiles use compile testing a trivial program to decide whether
>>> they build some x86_64 tests or not. Is that what should be done here too,
>>> test if __cpuid_count() compiles or not (and then build some #ifdeffery
>>> based on the result of that compile testing)?
>>>
>>
>> These build errors need to be fixed instead of restricting the build> In some cases when the test can't be supported on an architecture then it is okay
>> to suppress build. This is not a general solution to suppress build warnings
> 
> While there is an effort to support Arm in resctrl [1], this is not currently
> the case and the resctrl selftests as a consequence only support x86 with
> built-in assumptions that a test runs on either AMD or Intel. After the kernel gains support
> for Arm more changes will be needed for the resctrl tests to support another architecture
> so I do think the most appropriate change to address this build failure is to restrict
> resctrl tests to x86.
> 

Sounds good to me. This would be good case for suppressing test build.

>>
>> I would recommend against adding suppress build code when it can be fixed.
> 
> I expect after resctrl fs obtains support for Arm the resctrl selftests can be
> updated to support it with more fine grained architectural checks than a global
> enable/disable needed at this time.
> 
>>
>> Let's investigate this problem to fix it properly. I don't see any arm and arm64
>> maintainers and developers on this thread. It would be good to investigate to
>> see if this can be fixed.

thanks,
-- Shuah