* [PATCH 2/2] powerpc/perf: Add platform specific check_attr_config
2021-02-24 14:28 [PATCH 1/2] powerpc/perf: Infrastructure to support checking of attr.config* Madhavan Srinivasan
@ 2021-02-24 14:28 ` Madhavan Srinivasan
2021-02-24 14:47 ` [PATCH 1/2] powerpc/perf: Infrastructure to support checking of attr.config* Paul A. Clarke
1 sibling, 0 replies; 4+ messages in thread
From: Madhavan Srinivasan @ 2021-02-24 14:28 UTC (permalink / raw)
To: mpe; +Cc: Madhavan Srinivasan, linuxppc-dev
Add platform specific attr.config value checks. Patch
includes checks for power9 and power10 platforms.
Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
arch/powerpc/perf/isa207-common.c | 41 +++++++++++++++++++++++++++++++
arch/powerpc/perf/isa207-common.h | 2 ++
arch/powerpc/perf/power10-pmu.c | 13 ++++++++++
arch/powerpc/perf/power9-pmu.c | 13 ++++++++++
4 files changed, 69 insertions(+)
diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
index e4f577da33d8..b255799f5b51 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c
@@ -694,3 +694,44 @@ int isa207_get_alternatives(u64 event, u64 alt[], int size, unsigned int flags,
return num_alt;
}
+
+int isa3_X_check_attr_config(struct perf_event *ev)
+{
+ u64 val, sample_mode;
+ u64 event = ev->attr.config;
+
+ val = (event >> EVENT_SAMPLE_SHIFT) & EVENT_SAMPLE_MASK;
+ sample_mode = val & 0x3;
+
+ /*
+ * MMCRA[61:62] is Randome Sampling Mode (SM).
+ * value of 0b11 is reserved.
+ */
+ if (sample_mode == 0x3)
+ return -1;
+
+ /*
+ * Check for all reserved value
+ */
+ switch (val) {
+ case 0x5:
+ case 0x9:
+ case 0xD:
+ case 0x19:
+ case 0x1D:
+ case 0x1A:
+ case 0x1E:
+ return -1;
+ }
+
+ /*
+ * MMCRA[48:51]/[52:55]) Threshold Start/Stop
+ * Events Selection.
+ * 0b11110000/0b00001111 is reserved.
+ */
+ val = (event >> EVENT_THR_CTL_SHIFT) & EVENT_THR_CTL_MASK;
+ if (((val & 0xF0) == 0xF0) || ((val & 0xF) == 0xF))
+ return -1;
+
+ return 0;
+}
diff --git a/arch/powerpc/perf/isa207-common.h b/arch/powerpc/perf/isa207-common.h
index 1af0e8c97ac7..ae8eaf05efd1 100644
--- a/arch/powerpc/perf/isa207-common.h
+++ b/arch/powerpc/perf/isa207-common.h
@@ -280,4 +280,6 @@ void isa207_get_mem_data_src(union perf_mem_data_src *dsrc, u32 flags,
struct pt_regs *regs);
void isa207_get_mem_weight(u64 *weight);
+int isa3_X_check_attr_config(struct perf_event *ev);
+
#endif
diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
index a901c1348cad..bc64354cab6a 100644
--- a/arch/powerpc/perf/power10-pmu.c
+++ b/arch/powerpc/perf/power10-pmu.c
@@ -106,6 +106,18 @@ static int power10_get_alternatives(u64 event, unsigned int flags, u64 alt[])
return num_alt;
}
+static int power10_check_attr_config(struct perf_event *ev)
+{
+ u64 val;
+ u64 event = ev->attr.config;
+
+ val = (event >> EVENT_SAMPLE_SHIFT) & EVENT_SAMPLE_MASK;
+ if (val == 0x10 || isa3_X_check_attr_config(ev))
+ return -1;
+
+ return 0;
+}
+
GENERIC_EVENT_ATTR(cpu-cycles, PM_RUN_CYC);
GENERIC_EVENT_ATTR(instructions, PM_RUN_INST_CMPL);
GENERIC_EVENT_ATTR(branch-instructions, PM_BR_CMPL);
@@ -559,6 +571,7 @@ static struct power_pmu power10_pmu = {
.attr_groups = power10_pmu_attr_groups,
.bhrb_nr = 32,
.capabilities = PERF_PMU_CAP_EXTENDED_REGS,
+ .check_attr_config = power10_check_attr_config,
};
int init_power10_pmu(void)
diff --git a/arch/powerpc/perf/power9-pmu.c b/arch/powerpc/perf/power9-pmu.c
index 2a57e93a79dc..b3b9b226d053 100644
--- a/arch/powerpc/perf/power9-pmu.c
+++ b/arch/powerpc/perf/power9-pmu.c
@@ -151,6 +151,18 @@ static int power9_get_alternatives(u64 event, unsigned int flags, u64 alt[])
return num_alt;
}
+static int power9_check_attr_config(struct perf_event *ev)
+{
+ u64 val;
+ u64 event = ev->attr.config;
+
+ val = (event >> EVENT_SAMPLE_SHIFT) & EVENT_SAMPLE_MASK;
+ if (val == 0xC || isa3_X_check_attr_config(ev))
+ return -1;
+
+ return 0;
+}
+
GENERIC_EVENT_ATTR(cpu-cycles, PM_CYC);
GENERIC_EVENT_ATTR(stalled-cycles-frontend, PM_ICT_NOSLOT_CYC);
GENERIC_EVENT_ATTR(stalled-cycles-backend, PM_CMPLU_STALL);
@@ -437,6 +449,7 @@ static struct power_pmu power9_pmu = {
.attr_groups = power9_pmu_attr_groups,
.bhrb_nr = 32,
.capabilities = PERF_PMU_CAP_EXTENDED_REGS,
+ .check_attr_config = power9_check_attr_config,
};
int init_power9_pmu(void)
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] powerpc/perf: Infrastructure to support checking of attr.config*
2021-02-24 14:28 [PATCH 1/2] powerpc/perf: Infrastructure to support checking of attr.config* Madhavan Srinivasan
2021-02-24 14:28 ` [PATCH 2/2] powerpc/perf: Add platform specific check_attr_config Madhavan Srinivasan
@ 2021-02-24 14:47 ` Paul A. Clarke
2021-02-25 6:39 ` Madhavan Srinivasan
1 sibling, 1 reply; 4+ messages in thread
From: Paul A. Clarke @ 2021-02-24 14:47 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: Alexey Kardashevskiy, linuxppc-dev
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
^ permalink raw reply [flat|nested] 4+ messages in thread