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

[Madhavan Srinivasan <maddy@linux.ibm.com>]

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