linux-kernel - Re: [PATCH V2 1/4] selftests: Provide local define of __cpuid

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]
Date:   Tue, 19 Apr 2022 15:34:11 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Pengfei Xu <pengfei.xu@...el.com>
CC:     <shuah@...nel.org>, <linux-kselftest@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <dave.hansen@...ux.intel.com>,
        <sandipan@...ux.ibm.com>, <fweimer@...hat.com>,
        <desnesn@...ux.vnet.ibm.com>, <mingo@...nel.org>,
        <bauerman@...ux.ibm.com>, <mpe@...erman.id.au>,
        <msuchanek@...e.de>, <linux-mm@...ck.org>,
        <chang.seok.bae@...el.com>, <bp@...e.de>, <tglx@...utronix.de>,
        <hpa@...or.com>, <x86@...nel.org>, <luto@...nel.org>,
        <heng.su@...el.com>
Subject: Re: [PATCH V2 1/4] selftests: Provide local define of __cpuid_count()

Hi Pengfei,

On 4/18/2022 9:31 PM, Pengfei Xu wrote:
> On 2022-04-18 at 09:04:33 -0700, Reinette Chatre wrote:
>> Hi Pengfei,
>>
>> On 4/16/2022 12:52 AM, Pengfei Xu wrote:
>>> On 2022-03-15 at 09:44:25 -0700, Reinette Chatre wrote:
>>>> Some selftests depend on information provided by the CPUID instruction.
>>>> To support this dependency the selftests implement private wrappers for
>>>> CPUID.
>>>>
>>>> Duplication of the CPUID wrappers should be avoided.
>>>>
>>>> Both gcc and clang/LLVM provide __cpuid_count() macros but neither
>>>> the macro nor its header file are available in all the compiler
>>>> versions that need to be supported by the selftests. __cpuid_count()
>>>> as provided by gcc is available starting with gcc v4.4, so it is
>>>> not available if the latest tests need to be run in all the
>>>> environments required to support kernels v4.9 and v4.14 that
>>>> have the minimal required gcc v3.2.
>>>>
>>>> Provide a centrally defined macro for __cpuid_count() to help
>>>> eliminate the duplicate CPUID wrappers while continuing to
>>>> compile in older environments.
>>>>
>>>> Suggested-by: Shuah Khan <skhan@...uxfoundation.org>
>>>> Signed-off-by: Reinette Chatre <reinette.chatre@...el.com>
>>>> ---
>>>> Note to maintainers:
>>>> - Macro is identical to the one provided by gcc, but not liked by
>>>>   checkpatch.pl with message "Macros with complex values should
>>>>   be enclosed in parentheses". Similar style is used in kernel,
>>>>   for example in arch/x86/kernel/fpu/xstate.h.
>>>>
>>>>  tools/testing/selftests/kselftest.h | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
>>>> index f1180987492c..898d7b2fac6c 100644
>>>> --- a/tools/testing/selftests/kselftest.h
>>>> +++ b/tools/testing/selftests/kselftest.h
>>>> @@ -52,6 +52,21 @@
>>>> + * have __cpuid_count().
>>>> + */
>>>> +#ifndef __cpuid_count
>>>> +#define __cpuid_count(level, count, a, b, c, d)				\
>>>> +	__asm__ __volatile__ ("cpuid\n\t"				\
>>>> +			      : "=a" (a), "=b" (b), "=c" (c), "=d" (d)	\
>>>> +			      : "0" (level), "2" (count))
>>>> +#endif
>>>    Linux C check tool "scripts/checkpatch.pl" shows an error:
>>> "
>>> ERROR: Macros with complex values should be enclosed in parentheses
>>
>> I encountered this also and that is why this patch contains the "Note to
>> maintainers" above. It is not clear to me whether you considered the note
>> since your response does not acknowledge it.
>>
>   Sorry, I just made a suggestion to fix this problem mentioned by the script.
>   I didn't notice and reply for the note.
> 
>>> ...
>>> +#define __cpuid_count(level, count, a, b, c, d)                        \
>>> +       __asm__ __volatile__ ("cpuid\n\t"                               \
>>> +                             : "=a" (a), "=b" (b), "=c" (c), "=d" (d)  \
>>> +                             : "0" (level), "2" (count))
>>> "
>>> Googling:
>>> https://www.google.com/search?q=Macros+with+complex+values+should+be+enclosed+in+parentheses&rlz=1C1GCEB_enUS884US884&oq=Macros+with+complex+values+should+be+enclosed+in+parentheses&aqs=chrome.0.69i59j0i5i30l2.313j0j7&sourceid=chrome&ie=UTF-8
>>> -> https://stackoverflow.com/questions/8142280/why-do-we-need-parentheses-around-block-macro
>>
>> More information available in
>> https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html#Statement-Exprs
>> but from what I understand it does not apply to this macro. Even so, I do
>> not know what checkpatch.pl uses to determine that this is a "Macro with
>> complex values".
>>
>   Checked checkpatch.pl and it seems to suggest using ({ }) for any asm macro
>   definition.
> 
>>>
>>> Could we fix it as follow, shall we?
>>> "
>>> #ifndef __cpuid_count
>>> #define __cpuid_count(level, count, a, b, c, d) ({			\
>>> 	__asm__ __volatile__ ("cpuid\n\t"				\
>>> 			      : "=a" (a), "=b" (b), "=c" (c), "=d" (d)	\
>>> 			      : "0" (level), "2" (count))		\
>>> })
>>> #endif
>>> "
>>
>> Sure, I can do so.
>>
>   I just made a suggestion to fix the problem reported by the checkpatch.pl.
>   But I didn't think deeply enough before: I'm not sure is there any real
>   improvment or help after the fix.

In this case I would prefer to not implicitly follow the checkpatch.pl without
understanding what the concern is.

The goal of this change is to make the __cpuid_count() macro available
within kselftest and it does so by duplicating gcc's __cpuid_count() macro.

The macro style is not unique and you would, for example, encounter the same
checkpatch.pl complaint if you run:
./scripts/checkpatch.pl -f arch/x86/kernel/fpu/xstate.h

Reinette