RE: [PATCH] perf tool: arch: arm64: change the way for get_cpuid_str() function

[Joakim Zhang]

> -----Original Message-----
> From: Will Deacon <will@xxxxxxxxxx>
> Sent: 2019年7月19日 15:55
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Cc: peterz@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; acme@xxxxxxxxxx;
> jolsa@xxxxxxxxxx; namhyung@xxxxxxxxxx; Frank Li <frank.li@xxxxxxx>;
> dl-linux-imx <linux-imx@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> mark.rutland@xxxxxxx
> Subject: Re: [PATCH] perf tool: arch: arm64: change the way for get_cpuid_str()
> function
> 
> 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.

Hi Will,

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.

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

Best Regards,
Joakim Zhang
> Will
> 
> > diff --git a/tools/perf/arch/arm64/util/header.c
> > b/tools/perf/arch/arm64/util/header.c
> > index 534cd2507d83..f58f08af0be8 100644
> > --- a/tools/perf/arch/arm64/util/header.c
> > +++ b/tools/perf/arch/arm64/util/header.c
> > @@ -2,64 +2,46 @@
> >  #include <stdlib.h>
> >  #include <api/fs/fs.h>
> >  #include "header.h"
> > +#include <asm/hwcap.h>
> > +#include <sys/auxv.h>
> >
> > -#define MIDR "/regs/identification/midr_el1"
> >  #define MIDR_SIZE 19
> >  #define MIDR_REVISION_MASK      0xf
> >  #define MIDR_VARIANT_SHIFT      20
> >  #define MIDR_VARIANT_MASK       (0xf << MIDR_VARIANT_SHIFT)
> >
> > -char *get_cpuid_str(struct perf_pmu *pmu)
> > +#define get_cpuid(id) ({					\
> > +		unsigned long __val;				\
> > +		asm("mrs %0, "#id : "=r" (__val));		\
> > +		__val;						\
> > +	})
> > +
> > +char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
> >  {
> >  	char *buf = NULL;
> > -	char path[PATH_MAX];
> > -	const char *sysfs = sysfs__mountpoint();
> > -	int cpu;
> > -	u64 midr = 0;
> > -	struct cpu_map *cpus;
> > -	FILE *file;
> > +	unsigned long midr = 0;
> >
> > -	if (!sysfs || !pmu || !pmu->cpus)
> > +	if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
> > +		fputs("CPUID registers unavailable\n", stderr);
> >  		return NULL;
> > +	}
> >
> > -	buf = malloc(MIDR_SIZE);
> > -	if (!buf)
> > +	midr = get_cpuid(MIDR_EL1);
> > +	if (!midr) {
> > +		fputs("Failed to get cpuid string\n", stderr);
> >  		return NULL;
> > +	}
> >
> > -	/* read midr from list of cpus mapped to this pmu */
> > -	cpus = cpu_map__get(pmu->cpus);
> > -	for (cpu = 0; cpu < cpus->nr; cpu++) {
> > -		scnprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d"MIDR,
> > -				sysfs, cpus->map[cpu]);
> > -
> > -		file = fopen(path, "r");
> > -		if (!file) {
> > -			pr_debug("fopen failed for file %s\n", path);
> > -			continue;
> > -		}
> > -
> > -		if (!fgets(buf, MIDR_SIZE, file)) {
> > -			fclose(file);
> > -			continue;
> > -		}
> > -		fclose(file);
> > +	/* Ignore/clear Variant[23:20] and
> > +	 * Revision[3:0] of MIDR
> > +	 */
> > +	midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK));
> >
> > -		/* Ignore/clear Variant[23:20] and
> > -		 * Revision[3:0] of MIDR
> > -		 */
> > -		midr = strtoul(buf, NULL, 16);
> > -		midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK));
> > -		scnprintf(buf, MIDR_SIZE, "0x%016lx", midr);
> > -		/* got midr break loop */
> > -		break;
> > -	}
> > +	buf = malloc(MIDR_SIZE);
> > +	if (!buf)
> > +		return NULL;
> >
> > -	if (!midr) {
> > -		pr_err("failed to get cpuid string for PMU %s\n", pmu->name);
> > -		free(buf);
> > -		buf = NULL;
> > -	}
> > +	scnprintf(buf, MIDR_SIZE, "0x%016lx", midr);
> >
> > -	cpu_map__put(cpus);
> >  	return buf;
> >  }
> > --
> > 2.17.1
> >