linux-kernel - Re: [PATCH v2 3/3] kselftest: Provide __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]

Message-ID: <cd09f5e0-2353-4223-b02c-aa8461c1dbe5@intel.com>
Date: Thu, 22 Aug 2024 09:27:41 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, "Muhammad
 Usama Anjum" <usama.anjum@...labora.com>, Shuah Khan <shuah@...nel.org>,
	<linux-kselftest@...r.kernel.org>
CC: <linux-kernel@...r.kernel.org>, Shaopeng Tan
	<tan.shaopeng@...fujitsu.com>, Fenghua Yu <fenghua.yu@...el.com>
Subject: Re: [PATCH v2 3/3] kselftest: Provide __cpuid_count() stub on non-x86
 archs

Hi Ilpo,

On 8/22/24 1:11 AM, Ilpo Järvinen wrote:
> Building resctrl selftest fails on ARM because it uses __cpuid_count()
> that fails the build with error:
> 
>    CC       resctrl_tests
> In file included from resctrl.h:24,
>                   from cat_test.c:11:
> In function 'arch_supports_noncont_cat',
>      inlined from 'noncont_cat_run_test' at cat_test.c:323:6:
> ../kselftest.h:74:9: error: impossible constraint in 'asm'
>     74 |         __asm__ __volatile__ ("cpuid\n\t"       \
>        |         ^~~~~~~
> cat_test.c:301:17: note: in expansion of macro '__cpuid_count'
>    301 |                 __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
>        |                 ^~~~~~~~~~~~~
> ../kselftest.h:74:9: error: impossible constraint in 'asm'
>     74 |         __asm__ __volatile__ ("cpuid\n\t"       \
>        |         ^~~~~~~
> cat_test.c:303:17: note: in expansion of macro '__cpuid_count'
>    303 |                 __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
>        |                 ^~~~~~~~~~~~~
> 
> The resctrl selftest would run that code only on Intel CPUs but
> as is, the code cannot be build at all.
> 
> Provide an empty stub for __cpuid_count() if it is not supported to
> allow build to succeed. The stub casts its arguments to void to avoid
> causing variable unused warnings.
> 
> Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test")
> Reported-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Tested-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
> Reviewed-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
> ---
> 
> v2:
> - Removed RFC & added Fixes and Tested-by
> - Fixed the error message's line splits
> - Noted down the reason for void casts in the stub
> ---
>   tools/testing/selftests/kselftest.h | 6 ++++++
>   tools/testing/selftests/lib.mk      | 4 ++++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
> index b8967b6e29d5..71593add1b39 100644
> --- a/tools/testing/selftests/kselftest.h
> +++ b/tools/testing/selftests/kselftest.h
> @@ -70,10 +70,16 @@
>    * have __cpuid_count().
>    */
>   #ifndef __cpuid_count
> +#ifdef HAVE_CPUID
>   #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))
> +#else
> +#define __cpuid_count(level, count, a, b, c, d)	do {			\
> +	(void)a; (void)b; (void)c; (void)d;				\

The changelog states that this casting to void is done to avoid unused variable warnings.
It is thus unexpected that not all parameters obtain the same casting treatment. It looks
to me as though this only targets the resctrl selftest usage where the "level" and "count"
parameters are constants. This is intended as a general kselftest solution so I believe
that all parameters would need this casting to handle the cases where "level" and/or
"count" are variables.

> +} while (0)
> +#endif
>   #endif
>   
>   /* define kselftest exit codes */
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index d6edcfcb5be8..236db9b24037 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -199,6 +199,10 @@ clean: $(if $(TEST_GEN_MODS_DIR),clean_mods_dir)
>   # Build with _GNU_SOURCE by default
>   CFLAGS += -D_GNU_SOURCE=
>   
> +ifeq ($(ARCH),$(filter $(ARCH),x86 x86_64))
> +CFLAGS += -DHAVE_CPUID=
> +endif

My earlier comment [1] when this work started remains. This technique depends
on environment passing ARCH, which cannot be guaranteed. Looking at other
usages of ARCH in the kselftest Makefiles it seems that the pattern is to
initialize ARCH with "uname -m" if unset.

> +
>   # Enables to extend CFLAGS and LDFLAGS from command line, e.g.
>   # make USERCFLAGS=-Werror USERLDFLAGS=-static
>   CFLAGS += $(USERCFLAGS)

Reinette

[1] https://lore.kernel.org/lkml/db16db55-5f68-484f-ba9f-3312b41bf426@intel.com/