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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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