Re: [PATCH V2 2/2] powerpc/perf: Add platform specific check_attr_config

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 26/02/2021 17:50, Madhavan Srinivasan wrote:
> Add platform specific attr.config value checks. Patch
> includes checks for both power9 and power10.
> 
> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> ---
> Changelog v1:
> - No changes.
> 
>   arch/powerpc/perf/isa207-common.c | 41 +++++++++++++++++++++++++++++++
>   arch/powerpc/perf/isa207-common.h |  2 ++
>   arch/powerpc/perf/power10-pmu.c   | 13 ++++++++++
>   arch/powerpc/perf/power9-pmu.c    | 13 ++++++++++
>   4 files changed, 69 insertions(+)
> 
> diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
> index e4f577da33d8..b255799f5b51 100644
> --- a/arch/powerpc/perf/isa207-common.c
> +++ b/arch/powerpc/perf/isa207-common.c
> @@ -694,3 +694,44 @@ int isa207_get_alternatives(u64 event, u64 alt[], int size, unsigned int flags,
>   
>   	return num_alt;
>   }
> +
> +int isa3_X_check_attr_config(struct perf_event *ev)


"isa300" is used everywhere else to refer to ISA 3.00.


> +{
> +	u64 val, sample_mode;
> +	u64 event = ev->attr.config;
> +
> +	val = (event >> EVENT_SAMPLE_SHIFT) & EVENT_SAMPLE_MASK;

I am not familiar with the code - "Raw event encoding for Power9" from 
arch/powerpc/perf/power9-pmu.c - where is this from? Is this how linux 
defines encoding or it is P9 UM or something?

> +	sample_mode = val & 0x3;
> +
> +	/*
> +	 * MMCRA[61:62] is Randome Sampling Mode (SM).
> +	 * value of 0b11 is reserved.
> +	 */
> +	if (sample_mode == 0x3)
> +		return -1;
> +
> +	/*
> +	 * Check for all reserved value
> +	 */
> +	switch (val) {
> +	case 0x5:
> +	case 0x9:
> +	case 0xD:
> +	case 0x19:
> +	case 0x1D:
> +	case 0x1A:
> +	case 0x1E:


What spec did these numbers come from?

> +		return -1;
> +	}
> +
> +	/*
> +	 * MMCRA[48:51]/[52:55]) Threshold Start/Stop
> +	 * Events Selection.
> +	 * 0b11110000/0b00001111 is reserved.

The mapping between the event and MMCRA is very unclear :) But there are 
more reserved values in MMCRA in PowerISA_public.v3.0B.pdf:

===
0000 Reserved

Problem state access (SPR 770)
1000 - 1111 - ReservedPrivileged access (SPR 770 or 786)
1000 - 1111 - Implementation-dependent
===

Do not you need to filter these too?

> +	 */
> +	val = (event >> EVENT_THR_CTL_SHIFT) & EVENT_THR_CTL_MASK;
> +	if (((val & 0xF0) == 0xF0) || ((val & 0xF) == 0xF))
> +		return -1;

Since the filters may differ for problem and privileged, may be make 
these check_attr_config() hooks return EINVAL or EPERM and pass it on in 
the caller? Not sure there is much value in it though.


> +
> +	return 0;
> +}
> diff --git a/arch/powerpc/perf/isa207-common.h b/arch/powerpc/perf/isa207-common.h
> index 1af0e8c97ac7..ae8eaf05efd1 100644
> --- a/arch/powerpc/perf/isa207-common.h
> +++ b/arch/powerpc/perf/isa207-common.h
> @@ -280,4 +280,6 @@ void isa207_get_mem_data_src(union perf_mem_data_src *dsrc, u32 flags,
>   							struct pt_regs *regs);
>   void isa207_get_mem_weight(u64 *weight);
>   
> +int isa3_X_check_attr_config(struct perf_event *ev);
> +
>   #endif
> diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
> index a901c1348cad..bc64354cab6a 100644
> --- a/arch/powerpc/perf/power10-pmu.c
> +++ b/arch/powerpc/perf/power10-pmu.c
> @@ -106,6 +106,18 @@ static int power10_get_alternatives(u64 event, unsigned int flags, u64 alt[])
>   	return num_alt;
>   }
>   
> +static int power10_check_attr_config(struct perf_event *ev)
> +{
> +	u64 val;
> +	u64 event = ev->attr.config;
> +
> +	val = (event >> EVENT_SAMPLE_SHIFT) & EVENT_SAMPLE_MASK;
> +	if (val == 0x10 || isa3_X_check_attr_config(ev))
> +		return -1;
> +
> +	return 0;
> +}
> +
>   GENERIC_EVENT_ATTR(cpu-cycles,			PM_RUN_CYC);
>   GENERIC_EVENT_ATTR(instructions,		PM_RUN_INST_CMPL);
>   GENERIC_EVENT_ATTR(branch-instructions,		PM_BR_CMPL);
> @@ -559,6 +571,7 @@ static struct power_pmu power10_pmu = {
>   	.attr_groups		= power10_pmu_attr_groups,
>   	.bhrb_nr		= 32,
>   	.capabilities           = PERF_PMU_CAP_EXTENDED_REGS,
> +	.check_attr_config	= power10_check_attr_config,
>   };
>   
>   int init_power10_pmu(void)
> diff --git a/arch/powerpc/perf/power9-pmu.c b/arch/powerpc/perf/power9-pmu.c
> index 2a57e93a79dc..b3b9b226d053 100644
> --- a/arch/powerpc/perf/power9-pmu.c
> +++ b/arch/powerpc/perf/power9-pmu.c
> @@ -151,6 +151,18 @@ static int power9_get_alternatives(u64 event, unsigned int flags, u64 alt[])
>   	return num_alt;
>   }
>   
> +static int power9_check_attr_config(struct perf_event *ev)
> +{
> +	u64 val;
> +	u64 event = ev->attr.config;
> +
> +	val = (event >> EVENT_SAMPLE_SHIFT) & EVENT_SAMPLE_MASK;
> +	if (val == 0xC || isa3_X_check_attr_config(ev))
> +		return -1;
> +
> +	return 0;
> +}
> +
>   GENERIC_EVENT_ATTR(cpu-cycles,			PM_CYC);
>   GENERIC_EVENT_ATTR(stalled-cycles-frontend,	PM_ICT_NOSLOT_CYC);
>   GENERIC_EVENT_ATTR(stalled-cycles-backend,	PM_CMPLU_STALL);
> @@ -437,6 +449,7 @@ static struct power_pmu power9_pmu = {
>   	.attr_groups		= power9_pmu_attr_groups,
>   	.bhrb_nr		= 32,
>   	.capabilities           = PERF_PMU_CAP_EXTENDED_REGS,
> +	.check_attr_config	= power9_check_attr_config,
>   };
>   
>   int init_power9_pmu(void)
> 

-- 
Alexey