Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <a3ceede0-ebab-6586-dfd5-247c37b976ec@intel.com>
Date:   Tue, 19 Apr 2022 15:34:11 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.8.0
Subject: Re: [PATCH V2 1/4] selftests: Provide local define of __cpuid_count()
Content-Language: en-US
To:     Pengfei Xu <pengfei.xu@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>
References: <cover.1647360971.git.reinette.chatre@intel.com>
 <7c49dbfe5bab04389ed84c516fcbfe31d66df880.1647360971.git.reinette.chatre@intel.com>
 <Ylp1oFenjgxWpM0D@xpf.sh.intel.com>
 <50067c2d-5563-7d8c-f992-5fef787d4d38@intel.com>
 <Yl47IGLggYsFYolo@xpf.sh.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <Yl47IGLggYsFYolo@xpf.sh.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?V2VsZEE0TUJjYUtWd1BWbHlMUmR5b1UvTUYrUitNWGxnZ1Jza1ErZDFReXZZ?=
 =?utf-8?B?bzYvbDRPVkwzN2FrTVdGSEh6aU94alpwYjVxbExwTDdkQjgwQkJiMkVYTXFM?=
 =?utf-8?B?aW1GMVdDMzkwcDdGYzRtbjVNbEo0VlJseVdVSHcxbDU1c0V4UmtjV2NJT0R6?=
 =?utf-8?B?RWEyV05oNHNYUDlsOCtnRTMvVEpibUR2VUlIdS9tQVFOVmdtYy9uNDFJNFln?=
 =?utf-8?B?NFFGWWowUmVRZ0dlby9OWTdWczR4dXA0cEd4c2pFbEJkMUR4dmdzdEUxRzkz?=
 =?utf-8?B?ZjRhN3hBUlc1Wm9VQWhOOXdlVy9zM1kzeGVrd2ZKMjdTciswQVRVdklzNkt2?=
 =?utf-8?B?VWp5UGRVVWw2SjRTYmVMVkhQSWlORVovWmJ4eXNuSjMrQVgvZEp5MXQ2Tlky?=
 =?utf-8?B?Uk5PRW14RXJuamRtVUlYOFEyb0tPY2FYOTRpNEhxUkdBV0p3YmZBMk9Hc3Ji?=
 =?utf-8?B?ZzlaL0c4SGIrM3hMQ1ZSRUNJZnJvSzJjUC9jQ3ZmeXlYekRISkIyZEE3ZEJB?=
 =?utf-8?B?VFM1b1k4aU15RXIzUW9pYlFkZm5KTHp5WEJQQzU3WWowSzZ5ak01VlBDcTZ4?=
 =?utf-8?B?RVJXRm5oNDE3UmJ1a1V5bWw0cGxrVVQ4UEhpb3JLUy9aSktQRWN4cHh4K0xm?=
 =?utf-8?B?Q09lZ095cGtnaEVWdXFhblBUK1BSMGRCVWk2a3pBQ3QzbXE1SUgvcTJ4clc2?=
 =?utf-8?B?ODJzZ1laNHAwRnlDRDFQSDhNUmR1aWtad1JYV3Y3RmpQVklaemNDOHhNLytV?=
 =?utf-8?B?cmFtNitiYi9md2UwZGVUMTJ2bFIrVnZlWThmazdUaCtKTjZnTWZCb3VhemJ4?=
 =?utf-8?B?b0pFc093NHU2SUtRSStyT2FKdWZpVy9Lc00rOTU0b3RCN3Q4VzJsWWhLS0dk?=
 =?utf-8?B?NmNUZEVXQXZjTk9jRHRnRkgzZTVmSHhnYXVWNmZmSEMycG10cE9HNVBLbDFF?=
 =?utf-8?B?QjJkQmpoZ3ZnSXM0ZEg1bWpqQXJPSkZqVHE2MnhLMU1jUzBuV2ZYakNCR0lt?=
 =?utf-8?B?M3Jmd0JOeUMvVE1ZUlhKSVdTVFJ6dGdkYWRuekN3aWdmdXFwZGxRWmNQWlBh?=
 =?utf-8?B?VGdVNmtER1VKMElqQy9oN0hTU1Q4Z1czZG1vbkt0V0tLYkFmTzlVUzl5cFdN?=
 =?utf-8?B?dHhBMFBmTmFQbjRoU0J3eXEzOEtRajNKU09hVk4zVFkvRTVHcFhJai9jRU8z?=
 =?utf-8?B?Q0l1MGI3a0gwR3B6Q2lVUWFTK05LcXAwNldaRVloRndHaUxaMnJhVktJWi9r?=
 =?utf-8?B?VmdQa2xjQURyZFlnVWNrSEI4WFlER2F6TVloeXJBeHNBb0IyeUlBZlpqcmNr?=
 =?utf-8?B?Ri9FbzJFdjJ2ejgzZHVDazRMeVpxa2JRdVhTd3RScSszVHBUYVBlS2tXTitO?=
 =?utf-8?B?VmlWckVUaHlDUnprZkJ0WFJXS2lmZG1VVnEwS09XRnJZZVFnZVZmK0xxWVZC?=
 =?utf-8?B?TnlZWE94Uy9ib0tsUFVOd1hrcXFHb2FjaHcva2lIZGxiMGU3ZmZ5SlhoTWV6?=
 =?utf-8?B?L1dJTTdMWkZ5aDlMM2R5Q2xSa3F1TGNqOVVMbnpLYUptVmdxalEzazJEL3pL?=
 =?utf-8?B?OTVPeDlJQjgxNWExYi9JV3QyU0ZGZ3RKMjVZSzBUcGpVOWZ6cmp4TkN1bG1m?=
 =?utf-8?B?dERFRUVEOGtqYnY1UWZYcStJRGdxMFJpdFVWUHVUMmJKMEhDR2kvT0lEc0RY?=
 =?utf-8?B?Z0l5YjFiZVF4eUdqeHpIcDFxbjJySHNDa0FWUXltWE5tOExXQU5UNVZPaTlX?=
 =?utf-8?B?cEZ5S1R4TmhhZGRFZ0daR0w0cGx3dWxicXUxRksrNG40Rm5abi80TzcxQm5D?=
 =?utf-8?B?QmZ2TjdSb3NUWUo5K2g3UE51Tmx2UGRyMEoyZThaa1k2OWdTbGZnakg0a295?=
 =?utf-8?B?dktKUGRjeTRkMStQclpCU2VxQ0ZBbHJrRzMxaWtIQk5ZN2pjbXg3ZERpS3RM?=
 =?utf-8?B?NW5CUCt0QnBMSnYwbjJ6T3BadzhZcEJGZXgxOE9kSWUyUHNPRXVkSEoyUWtE?=
 =?utf-8?B?c0ZscWdzTFF5aHRsTTMzcW9yNmlkZkEyZkZ1QVJSYzlwcXlSRDl4OWJHcmd3?=
 =?utf-8?B?OVpoSWg3VUlqbTl2TmN6SlhvMmhnOGZoUjZqbkZzZlhIKzRjV1hKZXNkdXRB?=
 =?utf-8?B?OXJoeVBwZ3QxemtFQTV6bnNaOWIyL0xpR2l6dTRQbUFmYkVDSk9za09RbUFj?=
 =?utf-8?B?ai9rVGs0WUFJYXVzWWJwTUlNYVJlOXNyQlpoN0tBRVJmR3FWUUZWUlJZZ0pR?=
 =?utf-8?B?RUlsT1pTR2lDaUc0YnhWemRYTHhyWU9ReGl2WWl3Um5CNFRvYktreC9ZRHdt?=
 =?utf-8?B?MkxKSVVjY1dSK0U1dXh4U0t4S3o1djA3eVc2NW5sd2ZEMUZSSUVvQmxLQjg5?=
 =?utf-8?Q?NeBowhofsZ1FtjL0=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8220f024-89e4-4aed-2bbd-08da2254bc5c
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Apr 2022 22:34:16.2863
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AyRRMq5iq5B1BppqyNhzeFKkmV0sPnLlvu8uHwIg8PGmG7dveIeDRwboRUIYfrRyA9pO/vY3Pk44spU2uwZ1tbMfKHa36ypqFHvYfWvtNRk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB1423
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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

Reinette