Thread (16 messages) read the whole thread 16 messages, 6 authors, 2025-06-18

RE: perf usage of arch/arm64/include/asm/cputype.h

From: Shameerali Kolothum Thodi <hidden>
Date: 2025-06-16 09:54:54
Also in: lkml 

-----Original Message-----
From: James Clark <james.clark@linaro.org>
Sent: Monday, June 16, 2025 10:30 AM
To: yangyicong <redacted>; Arnaldo Carvalho de Melo
[off-list ref]; linux-arm-kernel@lists.infradead.org
Cc: Ali Saidi <redacted>; Leo Yan <redacted>; Will
Deacon [off-list ref]; James Morse [off-list ref]; Catalin
Marinas [off-list ref]; yangjinqian
[off-list ref]; Douglas Anderson
[off-list ref]; Dmitry Baryshkov
[off-list ref]; Adrian Hunter [off-list ref];
Ian Rogers [off-list ref]; Jiri Olsa [off-list ref]; Kan Liang
[off-list ref]; Namhyung Kim [off-list ref];
Linux Kernel Mailing List [off-list ref]; Shameerali
Kolothum Thodi [off-list ref]; yangyicong
[off-list ref]
Subject: Re: perf usage of arch/arm64/include/asm/cputype.h



On 16/06/2025 8:56 am, Yicong Yang wrote:
quoted
+ linux-arm-kernel

On 2025/6/14 4:13, Arnaldo Carvalho de Melo wrote:
quoted
Hi,

tools/perf (and other tools/ living code) uses a file from the
kernel, a copy, so that we don't break its build when something
changes in the kernel that tooling uses.

There is this tools/perf/check-headers.sh that does the "copy
coherency check", while trying to act on such a warning I stumbled on
the report below.

More details at:


https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tr
ee/tools/include/uapi/README


If you could please take a look at this that would be great, the
initial copy was made at:

commit 1314376d495f2d79cc58753ff3034ccc503c43c9
Author: Ali Saidi [off-list ref]
Date:   Thu Mar 24 18:33:20 2022 +0000

     tools arm64: Import cputype.h

     Bring-in the kernel's arch/arm64/include/asm/cputype.h into tools/
     for arm64 to make use of all the core-type definitions in perf.

     Replace sysreg.h with the version already imported into tools/.

     Committer notes:

     Added an entry to tools/perf/check-headers.sh, so that we get
notified
quoted
quoted
     when the original file in the kernel sources gets modified.

     Tester notes:

     LGTM. I did the testing on both my x86 and Arm64 platforms, thanks
for
quoted
quoted
     the fixing up.

     Signed-off-by: Ali Saidi [off-list ref]
     Tested-by: Leo Yan [off-list ref]

- Arnaldo

⬢ [acme@toolbx perf-tools]$ m
rm: cannot remove
'/home/acme/libexec/perf-core/scripts/python/Perf-Trace-Util/lib/Perf
/Trace/__pycache__/Core.cpython-313.pyc': Permission denied
make: Entering directory '/home/acme/git/perf-tools/tools/perf'
   BUILD:   Doing 'make -j32' parallel build
Warning: Kernel ABI header differences:
   diff -u tools/arch/x86/include/asm/cpufeatures.h
arch/x86/include/asm/cpufeatures.h
quoted
quoted
   diff -u tools/arch/arm64/include/asm/cputype.h
arch/arm64/include/asm/cputype.h

Auto-detecting system features:
...                                   libdw: [ on  ]
...                                   glibc: [ on  ]
...                                  libelf: [ on  ]
...                                 libnuma: [ on  ]
...                  numa_num_possible_cpus: [ on  ]
...                                 libperl: [ on  ]
...                               libpython: [ on  ]
...                               libcrypto: [ on  ]
...                             libcapstone: [ on  ]
...                               llvm-perf: [ on  ]
...                                    zlib: [ on  ]
...                                    lzma: [ on  ]
...                               get_cpuid: [ on  ]
...                                     bpf: [ on  ]
...                                  libaio: [ on  ]
...                                 libzstd: [ on  ]

   INSTALL libsubcmd_headers
   INSTALL libperf_headers
   INSTALL libapi_headers
   INSTALL libsymbol_headers
   INSTALL libbpf_headers
   INSTALL binaries
   INSTALL tests
   INSTALL libperf-jvmti.so
   INSTALL libexec
   INSTALL perf-archive
   INSTALL perf-iostat
   INSTALL perl-scripts
   INSTALL python-scripts
   INSTALL dlfilters
   INSTALL perf_completion-script
   INSTALL perf-tip
make: Leaving directory '/home/acme/git/perf-tools/tools/perf'
  18: 'import perf' in python                                         : Ok
⬢ [acme@toolbx perf-tools]$ cp arch/arm64/include/asm/cputype.h
tools/arch/arm64/include/asm/cputype.h
⬢ [acme@toolbx perf-tools]$ m
rm: cannot remove
'/home/acme/libexec/perf-core/scripts/python/Perf-Trace-Util/lib/Perf
/Trace/__pycache__/Core.cpython-313.pyc': Permission denied
make: Entering directory '/home/acme/git/perf-tools/tools/perf'
   BUILD:   Doing 'make -j32' parallel build
Warning: Kernel ABI header differences:
   diff -u tools/arch/x86/include/asm/cpufeatures.h
arch/x86/include/asm/cpufeatures.h

Auto-detecting system features:
...                                   libdw: [ on  ]
...                                   glibc: [ on  ]
...                                  libelf: [ on  ]
...                                 libnuma: [ on  ]
...                  numa_num_possible_cpus: [ on  ]
...                                 libperl: [ on  ]
...                               libpython: [ on  ]
...                               libcrypto: [ on  ]
...                             libcapstone: [ on  ]
...                               llvm-perf: [ on  ]
...                                    zlib: [ on  ]
...                                    lzma: [ on  ]
...                               get_cpuid: [ on  ]
...                                     bpf: [ on  ]
...                                  libaio: [ on  ]
...                                 libzstd: [ on  ]

   INSTALL libsubcmd_headers
   INSTALL libperf_headers
   INSTALL libapi_headers
   INSTALL libsymbol_headers
   INSTALL libbpf_headers
   CC      /tmp/build/perf-tools/util/arm-spe.o
util/arm-spe.c: In function ‘arm_spe__synth_ds’:
util/arm-spe.c:885:43: error: passing argument 1 of
‘is_midr_in_range_list’ makes pointer from integer without a cast [-Wint-
conversion]
quoted
quoted
   885 |                 if (is_midr_in_range_list(midr,
data_source_handles[i].midr_ranges)) {
quoted
quoted
       |                                           ^~~~
       |                                           |
       |                                           u64 {aka long unsigned int}
In file included from util/arm-spe.c:37:
util/../../arch/arm64/include/asm/cputype.h:306:53: note: expected
‘const struct midr_range *’ but argument is of type ‘u64’ {aka ‘long
unsigned int’}
quoted
quoted
   306 | bool is_midr_in_range_list(struct midr_range const *ranges);
       |                            ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
util/arm-spe.c:885:21: error: too many arguments to function
‘is_midr_in_range_list’; expected 1, have 2
quoted
quoted
   885 |                 if (is_midr_in_range_list(midr,
data_source_handles[i].midr_ranges)) {
quoted
quoted
       |                     ^~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
quoted
quoted
util/../../arch/arm64/include/asm/cputype.h:306:6: note: declared here
   306 | bool is_midr_in_range_list(struct midr_range const *ranges);
       |      ^~~~~~~~~~~~~~~~~~~~~
make[4]: ***
[/home/acme/git/perf-tools/tools/build/Makefile.build:85:
/tmp/build/perf-tools/util/arm-spe.o] Error 1
make[3]: ***
[/home/acme/git/perf-tools/tools/build/Makefile.build:142: util]
Error 2
make[2]: *** [Makefile.perf:798:
/tmp/build/perf-tools/perf-util-in.o] Error 2
make[1]: *** [Makefile.perf:290: sub-make] Error 2
make: *** [Makefile:119: install-bin] Error 2
make: Leaving directory '/home/acme/git/perf-tools/tools/perf'
⬢ [acme@toolbx perf-tools]$

The changes should be introduced by arm64's errata management on live
migration[1], specifically:
quoted
- commit e3121298c7fc ("arm64: Modify _midr_range() functions to read
MIDR/REVIDR internally")
quoted
   which changed the implementation of is_midr_in_range() that the MIDR
to
quoted
   test is always read on the current CPU. This isn't true in perf since
   the MIDR is acquired from the perf.data.
- commit c8c2647e69be ("arm64: Make  _midr_in_range_list() an exported
function")
quoted
   which moves the implementation out of the header file.

Below patch should keep the copy coherency of cputype.h to implement
the _midr_in_range_list() as before.

[1]
https://lore.kernel.org/all/20250221140229.12588-1-shameerali.kolothum
.thodi@huawei.com/

Thanks.

 From 44900e7d3d9fa34c817396275f55a2aab611cd32 Mon Sep 17
00:00:00
quoted
2001
From: Yicong Yang <yangyicong@hisilicon.com>
Date: Mon, 16 Jun 2025 15:18:11 +0800
Subject: [PATCH] arm64: cputype: Allow copy coherency on cputype.h
between
quoted
  tools/ and arch/
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

arch/arm64/include/asm/cputype.h is copied from arch/arm64 and used
by
quoted
perf to parsing vendor specific SPE packets according to the MIDR.
The header diverge after errata management handling for VM live
migration merged [1]:
- commit e3121298c7fc ("arm64: Modify _midr_range() functions to read
MIDR/REVIDR internally")
quoted
   which changed the implementation of is_midr_in_range() that the MIDR
to
quoted
   test is always read on the current CPU. This isn't true in perf since
   the MIDR is acquired from the perf.data.
- commit c8c2647e69be ("arm64: Make  _midr_in_range_list() an exported
function")
quoted
   which moves the implementation out of the header file.

In order to allow copy coherency on cputype.h [2], implement
is_midr_in_range_list() as before [1]. Introduce
is_cpuid_in_range_list() for kernel space to test the MIDR of current
running CPU is within the target MIDR ranges. Move
cpu_errata_set_target_impl() and
is_cpuid_in_range_list() to cpufeature.h since they're only used by
errata management in the kernel space and don't needed by tools/.

No funtional changes intended.

[1]
https://lore.kernel.org/all/20250221140229.12588-1-shameerali.kolothum
.thodi@huawei.com/ [2]
https://lore.kernel.org/lkml/aEyGg98z-MkcClXY@x1/#t (local)
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
  arch/arm64/include/asm/cpufeature.h           | 11 +++++
  arch/arm64/include/asm/cputype.h              | 40 +++++++++++--------
  arch/arm64/kernel/cpu_errata.c                | 30 +++++---------
  arch/arm64/kernel/cpufeature.c                |  6 +--
  arch/arm64/kernel/proton-pack.c               | 20 +++++-----
  arch/arm64/kvm/vgic/vgic-v3.c                 |  2 +-
  drivers/clocksource/arm_arch_timer.c          |  2 +-
  .../coresight/coresight-etm4x-core.c          |  2 +-
  8 files changed, 60 insertions(+), 53 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h
b/arch/arm64/include/asm/cpufeature.h
index c4326f1cb917..ba2d474fb393 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -1048,6 +1048,17 @@ static inline bool cpu_has_lpa2(void)
  #endif
  }

+struct target_impl_cpu {
+	u64 midr;
+	u64 revidr;
+	u64 aidr;
+};
+
+bool cpu_errata_set_target_impl(u64 num, void *impl_cpus);
+
+/* Different from is_midr_in_range() on using the MIDR of current CPU
+*/ bool is_cpuid_in_range_list(struct midr_range const *ranges);
+
  #endif /* __ASSEMBLY__ */

  #endif
diff --git a/arch/arm64/include/asm/cputype.h
b/arch/arm64/include/asm/cputype.h
index 661735616787..89fd197e2f03 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -251,16 +251,6 @@

  #define read_cpuid(reg)			read_sysreg_s(SYS_ ## reg)

-/*
- * The CPU ID never changes at run time, so we might as well tell the
- * compiler that it's constant.  Use this function to read the CPU ID
- * rather than directly reading processor_id or read_cpuid() directly.
- */
-static inline u32 __attribute_const__ read_cpuid_id(void) -{
-	return read_cpuid(MIDR_EL1);
-}
-
  /*
   * Represent a range of MIDR values for a given CPU model and a
   * range of variant/revision values.
@@ -296,14 +286,30 @@ static inline bool midr_is_cpu_model_range(u32
midr, u32 model, u32 rv_min,
quoted
  	return _model == model && rv >= rv_min && rv <= rv_max;
  }

-struct target_impl_cpu {
-	u64 midr;
-	u64 revidr;
-	u64 aidr;
-};
+static inline bool is_midr_in_range(u32 midr, struct midr_range const
+*range) {
+	return midr_is_cpu_model_range(midr, range->model,
+				       range->rv_min, range->rv_max); }

-bool cpu_errata_set_target_impl(u64 num, void *impl_cpus); -bool
is_midr_in_range_list(struct midr_range const *ranges);
+static inline bool
+is_midr_in_range_list(u32 midr, struct midr_range const *ranges) {
+	while (ranges->model)
+		if (is_midr_in_range(midr, ranges++))
+			return true;
+	return false;
+}
+
+/*
+ * The CPU ID never changes at run time, so we might as well tell the
+ * compiler that it's constant.  Use this function to read the CPU ID
+ * rather than directly reading processor_id or read_cpuid() directly.
+ */
+static inline u32 __attribute_const__ read_cpuid_id(void) {
+	return read_cpuid(MIDR_EL1);
+}

  static inline u64 __attribute_const__ read_cpuid_mpidr(void)
  {
diff --git a/arch/arm64/kernel/cpu_errata.c
b/arch/arm64/kernel/cpu_errata.c index 59d723c9ab8f..531ae67c7086
100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -27,38 +27,28 @@ bool cpu_errata_set_target_impl(u64 num, void
*impl_cpus)
quoted
  	return true;
  }

-static inline bool is_midr_in_range(struct midr_range const *range)
+bool is_cpuid_in_range_list(struct midr_range const *ranges)
  {
+	u32 midr = read_cpuid_id();
  	int i;

  	if (!target_impl_cpu_num)
-		return midr_is_cpu_model_range(read_cpuid_id(), range-
model,
-					       range->rv_min, range->rv_max);
+		return is_midr_in_range_list(midr, ranges);

-	for (i = 0; i < target_impl_cpu_num; i++) {
-		if (midr_is_cpu_model_range(target_impl_cpus[i].midr,
-					    range->model,
-					    range->rv_min, range->rv_max))
+	for (i = 0; i < target_impl_cpu_num; i++)
+		if (is_midr_in_range_list(midr, ranges))
  			return true;
-	}
-	return false;
-}

-bool is_midr_in_range_list(struct midr_range const *ranges) -{
-	while (ranges->model)
-		if (is_midr_in_range(ranges++))
-			return true;
  	return false;
  }
-EXPORT_SYMBOL_GPL(is_midr_in_range_list);
+EXPORT_SYMBOL_GPL(is_cpuid_in_range_list);

  static bool __maybe_unused
  __is_affected_midr_range(const struct arm64_cpu_capabilities *entry,
  			 u32 midr, u32 revidr)
  {
  	const struct arm64_midr_revidr *fix;
-	if (!is_midr_in_range(&entry->midr_range))
+	if (!is_midr_in_range(midr, &entry->midr_range))
  		return false;

  	midr &= MIDR_REVISION_MASK | MIDR_VARIANT_MASK; @@ -92,7
+82,7 @@
quoted
is_affected_midr_range_list(const struct arm64_cpu_capabilities *entry,
  			    int scope)
  {
  	WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
-	return is_midr_in_range_list(entry->midr_range_list);
+	return is_cpuid_in_range_list(entry->midr_range_list);
Looks ok to me.

You could do it with slightly less churn on the kernel side if you keep the
function name and arguments the same there. There's only one usage in
Perf so that one could be renamed and have the midr argument added back
in.
+1.

Can we use a separate one for perf here, something like below(untested)?

--- a/tools/perf/util/arm-spe.c
+++ b/tools/perf/util/arm-spe.c
@@ -842,6 +842,18 @@ static void arm_spe__synth_memory_level(const
struct arm_spe_record *record,
                data_src->mem_lvl |= PERF_MEM_LVL_REM_CCE1;
 }

+static bool is_perf_midr_in_range_list(u32 midr, struct midr_range
const *ranges)
+{
+       while (ranges->model) {
+               if (midr_is_cpu_model_range(midr, ranges->model,
+                                           ranges->rv_min, ranges->rv_max)) {
+                       return true;
+               }
+               ranges++;
+       }
+       return false;
+}
+
 static bool arm_spe__synth_ds(struct arm_spe_queue *speq,
                              const struct arm_spe_record *record,
                              union perf_mem_data_src *data_src)
@@ -882,7 +894,7 @@ static bool arm_spe__synth_ds(struct arm_spe_queue *speq,
        }

        for (i = 0; i < ARRAY_SIZE(data_source_handles); i++) {
-               if (is_midr_in_range_list(midr,
data_source_handles[i].midr_ranges)) {
+               if (is_perf_midr_in_range_list(midr,
data_source_handles[i].midr_ranges)) {
                        data_source_handles[i].ds_synth(record, data_src);
                        return true;
                }
Thanks,
Shameer

I was also wondering if we could just diverge on the tools side, but in reality
it also has to stay compatible with the definitions of all the MIDRs so might
as well keep the whole thing identical.
  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help