| Message ID | 20240307183730.2858264-1-usama.anjum@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: x86: skip the tests if prerequisites aren't fulfilled | expand |
On 3/11/2024 10:02 AM, Muhammad Usama Anjum wrote: > On 3/9/24 6:06 AM, Chang S. Bae wrote: >> On 3/7/2024 10:37 AM, Muhammad Usama Anjum wrote: >> >>> -static void check_cpuid_xtiledata(void) >>> +static int check_cpuid_xtiledata(void) >>> { >>> uint32_t eax, ebx, ecx, edx; >>> @@ -153,12 +160,16 @@ static void check_cpuid_xtiledata(void) >>> * eax: XTILEDATA state component size >>> * ebx: XTILEDATA state component offset in user buffer >>> */ >>> - if (!eax || !ebx) >>> - fatal_error("xstate cpuid: invalid tile data size/offset: %d/%d", >>> - eax, ebx); >>> + if (!eax || !ebx) { >>> + ksft_print_msg("xstate cpuid: invalid tile data size/offset: >>> %d/%d\n", >>> + eax, ebx); >>> + return -1; >>> + } >>> xtiledata.size = eax; >>> xtiledata.xbuf_offset = ebx; >>> + >>> + return 0; >>> } >> >> I don't think it is okay to silently skip the test here. If the feature is >> available, the tile data size and offset should not be zero. > We are logging that data size/offset are invalid if either eax or ebx are > invalid and then we are skipping. Not sure what you are asking me to change. You intention seems to skip the test when AMX is not available. But this function should only be invoked when AMX is actually available, not as part of the feature availability check. Therefore, I think this change is not relevant. Also, if we encounter invalid TILEDATA CPUID, it should be a reason to *fail* the test, rather than calling out a skip, right? Thanks, Chang
On 3/12/2024 2:26 AM, Muhammad Usama Anjum wrote: > > How can we check if AMX is available or not? After a successful check_cpuid_xsave(), examining CPUID(eax=0xd, ecx=0) EDX: EAX, which reports the support bits of XCR0, can give assurance of AMX availability. Perhaps, this change is considerable on top of your patch: static int check_cpuid_xtiledata(void) { uint32_t eax, ebx, ecx, edx; + uint64_t xfeatures; __cpuid_count(CPUID_LEAF_XSTATE, CPUID_SUBLEAF_XSTATE_USER, eax, ebx, ecx, edx); + xfeatures = eax + ((uint64_t)edx << 32); + if ((xfeatures & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE) { + ksft_print_msg("no AMX support\n"); + return -1; + } Nevertheless, I still believe that using arch_prctl(ARCH_GET_XCOMP_SUPP, ...) remains a simple and legitimate approach for directly checking dynamic feature support from the kernel: https://docs.kernel.org/arch/x86/xstate.html Thanks, Chang
diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c index d884fd69dd510..5d1ca0bbaaae7 100644 --- a/tools/testing/selftests/x86/amx.c +++ b/tools/testing/selftests/x86/amx.c @@ -103,9 +103,10 @@ static void clearhandler(int sig) #define CPUID_LEAF1_ECX_XSAVE_MASK (1 << 26) #define CPUID_LEAF1_ECX_OSXSAVE_MASK (1 << 27) -static inline void check_cpuid_xsave(void) +static inline int check_cpuid_xsave(void) { uint32_t eax, ebx, ecx, edx; + int ret = 0; /* * CPUID.1:ECX.XSAVE[bit 26] enumerates general @@ -113,10 +114,16 @@ static inline void check_cpuid_xsave(void) * XGETBV. */ __cpuid_count(1, 0, eax, ebx, ecx, edx); - if (!(ecx & CPUID_LEAF1_ECX_XSAVE_MASK)) - fatal_error("cpuid: no CPU xsave support"); - if (!(ecx & CPUID_LEAF1_ECX_OSXSAVE_MASK)) - fatal_error("cpuid: no OS xsave support"); + if (!(ecx & CPUID_LEAF1_ECX_XSAVE_MASK)) { + ksft_print_msg("cpuid: no CPU xsave support\n"); + ret = -1; + } + if (!(ecx & CPUID_LEAF1_ECX_OSXSAVE_MASK)) { + ksft_print_msg("cpuid: no OS xsave support\n"); + ret = -1; + } + + return ret; } static uint32_t xbuf_size; @@ -131,7 +138,7 @@ static struct { #define TILE_CPUID 0x1d #define TILE_PALETTE_ID 0x1 -static void check_cpuid_xtiledata(void) +static int check_cpuid_xtiledata(void) { uint32_t eax, ebx, ecx, edx; @@ -153,12 +160,16 @@ static void check_cpuid_xtiledata(void) * eax: XTILEDATA state component size * ebx: XTILEDATA state component offset in user buffer */ - if (!eax || !ebx) - fatal_error("xstate cpuid: invalid tile data size/offset: %d/%d", - eax, ebx); + if (!eax || !ebx) { + ksft_print_msg("xstate cpuid: invalid tile data size/offset: %d/%d\n", + eax, ebx); + return -1; + } xtiledata.size = eax; xtiledata.xbuf_offset = ebx; + + return 0; } /* The helpers for managing XSAVE buffer and tile states: */ @@ -929,8 +940,8 @@ static void test_ptrace(void) int main(void) { /* Check hardware availability at first */ - check_cpuid_xsave(); - check_cpuid_xtiledata(); + if (check_cpuid_xsave() || check_cpuid_xtiledata()) + return KSFT_SKIP; init_stashed_xsave(); sethandler(SIGILL, handle_noperm, 0); diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c index 215b8150b7cca..c0f016f45ee17 100644 --- a/tools/testing/selftests/x86/lam.c +++ b/tools/testing/selftests/x86/lam.c @@ -1183,7 +1183,7 @@ int main(int argc, char **argv) if (!cpu_has_lam()) { ksft_print_msg("Unsupported LAM feature!\n"); - return -1; + return KSFT_SKIP; } while ((c = getopt(argc, argv, "ht:")) != -1) {
Skip instead of failing when prerequisite conditions aren't fulfilled, such as invalid xstate values etc. This patch would make the tests show as skip when run by: make -C tools/testing/selftest/ TARGETS=x86 run_tests ... # timeout set to 45 # selftests: x86: amx_64 # # xstate cpuid: invalid tile data size/offset: 0/0 ok 42 selftests: x86: amx_64 # SKIP # timeout set to 45 # selftests: x86: lam_64 # # Unsupported LAM feature! ok 43 selftests: x86: lam_64 # SKIP ... Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> --- I'm not sure if xstate values should be correct on all the x86 processors. If the xstate is invalid on a CPU, the test should be skipped instead of failing. --- tools/testing/selftests/x86/amx.c | 33 ++++++++++++++++++++----------- tools/testing/selftests/x86/lam.c | 2 +- 2 files changed, 23 insertions(+), 12 deletions(-)