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

[PATCH v4 0/4] 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
selftests 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.

I've dropped reviewed and tested by tags from the last patch in v3 due
to major changes into the makefile logic. So it would be helpful if
Muhammad could retest with this version.

Acquiring ARCH in lib.mk will likely allow some cleanup into some
subdirectory makefiles but that is left as future work because this
series focuses in fixing cpuid/build.

v4:
- New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
  (would cause __cpuid_count() related build fail otherwise)

v3:
- Remove "empty" wording
- Also cast input parameters to void
- Initialize ARCH from uname -m if not set (this might allow cleaning
  up some other makefiles but that is left as future work)

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 (4):
  selftests/resctrl: Generalize non-contiguous CAT check
  selftests/resctrl: Always initialize ecx to avoid build warnings
  selftests/x86: don't clobber CFLAGS
  kselftest: Provide __cpuid_count() stub on non-x86 archs

 tools/testing/selftests/kselftest.h        |  6 +++++
 tools/testing/selftests/lib.mk             |  6 +++++
 tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++---------
 tools/testing/selftests/x86/Makefile       |  4 +++-
 4 files changed, 32 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH v4 1/4] 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

[PATCH v4 2/4] 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

[PATCH v4 3/4] selftests/x86: don't clobber CFLAGS

[Ilpo Järvinen]

The x86 selftests makefile clobbers CFLAGS preventing lib.mk from
making the necessary adjustments into CFLAGS. This would lead to a
build failure after upcoming change which wants to add -DHAVE_CPUID=
into CFLAGS.

Reorder CFLAGS initialization in x86 selftest. Place the constant part
of CFLAGS initialization before inclusion of lib.mk but leave adding
KHDR_INCLUDES into CFLAGS into the existing position because
KHDR_INCLUDES might be generated inside lib.mk.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v4:
- New patch

 tools/testing/selftests/x86/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index 5c8757a25998..88a6576de92f 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -1,4 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
+CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
+
 all:
 
 include ../lib.mk
@@ -35,7 +37,7 @@ BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64)
 BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32))
 BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64))
 
-CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES)
+CFLAGS += $(KHDR_INCLUDES)
 
 # call32_from_64 in thunks.S uses absolute addresses.
 ifeq ($(CAN_BUILD_WITH_NOPIE),1)
-- 
2.39.2

Re: [PATCH v4 3/4] selftests/x86: don't clobber CFLAGS

[Reinette Chatre]

Hi Ilpo,

On 9/3/24 7:45 AM, Ilpo Järvinen wrote:
> The x86 selftests makefile clobbers CFLAGS preventing lib.mk from
> making the necessary adjustments into CFLAGS. This would lead to a
> build failure after upcoming change which wants to add -DHAVE_CPUID=
> into CFLAGS.
> 
> Reorder CFLAGS initialization in x86 selftest. Place the constant part
> of CFLAGS initialization before inclusion of lib.mk but leave adding
> KHDR_INCLUDES into CFLAGS into the existing position because
> KHDR_INCLUDES might be generated inside lib.mk.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v4:
> - New patch
> 
>   tools/testing/selftests/x86/Makefile | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> index 5c8757a25998..88a6576de92f 100644
> --- a/tools/testing/selftests/x86/Makefile
> +++ b/tools/testing/selftests/x86/Makefile
> @@ -1,4 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0
> +CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
> +
>   all:
>   
>   include ../lib.mk
> @@ -35,7 +37,7 @@ BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64)
>   BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32))
>   BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64))
>   
> -CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES)
> +CFLAGS += $(KHDR_INCLUDES)
>   
>   # call32_from_64 in thunks.S uses absolute addresses.
>   ifeq ($(CAN_BUILD_WITH_NOPIE),1)

These changes are becoming less obvious to me. The first two
red flags are:
- Most other top level Makefiles assign KHDR_INCLUDES to CFLAGS
   *before* including lib.mk
- What current Makefiles do matches the guidance from
   Documentation/dev-tools/kselftest.rst as per example (verbatim copy):
     CFLAGS = $(KHDR_INCLUDES)
     TEST_GEN_PROGS := close_range_test
     include ../lib.mk

Looking closer it is not clear to me why lib.mk sets KHDR_INCLUDES.
As I understand the usage is intended to be:
   make TARGETS="target" -C tools/testing/selftests
This means that it is tools/testing/selftests/Makefile that will
run first and it ensures that KHDR_INCLUDES is set and supports
the usage documented in Documentation/dev-tools/kselftest.rst

One usage that a change like in this patch could support is
for users to be able to run "make" from within the test
directory self ... but considering the current KHDR_INCLUDES
custom this does not seem to be supported? (but we cannot
know which tests/users rely on this behavior)

Looking further I also noticed that
tools/testing/selftests/Makefile even sets ARCH (but does
not export it). When considering the next patch it almost looks
like what is missing is for tools/testing/selftests/Makefile to
"export ARCH" ... but that potentially impacts the Makefiles that
do manipulation of ARCH.

I initially started to look at this because of the
resctrl impact, but I clearly am not familiar enough
with the kselftest build files to understand the
impacts nor provide guidance. I do hope the kselftest
folks can help here.

Reinette

Re: [PATCH v4 3/4] selftests/x86: don't clobber CFLAGS

[Ilpo Järvinen]

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

On Tue, 3 Sep 2024, Reinette Chatre wrote:
> On 9/3/24 7:45 AM, Ilpo Järvinen wrote:
> > The x86 selftests makefile clobbers CFLAGS preventing lib.mk from
> > making the necessary adjustments into CFLAGS. This would lead to a
> > build failure after upcoming change which wants to add -DHAVE_CPUID=
> > into CFLAGS.
> > 
> > Reorder CFLAGS initialization in x86 selftest. Place the constant part
> > of CFLAGS initialization before inclusion of lib.mk but leave adding
> > KHDR_INCLUDES into CFLAGS into the existing position because
> > KHDR_INCLUDES might be generated inside lib.mk.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > v4:
> > - New patch
> > 
> >   tools/testing/selftests/x86/Makefile | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/x86/Makefile
> > b/tools/testing/selftests/x86/Makefile
> > index 5c8757a25998..88a6576de92f 100644
> > --- a/tools/testing/selftests/x86/Makefile
> > +++ b/tools/testing/selftests/x86/Makefile
> > @@ -1,4 +1,6 @@
> >   # SPDX-License-Identifier: GPL-2.0
> > +CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
> > +
> >   all:
> >     include ../lib.mk
> > @@ -35,7 +37,7 @@ BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64)
> >   BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32))
> >   BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64))
> >   -CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES)
> > +CFLAGS += $(KHDR_INCLUDES)
> >     # call32_from_64 in thunks.S uses absolute addresses.
> >   ifeq ($(CAN_BUILD_WITH_NOPIE),1)
> 
> These changes are becoming less obvious to me. The first two
> red flags are:
> - Most other top level Makefiles assign KHDR_INCLUDES to CFLAGS
>   *before* including lib.mk

Most toplevel makefiles assigned CFLAGS before including lib.mk so x86 
selftest was an exception overwriting CFLAGS after including lib.mk.
That looks like a real problem/bug and you don't seem to contest lib.mk 
having the right to alter CFLAGS. So I'm still convinced this patch is
necessary independent of the cpuid/resctrl selftest issue.

I believe most of those Makefile are _buggy_ wrt. KHDR_INCLUDES but I 
don't care where $(KHDR_INCLUDES) goes, it's irrelevant for the problem
I'm trying to solve which is the CFLAGS clobber. ...I just didn't want to
add yet another problem by moving KHDR_INCLUDES before including lib.mk. 
I'm fine with leaving that can of worms for somebody else to sort out :-).

> - What current Makefiles do matches the guidance from
>   Documentation/dev-tools/kselftest.rst as per example (verbatim copy):
>     CFLAGS = $(KHDR_INCLUDES)
>     TEST_GEN_PROGS := close_range_test
>     include ../lib.mk

I'm not objecting moving the entire CFLAGS line before including lib.mk
in the x86 selftests makefile if that is what you'd want me to do?

> Looking closer it is not clear to me why lib.mk sets KHDR_INCLUDES.

So are you suggesting the commit a52540522c95 ("selftests/landlock: Fix 
out-of-tree builds") added it for no reason? The commit message indicates
it was added on purpose but I don't fully understand what "to other test 
collections when building in their directory" means.

> As I understand the usage is intended to be:
>   make TARGETS="target" -C tools/testing/selftests
> This means that it is tools/testing/selftests/Makefile that will
> run first and it ensures that KHDR_INCLUDES is set and supports
> the usage documented in Documentation/dev-tools/kselftest.rst
> 
> One usage that a change like in this patch could support is
> for users to be able to run "make" from within the test
> directory self ... but considering the current KHDR_INCLUDES
> custom this does not seem to be supported? (but we cannot
> know which tests/users rely on this behavior)

Perhaps "when building in their directory" is exactly that?

I don't doubt the makefiles are very full of problems.

> Looking further I also noticed that
> tools/testing/selftests/Makefile even sets ARCH (but does
> not export it). When considering the next patch it almost looks
> like what is missing is for tools/testing/selftests/Makefile to
> "export ARCH" ... but that potentially impacts the Makefiles that
> do manipulation of ARCH.

The ARCH handling in various Makefiles is another mess.

> I initially started to look at this because of the
> resctrl impact, but I clearly am not familiar enough
> with the kselftest build files to understand the
> impacts nor provide guidance. I do hope the kselftest
> folks can help here.

Luckily this seems to have caught Shuah attention now so hopefully we've 
soon some reasonable resolution to ARCH dependent building and code 
fragments so each selftest doesn't need to come up their own way of 
handling it. :-)

-- 
 i.

[PATCH v4 4/4] 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.

Define HAVE_CPUID in lib.mk based on ARCH (x86 or x86_64). If ARCH is
not set, acquire it using uname -m.

Provide a stub for __cpuid_count() if HAVE_CPUID is not present to
allow build to succeed. The stub casts its arguments to void to avoid
causing "unused variable" or "set but not used" 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>
---
v3:
- Remove "empty" wording
- Also cast input parameters to void
- Initialize ARCH from uname -m if not set (this might allow cleaning
  up some other makefiles but that is left as future work)
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      | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index b8967b6e29d5..9c4bfbf107f1 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)level; (void)count; (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..8e3069926153 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -23,6 +23,8 @@ CLANG_TARGET_FLAGS_x86_64       := x86_64-linux-gnu
 
 # Default to host architecture if ARCH is not explicitly given.
 ifeq ($(ARCH),)
+ARCH := $(shell uname -m 2>/dev/null || echo not)
+ARCH := $(shell echo $(ARCH) | sed -e s/i.86/x86/)
 CLANG_TARGET_FLAGS := $(shell $(CLANG) -print-target-triple)
 else
 CLANG_TARGET_FLAGS := $(CLANG_TARGET_FLAGS_$(ARCH))
@@ -199,6 +201,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 v4 0/4] selftests: Fix cpuid / vendor checking build issues

[Shuah Khan]

On 9/3/24 08:45, Ilpo Järvinen wrote:
> This series first generalizes resctrl selftest non-contiguous CAT check
> to not assume non-AMD vendor implies Intel. Second, it improves
> selftests 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.
> 
> I've dropped reviewed and tested by tags from the last patch in v3 due
> to major changes into the makefile logic. So it would be helpful if
> Muhammad could retest with this version.
> 
> Acquiring ARCH in lib.mk will likely allow some cleanup into some
> subdirectory makefiles but that is left as future work because this
> series focuses in fixing cpuid/build.

> 
> v4:
> - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
>    (would cause __cpuid_count() related build fail otherwise)
> 
I don't like the way this patch series is mushrooming. I am not
convinced that changes to lib.mk and x86 Makefile are necessary.

I will take a look at this to see if this can be simplified.

thanks,
-- Shuah

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

[Ilpo Järvinen]

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

On Tue, 3 Sep 2024, Shuah Khan wrote:

> On 9/3/24 08:45, Ilpo Järvinen wrote:
> > This series first generalizes resctrl selftest non-contiguous CAT check
> > to not assume non-AMD vendor implies Intel. Second, it improves
> > selftests 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.
> > 
> > I've dropped reviewed and tested by tags from the last patch in v3 due
> > to major changes into the makefile logic. So it would be helpful if
> > Muhammad could retest with this version.
> > 
> > Acquiring ARCH in lib.mk will likely allow some cleanup into some
> > subdirectory makefiles but that is left as future work because this
> > series focuses in fixing cpuid/build.
> 
> > 
> > v4:
> > - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
> >    (would cause __cpuid_count() related build fail otherwise)
> > 
> I don't like the way this patch series is mushrooming. I am not
> convinced that changes to lib.mk and x86 Makefile are necessary.

I didn't like it either what I found from the various makefiles. I think 
there are many things done which conflict with what lib.mk seems to try to 
do.

I tried to ask in the first submission what test I should use in the 
header file as I'm not very familiar with how arch specific is done in 
userspace in the first place nor how it should be done within kselftest 
framework.

> I will take a look at this to see if this can be simplified.

Sure, thanks.

-- 
 i.

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

[Shuah Khan]

On 9/4/24 06:18, Ilpo Järvinen wrote:
> On Tue, 3 Sep 2024, Shuah Khan wrote:
> 
>> On 9/3/24 08:45, Ilpo Järvinen wrote:
>>> This series first generalizes resctrl selftest non-contiguous CAT check
>>> to not assume non-AMD vendor implies Intel. Second, it improves
>>> selftests 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.
>>>
>>> I've dropped reviewed and tested by tags from the last patch in v3 due
>>> to major changes into the makefile logic. So it would be helpful if
>>> Muhammad could retest with this version.
>>>
>>> Acquiring ARCH in lib.mk will likely allow some cleanup into some
>>> subdirectory makefiles but that is left as future work because this
>>> series focuses in fixing cpuid/build.
>>
>>>
>>> v4:
>>> - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
>>>     (would cause __cpuid_count() related build fail otherwise)
>>>
>> I don't like the way this patch series is mushrooming. I am not
>> convinced that changes to lib.mk and x86 Makefile are necessary.
> 
> I didn't like it either what I found from the various makefiles. I think
> there are many things done which conflict with what lib.mk seems to try to
> do.
> 

Some of it by desig. lib.mk offers framework for common things. There
are provisions to override like in the case of x86, powerpc. lib.mk
tries to be flexible as well.

> I tried to ask in the first submission what test I should use in the
> header file as I'm not very familiar with how arch specific is done in
> userspace in the first place nor how it should be done within kselftest
> framework.
> 

Thoughts on cpuid:

- It is x86 specific. Moving this to kselftest.h was done to avoid
   duplicate. However now we are running into arm64/arm compile
   errors due to this which need addressing one way or the other.

I have some ideas on how to solve this - but I need answers to
the following questions.

This is a question for you and Usama.

- Does resctrl run on arm64/arm and what's the output?
- Can all other tests in resctrl other tests except
   noncont_cat_run_test?
- If so send me the output.

thanks,
-- Shuah

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

[Ilpo Järvinen]

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

On Wed, 4 Sep 2024, Shuah Khan wrote:

> On 9/4/24 06:18, Ilpo Järvinen wrote:
> > On Tue, 3 Sep 2024, Shuah Khan wrote:
> > 
> > > On 9/3/24 08:45, Ilpo Järvinen wrote:
> > > > This series first generalizes resctrl selftest non-contiguous CAT check
> > > > to not assume non-AMD vendor implies Intel. Second, it improves
> > > > selftests 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.
> > > > 
> > > > I've dropped reviewed and tested by tags from the last patch in v3 due
> > > > to major changes into the makefile logic. So it would be helpful if
> > > > Muhammad could retest with this version.
> > > > 
> > > > Acquiring ARCH in lib.mk will likely allow some cleanup into some
> > > > subdirectory makefiles but that is left as future work because this
> > > > series focuses in fixing cpuid/build.
> > > 
> > > > 
> > > > v4:
> > > > - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
> > > >     (would cause __cpuid_count() related build fail otherwise)
> > > > 
> > > I don't like the way this patch series is mushrooming. I am not
> > > convinced that changes to lib.mk and x86 Makefile are necessary.
> > 
> > I didn't like it either what I found from the various makefiles. I think
> > there are many things done which conflict with what lib.mk seems to try to
> > do.
> > 
> 
> Some of it by desig. lib.mk offers framework for common things. There
> are provisions to override like in the case of x86, powerpc. lib.mk
> tries to be flexible as well.
> 
> > I tried to ask in the first submission what test I should use in the
> > header file as I'm not very familiar with how arch specific is done in
> > userspace in the first place nor how it should be done within kselftest
> > framework.
> > 
> 
> Thoughts on cpuid:
> 
> - It is x86 specific. Moving this to kselftest.h was done to avoid
>   duplicate. However now we are running into arm64/arm compile
>   errors due to this which need addressing one way or the other.
> 
> I have some ideas on how to solve this - but I need answers to
> the following questions.
> 
> This is a question for you and Usama.
> 
> - Does resctrl run on arm64/arm and what's the output?
> - Can all other tests in resctrl other tests except
>   noncont_cat_run_test?
> - If so send me the output.

Hi Shuah,

As mentioned in my coverletter above, resctrl does not currently support 
arm but there's an ongoing work to add arm support. On kernel side it 
requires major refactoring to move non-arch specific stuff out from 
arch/x86 so has (predictably) taken long time.

The resctrl selftests are mostly written in arch independent way (*) but 
there's also a way to limit a test only to CPUs from a particular vendor.
And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs 
(to read the supported flag), it's not needed even on AMD CPUs as they 
always support non-contiguous CAT bitmask.

So to summarize, it would be possible to disable resctrl test for non-x86 
but it does not address the underlying problem with cpuid which will just 
come back later I think.

Alternatively, if there's some a good way in C code to do ifdeffery around 
that cpuid call, I could make that too, but I need to know which symbol to 
use for that ifdef.

(*) The cache topology may make some selftest unusable on new archs but 
not the selftest code itself.


-- 
 i.

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

[Shuah Khan]

On 9/4/24 06:54, Ilpo Järvinen wrote:
> On Wed, 4 Sep 2024, Shuah Khan wrote:
> 
>> On 9/4/24 06:18, Ilpo Järvinen wrote:
>>> On Tue, 3 Sep 2024, Shuah Khan wrote:
>>>
>>>> On 9/3/24 08:45, Ilpo Järvinen wrote:
>>>>> This series first generalizes resctrl selftest non-contiguous CAT check
>>>>> to not assume non-AMD vendor implies Intel. Second, it improves
>>>>> selftests 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.
>>>>>
>>>>> I've dropped reviewed and tested by tags from the last patch in v3 due
>>>>> to major changes into the makefile logic. So it would be helpful if
>>>>> Muhammad could retest with this version.
>>>>>
>>>>> Acquiring ARCH in lib.mk will likely allow some cleanup into some
>>>>> subdirectory makefiles but that is left as future work because this
>>>>> series focuses in fixing cpuid/build.
>>>>
>>>>>
>>>>> v4:
>>>>> - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
>>>>>      (would cause __cpuid_count() related build fail otherwise)
>>>>>
>>>> I don't like the way this patch series is mushrooming. I am not
>>>> convinced that changes to lib.mk and x86 Makefile are necessary.
>>>
>>> I didn't like it either what I found from the various makefiles. I think
>>> there are many things done which conflict with what lib.mk seems to try to
>>> do.
>>>
>>
>> Some of it by desig. lib.mk offers framework for common things. There
>> are provisions to override like in the case of x86, powerpc. lib.mk
>> tries to be flexible as well.
>>
>>> I tried to ask in the first submission what test I should use in the
>>> header file as I'm not very familiar with how arch specific is done in
>>> userspace in the first place nor how it should be done within kselftest
>>> framework.
>>>
>>
>> Thoughts on cpuid:
>>
>> - It is x86 specific. Moving this to kselftest.h was done to avoid
>>    duplicate. However now we are running into arm64/arm compile
>>    errors due to this which need addressing one way or the other.
>>
>> I have some ideas on how to solve this - but I need answers to
>> the following questions.
>>
>> This is a question for you and Usama.
>>
>> - Does resctrl run on arm64/arm and what's the output?
>> - Can all other tests in resctrl other tests except
>>    noncont_cat_run_test?
>> - If so send me the output.
> 
> Hi Shuah,
> 
> As mentioned in my coverletter above, resctrl does not currently support
> arm but there's an ongoing work to add arm support. On kernel side it
> requires major refactoring to move non-arch specific stuff out from
> arch/x86 so has (predictably) taken long time.
> 
> The resctrl selftests are mostly written in arch independent way (*) but
> there's also a way to limit a test only to CPUs from a particular vendor.
> And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs
> (to read the supported flag), it's not needed even on AMD CPUs as they
> always support non-contiguous CAT bitmask.
> 
> So to summarize, it would be possible to disable resctrl test for non-x86
> but it does not address the underlying problem with cpuid which will just
> come back later I think.
> 
> Alternatively, if there's some a good way in C code to do ifdeffery around
> that cpuid call, I could make that too, but I need to know which symbol to
> use for that ifdef.
> 
> (*) The cache topology may make some selftest unusable on new archs but
> not the selftest code itself.
> 
> 

I agree that suppressing resctrl build is not a solution. The real problem
is is in defining __cpuid_count() in common code path.

I fixed it and send patch in. As I was testing I noticed the following on
AMD platform:

- it ran the L3_NONCONT_CAT test which is expected.

# # Starting L3_NONCONT_CAT test ...
# # Mounting resctrl to "/sys/fs/resctrl"
# ARCH_AMD - supports non-contiguous CBM
# # Write schema "L3:0=ff" to resctrl FS
# # Write schema "L3:0=fc3f" to resctrl FS
# ok 5 L3_NONCONT_CAT: test

- It went on to run L2_NONCONT_CAT - failed

# ok 6 # SKIP Hardware does not support L2_NONCONT_CAT or L2_NONCONT_CAT is disabled

Does it make sense to run both L3_NONCONT_CAT and L2_NONCONT_CAT
on AMD? Maybe it is? resctrl checks L3 or L2 support on Intel.

Anyway - the problem is fixed now. Please review and test.

thanks,
-- Shuah

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

[Reinette Chatre]

Hi Shuah,

On 9/5/24 11:06 AM, Shuah Khan wrote:
> On 9/4/24 06:54, Ilpo Järvinen wrote:
>> On Wed, 4 Sep 2024, Shuah Khan wrote:
>>
>>> On 9/4/24 06:18, Ilpo Järvinen wrote:
>>>> On Tue, 3 Sep 2024, Shuah Khan wrote:
>>>>
>>>>> On 9/3/24 08:45, Ilpo Järvinen wrote:
>>>>>> This series first generalizes resctrl selftest non-contiguous CAT check
>>>>>> to not assume non-AMD vendor implies Intel. Second, it improves
>>>>>> selftests 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.
>>>>>>
>>>>>> I've dropped reviewed and tested by tags from the last patch in v3 due
>>>>>> to major changes into the makefile logic. So it would be helpful if
>>>>>> Muhammad could retest with this version.
>>>>>>
>>>>>> Acquiring ARCH in lib.mk will likely allow some cleanup into some
>>>>>> subdirectory makefiles but that is left as future work because this
>>>>>> series focuses in fixing cpuid/build.
>>>>>
>>>>>>
>>>>>> v4:
>>>>>> - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
>>>>>>      (would cause __cpuid_count() related build fail otherwise)
>>>>>>
>>>>> I don't like the way this patch series is mushrooming. I am not
>>>>> convinced that changes to lib.mk and x86 Makefile are necessary.
>>>>
>>>> I didn't like it either what I found from the various makefiles. I think
>>>> there are many things done which conflict with what lib.mk seems to try to
>>>> do.
>>>>
>>>
>>> Some of it by desig. lib.mk offers framework for common things. There
>>> are provisions to override like in the case of x86, powerpc. lib.mk
>>> tries to be flexible as well.
>>>
>>>> I tried to ask in the first submission what test I should use in the
>>>> header file as I'm not very familiar with how arch specific is done in
>>>> userspace in the first place nor how it should be done within kselftest
>>>> framework.
>>>>
>>>
>>> Thoughts on cpuid:
>>>
>>> - It is x86 specific. Moving this to kselftest.h was done to avoid
>>>    duplicate. However now we are running into arm64/arm compile
>>>    errors due to this which need addressing one way or the other.
>>>
>>> I have some ideas on how to solve this - but I need answers to
>>> the following questions.
>>>
>>> This is a question for you and Usama.
>>>
>>> - Does resctrl run on arm64/arm and what's the output?
>>> - Can all other tests in resctrl other tests except
>>>    noncont_cat_run_test?
>>> - If so send me the output.
>>
>> Hi Shuah,
>>
>> As mentioned in my coverletter above, resctrl does not currently support
>> arm but there's an ongoing work to add arm support. On kernel side it
>> requires major refactoring to move non-arch specific stuff out from
>> arch/x86 so has (predictably) taken long time.
>>
>> The resctrl selftests are mostly written in arch independent way (*) but
>> there's also a way to limit a test only to CPUs from a particular vendor.
>> And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs
>> (to read the supported flag), it's not needed even on AMD CPUs as they
>> always support non-contiguous CAT bitmask.
>>
>> So to summarize, it would be possible to disable resctrl test for non-x86
>> but it does not address the underlying problem with cpuid which will just
>> come back later I think.
>>
>> Alternatively, if there's some a good way in C code to do ifdeffery around
>> that cpuid call, I could make that too, but I need to know which symbol to
>> use for that ifdef.
>>
>> (*) The cache topology may make some selftest unusable on new archs but
>> not the selftest code itself.
>>
>>
> 
> I agree that suppressing resctrl build is not a solution. The real problem
> is is in defining __cpuid_count() in common code path.
> 
> I fixed it and send patch in. As I was testing I noticed the following on
> AMD platform:
> 
> - it ran the L3_NONCONT_CAT test which is expected.
> 
> # # Starting L3_NONCONT_CAT test ...
> # # Mounting resctrl to "/sys/fs/resctrl"
> # ARCH_AMD - supports non-contiguous CBM
> # # Write schema "L3:0=ff" to resctrl FS
> # # Write schema "L3:0=fc3f" to resctrl FS
> # ok 5 L3_NONCONT_CAT: test
> 
> - It went on to run L2_NONCONT_CAT - failed

It is not intended to appear as a failure but instead just skipping of
a test since the platform does not support the feature being tested.

> 
> # ok 6 # SKIP Hardware does not support L2_NONCONT_CAT or L2_NONCONT_CAT is disabled

The output looks as intended. When I run the test on an Intel system without
L2 CAT the output looks the same.

> 
> Does it make sense to run both L3_NONCONT_CAT and L2_NONCONT_CAT
> on AMD? Maybe it is? resctrl checks L3 or L2 support on Intel.

The selftests test the features as exposed by the generic resctrl kernel
subsystem instead of relying on its own inventory of what features
need to be checked for which vendor. selftests will thus only
test L3 or L2 if resctrl kernel subsystem indicates it is supported on
underlying platform. Only afterwards may it use platform specific
knowledge to help validate the feature.
In this scenario resctrl indicated that L2 CAT is not supported
by underlying platform and the test was skipped. It looks good
to me.

> 
> Anyway - the problem is fixed now. Please review and test.
> 

Thank you very much. Will do.

Reinette

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

[Shuah Khan]

On 9/5/24 14:43, Reinette Chatre wrote:
> Hi Shuah,
> 
> On 9/5/24 11:06 AM, Shuah Khan wrote:
>> On 9/4/24 06:54, Ilpo Järvinen wrote:
>>> On Wed, 4 Sep 2024, Shuah Khan wrote:
>>>
>>>> On 9/4/24 06:18, Ilpo Järvinen wrote:
>>>>> On Tue, 3 Sep 2024, Shuah Khan wrote:
>>>>>
>>>>>> On 9/3/24 08:45, Ilpo Järvinen wrote:
>>>>>>> This series first generalizes resctrl selftest non-contiguous CAT check
>>>>>>> to not assume non-AMD vendor implies Intel. Second, it improves
>>>>>>> selftests 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.
>>>>>>>
>>>>>>> I've dropped reviewed and tested by tags from the last patch in v3 due
>>>>>>> to major changes into the makefile logic. So it would be helpful if
>>>>>>> Muhammad could retest with this version.
>>>>>>>
>>>>>>> Acquiring ARCH in lib.mk will likely allow some cleanup into some
>>>>>>> subdirectory makefiles but that is left as future work because this
>>>>>>> series focuses in fixing cpuid/build.
>>>>>>
>>>>>>>
>>>>>>> v4:
>>>>>>> - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
>>>>>>>      (would cause __cpuid_count() related build fail otherwise)
>>>>>>>
>>>>>> I don't like the way this patch series is mushrooming. I am not
>>>>>> convinced that changes to lib.mk and x86 Makefile are necessary.
>>>>>
>>>>> I didn't like it either what I found from the various makefiles. I think
>>>>> there are many things done which conflict with what lib.mk seems to try to
>>>>> do.
>>>>>
>>>>
>>>> Some of it by desig. lib.mk offers framework for common things. There
>>>> are provisions to override like in the case of x86, powerpc. lib.mk
>>>> tries to be flexible as well.
>>>>
>>>>> I tried to ask in the first submission what test I should use in the
>>>>> header file as I'm not very familiar with how arch specific is done in
>>>>> userspace in the first place nor how it should be done within kselftest
>>>>> framework.
>>>>>
>>>>
>>>> Thoughts on cpuid:
>>>>
>>>> - It is x86 specific. Moving this to kselftest.h was done to avoid
>>>>    duplicate. However now we are running into arm64/arm compile
>>>>    errors due to this which need addressing one way or the other.
>>>>
>>>> I have some ideas on how to solve this - but I need answers to
>>>> the following questions.
>>>>
>>>> This is a question for you and Usama.
>>>>
>>>> - Does resctrl run on arm64/arm and what's the output?
>>>> - Can all other tests in resctrl other tests except
>>>>    noncont_cat_run_test?
>>>> - If so send me the output.
>>>
>>> Hi Shuah,
>>>
>>> As mentioned in my coverletter above, resctrl does not currently support
>>> arm but there's an ongoing work to add arm support. On kernel side it
>>> requires major refactoring to move non-arch specific stuff out from
>>> arch/x86 so has (predictably) taken long time.
>>>
>>> The resctrl selftests are mostly written in arch independent way (*) but
>>> there's also a way to limit a test only to CPUs from a particular vendor.
>>> And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs
>>> (to read the supported flag), it's not needed even on AMD CPUs as they
>>> always support non-contiguous CAT bitmask.
>>>
>>> So to summarize, it would be possible to disable resctrl test for non-x86
>>> but it does not address the underlying problem with cpuid which will just
>>> come back later I think.
>>>
>>> Alternatively, if there's some a good way in C code to do ifdeffery around
>>> that cpuid call, I could make that too, but I need to know which symbol to
>>> use for that ifdef.
>>>
>>> (*) The cache topology may make some selftest unusable on new archs but
>>> not the selftest code itself.
>>>
>>>
>>
>> I agree that suppressing resctrl build is not a solution. The real problem
>> is is in defining __cpuid_count() in common code path.
>>
>> I fixed it and send patch in. As I was testing I noticed the following on
>> AMD platform:
>>
>> - it ran the L3_NONCONT_CAT test which is expected.
>>
>> # # Starting L3_NONCONT_CAT test ...
>> # # Mounting resctrl to "/sys/fs/resctrl"
>> # ARCH_AMD - supports non-contiguous CBM
>> # # Write schema "L3:0=ff" to resctrl FS
>> # # Write schema "L3:0=fc3f" to resctrl FS
>> # ok 5 L3_NONCONT_CAT: test
>>
>> - It went on to run L2_NONCONT_CAT - failed
> 
> It is not intended to appear as a failure but instead just skipping of
> a test since the platform does not support the feature being tested.
> 
>>
>> # ok 6 # SKIP Hardware does not support L2_NONCONT_CAT or L2_NONCONT_CAT is disabled
> 
> The output looks as intended. When I run the test on an Intel system without
> L2 CAT the output looks the same.
> 
>>
>> Does it make sense to run both L3_NONCONT_CAT and L2_NONCONT_CAT
>> on AMD? Maybe it is? resctrl checks L3 or L2 support on Intel.
> 
> The selftests test the features as exposed by the generic resctrl kernel
> subsystem instead of relying on its own inventory of what features
> need to be checked for which vendor. selftests will thus only
> test L3 or L2 if resctrl kernel subsystem indicates it is supported on
> underlying platform. Only afterwards may it use platform specific
> knowledge to help validate the feature.
> In this scenario resctrl indicated that L2 CAT is not supported
> by underlying platform and the test was skipped. It looks good
> to me.
> 

Thanks for the detailed explanation. Sounds good to me.

thanks,
-- Shuah