Re: [PATCH V2 1/4] selftests: Provide local define of __cpuid_count()

[Pengfei Xu <pengfei.xu@intel.com>]

On 2022-04-19 at 15:34:11 -0700, Reinette Chatre wrote:
> 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@linuxfoundation.org>
> >>>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.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
  Ok, no question from my side.

  Thanks!
  --Pengfei
> 
> Reinette