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

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

On 4/7/21 5:08 PM, Michael Ellerman wrote:
> Madhavan Srinivasan <maddy@linux.ibm.com> writes:
>> 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;
> It's not obvious from the diff, but you're only doing the check for RAW
> events.
>
> But I think that's wrong, we should always check, even if the event code
> comes from the kernel we should still apply the same checks. Otherwise
> we might inadvertently use an invalid event code for a HARDWARE or CACHE
Reason for not including HARDWARE and CACHE events are thats,
they are straight forward, meaning they dont use sampling or thresholding
features. Currently, checks are mostly in that spaces to check for any 
invalid
values. We could include the check for all types the events.

I will respin the patch with that change

Thanks for review
Maddy


> event.
>
> cheers