Messages in this thread | | | Date | Wed, 16 Aug 2023 11:27:27 +0100 | Subject | Re: [PATCH v5 3/6] perf test: Add a test for the new Arm CPU ID comparison behavior | From | John Garry <> |
| |
On 16/08/2023 10:14, James Clark wrote: > > > On 15/08/2023 10:47, John Garry wrote: >> On 11/08/2023 15:39, James Clark wrote: >>> Now that variant and revision fields are taken into account the behavior >>> is slightly more complicated so add a test to ensure that this behaves >>> as expected. >>> >>> Signed-off-by: James Clark <james.clark@arm.com> >>> --- >>> tools/perf/arch/arm64/include/arch-tests.h | 3 ++ >>> tools/perf/arch/arm64/tests/Build | 1 + >>> tools/perf/arch/arm64/tests/arch-tests.c | 4 +++ >>> tools/perf/arch/arm64/tests/cpuid-match.c | 38 ++++++++++++++++++++++ >>> 4 files changed, 46 insertions(+) >>> create mode 100644 tools/perf/arch/arm64/tests/cpuid-match.c >>> >>> diff --git a/tools/perf/arch/arm64/include/arch-tests.h >>> b/tools/perf/arch/arm64/include/arch-tests.h >>> index 452b3d904521..474d7cf5afbd 100644 >>> --- a/tools/perf/arch/arm64/include/arch-tests.h >>> +++ b/tools/perf/arch/arm64/include/arch-tests.h >>> @@ -2,6 +2,9 @@ >>> #ifndef ARCH_TESTS_H >>> #define ARCH_TESTS_H >>> +struct test_suite; >>> + >>> +int test__cpuid_match(struct test_suite *test, int subtest); >>> extern struct test_suite *arch_tests[]; >>> #endif >>> diff --git a/tools/perf/arch/arm64/tests/Build >>> b/tools/perf/arch/arm64/tests/Build >>> index a61c06bdb757..e337c09e7f56 100644 >>> --- a/tools/perf/arch/arm64/tests/Build >>> +++ b/tools/perf/arch/arm64/tests/Build >>> @@ -2,3 +2,4 @@ perf-y += regs_load.o >>> perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o >>> perf-y += arch-tests.o >>> +perf-y += cpuid-match.o >>> diff --git a/tools/perf/arch/arm64/tests/arch-tests.c >>> b/tools/perf/arch/arm64/tests/arch-tests.c >>> index ad16b4f8f63e..74932e72c727 100644 >>> --- a/tools/perf/arch/arm64/tests/arch-tests.c >>> +++ b/tools/perf/arch/arm64/tests/arch-tests.c >>> @@ -3,9 +3,13 @@ >>> #include "tests/tests.h" >>> #include "arch-tests.h" >>> + >>> +DEFINE_SUITE("arm64 CPUID matching", cpuid_match); >>> + >>> struct test_suite *arch_tests[] = { >>> #ifdef HAVE_DWARF_UNWIND_SUPPORT >>> &suite__dwarf_unwind, >>> #endif >>> + &suite__cpuid_match, >>> NULL, >>> }; >>> diff --git a/tools/perf/arch/arm64/tests/cpuid-match.c >>> b/tools/perf/arch/arm64/tests/cpuid-match.c >>> new file mode 100644 >>> index 000000000000..af0871b54ae7 >>> --- /dev/null >>> +++ b/tools/perf/arch/arm64/tests/cpuid-match.c >>> @@ -0,0 +1,38 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +#include <linux/compiler.h> >>> + >>> +#include "arch-tests.h" >>> +#include "tests/tests.h" >>> +#include "util/header.h" >>> + >>> +int test__cpuid_match(struct test_suite *test __maybe_unused, >>> + int subtest __maybe_unused) >>> +{ >>> + /* midr with no leading zeros matches */ >>> + if (strcmp_cpuid_str("0x410fd0c0", "0x00000000410fd0c0")) >>> + return -1; >>> + /* Upper case matches */ >>> + if (strcmp_cpuid_str("0x410fd0c0", "0x00000000410FD0C0")) >>> + return -1; >>> + /* r0p0 = r0p0 matches */ >>> + if (strcmp_cpuid_str("0x00000000410fd480", "0x00000000410fd480")) >>> + return -1; >>> + /* r0p1 > r0p0 matches */ >>> + if (strcmp_cpuid_str("0x00000000410fd480", "0x00000000410fd481")) >>> + return -1; >>> + /* r1p0 > r0p0 matches*/ >>> + if (strcmp_cpuid_str("0x00000000410fd480", "0x00000000411fd480")) >>> + return -1; >>> + /* r0p0 < r0p1 doesn't match */ >>> + if (!strcmp_cpuid_str("0x00000000410fd481", "0x00000000410fd480")) >>> + return -1; >>> + /* r0p0 < r1p0 doesn't match */ >>> + if (!strcmp_cpuid_str("0x00000000411fd480", "0x00000000410fd480")) >>> + return -1; >>> + /* Different CPU doesn't match */ >>> + if (!strcmp_cpuid_str("0x00000000410fd4c0", "0x00000000430f0af0")) >>> + return -1; >>> + >>> + return 0; >>> +} >>> + >> >> Would it be possible to put this in core test code, since x86 also >> supports strcmp_cpuid_str()? >> > > That's how I started, but Ian suggested to move it to an arch specific > folder because that's what it was testing. >
Yeah, I see that comment now.
> We could still add test__cpuid_match() in the x86 folder rather than > adding it with #ifdefs
I was thinking to make cpuid_match_array[] exposed by the arch code and have a "weak", i.e. version for other archs.
, but I don't think it needs to be done here > because I haven't touched the x86 code.
For the moment, I don't feel too strongly about this and it can be done as a follow-up
Reviewed-by: John Garry <john.g.garry@oracle.com>
> >> Maybe we would have an structure per arch of cpuids and expected >> results, like >> >> struct cpuid_match { >> char *cpuid1; >> char *cpuid1; >> int expected_result; >> }; >> >> >> #ifdef ARM64 >> cpuid_match_array[] = { >> {"0x410fd0c0", "0x00000000410FD0C0", -1}, >> {"0x00000000410fd480", "0x00000000410fd480", -1}, >> ... >> {} /* sentinel */ >> >> }; >> #else if defined(X86) >> cpuid_match_array[] = { >> {....} >> ... >> {} /* sentinel */ >> >> }; >> #else >> /* no support */ >> #endif >> >> Thanks, >> John
| |