linux-kernel - Re: [PATCH v2 03/14] drivers/perf: apple_m1: Provide helper for mapping PMUv3 events

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <867c5lsxej.wl-maz@kernel.org>
Date: Wed, 19 Feb 2025 16:37:56 +0000
From: Marc Zyngier <maz@...nel.org>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: kvmarm@...ts.linux.dev,
	Joey Gouly <joey.gouly@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	Mingwei Zhang <mizhang@...gle.com>,
	Colton Lewis <coltonlewis@...gle.com>,
	Raghavendra Rao Ananta <rananta@...gle.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	Janne Grunau <j@...nau.net>
Subject: Re: [PATCH v2 03/14] drivers/perf: apple_m1: Provide helper for mapping PMUv3 events

On Mon, 03 Feb 2025 18:31:00 +0000,
Oliver Upton <oliver.upton@...ux.dev> wrote:
> 
> Apple M* parts carry some IMP DEF traps for guest accesses to PMUv3
> registers, even though the underlying hardware doesn't implement PMUv3.
> This means it is possible to virtualize PMUv3 for KVM guests.
> 
> Add a helper for mapping common PMUv3 event IDs onto hardware event IDs,
> keeping the implementation-specific crud in the PMU driver rather than
> KVM proper.
> 
> Tested-by: Janne Grunau <j@...nau.net>
> Signed-off-by: Oliver Upton <oliver.upton@...ux.dev>
> ---
>  drivers/perf/apple_m1_cpu_pmu.c | 35 +++++++++++++++++++++++++++++++++
>  include/linux/perf/arm_pmu.h    |  1 +
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/perf/apple_m1_cpu_pmu.c b/drivers/perf/apple_m1_cpu_pmu.c
> index d6d4ff6da862..0e54d3f900a7 100644
> --- a/drivers/perf/apple_m1_cpu_pmu.c
> +++ b/drivers/perf/apple_m1_cpu_pmu.c
> @@ -12,6 +12,7 @@
>  
>  #include <linux/of.h>
>  #include <linux/perf/arm_pmu.h>
> +#include <linux/perf/arm_pmuv3.h>
>  #include <linux/platform_device.h>
>  
>  #include <asm/apple_m1_pmu.h>
> @@ -174,6 +175,17 @@ static const unsigned m1_pmu_perf_map[PERF_COUNT_HW_MAX] = {
>  	[PERF_COUNT_HW_BRANCH_MISSES]		= M1_PMU_PERFCTR_BRANCH_MISPRED_NONSPEC,
>  };
>  
> +#define M1_PMUV3_EVENT_MAP(pmuv3_event, m1_event)							\
> +	[ARMV8_PMUV3_PERFCTR_##pmuv3_event]			= M1_PMU_PERFCTR_##m1_event
> +
> +static const unsigned int m1_pmu_pmceid_map[ARMV8_PMUV3_MAX_COMMON_EVENTS] = {

nit: M1_PMU_PERFCTR* are limited to 8bit quantities, and
HW_OP_UNSUPPORTED is a 16bit constant. Probably worth changing the
type to u16 and save a whole 128 bytes! ;-)

> +	[0 ... ARMV8_PMUV3_MAX_COMMON_EVENTS - 1]	= HW_OP_UNSUPPORTED,
> +	M1_PMUV3_EVENT_MAP(INST_RETIRED,	INST_ALL),
> +	M1_PMUV3_EVENT_MAP(CPU_CYCLES,		CORE_ACTIVE_CYCLE),
> +	M1_PMUV3_EVENT_MAP(BR_RETIRED,		INST_BRANCH),
> +	M1_PMUV3_EVENT_MAP(BR_MIS_PRED_RETIRED,	BRANCH_MISPRED_NONSPEC),
> +};
> +
>  /* sysfs definitions */
>  static ssize_t m1_pmu_events_sysfs_show(struct device *dev,
>  					struct device_attribute *attr,
> @@ -558,6 +570,26 @@ static int m2_pmu_map_event(struct perf_event *event)
>  	return armpmu_map_event(event, &m1_pmu_perf_map, NULL, M1_PMU_CFG_EVENT);
>  }
>  
> +static int m1_pmu_map_pmuv3_event(unsigned int eventsel)
> +{
> +	int m1_event = HW_OP_UNSUPPORTED;

unsigned?

> +
> +	if (eventsel < ARMV8_PMUV3_MAX_COMMON_EVENTS)
> +		m1_event = m1_pmu_pmceid_map[eventsel];
> +
> +	return m1_event == HW_OP_UNSUPPORTED ? -EOPNOTSUPP : m1_event;
> +}
> +
> +static void m1_pmu_init_pmceid(struct arm_pmu *pmu)
> +{
> +	unsigned int event;
> +
> +	for (event = 0; event < ARMV8_PMUV3_MAX_COMMON_EVENTS; event++) {
> +		if (m1_pmu_map_pmuv3_event(event) >= 0)
> +			set_bit(event, pmu->pmceid_bitmap);
> +	}
> +}
> +
>  static void m1_pmu_reset(void *info)
>  {
>  	int i;
> @@ -618,6 +650,9 @@ static int m1_pmu_init(struct arm_pmu *cpu_pmu, u32 flags)
>  	cpu_pmu->reset		  = m1_pmu_reset;
>  	cpu_pmu->set_event_filter = m1_pmu_set_event_filter;
>  
> +	cpu_pmu->map_pmuv3_event  = m1_pmu_map_pmuv3_event;
> +	m1_pmu_init_pmceid(cpu_pmu);
> +
>  	bitmap_set(cpu_pmu->cntr_mask, 0, M1_PMU_NR_COUNTERS);
>  	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_EVENTS] = &m1_pmu_events_attr_group;
>  	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] = &m1_pmu_format_attr_group;
> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index 4b5b83677e3f..35f3778ae20e 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -100,6 +100,7 @@ struct arm_pmu {
>  	void		(*stop)(struct arm_pmu *);
>  	void		(*reset)(void *);
>  	int		(*map_event)(struct perf_event *event);
> +	int		(*map_pmuv3_event)(unsigned int eventsel);
>  	DECLARE_BITMAP(cntr_mask, ARMPMU_MAX_HWEVENTS);
>  	bool		secure_access; /* 32-bit ARM only */
>  #define ARMV8_PMUV3_MAX_COMMON_EVENTS		0x40

I think it may be more logical to introduce this new callback first,
with its usage in KVM, and only then plug it in the PMU driver. Not a
big deal though.

	M.

-- 
Without deviation from the norm, progress is not possible.