Re: [PATCH V2 2/2] powerpc/perf: Add platform specific check_attr_config
flat view
From: Alexey Kardashevskiy <hidden>
Date: 2021-03-10 13:17:04
On 26/02/2021 17:50, Madhavan Srinivasan wrote:
quoted hunk ↗ jump to hunk
Add platform specific attr.config value checks. Patch includes checks for both power9 and power10. Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com> --- Changelog v1: - No changes. 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)
"isa300" is used everywhere else to refer to ISA 3.00.
quoted hunk ↗ jump to hunk
+{ + u64 val, sample_mode; + u64 event = ev->attr.config; + + val = (event >> EVENT_SAMPLE_SHIFT) & EVENT_SAMPLE_MASK;
I am not familiar with the code - "Raw event encoding for Power9" from arch/powerpc/perf/power9-pmu.c - where is this from? Is this how linux defines encoding or it is P9 UM or something?
quoted hunk ↗ jump to hunk
+ 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:
What spec did these numbers come from?
quoted hunk ↗ jump to hunk
+ return -1; + } + + /* + * MMCRA[48:51]/[52:55]) Threshold Start/Stop + * Events Selection. + * 0b11110000/0b00001111 is reserved.
The mapping between the event and MMCRA is very unclear :) But there are more reserved values in MMCRA in PowerISA_public.v3.0B.pdf: === 0000 Reserved Problem state access (SPR 770) 1000 - 1111 - ReservedPrivileged access (SPR 770 or 786) 1000 - 1111 - Implementation-dependent === Do not you need to filter these too?
quoted hunk ↗ jump to hunk
+ */ + val = (event >> EVENT_THR_CTL_SHIFT) & EVENT_THR_CTL_MASK; + if (((val & 0xF0) == 0xF0) || ((val & 0xF) == 0xF)) + return -1;
Since the filters may differ for problem and privileged, may be make these check_attr_config() hooks return EINVAL or EPERM and pass it on in the caller? Not sure there is much value in it though.
quoted hunk ↗ jump to hunk
+ + 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); + #endifdiff --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)
-- Alexey