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

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

[Ilpo Järvinen]

First, generalize resctrl selftest non-contiguous CAT check to not
assume non-AMD vendor implies Intel. Second, improve 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).

The last patch might still require some work on which symbol the
conditional in kselftest.h is implemented. I could not find any
pre-existing one that could be used. Perhaps somebody who's more
familiar with the kselftest build system has a better suggestion on
which symbol the logic should be based at?

Ilpo Järvinen (3):
  selftests/resctrl: Generalize non-contiguous CAT check
  selftests/resctrl: Always initialize ecx to avoid build warnings
  [RFC] 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 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>
---
 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

[PATCH 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>
---
 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

[RFC PATCH 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.

Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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 0/3] selftests: Fix cpuid / vendor checking build issues

[Shuah Khan]

On 8/13/24 04:45, Ilpo Järvinen wrote:
> First, generalize resctrl selftest non-contiguous CAT check to not
> assume non-AMD vendor implies Intel. Second, improve 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).
> 
> The last patch might still require some work on which symbol the
> conditional in kselftest.h is implemented. I could not find any
> pre-existing one that could be used. Perhaps somebody who's more
> familiar with the kselftest build system has a better suggestion on
> which symbol the logic should be based at?
> 
> Ilpo Järvinen (3):
>    selftests/resctrl: Generalize non-contiguous CAT check
>    selftests/resctrl: Always initialize ecx to avoid build warnings
>    [RFC] 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(-)
> 

These changes look good to me. Can you send the RFC patch without the RFC tag
for me to pull in? I don't apply RFC patches.

Usama, does this fix the problem you are seeing?

Hi Reinette - do these look okay to you? Can you give me an ack if they do?

thanks,
-- Shuah

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

[Muhammad Usama Anjum]

On 8/13/24 3:45 PM, 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>

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

-- 
BR,
Muhammad Usama Anjum

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

[Muhammad Usama Anjum]

On 8/13/24 3:45 PM, 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>

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

-- 
BR,
Muhammad Usama Anjum

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

[Muhammad Usama Anjum]

On 8/13/24 3:45 PM, 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.
> 
> Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

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

-- 
BR,
Muhammad Usama Anjum

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

[Muhammad Usama Anjum]

On 8/21/24 11:30 AM, Shuah Khan wrote:
> On 8/13/24 04:45, Ilpo Järvinen wrote:
>> First, generalize resctrl selftest non-contiguous CAT check to not
>> assume non-AMD vendor implies Intel. Second, improve 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).
>>
>> The last patch might still require some work on which symbol the
>> conditional in kselftest.h is implemented. I could not find any
>> pre-existing one that could be used. Perhaps somebody who's more
>> familiar with the kselftest build system has a better suggestion on
>> which symbol the logic should be based at?
>>
>> Ilpo Järvinen (3):
>>    selftests/resctrl: Generalize non-contiguous CAT check
>>    selftests/resctrl: Always initialize ecx to avoid build warnings
>>    [RFC] 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(-)
>>
> 
> These changes look good to me. Can you send the RFC patch without the
> RFC tag
> for me to pull in? I don't apply RFC patches.
> 
> Usama, does this fix the problem you are seeing?
Yeah, build errors are resolved.

> 
> Hi Reinette - do these look okay to you? Can you give me an ack if they do?
> 
> thanks,
> -- Shuah
> 
-- 
BR,
Muhammad Usama Anjum

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

[Ilpo Järvinen]

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

On Thu, 22 Aug 2024, Muhammad Usama Anjum wrote:

> On 8/21/24 11:30 AM, Shuah Khan wrote:
> > On 8/13/24 04:45, Ilpo Järvinen wrote:
> >> First, generalize resctrl selftest non-contiguous CAT check to not
> >> assume non-AMD vendor implies Intel. Second, improve 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).
> >>
> >> The last patch might still require some work on which symbol the
> >> conditional in kselftest.h is implemented. I could not find any
> >> pre-existing one that could be used. Perhaps somebody who's more
> >> familiar with the kselftest build system has a better suggestion on
> >> which symbol the logic should be based at?
> >>
> >> Ilpo Järvinen (3):
> >>    selftests/resctrl: Generalize non-contiguous CAT check
> >>    selftests/resctrl: Always initialize ecx to avoid build warnings
> >>    [RFC] 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(-)
> >>
> > 
> > These changes look good to me. Can you send the RFC patch without the
> > RFC tag
> > for me to pull in? I don't apply RFC patches.
> > 
> > Usama, does this fix the problem you are seeing?
>
> Yeah, build errors are resolved.

Great, thanks for testing!

I'll send v2 with RFC removed soon.

> > Hi Reinette - do these look okay to you? Can you give me an ack if they do?
> > 
> > thanks,
> > -- Shuah
> > 
> 

-- 
 i.