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

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:

quoted

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".

quoted

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.

quoted

and value of 0b11 to this field is reserved.

s/to/for/

quoted

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

quoted

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