> -----Original Message-----
> From: James Clark <james.clark@linaro.org>
> Sent: Monday, June 16, 2025 10:30 AM
> To: yangyicong <yangyicong@huawei.com>; Arnaldo Carvalho de Melo
> <acme@kernel.org>; linux-arm-kernel@lists.infradead.org
> Cc: Ali Saidi <alisaidi@amazon.com>; Leo Yan <leo.yan@linaro.org>; Will
> Deacon <will@kernel.org>; James Morse <james.morse@arm.com>; Catalin
> Marinas <catalin.marinas@arm.com>; yangjinqian
> <yangjinqian1@huawei.com>; Douglas Anderson
> <dianders@chromium.org>; Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org>; Adrian Hunter <adrian.hunter@intel.com>;
> Ian Rogers <irogers@google.com>; Jiri Olsa <jolsa@kernel.org>; Kan Liang
> <kan.liang@linux.intel.com>; Namhyung Kim <namhyung@kernel.org>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Shameerali
> Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; yangyicong
> <yangyicong@huawei.com>
> Subject: Re: perf usage of arch/arm64/include/asm/cputype.h
>
>
>
> On 16/06/2025 8:56 am, Yicong Yang wrote:
> > + linux-arm-kernel
> >
> > On 2025/6/14 4:13, Arnaldo Carvalho de Melo wrote:
> >> 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 <alisaidi@amazon.com>
> >> 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
> >> 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
> >> the fixing up.
> >>
> >> Signed-off-by: Ali Saidi <alisaidi@amazon.com>
> >> Tested-by: Leo Yan <leo.yan@linaro.org>
> >>
> >> - 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
> >> 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]
> >> 885 | if (is_midr_in_range_list(midr,
> data_source_handles[i].midr_ranges)) {
> >> | ^~~~
> >> | |
> >> | 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’}
> >> 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
> >> 885 | if (is_midr_in_range_list(midr,
> data_source_handles[i].midr_ranges)) {
> >> | ^~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> 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:
> > - commit e3121298c7fc ("arm64: Modify _midr_range() functions to read
> MIDR/REVIDR internally")
> > which changed the implementation of is_midr_in_range() that the MIDR
> to
> > 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")
> > 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
> > 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
> > 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
> > 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")
> > which changed the implementation of is_midr_in_range() that the MIDR
> to
> > 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")
> > 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
> > 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,
> > 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)
> > 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 @@
> > 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.
On Mon, Jun 16, 2025 at 09:54:43AM +0000, Shameerali Kolothum Thodi wrote:
[...]
> > > -bool is_midr_in_range_list(struct midr_range const *ranges) -{
> > > - while (ranges->model)
> > > - if (is_midr_in_range(ranges++))
> > > - return true;
> > > return false;
> > > }
> > 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)?
Thanks for working on this. Agreed.
> --- 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;
> +}
Maybe we can make it more general. For example, move this function into
a common header such as tools/perf/arch/arm64/include/cputype.h. Then,
util/arm-spe.c can include this header.
Thanks,
Leo
On 2025/6/16 21:07, Leo Yan wrote:
> On Mon, Jun 16, 2025 at 09:54:43AM +0000, Shameerali Kolothum Thodi wrote:
>
> [...]
>
>>>> -bool is_midr_in_range_list(struct midr_range const *ranges) -{
>>>> - while (ranges->model)
>>>> - if (is_midr_in_range(ranges++))
>>>> - return true;
>>>> return false;
>>>> }
>
>>> 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)?
>
> Thanks for working on this. Agreed.
>
>> --- 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;
>> +}
>
> Maybe we can make it more general. For example, move this function into
> a common header such as tools/perf/arch/arm64/include/cputype.h. Then,
> util/arm-spe.c can include this header.
>
ok this sounds just like as before except rename the midr check function and modify the
users in perf. will do in below steps:
- move cpu_errata_set_target_impl()/is_midr_in_range_list() out of cputype.h
since they're only used in the kernel with errata information
- introduce is_target_midr_in_range_list() in cputype.h to test certain MIDR
is within the ranges. (is_perf_midr_in_range_list() only make sense in
userspace and is a bit strange to me in a kernel header). maybe reimplement
is_midr_in_range_list() with is_target_midr_in_range_list() otherwise there's
no users in kernel
- copy cputype.h to userspace and make users use new is_target_midr_in_range_list()
this will avoid touching the kernel too much and userspace don't need to implement
a separate function.
Thanks.
On Mon, Jun 16, 2025 at 11:04:08PM +0800, Yicong Yang wrote:
[...]
> >> +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;
> >> +}
> >
> > Maybe we can make it more general. For example, move this function into
> > a common header such as tools/perf/arch/arm64/include/cputype.h. Then,
> > util/arm-spe.c can include this header.
> >
>
> ok this sounds just like as before except rename the midr check function and modify the
> users in perf. will do in below steps:
> - move cpu_errata_set_target_impl()/is_midr_in_range_list() out of cputype.h
> since they're only used in the kernel with errata information
> - introduce is_target_midr_in_range_list() in cputype.h to test certain MIDR
> is within the ranges. (is_perf_midr_in_range_list() only make sense in
> userspace and is a bit strange to me in a kernel header). maybe reimplement
> is_midr_in_range_list() with is_target_midr_in_range_list() otherwise there's
> no users in kernel
> - copy cputype.h to userspace and make users use new is_target_midr_in_range_list()
>
> this will avoid touching the kernel too much and userspace don't need to implement
> a separate function.
My understanding is we don't need to touch anything in kernel side, we
simply add a wrapper in perf tool to call midr_is_cpu_model_range().
When introduce is_target_midr_in_range_list() in kernel's cputype.h,
if no consumers in kernel use it and only useful for perf tool, then
it is unlikely to be accepted.
Thanks,
Leo
On Mon, Jun 16, 2025 at 05:08:11PM +0100, Leo Yan wrote:
> On Mon, Jun 16, 2025 at 11:04:08PM +0800, Yicong Yang wrote:
>
> [...]
>
> > >> +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;
> > >> +}
> > >
> > > Maybe we can make it more general. For example, move this function into
> > > a common header such as tools/perf/arch/arm64/include/cputype.h. Then,
> > > util/arm-spe.c can include this header.
> > >
> >
> > ok this sounds just like as before except rename the midr check function and modify the
> > users in perf. will do in below steps:
> > - move cpu_errata_set_target_impl()/is_midr_in_range_list() out of cputype.h
> > since they're only used in the kernel with errata information
> > - introduce is_target_midr_in_range_list() in cputype.h to test certain MIDR
> > is within the ranges. (is_perf_midr_in_range_list() only make sense in
> > userspace and is a bit strange to me in a kernel header). maybe reimplement
> > is_midr_in_range_list() with is_target_midr_in_range_list() otherwise there's
> > no users in kernel
> > - copy cputype.h to userspace and make users use new is_target_midr_in_range_list()
> >
> > this will avoid touching the kernel too much and userspace don't need to implement
> > a separate function.
>
> My understanding is we don't need to touch anything in kernel side, we
> simply add a wrapper in perf tool to call midr_is_cpu_model_range().
>
> When introduce is_target_midr_in_range_list() in kernel's cputype.h,
> if no consumers in kernel use it and only useful for perf tool, then
> it is unlikely to be accepted.
I think all of this is just working around the problem that
asm/cputype.h was never intended to be used in userspace. Likewise with
the other headers that we copy into tools/.
If there are bits that we *want* to share with tools/, let's factor that
out. The actual MIDR values are a good candidate for that -- we can
follow the same approach as with sysreg-defs.h.
Other than that, I think that userspace should just maintain its own
infrastructure, and only pull in things from kernel sources when there's
a specific reason to. Otherwise we're just creating busywork.
Mark.
On Mon, Jun 16, 2025 at 06:47:25PM +0100, Mark Rutland wrote:
[...]
> > > ok this sounds just like as before except rename the midr check function and modify the
> > > users in perf. will do in below steps:
> > > - move cpu_errata_set_target_impl()/is_midr_in_range_list() out of cputype.h
> > > since they're only used in the kernel with errata information
> > > - introduce is_target_midr_in_range_list() in cputype.h to test certain MIDR
> > > is within the ranges. (is_perf_midr_in_range_list() only make sense in
> > > userspace and is a bit strange to me in a kernel header). maybe reimplement
> > > is_midr_in_range_list() with is_target_midr_in_range_list() otherwise there's
> > > no users in kernel
> > > - copy cputype.h to userspace and make users use new is_target_midr_in_range_list()
> > >
> > > this will avoid touching the kernel too much and userspace don't need to implement
> > > a separate function.
> >
> > My understanding is we don't need to touch anything in kernel side, we
> > simply add a wrapper in perf tool to call midr_is_cpu_model_range().
> >
> > When introduce is_target_midr_in_range_list() in kernel's cputype.h,
> > if no consumers in kernel use it and only useful for perf tool, then
> > it is unlikely to be accepted.
>
> I think all of this is just working around the problem that
> asm/cputype.h was never intended to be used in userspace. Likewise with
> the other headers that we copy into tools/.
>
> If there are bits that we *want* to share with tools/, let's factor that
> out. The actual MIDR values are a good candidate for that -- we can
> follow the same approach as with sysreg-defs.h.
Thanks for suggestion, Mark.
It makes sense to me for extracting MIDR and sharing between kernel and
tools/. I have created a task for following up the refactoring.
> Other than that, I think that userspace should just maintain its own
> infrastructure, and only pull in things from kernel sources when there's
> a specific reason to. Otherwise we're just creating busywork.
I agree with the methodology.
Since Arnaldo is facing build failure when sync headers between kernel
and perf tool, to avoid long latency, let us split the refactoriing
into separate steps.
As a first step, I think my previous suggestion is valid, we can create a
header tools/perf/arch/arm64/include/cputype.h with below code:
#include "../../../../arch/arm64/include/asm/cputype.h"
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;
}
Then, once we can generate a dynamic MIDR header file, we can use that
header and define the midr_range structure specifically in the perf.
In the end, perf can avoid to include kernel's cputype.h.
If no objection, Yicong, do you mind preparing the patch mentioned
above? Thanks!
Leo
On Tue, Jun 17, 2025 at 03:18:10PM +0100, Leo Yan wrote:
> On Mon, Jun 16, 2025 at 06:47:25PM +0100, Mark Rutland wrote:
>
> [...]
>
> > > > ok this sounds just like as before except rename the midr check function and modify the
> > > > users in perf. will do in below steps:
> > > > - move cpu_errata_set_target_impl()/is_midr_in_range_list() out of cputype.h
> > > > since they're only used in the kernel with errata information
> > > > - introduce is_target_midr_in_range_list() in cputype.h to test certain MIDR
> > > > is within the ranges. (is_perf_midr_in_range_list() only make sense in
> > > > userspace and is a bit strange to me in a kernel header). maybe reimplement
> > > > is_midr_in_range_list() with is_target_midr_in_range_list() otherwise there's
> > > > no users in kernel
> > > > - copy cputype.h to userspace and make users use new is_target_midr_in_range_list()
> > > >
> > > > this will avoid touching the kernel too much and userspace don't need to implement
> > > > a separate function.
> > >
> > > My understanding is we don't need to touch anything in kernel side, we
> > > simply add a wrapper in perf tool to call midr_is_cpu_model_range().
> > >
> > > When introduce is_target_midr_in_range_list() in kernel's cputype.h,
> > > if no consumers in kernel use it and only useful for perf tool, then
> > > it is unlikely to be accepted.
> >
> > I think all of this is just working around the problem that
> > asm/cputype.h was never intended to be used in userspace. Likewise with
> > the other headers that we copy into tools/.
> >
> > If there are bits that we *want* to share with tools/, let's factor that
> > out. The actual MIDR values are a good candidate for that -- we can
> > follow the same approach as with sysreg-defs.h.
>
> Thanks for suggestion, Mark.
>
> It makes sense to me for extracting MIDR and sharing between kernel and
> tools/. I have created a task for following up the refactoring.
>
> > Other than that, I think that userspace should just maintain its own
> > infrastructure, and only pull in things from kernel sources when there's
> > a specific reason to. Otherwise we're just creating busywork.
>
> I agree with the methodology.
>
> Since Arnaldo is facing build failure when sync headers between kernel
> and perf tool, to avoid long latency, let us split the refactoriing
> into separate steps.
>
> As a first step, I think my previous suggestion is valid, we can create a
> header tools/perf/arch/arm64/include/cputype.h with below code:
>
> #include "../../../../arch/arm64/include/asm/cputype.h"
Directly including the kernel header introduces the very fragility that
having a copy was intended to avoid. NAK to that.
I've replied to the same effect Yicong's patch [1,2].
If we want to share headers between userspace and kernel, we should
refactor those headers such that this is safe by construction.
There is no need to update the userspace headers just because the kernel
headers have changed, so the simple solution in the short term is to
suppress the warning from check-headers.sh.
[1] https://lore.kernel.org/linux-arm-kernel/dc5afc5c-060c-8bcb-c3a7-0de49a7455fb@huawei.com/T/#m23dfbea6af559f3765d89b9d8427213588871ffd
[2] https://lore.kernel.org/linux-arm-kernel/dc5afc5c-060c-8bcb-c3a7-0de49a7455fb@huawei.com/T/#m6acbfa00002af8ee791266ea86a58f8f994ed710
Mark.
>
> 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;
> }
>
> Then, once we can generate a dynamic MIDR header file, we can use that
> header and define the midr_range structure specifically in the perf.
> In the end, perf can avoid to include kernel's cputype.h.
>
> If no objection, Yicong, do you mind preparing the patch mentioned
> above? Thanks!
>
> Leo
On Wed, Jun 18, 2025 at 09:52:53AM +0100, Mark Rutland wrote:
[...]
> > > Other than that, I think that userspace should just maintain its own
> > > infrastructure, and only pull in things from kernel sources when there's
> > > a specific reason to. Otherwise we're just creating busywork.
> >
> > I agree with the methodology.
> >
> > Since Arnaldo is facing build failure when sync headers between kernel
> > and perf tool, to avoid long latency, let us split the refactoriing
> > into separate steps.
> >
> > As a first step, I think my previous suggestion is valid, we can create a
> > header tools/perf/arch/arm64/include/cputype.h with below code:
> >
> > #include "../../../../arch/arm64/include/asm/cputype.h"
>
> Directly including the kernel header introduces the very fragility that
> having a copy was intended to avoid. NAK to that.
My suggestion is not to include the kernel header, nor to modify the
copy header. :)
Instead, I suggested creating a new header within the perf tool (under
perf's arm64 folder) and then include the copy header in tools:
tools/arch/arm64/include/asm/cputype.h
> I've replied to the same effect Yicong's patch [1,2].
>
> If we want to share headers between userspace and kernel, we should
> refactor those headers such that this is safe by construction.
>
> There is no need to update the userspace headers just because the kernel
> headers have changed, so the simple solution in the short term is to
> suppress the warning from check-headers.sh.
Sure, makes sense for me.
@Arnaldo, as Mark suggested, do you want me to send a patch to remove
cputype.h checking in check-headers.sh or it is fine to keep the warning
until finish the header refactoring?
@Yicong, could you confirm if you proceed to refactor the MIDR? thanks!
Just note, I searched tools folder and found kselftest also uses the
cputype.h header. The refactoring should not break the files below.
$ git grep cputype.h
perf/check-headers.sh:check arch/arm64/include/asm/cputype.h '-I "^#include [<\"]\(asm/\)*sysreg.h"'
perf/util/arm-spe.c:#include "../../arch/arm64/include/asm/cputype.h"
testing/selftests/kvm/arm64/psci_test.c:#include <asm/cputype.h>
testing/selftests/kvm/lib/arm64/vgic.c:#include <asm/cputype.h>
Thanks,
Leo
> [1] https://lore.kernel.org/linux-arm-kernel/dc5afc5c-060c-8bcb-c3a7-0de49a7455fb@huawei.com/T/#m23dfbea6af559f3765d89b9d8427213588871ffd
> [2] https://lore.kernel.org/linux-arm-kernel/dc5afc5c-060c-8bcb-c3a7-0de49a7455fb@huawei.com/T/#m6acbfa00002af8ee791266ea86a58f8f994ed710
>
> Mark.
>
> >
> > 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;
> > }
> >
> > Then, once we can generate a dynamic MIDR header file, we can use that
> > header and define the midr_range structure specifically in the perf.
> > In the end, perf can avoid to include kernel's cputype.h.
> >
> > If no objection, Yicong, do you mind preparing the patch mentioned
> > above? Thanks!
> >
> > Leo
On Wed, Jun 18, 2025 at 12:24:40PM +0100, Leo Yan wrote: > On Wed, Jun 18, 2025 at 09:52:53AM +0100, Mark Rutland wrote: > > If we want to share headers between userspace and kernel, we should > > refactor those headers such that this is safe by construction. > > There is no need to update the userspace headers just because the kernel > > headers have changed, so the simple solution in the short term is to > > suppress the warning from check-headers.sh. > Sure, makes sense for me. > > @Arnaldo, as Mark suggested, do you want me to send a patch to remove > cputype.h checking in check-headers.sh or it is fine to keep the warning > until finish the header refactoring? It is ok to have the warning, its just this one at this point and it is serving its purpose. When the refactoring gets done, it will go away. Think of it as a reminder :-) - Arnaldo
On Wed, Jun 18, 2025 at 10:15:04AM -0300, Arnaldo Carvalho de Melo wrote: [...] > > @Arnaldo, as Mark suggested, do you want me to send a patch to remove > > cputype.h checking in check-headers.sh or it is fine to keep the warning > > until finish the header refactoring? > > It is ok to have the warning, its just this one at this point and it is > serving its purpose. > > When the refactoring gets done, it will go away. Think of it as a > reminder :-) Fair point. Thanks for confirmation!
On 2025/6/18 19:24, Leo Yan wrote:
> On Wed, Jun 18, 2025 at 09:52:53AM +0100, Mark Rutland wrote:
>
> [...]
>
>>>> Other than that, I think that userspace should just maintain its own
>>>> infrastructure, and only pull in things from kernel sources when there's
>>>> a specific reason to. Otherwise we're just creating busywork.
>>>
>>> I agree with the methodology.
>>>
>>> Since Arnaldo is facing build failure when sync headers between kernel
>>> and perf tool, to avoid long latency, let us split the refactoriing
>>> into separate steps.
>>>
>>> As a first step, I think my previous suggestion is valid, we can create a
>>> header tools/perf/arch/arm64/include/cputype.h with below code:
>>>
>>> #include "../../../../arch/arm64/include/asm/cputype.h"
>>
>> Directly including the kernel header introduces the very fragility that
>> having a copy was intended to avoid. NAK to that.
>
> My suggestion is not to include the kernel header, nor to modify the
> copy header. :)
>
> Instead, I suggested creating a new header within the perf tool (under
> perf's arm64 folder) and then include the copy header in tools:
>
> tools/arch/arm64/include/asm/cputype.h
>
sorry for the misunderstood.:(
in this way we still have the divergency in the long term and as a workaround
this works same if we partly update the tools/arch/arm64/include/asm/cputype.h
with only necessary MIDR updates and keep is_midr_in_range_list() unchanged.
>> I've replied to the same effect Yicong's patch [1,2].
>>
>> If we want to share headers between userspace and kernel, we should
>> refactor those headers such that this is safe by construction.
>>
>> There is no need to update the userspace headers just because the kernel
>> headers have changed, so the simple solution in the short term is to
>> suppress the warning from check-headers.sh.
>
> Sure, makes sense for me.
>
> @Arnaldo, as Mark suggested, do you want me to send a patch to remove
> cputype.h checking in check-headers.sh or it is fine to keep the warning
> until finish the header refactoring?
>
> @Yicong, could you confirm if you proceed to refactor the MIDR? thanks!
>
please feel free to take this over.
> Just note, I searched tools folder and found kselftest also uses the
> cputype.h header. The refactoring should not break the files below.
>
they shouldn't affected. I did a kselftest build test with my latest patch
and they were not affected.
thanks.
> $ git grep cputype.h
> perf/check-headers.sh:check arch/arm64/include/asm/cputype.h '-I "^#include [<\"]\(asm/\)*sysreg.h"'
> perf/util/arm-spe.c:#include "../../arch/arm64/include/asm/cputype.h"
> testing/selftests/kvm/arm64/psci_test.c:#include <asm/cputype.h>
> testing/selftests/kvm/lib/arm64/vgic.c:#include <asm/cputype.h>
>
> Thanks,
> Leo
>
>> [1] https://lore.kernel.org/linux-arm-kernel/dc5afc5c-060c-8bcb-c3a7-0de49a7455fb@huawei.com/T/#m23dfbea6af559f3765d89b9d8427213588871ffd
>> [2] https://lore.kernel.org/linux-arm-kernel/dc5afc5c-060c-8bcb-c3a7-0de49a7455fb@huawei.com/T/#m6acbfa00002af8ee791266ea86a58f8f994ed710
>>
>> Mark.
>>
>>>
>>> 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;
>>> }
>>>
>>> Then, once we can generate a dynamic MIDR header file, we can use that
>>> header and define the midr_range structure specifically in the perf.
>>> In the end, perf can avoid to include kernel's cputype.h.
>>>
>>> If no objection, Yicong, do you mind preparing the patch mentioned
>>> above? Thanks!
>>>
>>> Leo
>
> .
>
On Wed, Jun 18, 2025 at 07:51:03PM +0800, Yicong Yang wrote: [...] > >> Directly including the kernel header introduces the very fragility that > >> having a copy was intended to avoid. NAK to that. > > > > My suggestion is not to include the kernel header, nor to modify the > > copy header. :) > > > > Instead, I suggested creating a new header within the perf tool (under > > perf's arm64 folder) and then include the copy header in tools: > > > > tools/arch/arm64/include/asm/cputype.h > > > > sorry for the misunderstood.:( > in this way we still have the divergency in the long term and as a workaround > this works same if we partly update the tools/arch/arm64/include/asm/cputype.h > with only necessary MIDR updates and keep is_midr_in_range_list() unchanged. Yes. So Mark's suggestion is reasonable that we can do refactoring first to avoid syncing header. [...] > > @Yicong, could you confirm if you proceed to refactor the MIDR? thanks! > > please feel free to take this over. Thanks a lot for confirmation! And thanks for working on the reported issue. > > Just note, I searched tools folder and found kselftest also uses the > > cputype.h header. The refactoring should not break the files below. > > > > they shouldn't affected. I did a kselftest build test with my latest patch > and they were not affected. I expect tools/arch/arm64/include/asm/cputype.h will be removed, and a generated header (something like sys-midr.h) for MIDR refactoring. If this is true, then we need to take care kselftest. Thanks, Leo
On 2025/6/17 22:18, Leo Yan wrote:
> On Mon, Jun 16, 2025 at 06:47:25PM +0100, Mark Rutland wrote:
>
> [...]
>
>>>> ok this sounds just like as before except rename the midr check function and modify the
>>>> users in perf. will do in below steps:
>>>> - move cpu_errata_set_target_impl()/is_midr_in_range_list() out of cputype.h
>>>> since they're only used in the kernel with errata information
>>>> - introduce is_target_midr_in_range_list() in cputype.h to test certain MIDR
>>>> is within the ranges. (is_perf_midr_in_range_list() only make sense in
>>>> userspace and is a bit strange to me in a kernel header). maybe reimplement
>>>> is_midr_in_range_list() with is_target_midr_in_range_list() otherwise there's
>>>> no users in kernel
>>>> - copy cputype.h to userspace and make users use new is_target_midr_in_range_list()
>>>>
>>>> this will avoid touching the kernel too much and userspace don't need to implement
>>>> a separate function.
>>>
>>> My understanding is we don't need to touch anything in kernel side, we
>>> simply add a wrapper in perf tool to call midr_is_cpu_model_range().
>>>
>>> When introduce is_target_midr_in_range_list() in kernel's cputype.h,
>>> if no consumers in kernel use it and only useful for perf tool, then
>>> it is unlikely to be accepted.
>>
>> I think all of this is just working around the problem that
>> asm/cputype.h was never intended to be used in userspace. Likewise with
>> the other headers that we copy into tools/.
>>
>> If there are bits that we *want* to share with tools/, let's factor that
>> out. The actual MIDR values are a good candidate for that -- we can
>> follow the same approach as with sysreg-defs.h.
>
> Thanks for suggestion, Mark.
>
> It makes sense to me for extracting MIDR and sharing between kernel and
> tools/. I have created a task for following up the refactoring.
>
>> Other than that, I think that userspace should just maintain its own
>> infrastructure, and only pull in things from kernel sources when there's
>> a specific reason to. Otherwise we're just creating busywork.
>
> I agree with the methodology.
>
> Since Arnaldo is facing build failure when sync headers between kernel
> and perf tool, to avoid long latency, let us split the refactoriing
> into separate steps.
>
> As a first step, I think my previous suggestion is valid, we can create a
> header tools/perf/arch/arm64/include/cputype.h with below code:
>
> #include "../../../../arch/arm64/include/asm/cputype.h"
>
> 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;
> }
>
> Then, once we can generate a dynamic MIDR header file, we can use that
> header and define the midr_range structure specifically in the perf.
> In the end, perf can avoid to include kernel's cputype.h.
>
> If no objection, Yicong, do you mind preparing the patch mentioned
> above? Thanks!
>
sure. will post today.
Thanks.
© 2016 - 2025 Red Hat, Inc.