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

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

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.

  Thanks!
  --Pengfei

> Reinette