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

From: Pengfei Xu <pengfei.xu@intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: shuah@kernel.org, linux-kselftest@vger.kernel.org,
	linux-kernel@vger.kernel.org, dave.hansen@linux.intel.com,
	sandipan@linux.ibm.com, fweimer@redhat.com,
	desnesn@linux.vnet.ibm.com, mingo@kernel.org,
	bauerman@linux.ibm.com, mpe@ellerman.id.au, msuchanek@suse.de,
	linux-mm@kvack.org, chang.seok.bae@intel.com, bp@suse.de,
	tglx@linutronix.de, hpa@zytor.com, x86@kernel.org,
	luto@kernel.org, heng.su@intel.com
Subject: Re: [PATCH V2 1/4] selftests: Provide local define of __cpuid_count()
Date: Sat, 16 Apr 2022 15:52:00 +0800	[thread overview]
Message-ID: <Ylp1oFenjgxWpM0D@xpf.sh.intel.com> (raw)
In-Reply-To: <7c49dbfe5bab04389ed84c516fcbfe31d66df880.1647360971.git.reinette.chatre@intel.com>

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
...
+#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

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
"
Thanks!
--Pengfei