On 08-Jul-2020, at 5:34 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:

Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
Include capability flag `PERF_PMU_CAP_EXTENDED_REGS` for power10
and expose MMCR3, SIER2, SIER3 registers as part of extended regs.
Also introduce `PERF_REG_PMU_MASK_31` to define extended mask
value at runtime for power10

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
arch/powerpc/include/uapi/asm/perf_regs.h       |  6 ++++++
arch/powerpc/perf/perf_regs.c                   | 10 +++++++++-
arch/powerpc/perf/power10-pmu.c                 |  6 ++++++
tools/arch/powerpc/include/uapi/asm/perf_regs.h |  6 ++++++
tools/perf/arch/powerpc/include/perf_regs.h     |  3 +++
tools/perf/arch/powerpc/util/perf_regs.c        |  6 ++++++

Please split into a kernel patch and a tools patch. And cc the tools people.

Ok sure

6 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h b/arch/powerpc/include/uapi/asm/perf_regs.h
index 485b1d5..020b51c 100644
--- a/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/arch/powerpc/include/uapi/asm/perf_regs.h
@@ -52,6 +52,9 @@ enum perf_event_powerpc_regs {
PERF_REG_POWERPC_MMCR0,
PERF_REG_POWERPC_MMCR1,
PERF_REG_POWERPC_MMCR2,
+ PERF_REG_POWERPC_MMCR3,
+ PERF_REG_POWERPC_SIER2,
+ PERF_REG_POWERPC_SIER3,
/* Max regs without the extended regs */
PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
};
@@ -62,4 +65,7 @@ enum perf_event_powerpc_regs {
#define PERF_REG_PMU_MASK_300   (((1ULL << (PERF_REG_POWERPC_MMCR2 + 1)) - 1) \
- PERF_REG_PMU_MASK)

+/* PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 */
+#define PERF_REG_PMU_MASK_31 (((1ULL << (PERF_REG_POWERPC_SIER3 + 1)) - 1) \
+ - PERF_REG_PMU_MASK)

Wrapping that provides no benefit, just let it be long.


Ok,

#endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
index c8a7e8c..c969935 100644
--- a/arch/powerpc/perf/perf_regs.c
+++ b/arch/powerpc/perf/perf_regs.c
@@ -81,6 +81,12 @@ static u64 get_ext_regs_value(int idx)
return mfspr(SPRN_MMCR1);
case PERF_REG_POWERPC_MMCR2:
return mfspr(SPRN_MMCR2);
+ case PERF_REG_POWERPC_MMCR3:
+ return mfspr(SPRN_MMCR3);
+ case PERF_REG_POWERPC_SIER2:
+ return mfspr(SPRN_SIER2);
+ case PERF_REG_POWERPC_SIER3:
+ return mfspr(SPRN_SIER3);

Indentation is wrong.

default: return 0;
}
}
@@ -89,7 +95,9 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
{
u64 PERF_REG_EXTENDED_MAX;

- if (cpu_has_feature(CPU_FTR_ARCH_300))
+ if (cpu_has_feature(CPU_FTR_ARCH_31))
+ PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_SIER3 + 1;

There's no way to know if that's correct other than going back to the
header to look at the list of values.

So instead you should define it in the header, next to the other values,
with a meaningful name, like PERF_REG_MAX_ISA_31 or something.

+ else if (cpu_has_feature(CPU_FTR_ARCH_300))
PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_MMCR2 + 1;

Same.


Ok, will make this change

if (idx == PERF_REG_POWERPC_SIER &&
diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
index 07fb919..51082d6 100644
--- a/arch/powerpc/perf/power10-pmu.c
+++ b/arch/powerpc/perf/power10-pmu.c
@@ -86,6 +86,8 @@
#define POWER10_MMCRA_IFM3 0x00000000C0000000UL
#define POWER10_MMCRA_BHRB_MASK 0x00000000C0000000UL

+extern u64 mask_var;

Why is it extern? Also not a good name for a global.

Hang on, it's not even used? Is there some macro magic somewhere?

This is defined in patch 8 "powerpc/perf: Add support for outputting extended regs in perf intr_regs”, 
which adds the base support for extended regs in powerpc. Current patch covers changes to support
It for power10. 

`mask_var` is used to define `PERF_REG_EXTENDED_MASK` at run time. 
`PERF_REG_EXTENDED_MASK` basically contains mask value of supported extended registers.
And since supported registers may differ between processor versions, we are defining this mask at runtime.

The #define is done in arch/powerpc/include/asm/perf_event_server.h ( in patch 8 ).
In the PMU driver init, we will set the respective mask value ( in the below code ). Hence it is extern

Sorry for the confusion here. 

Thanks
Athira


/* Table of alternatives, sorted by column 0 */
static const unsigned int power10_event_alternatives[][MAX_ALT] = {
{ PM_RUN_CYC_ALT, PM_RUN_CYC },
@@ -397,6 +399,7 @@ static void power10_config_bhrb(u64 pmu_bhrb_filter)
.cache_events = &power10_cache_events,
.attr_groups = power10_pmu_attr_groups,
.bhrb_nr = 32,
+ .capabilities           = PERF_PMU_CAP_EXTENDED_REGS,
};

int init_power10_pmu(void)
@@ -408,6 +411,9 @@ int init_power10_pmu(void)
    strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power10"))
return -ENODEV;

+ /* Set the PERF_REG_EXTENDED_MASK here */
+ mask_var = PERF_REG_PMU_MASK_31;
+
rc = register_power_pmu(&power10_pmu);
if (rc)
return rc;


cheers