[PATCH v2 0/3] selftests: Fix cpuid / vendor checking build issues

[PATCH v2 0/3] selftests: Fix cpuid / vendor checking build issues

[Ilpo Järvinen]

This series first generalizes resctrl selftest non-contiguous CAT check
to not assume non-AMD vendor implies Intel. Second, it improves
kselftest common parts and resctrl selftest such that the use of
__cpuid_count() does not lead into a build failure (happens at least on
ARM).

While ARM does not currently support resctrl features, there's an
ongoing work to enable resctrl support also for it on the kernel side.
In any case, a common header such as kselftest.h should have a proper
fallback in place for what it provides, thus it seems justified to fix
this common level problem on the common level rather than e.g.
disabling build for resctrl selftest for archs lacking resctrl support.

v2:
- Removed RFC from the last patch & added Fixes and tags
- Fixed the error message's line splits
- Noted down the reason for void casts in the stub

Ilpo Järvinen (3):
  selftests/resctrl: Generalize non-contiguous CAT check
  selftests/resctrl: Always initialize ecx to avoid build warnings
  kselftest: Provide __cpuid_count() stub on non-x86 archs

 tools/testing/selftests/kselftest.h        |  6 +++++
 tools/testing/selftests/lib.mk             |  4 ++++
 tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++---------
 3 files changed, 27 insertions(+), 11 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] selftests/resctrl: Generalize non-contiguous CAT check

[Ilpo Järvinen]

arch_supports_noncont_cat() checks if vendor is ARCH_AMD and if that is
not true, ARCH_INTEL is assumed which might not be true either because
get_vendor() can also return zero if neither AMD nor Intel is detected.

Generalize the vendor check using switch/case logic and return false
for unknown vendors.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/resctrl/cat_test.c | 26 +++++++++++++---------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 742782438ca3..51a1cb6aac34 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -292,19 +292,25 @@ static bool arch_supports_noncont_cat(const struct resctrl_test *test)
 {
 	unsigned int eax, ebx, ecx, edx;
 
-	/* AMD always supports non-contiguous CBM. */
-	if (get_vendor() == ARCH_AMD)
+	switch (get_vendor()) {
+	case ARCH_AMD:
+		/* AMD always supports non-contiguous CBM. */
 		return true;
 
-	/* Intel support for non-contiguous CBM needs to be discovered. */
-	if (!strcmp(test->resource, "L3"))
-		__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
-	else if (!strcmp(test->resource, "L2"))
-		__cpuid_count(0x10, 2, eax, ebx, ecx, edx);
-	else
-		return false;
+	case ARCH_INTEL:
+		/* Intel support for non-contiguous CBM needs to be discovered. */
+		if (!strcmp(test->resource, "L3"))
+			__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
+		else if (!strcmp(test->resource, "L2"))
+			__cpuid_count(0x10, 2, eax, ebx, ecx, edx);
+		else
+			return false;
+
+		return ((ecx >> 3) & 1);
 
-	return ((ecx >> 3) & 1);
+	default:
+		return false;
+	}
 }
 
 static int noncont_cat_run_test(const struct resctrl_test *test,
-- 
2.39.2

Re: [PATCH v2 1/3] selftests/resctrl: Generalize non-contiguous CAT check

[Reinette Chatre]

Hi Ilpo,

On 8/22/24 1:11 AM, Ilpo Järvinen wrote:
> arch_supports_noncont_cat() checks if vendor is ARCH_AMD and if that is
> not true, ARCH_INTEL is assumed which might not be true either because
> get_vendor() can also return zero if neither AMD nor Intel is detected.
> 
> Generalize the vendor check using switch/case logic and return false
> for unknown vendors.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---

Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v2 2/3] selftests/resctrl: Always initialize ecx to avoid build warnings

[Ilpo Järvinen]

To avoid warnings when __cpuid_count() is an empty stub, always
initialize ecx because it is used in the return statement.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/resctrl/cat_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 51a1cb6aac34..9882c5d19408 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -290,7 +290,7 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
 
 static bool arch_supports_noncont_cat(const struct resctrl_test *test)
 {
-	unsigned int eax, ebx, ecx, edx;
+	unsigned int eax, ebx, ecx = 0, edx;
 
 	switch (get_vendor()) {
 	case ARCH_AMD:
-- 
2.39.2

Re: [PATCH v2 2/3] selftests/resctrl: Always initialize ecx to avoid build warnings

[Reinette Chatre]

Hi Ilpo,

On 8/22/24 1:11 AM, Ilpo Järvinen wrote:
> To avoid warnings when __cpuid_count() is an empty stub, always
> initialize ecx because it is used in the return statement.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---

Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v2 3/3] kselftest: Provide __cpuid_count() stub on non-x86 archs

[Ilpo Järvinen]

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@collabora.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.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;				\
+} 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
+
 # Enables to extend CFLAGS and LDFLAGS from command line, e.g.
 # make USERCFLAGS=-Werror USERLDFLAGS=-static
 CFLAGS += $(USERCFLAGS)
-- 
2.39.2

Re: [PATCH v2 3/3] kselftest: Provide __cpuid_count() stub on non-x86 archs

[Reinette Chatre]

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@collabora.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.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/

Re: [PATCH v2 3/3] kselftest: Provide __cpuid_count() stub on non-x86 archs

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5295 bytes --]

On Thu, 22 Aug 2024, Reinette Chatre wrote:

> 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@collabora.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> > Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.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.

The reason is entirely separate from what resctrl selftest expects. 
a-d are output parameters for __cpuid_count(), they need this treatment 
because they are typically not initialized but set by __cpuid_count() so 
if __cpuid_count() is doing literally nothing, nothing touches those 
four variables leading to unused variable warning.

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

No, the same issue does not exist for input parameters because it would be 
a valid warning. Passing uninitialized (and thus unused) input variable 
is wrong so the calling logic is wrong. Thus, I don't see how the same 
error could ever occur in a legitimate case for those two parameters.

> > +} 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/

Ah, sorry. I'd missed that comment because it started mid-paragraph.

I wonder if I can safely touch ARCH or if there are caveats and it would 
be better to use some other makefile variable.

-- 
 i.

Re: [PATCH v2 3/3] kselftest: Provide __cpuid_count() stub on non-x86 archs

[Reinette Chatre]

Hi Ilpo,

On 8/23/24 3:47 AM, Ilpo Järvinen wrote:
> On Thu, 22 Aug 2024, Reinette Chatre wrote:
>> 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@collabora.com>
>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>>> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.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.
> 
> The reason is entirely separate from what resctrl selftest expects.
> a-d are output parameters for __cpuid_count(), they need this treatment
> because they are typically not initialized but set by __cpuid_count() so
> if __cpuid_count() is doing literally nothing, nothing touches those
> four variables leading to unused variable warning.
> 
>> 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.
> 
> No, the same issue does not exist for input parameters because it would be
> a valid warning. Passing uninitialized (and thus unused) input variable
> is wrong so the calling logic is wrong. Thus, I don't see how the same
> error could ever occur in a legitimate case for those two parameters.

If I understand correctly, the scenarios below are legitimate cases and
will produce compile warnings with this patch applied. It is not obvious
to me that the calling logic is wrong in these cases. If the output
parameters get special treatment to avoid compile warnings, should input
parameters not also?

scenario 1:
	unsigned int level = 0x10, count = 1;
	unsigned int a, b, c, d;

	__cpuid_count(level, count, a, b, c ,d);

Above produces "unused variable" warnings for level and count.

scenario 2:
	unsigned int level, count, a, b, c, d;

	level = 0x10;
	count = 1;
	__cpuid_count(level, count, a, b, c ,d);

Above produces "set but not used" warnings for level and count.

The changelog states that the goal of this change is to produce an
empty stub. To me this creates expectation of what we are used to
and expect from if it would be an actual empty stub. For example,
static inline void __cpuid_count(unsigned level, unsigned count,
				 unsigned int a, unsigned b,
				 unsigned int c, unsigned d) { }

>>> +} 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/
> 
> Ah, sorry. I'd missed that comment because it started mid-paragraph.

Where are comments required to start?

Reinette

Re: [PATCH v2 3/3] kselftest: Provide __cpuid_count() stub on non-x86 archs

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7768 bytes --]

On Fri, 23 Aug 2024, Reinette Chatre wrote:
> On 8/23/24 3:47 AM, Ilpo Järvinen wrote:
> > On Thu, 22 Aug 2024, Reinette Chatre wrote:
> > > 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@collabora.com>
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> > > > Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.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.
> > 
> > The reason is entirely separate from what resctrl selftest expects.
> > a-d are output parameters for __cpuid_count(), they need this treatment
> > because they are typically not initialized but set by __cpuid_count() so
> > if __cpuid_count() is doing literally nothing, nothing touches those
> > four variables leading to unused variable warning.
> > 
> > > 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.
> > 
> > No, the same issue does not exist for input parameters because it would be
> > a valid warning. Passing uninitialized (and thus unused) input variable
> > is wrong so the calling logic is wrong. Thus, I don't see how the same
> > error could ever occur in a legitimate case for those two parameters.
> 
> If I understand correctly, the scenarios below are legitimate cases and
> will produce compile warnings with this patch applied. It is not obvious
> to me that the calling logic is wrong in these cases. If the output
> parameters get special treatment to avoid compile warnings, should input
> parameters not also?
> 
> scenario 1:
> 	unsigned int level = 0x10, count = 1;
> 	unsigned int a, b, c, d;
> 
> 	__cpuid_count(level, count, a, b, c ,d);
> 
> Above produces "unused variable" warnings for level and count.
> 
> scenario 2:
> 	unsigned int level, count, a, b, c, d;
> 
> 	level = 0x10;
> 	count = 1;
> 	__cpuid_count(level, count, a, b, c ,d);
> 
> Above produces "set but not used" warnings for level and count.

Ah, so you meant a different warning. Yes, I'll add void casts for 
those input parameters as well to avoid this.

> The changelog states that the goal of this change is to produce an
> empty stub. To me this creates expectation of what we are used to
> and expect from if it would be an actual empty stub. For example,
> static inline void __cpuid_count(unsigned level, unsigned count,
> 				 unsigned int a, unsigned b,
> 				 unsigned int c, unsigned d) { }

For void functions, yes. But if it would return int, it wouldn't be 
literally "empty", that is, it is still be empty stub but does something 
so a very literal interpretation of "empty" is flawed anyway. But I can 
drop empty word from there, "stub" seems enough for the purpose.

If you on the other hand meant macros cannot be called "stub", what should 
they be called if not "stub"?

> > > > +} 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/
> > 
> > Ah, sorry. I'd missed that comment because it started mid-paragraph.
> 
> Where are comments required to start?

Obviously it's not a requirement, but I'd put each recommendation into own 
paragraph to maximize likelihoods its not missed. Despite the paragraph 
containing answers to two different questions, my mind I only registered 
the answer for the trivial program question. Again, I'm sorry about that.

-- 
 i.

Re: [PATCH v2 3/3] kselftest: Provide __cpuid_count() stub on non-x86 archs

[Reinette Chatre]

Hi Ilpo,

On 8/26/24 3:44 AM, Ilpo Järvinen wrote:
> On Fri, 23 Aug 2024, Reinette Chatre wrote:
>> On 8/23/24 3:47 AM, Ilpo Järvinen wrote:
>>> On Thu, 22 Aug 2024, Reinette Chatre wrote:
>>>> 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@collabora.com>
>>>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>>>> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>>>>> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.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.
>>>
>>> The reason is entirely separate from what resctrl selftest expects.
>>> a-d are output parameters for __cpuid_count(), they need this treatment
>>> because they are typically not initialized but set by __cpuid_count() so
>>> if __cpuid_count() is doing literally nothing, nothing touches those
>>> four variables leading to unused variable warning.
>>>
>>>> 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.
>>>
>>> No, the same issue does not exist for input parameters because it would be
>>> a valid warning. Passing uninitialized (and thus unused) input variable
>>> is wrong so the calling logic is wrong. Thus, I don't see how the same
>>> error could ever occur in a legitimate case for those two parameters.
>>
>> If I understand correctly, the scenarios below are legitimate cases and
>> will produce compile warnings with this patch applied. It is not obvious
>> to me that the calling logic is wrong in these cases. If the output
>> parameters get special treatment to avoid compile warnings, should input
>> parameters not also?
>>
>> scenario 1:
>> 	unsigned int level = 0x10, count = 1;
>> 	unsigned int a, b, c, d;
>>
>> 	__cpuid_count(level, count, a, b, c ,d);
>>
>> Above produces "unused variable" warnings for level and count.
>>
>> scenario 2:
>> 	unsigned int level, count, a, b, c, d;
>>
>> 	level = 0x10;
>> 	count = 1;
>> 	__cpuid_count(level, count, a, b, c ,d);
>>
>> Above produces "set but not used" warnings for level and count.
> 
> Ah, so you meant a different warning. Yes, I'll add void casts for
> those input parameters as well to avoid this.
> 
>> The changelog states that the goal of this change is to produce an
>> empty stub. To me this creates expectation of what we are used to
>> and expect from if it would be an actual empty stub. For example,
>> static inline void __cpuid_count(unsigned level, unsigned count,
>> 				 unsigned int a, unsigned b,
>> 				 unsigned int c, unsigned d) { }
> 
> For void functions, yes. But if it would return int, it wouldn't be
> literally "empty", that is, it is still be empty stub but does something
> so a very literal interpretation of "empty" is flawed anyway. But I can
> drop empty word from there, "stub" seems enough for the purpose.

Considering the context of the discussion my focus was on expectations
surrounding parameter handling. In this context the "custom" appears
to be that stubs do not trigger compile warnings for parameters
that are not used.

> 
> If you on the other hand meant macros cannot be called "stub", what should
> they be called if not "stub"?

I do not know if there is a more appropriate term. "stub" seems appropriate
to me.

> 
>>>>> +} 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/
>>>
>>> Ah, sorry. I'd missed that comment because it started mid-paragraph.
>>
>> Where are comments required to start?
> 
> Obviously it's not a requirement, but I'd put each recommendation into own
> paragraph to maximize likelihoods its not missed. Despite the paragraph
> containing answers to two different questions, my mind I only registered
> the answer for the trivial program question. Again, I'm sorry about that.

Ack. No problem.

Reinette