Message ID | 20210325115326.143151-1-maddy@linux.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v3,1/2] powerpc/perf: Infrastructure to support checking of attr.config* | expand |
Related | show |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (87d76f542a24ecfa797e9bd3bb56c0f19aabff57) |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
> On 25-Mar-2021, at 5:23 PM, Madhavan Srinivasan <maddy@linux.ibm.com> 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 some specific values for > bit fields are reserved. For ex., MMCRA[61:62] is > Random Sampling Mode (SM) and value of 0b11 for this field > is reserved. > > Writing non-zero or invalid values in these 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 v2: > -Fixed commit message > > Changelog v1: > -Fixed commit message and in-code comments Changes looks fine to me. Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> Thanks, Athira > > 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 >
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 event. cheers
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
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;
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 some specific values for bit fields are reserved. For ex., MMCRA[61:62] is Random Sampling Mode (SM) and value of 0b11 for this field is reserved. Writing non-zero or invalid values in these 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 v2: -Fixed commit message 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(+)