Message-ID: <da87fe73-c39d-8b60-753d-7735c9abf569@linux.intel.com>
Date: Tue, 13 Aug 2024 10:39:16 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Reinette Chatre <reinette.chatre@...el.com>
cc: Shuah Khan <skhan@...uxfoundation.org>, 
    Muhammad Usama Anjum <Usama.Anjum@...labora.com>, 
    Fenghua Yu <fenghua.yu@...el.com>, 
    Shaopeng Tan <tan.shaopeng@...fujitsu.com>, kernel@...labora.com, 
    LKML <linux-kernel@...r.kernel.org>, linux-kselftest@...r.kernel.org, 
    Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>
Subject: Re: [PATCH] selftests: resctrl: ignore builds for unsupported
 architectures

On Mon, 12 Aug 2024, Reinette Chatre wrote:
> On 8/12/24 3:49 PM, Shuah Khan wrote:
> > On 8/9/24 02:45, Ilpo Järvinen wrote:
> > > Adding Maciej.
> > > 
> > > On Fri, 9 Aug 2024, Muhammad Usama Anjum wrote:
> > > > On 8/9/24 12:23 PM, Ilpo Järvinen wrote:
> > > > > On Fri, 9 Aug 2024, Muhammad Usama Anjum wrote:
> > > > > 
> > > > > > This test doesn't have support for other architectures. Altough
> > > > > > resctrl
> > > > > > is supported on x86 and ARM, but arch_supports_noncont_cat() shows
> > > > > > that
> > > > > > only x86 for AMD and Intel are supported by the test.
> > > > > 
> > > > > One does not follow from the other. arch_supports_noncont_cat() is
> > > > > only
> > > > > small part of the tests so saying "This test" based on a small subset
> > > > > of
> > > > > all tests is bogus. Also, I don't see any reason why ARCH_ARM could
> > > > > not be
> > > > > added and arch_supports_noncont_cat() adapted accordingly.
> > > > I'm not familiar with resctrl and the architectural part of it. Feel
> > > > free to fix it and ignore this patch.
> > > > 
> > > > If more things are missing than just adjusting
> > > > arch_supports_noncont_cat(), the test should be turned off until proper
> > > > support is added to the test.
> > > > 
> > > > > > We get build
> > > > > > errors when built for ARM and ARM64.
> > > > > 
> > > > > As this seems the real reason, please quote any errors when you use
> > > > > them
> > > > > as justification so it can be reviewed if the reasoning is sound or
> > > > > not.
> > > > 
> > > >    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);
> > > >        |                 ^~~~~~~~~~~~~
> > > 
> > > Okay, so it's specific to lack of CPUID. This seems a kselftest common
> > > level problem to me, since __cpuid_count() is provided in kselftest.h.
> > > 
> > > Shuah (or others), what is the intended mechanism for selftests to know if
> > > it can be used or not since as is, it's always defined?
> > _cpuid_count() gets defined in ksefltest.h if it can't find it.
> > 
> > As the comment says both gcc and cland probide __cpuid_count()
> > 
> >    gcc cpuid.h provides __cpuid_count() since v4.4.
> >    Clang/LLVM cpuid.h provides  __cpuid_count() since v3.4.0.
> > 
> > > 
> > > I see some Makefiles use compile testing a trivial program to decide
> > > whether
> > > they build some x86_64 tests or not. Is that what should be done here too,
> > > test if __cpuid_count() compiles or not (and then build some #ifdeffery
> > > based on the result of that compile testing)?
> > > 
> > 
> > These build errors need to be fixed instead of restricting the build> In
> > some cases when the test can't be supported on an architecture then it is
> > okay
> > to suppress build. This is not a general solution to suppress build warnings
> 
> While there is an effort to support Arm in resctrl [1], this is not currently
> the case and the resctrl selftests as a consequence only support x86 with
> built-in assumptions that a test runs on either AMD or Intel. After the kernel
> gains support
> for Arm more changes will be needed for the resctrl tests to support another
> architecture
> so I do think the most appropriate change to address this build failure is to
> restrict
> resctrl tests to x86.

While ARM lacks resctrl support at the moment (the patch BTW claims 
otherwise), this problem is general-level problem in selftests. When 
somebody includes kselftest.h, the header provided __cpuid_count() which 
seems to not be compilable on ARMs (even if the test itself would never 
call it on other than when running on Intel). Some #ifdeffery is necessary 
either in kselftest.h or in the test code.

> > I would recommend against adding suppress build code when it can be fixed.
> 
> I expect after resctrl fs obtains support for Arm the resctrl selftests can be
> updated to support it with more fine grained architectural checks than a
> global
> enable/disable needed at this time.

That won't help to a build failure. The build would fail on ARM even if 
there's some resctrl specific test for arch done by the test itself.

> > Let's investigate this problem to fix it properly. I don't see any arm and
> > arm64
> > maintainers and developers on this thread. It would be good to investigate
> > to
> > see if this can be fixed.

Yes, I was hoping there would be a general level solution which would 
provide e.g. HAS_CPUID_COUNT or an empty stub for __cpuid_count() or 
something along those lines.

-- 
 i.

> [1] https://lore.kernel.org/lkml/20240802172853.22529-1-james.morse@arm.com/

Powered by blists - more mailing lists