linux-kernel - Re: [PATCH] perf tool: arch: arm64: change the way for get_cpuid

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190719100219.b3bbrv4t4qcv2gid@willie-the-truck>
Date:   Fri, 19 Jul 2019 11:02:20 +0100
From:   Will Deacon <will@...nel.org>
To:     Joakim Zhang <qiangqing.zhang@....com>
Cc:     "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "acme@...nel.org" <acme@...nel.org>,
        "jolsa@...hat.com" <jolsa@...hat.com>,
        "namhyung@...nel.org" <namhyung@...nel.org>,
        Frank Li <frank.li@....com>, dl-linux-imx <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mark.rutland@....com" <mark.rutland@....com>
Subject: Re: [PATCH] perf tool: arch: arm64: change the way for
 get_cpuid_str() function

On Fri, Jul 19, 2019 at 08:29:12AM +0000, Joakim Zhang wrote:
> > On Thu, Jul 18, 2019 at 06:21:30AM +0000, Joakim Zhang wrote:
> > > Now the get_cpuid__str function returns the MIDR string of the first
> > > online cpu from the range of cpus asscociated with the PMU CORE device.
> > >
> > > It can work when pass a perf_pmu entity to get_cpuid_str. However, it
> > > will pass NULL via perf_pmu__find_map from metricgroup.c if we want to
> > > add metric group for arm64. When pass NULL to get_cpuid_str, it can't
> > > return the MIDR string.
> > 
> > Why is this code passing a NULL PMU? What information does it actually need?
> > Other functions, such as print_pmu_events(), iterate over all PMUs.
> 
> tools/perf/utils/metricgroup.c:
> metricgroup__add_metric()/metricgroup__has_metric()/metricgroup__print()---->perf_pmu__find_map(NULL)------>perf_pmu__getcpuid(NULL)----->get_cpuid_str(NULL)
> So, it will eventually pass NULL to get_cpuid_str() function for arm64, and now get_cpuid_str() for arm64 need *struct perf_pmu* entity to return MIDR string.
> 
> And the declaration for get_cpuid_str() is char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused) in tools/perf/utils/header.h file.
> So, it can return MIDR string without passing *struct perf_pmu* entity. Arch PowerPC/x86/s390 which implement metricgroup all add __maybe_unused.
> 
> > > There are three methods from userspace getting MIDR string for arm64:
> > > 1. parse
> > > sysfs(/sys/devices/system/cpu/cpu?/regs/identification/midr_el1)
> > > 2. parse procfs(/proc/cpuinfo)
> > > 3. read the hwcaps(MIDR register) with getauxval(AT_HWCAP)
> > >
> > > Perfer to select #3 as it is more simple and direct.
> > 
> > That's probably terminally broken for heterogeneous systems, so I'm not at all
> > happy with this patch.
> 
> The implement of get_cpuid_str() for arm64 now only can return the MIDR
> string of the first online cpu. I think it is also broken for
> heterogeneous systems.

I disagree: the current code returns the MIDR string for the first online
CPU *corresponding to the PMU* (e.g. pmu->cpus) and therefore handles
heterogeneous systems like that.

> Will, do you know how to fix this issue more reasonable?

I still don't know what the metricgroup coce is trying to achieve, but my
previous suggestion about looking at print_pmu_events() is probably still a
good place to start.

Will