<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 3/10/21 6:46 PM, Alexey
Kardashevskiy wrote:<br>
</div>
<blockquote type="cite"
cite="mid:dde47591-db00-4c4f-ed24-a8ab5a7a4c6a@ozlabs.ru">
<br>
<br>
On 26/02/2021 17:50, Madhavan Srinivasan wrote:
<br>
<blockquote type="cite">Add platform specific attr.config value
checks. Patch
<br>
includes checks for both power9 and power10.
<br>
<br>
Signed-off-by: Madhavan Srinivasan <a class="moz-txt-link-rfc2396E" href="mailto:maddy@linux.ibm.com"><maddy@linux.ibm.com></a>
<br>
---
<br>
Changelog v1:
<br>
- No changes.
<br>
<br>
arch/powerpc/perf/isa207-common.c | 41
+++++++++++++++++++++++++++++++
<br>
arch/powerpc/perf/isa207-common.h | 2 ++
<br>
arch/powerpc/perf/power10-pmu.c | 13 ++++++++++
<br>
arch/powerpc/perf/power9-pmu.c | 13 ++++++++++
<br>
4 files changed, 69 insertions(+)
<br>
<br>
diff --git a/arch/powerpc/perf/isa207-common.c
b/arch/powerpc/perf/isa207-common.c
<br>
index e4f577da33d8..b255799f5b51 100644
<br>
--- a/arch/powerpc/perf/isa207-common.c
<br>
+++ b/arch/powerpc/perf/isa207-common.c
<br>
@@ -694,3 +694,44 @@ int isa207_get_alternatives(u64 event, u64
alt[], int size, unsigned int flags,
<br>
return num_alt;
<br>
}
<br>
+
<br>
+int isa3_X_check_attr_config(struct perf_event *ev)
<br>
</blockquote>
<br>
<br>
"isa300" is used everywhere else to refer to ISA 3.00.
<br>
</blockquote>
<p><br>
</p>
<p>ok will rename if as isa3XX_check_attr_config then.</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:dde47591-db00-4c4f-ed24-a8ab5a7a4c6a@ozlabs.ru">
<br>
<br>
<blockquote type="cite">+{
<br>
+ u64 val, sample_mode;
<br>
+ u64 event = ev->attr.config;
<br>
+
<br>
+ val = (event >> EVENT_SAMPLE_SHIFT) &
EVENT_SAMPLE_MASK;
<br>
</blockquote>
<br>
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?
<br>
</blockquote>
<p>mpe and pmu folks defined the encoding format for power8, and
then it was carried forward slightly modified for P9.<span
style="color: rgb(29, 28, 29); font-family: Slack-Lato,
appleLogo, sans-serif; font-size: 15px; font-style: normal;
font-variant-ligatures: common-ligatures; font-variant-caps:
normal; font-weight: 400; letter-spacing: normal; orphans: 2;
text-align: left; text-indent: 0px; text-transform: none;
white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; background-color: rgb(248, 248,
248); text-decoration-style: initial; text-decoration-color:
initial; display: inline !important; float: none;"></span></p>
<blockquote type="cite"
cite="mid:dde47591-db00-4c4f-ed24-a8ab5a7a4c6a@ozlabs.ru">
<br>
<blockquote type="cite">+ sample_mode = val & 0x3;
<br>
+
<br>
+ /*
<br>
+ * MMCRA[61:62] is Randome Sampling Mode (SM).
<br>
+ * value of 0b11 is reserved.
<br>
+ */
<br>
+ if (sample_mode == 0x3)
<br>
+ return -1;
<br>
+
<br>
+ /*
<br>
+ * Check for all reserved value
<br>
+ */
<br>
+ switch (val) {
<br>
+ case 0x5:
<br>
+ case 0x9:
<br>
+ case 0xD:
<br>
+ case 0x19:
<br>
+ case 0x1D:
<br>
+ case 0x1A:
<br>
+ case 0x1E:
<br>
</blockquote>
<br>
<br>
What spec did these numbers come from?
<br>
</blockquote>
<p><br>
</p>
<p>Thanks for asking. Yes, these are published as part of
Performance monitoring unit user guide.</p>
<p><a class="moz-txt-link-freetext" href="https://wiki.raptorcs.com/w/images/6/6b/POWER9_PMU_UG_v12_28NOV2018_pub.pdf">https://wiki.raptorcs.com/w/images/6/6b/POWER9_PMU_UG_v12_28NOV2018_pub.pdf</a><br>
</p>
<p>Will add a comment about the source of these bits.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:dde47591-db00-4c4f-ed24-a8ab5a7a4c6a@ozlabs.ru">
<br>
<blockquote type="cite">+ return -1;
<br>
+ }
<br>
+
<br>
+ /*
<br>
+ * MMCRA[48:51]/[52:55]) Threshold Start/Stop
<br>
+ * Events Selection.
<br>
+ * 0b11110000/0b00001111 is reserved.
<br>
</blockquote>
<br>
The mapping between the event and MMCRA is very unclear :) But
there are more reserved values in MMCRA in
PowerISA_public.v3.0B.pdf:
<br>
<br>
===
<br>
0000 Reserved
<br>
<br>
Problem state access (SPR 770)
<br>
1000 - 1111 - ReservedPrivileged access (SPR 770 or 786)
<br>
1000 - 1111 - Implementation-dependent
<br>
===
<br>
<br>
Do not you need to filter these too?
<br>
</blockquote>
<p><br>
</p>
<p>Most of these bits are not architected but one should refer user
guide spec which talks about each bit and supported values.</p>
<p><a class="moz-txt-link-freetext" href="https://wiki.raptorcs.com/w/images/6/6b/POWER9_PMU_UG_v12_28NOV2018_pub.pdf">https://wiki.raptorcs.com/w/images/6/6b/POWER9_PMU_UG_v12_28NOV2018_pub.pdf</a></p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:dde47591-db00-4c4f-ed24-a8ab5a7a4c6a@ozlabs.ru">
<br>
<blockquote type="cite">+ */
<br>
+ val = (event >> EVENT_THR_CTL_SHIFT) &
EVENT_THR_CTL_MASK;
<br>
+ if (((val & 0xF0) == 0xF0) || ((val & 0xF) == 0xF))
<br>
+ return -1;
<br>
</blockquote>
<br>
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.</blockquote>
<p><br>
</p>
<p>Since these are reserved values, privilege state does not matter.
Will change it to return EINVAL.</p>
<p><br>
</p>
<p>Thanks for the review.</p>
<p>Maddy<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:dde47591-db00-4c4f-ed24-a8ab5a7a4c6a@ozlabs.ru">
<br>
<br>
<br>
<blockquote type="cite">+
<br>
+ return 0;
<br>
+}
<br>
diff --git a/arch/powerpc/perf/isa207-common.h
b/arch/powerpc/perf/isa207-common.h
<br>
index 1af0e8c97ac7..ae8eaf05efd1 100644
<br>
--- a/arch/powerpc/perf/isa207-common.h
<br>
+++ b/arch/powerpc/perf/isa207-common.h
<br>
@@ -280,4 +280,6 @@ void isa207_get_mem_data_src(union
perf_mem_data_src *dsrc, u32 flags,
<br>
struct pt_regs *regs);
<br>
void isa207_get_mem_weight(u64 *weight);
<br>
+int isa3_X_check_attr_config(struct perf_event *ev);
<br>
+
<br>
#endif
<br>
diff --git a/arch/powerpc/perf/power10-pmu.c
b/arch/powerpc/perf/power10-pmu.c
<br>
index a901c1348cad..bc64354cab6a 100644
<br>
--- a/arch/powerpc/perf/power10-pmu.c
<br>
+++ b/arch/powerpc/perf/power10-pmu.c
<br>
@@ -106,6 +106,18 @@ static int power10_get_alternatives(u64
event, unsigned int flags, u64 alt[])
<br>
return num_alt;
<br>
}
<br>
+static int power10_check_attr_config(struct perf_event *ev)
<br>
+{
<br>
+ u64 val;
<br>
+ u64 event = ev->attr.config;
<br>
+
<br>
+ val = (event >> EVENT_SAMPLE_SHIFT) &
EVENT_SAMPLE_MASK;
<br>
+ if (val == 0x10 || isa3_X_check_attr_config(ev))
<br>
+ return -1;
<br>
+
<br>
+ return 0;
<br>
+}
<br>
+
<br>
GENERIC_EVENT_ATTR(cpu-cycles, PM_RUN_CYC);
<br>
GENERIC_EVENT_ATTR(instructions, PM_RUN_INST_CMPL);
<br>
GENERIC_EVENT_ATTR(branch-instructions, PM_BR_CMPL);
<br>
@@ -559,6 +571,7 @@ static struct power_pmu power10_pmu = {
<br>
.attr_groups = power10_pmu_attr_groups,
<br>
.bhrb_nr = 32,
<br>
.capabilities = PERF_PMU_CAP_EXTENDED_REGS,
<br>
+ .check_attr_config = power10_check_attr_config,
<br>
};
<br>
int init_power10_pmu(void)
<br>
diff --git a/arch/powerpc/perf/power9-pmu.c
b/arch/powerpc/perf/power9-pmu.c
<br>
index 2a57e93a79dc..b3b9b226d053 100644
<br>
--- a/arch/powerpc/perf/power9-pmu.c
<br>
+++ b/arch/powerpc/perf/power9-pmu.c
<br>
@@ -151,6 +151,18 @@ static int power9_get_alternatives(u64
event, unsigned int flags, u64 alt[])
<br>
return num_alt;
<br>
}
<br>
+static int power9_check_attr_config(struct perf_event *ev)
<br>
+{
<br>
+ u64 val;
<br>
+ u64 event = ev->attr.config;
<br>
+
<br>
+ val = (event >> EVENT_SAMPLE_SHIFT) &
EVENT_SAMPLE_MASK;
<br>
+ if (val == 0xC || isa3_X_check_attr_config(ev))
<br>
+ return -1;
<br>
+
<br>
+ return 0;
<br>
+}
<br>
+
<br>
GENERIC_EVENT_ATTR(cpu-cycles, PM_CYC);
<br>
GENERIC_EVENT_ATTR(stalled-cycles-frontend,
PM_ICT_NOSLOT_CYC);
<br>
GENERIC_EVENT_ATTR(stalled-cycles-backend, PM_CMPLU_STALL);
<br>
@@ -437,6 +449,7 @@ static struct power_pmu power9_pmu = {
<br>
.attr_groups = power9_pmu_attr_groups,
<br>
.bhrb_nr = 32,
<br>
.capabilities = PERF_PMU_CAP_EXTENDED_REGS,
<br>
+ .check_attr_config = power9_check_attr_config,
<br>
};
<br>
int init_power9_pmu(void)
<br>
<br>
</blockquote>
<br>
</blockquote>
</body>
</html>