| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0b839ec1ae89439e95d7069adcbb95ab@huawei.com>
Date: Mon, 16 Jun 2025 09:54:43 +0000
From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>
To: James Clark <james.clark@...aro.org>, yangyicong <yangyicong@...wei.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
CC: Ali Saidi <alisaidi@...zon.com>, Leo Yan <leo.yan@...aro.org>, Will Deacon
<will@...nel.org>, James Morse <james.morse@....com>, Catalin Marinas
<catalin.marinas@....com>, yangjinqian <yangjinqian1@...wei.com>, "Douglas
Anderson" <dianders@...omium.org>, Dmitry Baryshkov
<dmitry.baryshkov@...aro.org>, Adrian Hunter <adrian.hunter@...el.com>, "Ian
Rogers" <irogers@...gle.com>, Jiri Olsa <jolsa@...nel.org>, Kan Liang
<kan.liang@...ux.intel.com>, Namhyung Kim <namhyung@...nel.org>, "Linux
Kernel Mailing List" <linux-kernel@...r.kernel.org>, yangyicong
<yangyicong@...wei.com>
Subject: RE: perf usage of arch/arm64/include/asm/cputype.h
> -----Original Message-----
> From: James Clark <james.clark@...aro.org>
> Sent: Monday, June 16, 2025 10:30 AM
> To: yangyicong <yangyicong@...wei.com>; Arnaldo Carvalho de Melo
> <acme@...nel.org>; linux-arm-kernel@...ts.infradead.org
> Cc: Ali Saidi <alisaidi@...zon.com>; Leo Yan <leo.yan@...aro.org>; Will
> Deacon <will@...nel.org>; James Morse <james.morse@....com>; Catalin
> Marinas <catalin.marinas@....com>; yangjinqian
> <yangjinqian1@...wei.com>; Douglas Anderson
> <dianders@...omium.org>; Dmitry Baryshkov
> <dmitry.baryshkov@...aro.org>; Adrian Hunter <adrian.hunter@...el.com>;
> Ian Rogers <irogers@...gle.com>; Jiri Olsa <jolsa@...nel.org>; Kan Liang
> <kan.liang@...ux.intel.com>; Namhyung Kim <namhyung@...nel.org>;
> Linux Kernel Mailing List <linux-kernel@...r.kernel.org>; Shameerali
> Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>; yangyicong
> <yangyicong@...wei.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@...zon.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@...zon.com>
> >> Tested-by: Leo Yan <leo.yan@...aro.org>
> >>
> >> - Arnaldo
> >>
> >> ⬢ [acme@...lbx 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@...lbx perf-tools]$ cp arch/arm64/include/asm/cputype.h
> >> tools/arch/arm64/include/asm/cputype.h
> >> ⬢ [acme@...lbx 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@...lbx 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@...wei.com/
> >
> > Thanks.
> >
> > From 44900e7d3d9fa34c817396275f55a2aab611cd32 Mon Sep 17
> 00:00:00
> > 2001
> > From: Yicong Yang <yangyicong@...ilicon.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@...wei.com/ [2]
> > https://lore.kernel.org/lkml/aEyGg98z-MkcClXY@x1/#t
> > Signed-off-by: Yicong Yang <yangyicong@...ilicon.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.
Powered by blists - more mailing lists