[PATCH V2 1/2] powerpc/perf: Infrastructure to support checking of attr.config*

[PATCH V2 1/2] powerpc/perf: Infrastructure to support checking of attr.config*

[Madhavan Srinivasan]

Introduce code to support the checking of attr.config* for
values which are reserved for a given platform.
Performance Monitoring Unit (PMU) configuration registers
have fields that are reserved and specific value to bit field
as reserved. For ex., MMCRA[61:62] is Randome Sampling Mode (SM)
and value of 0b11 to this field is reserved.

Writing a non-zero values in these fields or writing invalid
value to bit fields will have unknown behaviours.

Patch adds a generic call-back function "check_attr_config"
in "struct power_pmu", to be called in event_init to
check for attr.config* values for a given platform.
"check_attr_config" is valid only for raw event type.

Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
Changelog v1:
-Fixed commit message and in-code comments

 arch/powerpc/include/asm/perf_event_server.h |  6 ++++++
 arch/powerpc/perf/core-book3s.c              | 14 ++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
index 00e7e671bb4b..dde97d7d9253 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -67,6 +67,12 @@ struct power_pmu {
 	 * the pmu supports extended perf regs capability
 	 */
 	int		capabilities;
+	/*
+	 * Function to check event code for values which are
+	 * reserved. Function takes struct perf_event as input,
+	 * since event code could be spread in attr.config*
+	 */
+	int		(*check_attr_config)(struct perf_event *ev);
 };
 
 /*
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 6817331e22ff..c6eeb4fdc5fd 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1958,6 +1958,20 @@ static int power_pmu_event_init(struct perf_event *event)
 
 		if (ppmu->blacklist_ev && is_event_blacklisted(ev))
 			return -EINVAL;
+		/*
+		 * PMU config registers have fields that are
+		 * reserved and specific value to bit field as reserved.
+		 * For ex., MMCRA[61:62] is Randome Sampling Mode (SM)
+		 * and value of 0b11 to this field is reserved.
+		 *
+		 * This check is needed only for raw event type,
+		 * since tools like fuzzer use raw event type to
+		 * provide randomized event code values for test.
+		 *
+		 */
+		if (ppmu->check_attr_config &&
+		    ppmu->check_attr_config(event))
+			return -EINVAL;
 		break;
 	default:
 		return -ENOENT;
-- 
2.26.2

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

[Madhavan Srinivasan]

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)
+{
+	u64 val, sample_mode;
+	u64 event = ev->attr.config;
+
+	val = (event >> EVENT_SAMPLE_SHIFT) & EVENT_SAMPLE_MASK;
+	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:
+		return -1;
+	}
+
+	/*
+	 * MMCRA[48:51]/[52:55]) Threshold Start/Stop
+	 * Events Selection.
+	 * 0b11110000/0b00001111 is reserved.
+	 */
+	val = (event >> EVENT_THR_CTL_SHIFT) & EVENT_THR_CTL_MASK;
+	if (((val & 0xF0) == 0xF0) || ((val & 0xF) == 0xF))
+		return -1;
+
+	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)
-- 
2.26.2

Re: [PATCH V2 1/2] powerpc/perf: Infrastructure to support checking of attr.config*

[Paul A. Clarke]

Another drive-by review... just some minor nits, below...

On Fri, Feb 26, 2021 at 12:20:24PM +0530, Madhavan Srinivasan wrote:
> Introduce code to support the checking of attr.config* for
> values which are reserved for a given platform.
> Performance Monitoring Unit (PMU) configuration registers
> have fields that are reserved and specific value to bit field

I'd reword to "some specific values for bit fields are reserved".

> as reserved. For ex., MMCRA[61:62] is Randome Sampling Mode (SM)

s/Randome/Random/
This occurs here, and below, and in patch 2/2.

> and value of 0b11 to this field is reserved.

s/to/for/

> Writing a non-zero values in these fields or writing invalid
> value to bit fields will have unknown behaviours.

Suggest: Writing non-zero or invalid values in these fields
will have unknown behaviors. (or "behaviours" ;-)

PC

> Patch adds a generic call-back function "check_attr_config"
> in "struct power_pmu", to be called in event_init to
> check for attr.config* values for a given platform.
> "check_attr_config" is valid only for raw event type.
> 
> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> ---
> Changelog v1:
> -Fixed commit message and in-code comments
> 
>  arch/powerpc/include/asm/perf_event_server.h |  6 ++++++
>  arch/powerpc/perf/core-book3s.c              | 14 ++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
> index 00e7e671bb4b..dde97d7d9253 100644
> --- a/arch/powerpc/include/asm/perf_event_server.h
> +++ b/arch/powerpc/include/asm/perf_event_server.h
> @@ -67,6 +67,12 @@ struct power_pmu {
>  	 * the pmu supports extended perf regs capability
>  	 */
>  	int		capabilities;
> +	/*
> +	 * Function to check event code for values which are
> +	 * reserved. Function takes struct perf_event as input,
> +	 * since event code could be spread in attr.config*
> +	 */
> +	int		(*check_attr_config)(struct perf_event *ev);
>  };
> 
>  /*
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 6817331e22ff..c6eeb4fdc5fd 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1958,6 +1958,20 @@ static int power_pmu_event_init(struct perf_event *event)
> 
>  		if (ppmu->blacklist_ev && is_event_blacklisted(ev))
>  			return -EINVAL;
> +		/*
> +		 * PMU config registers have fields that are
> +		 * reserved and specific value to bit field as reserved.
> +		 * For ex., MMCRA[61:62] is Randome Sampling Mode (SM)
> +		 * and value of 0b11 to this field is reserved.
> +		 *
> +		 * This check is needed only for raw event type,
> +		 * since tools like fuzzer use raw event type to
> +		 * provide randomized event code values for test.
> +		 *
> +		 */
> +		if (ppmu->check_attr_config &&
> +		    ppmu->check_attr_config(event))
> +			return -EINVAL;
>  		break;
>  	default:
>  		return -ENOENT;
> -- 
> 2.26.2
> 

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

[Alexey Kardashevskiy]

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

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

[Segher Boessenkool]

On Thu, Mar 11, 2021 at 12:16:25AM +1100, Alexey Kardashevskiy wrote:
> On 26/02/2021 17:50, Madhavan Srinivasan wrote:
> >+int isa3_X_check_attr_config(struct perf_event *ev)
> 
> "isa300" is used everywhere else to refer to ISA 3.00.

And that itself is a misspelling, there is nothing called "ISA 3.00" (it
is called "ISA 3.0").


Segher

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

[Madhavan Srinivasan]

[-- Attachment #1: Type: text/plain, Size: 6757 bytes --]


On 3/10/21 6:46 PM, Alexey Kardashevskiy wrote:
>
>
> 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.


ok will rename if as isa3XX_check_attr_config then.


>
>
>> +{
>> +    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?

mpe and pmu folks defined the encoding format for power8, and then it 
was carried forward slightly modified for P9.

>
>> +    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?


Thanks for asking. Yes, these are published as part of Performance 
monitoring unit user guide.

https://wiki.raptorcs.com/w/images/6/6b/POWER9_PMU_UG_v12_28NOV2018_pub.pdf

Will add a comment about the source of these bits.


>
>> +        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?


Most of these bits are not architected but one should refer user guide 
spec which talks about each bit and supported values.

https://wiki.raptorcs.com/w/images/6/6b/POWER9_PMU_UG_v12_28NOV2018_pub.pdf


>
>> +     */
>> +    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.


Since these are reserved values, privilege state does not matter. Will 
change it to return EINVAL.


Thanks for the review.

Maddy


>
>
>
>> +
>> +    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)
>>
>

[-- Attachment #2: Type: text/html, Size: 11998 bytes --]

Re: [PATCH V2 1/2] powerpc/perf: Infrastructure to support checking of attr.config*

[Madhavan Srinivasan]

On 2/26/21 7:33 PM, Paul A. Clarke wrote:
> Another drive-by review... just some minor nits, below...
>
> On Fri, Feb 26, 2021 at 12:20:24PM +0530, Madhavan Srinivasan wrote:
>> Introduce code to support the checking of attr.config* for
>> values which are reserved for a given platform.
>> Performance Monitoring Unit (PMU) configuration registers
>> have fields that are reserved and specific value to bit field
> I'd reword to "some specific values for bit fields are reserved".
>
>> as reserved. For ex., MMCRA[61:62] is Randome Sampling Mode (SM)
> s/Randome/Random/
> This occurs here, and below, and in patch 2/2.
>
>> and value of 0b11 to this field is reserved.
> s/to/for/
>
>> Writing a non-zero values in these fields or writing invalid
>> value to bit fields will have unknown behaviours.
> Suggest: Writing non-zero or invalid values in these fields
> will have unknown behaviors. (or "behaviours" ;-)
>
> PC

Thanks for the review. Will fix it.

Maddy

>
>> Patch adds a generic call-back function "check_attr_config"
>> in "struct power_pmu", to be called in event_init to
>> check for attr.config* values for a given platform.
>> "check_attr_config" is valid only for raw event type.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
>> ---
>> Changelog v1:
>> -Fixed commit message and in-code comments
>>
>>   arch/powerpc/include/asm/perf_event_server.h |  6 ++++++
>>   arch/powerpc/perf/core-book3s.c              | 14 ++++++++++++++
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
>> index 00e7e671bb4b..dde97d7d9253 100644
>> --- a/arch/powerpc/include/asm/perf_event_server.h
>> +++ b/arch/powerpc/include/asm/perf_event_server.h
>> @@ -67,6 +67,12 @@ struct power_pmu {
>>   	 * the pmu supports extended perf regs capability
>>   	 */
>>   	int		capabilities;
>> +	/*
>> +	 * Function to check event code for values which are
>> +	 * reserved. Function takes struct perf_event as input,
>> +	 * since event code could be spread in attr.config*
>> +	 */
>> +	int		(*check_attr_config)(struct perf_event *ev);
>>   };
>>
>>   /*
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 6817331e22ff..c6eeb4fdc5fd 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -1958,6 +1958,20 @@ static int power_pmu_event_init(struct perf_event *event)
>>
>>   		if (ppmu->blacklist_ev && is_event_blacklisted(ev))
>>   			return -EINVAL;
>> +		/*
>> +		 * PMU config registers have fields that are
>> +		 * reserved and specific value to bit field as reserved.
>> +		 * For ex., MMCRA[61:62] is Randome Sampling Mode (SM)
>> +		 * and value of 0b11 to this field is reserved.
>> +		 *
>> +		 * This check is needed only for raw event type,
>> +		 * since tools like fuzzer use raw event type to
>> +		 * provide randomized event code values for test.
>> +		 *
>> +		 */
>> +		if (ppmu->check_attr_config &&
>> +		    ppmu->check_attr_config(event))
>> +			return -EINVAL;
>>   		break;
>>   	default:
>>   		return -ENOENT;
>> -- 
>> 2.26.2
>>