| Message ID | 20210224142840.1170088-1-maddy@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] powerpc/perf: Infrastructure to support checking of attr.config* | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (626a6c3d2e20da80aaa710104f34ea6037b28b33) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 30 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On Wed, Feb 24, 2021 at 07:58:39PM +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 fileds that are reserved and specific values to bit fields s/fileds/fields/ > as reserved. Writing a none zero values in these fields Should the previous sentences say something like "required values for specific bit fields" or "specific bit fields that are reserved"? s/none zero/non-zero/ > or writing invalid value to bit fields will have unknown > behaviours. > > Patch here add a generic call-back function "check_attr_config" s/add/adds/ or "This patch adds ..." or just "Add ...". > 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. > > Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com> > --- > arch/powerpc/include/asm/perf_event_server.h | 6 ++++++ > arch/powerpc/perf/core-book3s.c | 12 ++++++++++++ > 2 files changed, 18 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..679d67506299 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -1958,6 +1958,18 @@ static int power_pmu_event_init(struct perf_event *event) > > if (ppmu->blacklist_ev && is_event_blacklisted(ev)) > return -EINVAL; > + /* > + * PMU config registers have fileds that are > + * reserved and spacific values to bit fileds be reserved. s/spacific/specific/ s/fileds/fields/ Same comment about "specific values to bit fields be reserved", and rewording that to be more clear. > + * This call-back will check the event code for same. > + * > + * Event type hardware and hw_cache will not value > + * invalid values in the event code which is not true > + * for raw event type. I confess I don't understand what this means. (But it could be just me!) > + */ > + if (ppmu->check_attr_config && > + ppmu->check_attr_config(event)) > + return -EINVAL; > break; > default: > return -ENOENT; > -- PC
On 2/24/21 8:17 PM, Paul A. Clarke wrote: > On Wed, Feb 24, 2021 at 07:58:39PM +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 fileds that are reserved and specific values to bit fields > s/fileds/fields/ > >> as reserved. Writing a none zero values in these fields > Should the previous sentences say something like "required values > for specific bit fields" or "specific bit fields that are reserved"? > > s/none zero/non-zero/ > >> or writing invalid value to bit fields will have unknown >> behaviours. >> >> Patch here add a generic call-back function "check_attr_config" > s/add/adds/ or "This patch adds ..." or just "Add ...". Thanks for the review. Will fix it. > >> 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. >> >> Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com> >> --- >> arch/powerpc/include/asm/perf_event_server.h | 6 ++++++ >> arch/powerpc/perf/core-book3s.c | 12 ++++++++++++ >> 2 files changed, 18 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..679d67506299 100644 >> --- a/arch/powerpc/perf/core-book3s.c >> +++ b/arch/powerpc/perf/core-book3s.c >> @@ -1958,6 +1958,18 @@ static int power_pmu_event_init(struct perf_event *event) >> >> if (ppmu->blacklist_ev && is_event_blacklisted(ev)) >> return -EINVAL; >> + /* >> + * PMU config registers have fileds that are >> + * reserved and spacific values to bit fileds be reserved. > s/spacific/specific/ > s/fileds/fields/ > Same comment about "specific values to bit fields be reserved", and > rewording that to be more clear. > >> + * This call-back will check the event code for same. >> + * >> + * Event type hardware and hw_cache will not value >> + * invalid values in the event code which is not true >> + * for raw event type. > I confess I don't understand what this means. (But it could be just me!) My bad. What I wanted to say was, this check is needed only for raw event type, since tools like fuzzer use it to provide randomized event code values for test. Will fix the comment Thanks for the review comments. > >> + */ >> + if (ppmu->check_attr_config && >> + ppmu->check_attr_config(event)) >> + return -EINVAL; >> break; >> default: >> return -ENOENT; >> -- > PC
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..679d67506299 100644 --- a/arch/powerpc/perf/core-book3s.c +++ b/arch/powerpc/perf/core-book3s.c @@ -1958,6 +1958,18 @@ static int power_pmu_event_init(struct perf_event *event) if (ppmu->blacklist_ev && is_event_blacklisted(ev)) return -EINVAL; + /* + * PMU config registers have fileds that are + * reserved and spacific values to bit fileds be reserved. + * This call-back will check the event code for same. + * + * Event type hardware and hw_cache will not value + * invalid values in the event code which is not true + * for raw event type. + */ + 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 fileds that are reserved and specific values to bit fields as reserved. Writing a none zero values in these fields or writing invalid value to bit fields will have unknown behaviours. Patch here add 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. Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com> --- arch/powerpc/include/asm/perf_event_server.h | 6 ++++++ arch/powerpc/perf/core-book3s.c | 12 ++++++++++++ 2 files changed, 18 insertions(+)