Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/2] powerpc/perf: Add support for outputting extended regs in perf intr_regs | From | Ravi Bangoria <> | Date | Tue, 5 May 2020 16:01:15 +0530 |
| |
Hi Anju,
Minor neats...
> /* > diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h b/arch/powerpc/include/uapi/asm/perf_regs.h > index f599064dd8dc..604b831378fe 100644 > --- a/arch/powerpc/include/uapi/asm/perf_regs.h > +++ b/arch/powerpc/include/uapi/asm/perf_regs.h > @@ -48,6 +48,17 @@ enum perf_event_powerpc_regs { > PERF_REG_POWERPC_DSISR, > PERF_REG_POWERPC_SIER, > PERF_REG_POWERPC_MMCRA, > - PERF_REG_POWERPC_MAX, > + /* Extended registers */ > + PERF_REG_POWERPC_MMCR0, > + PERF_REG_POWERPC_MMCR1, > + PERF_REG_POWERPC_MMCR2, > + PERF_REG_EXTENDED_MAX, > + /* Max regs without the extended regs */ > + PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1, > }; > + > +#define PERF_REG_PMU_MASK ((1ULL << PERF_REG_POWERPC_MAX) - 1)
Would it make sense to reuse PERF_REG_MASK? Userspace code already uses that name for the same expression.
> +#define PERF_REG_EXTENDED_MASK (((1ULL << (PERF_REG_EXTENDED_MAX)) \ > + - 1) - PERF_REG_PMU_MASK)
You don't need parenthesis in (PERF_REG_EXTENDED_MAX). Also, better to keep that `- 1)` in first line.
> + > #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */ > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c > index 3dcfecf858f3..f56b77800a7b 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -2276,6 +2276,7 @@ int register_power_pmu(struct power_pmu *pmu) > > power_pmu.attr_groups = ppmu->attr_groups; > > + power_pmu.capabilities |= (ppmu->capabilities & PERF_PMU_CAP_EXTENDED_REGS); > #ifdef MSR_HV > /* > * Use FCHV to ignore kernel events if MSR.HV is set. > diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c > index a213a0aa5d25..57aa02568caf 100644 > --- a/arch/powerpc/perf/perf_regs.c > +++ b/arch/powerpc/perf/perf_regs.c > @@ -15,7 +15,8 @@ > > #define PT_REGS_OFFSET(id, r) [id] = offsetof(struct pt_regs, r) > > -#define REG_RESERVED (~((1ULL << PERF_REG_POWERPC_MAX) - 1)) > +#define REG_RESERVED (~(PERF_REG_EXTENDED_MASK) & \ > + (~((1ULL << PERF_REG_POWERPC_MAX) - 1)))
Can we reuse PERF_REG_PMU_MASK here and simplify it to: #define REG_RESERVED (~(PERF_REG_EXTENDED_MASK | PERF_REG_PMU_MASK))
> > static unsigned int pt_regs_offset[PERF_REG_POWERPC_MAX] = { > PT_REGS_OFFSET(PERF_REG_POWERPC_R0, gpr[0]), > @@ -69,10 +70,22 @@ static unsigned int pt_regs_offset[PERF_REG_POWERPC_MAX] = { > PT_REGS_OFFSET(PERF_REG_POWERPC_MMCRA, dsisr), > }; > > +/* Function to return the extended register values */ > +static u64 get_ext_regs_value(int idx) > +{ > + switch (idx) { > + case PERF_REG_POWERPC_MMCR0: > + return mfspr(SPRN_MMCR0); > + case PERF_REG_POWERPC_MMCR1: > + return mfspr(SPRN_MMCR1); > + case PERF_REG_POWERPC_MMCR2: > + return mfspr(SPRN_MMCR2);
Unnecessary tabs.
[...]
> diff --git a/tools/arch/powerpc/include/uapi/asm/perf_regs.h b/tools/arch/powerpc/include/uapi/asm/perf_regs.h > index f599064dd8dc..d66953294c73 100644 > --- a/tools/arch/powerpc/include/uapi/asm/perf_regs.h > +++ b/tools/arch/powerpc/include/uapi/asm/perf_regs.h > @@ -48,6 +48,17 @@ enum perf_event_powerpc_regs { > PERF_REG_POWERPC_DSISR, > PERF_REG_POWERPC_SIER, > PERF_REG_POWERPC_MMCRA, > - PERF_REG_POWERPC_MAX, > + /* Extended arch registers */ > + PERF_REG_POWERPC_MMCR0, > + PERF_REG_POWERPC_MMCR1, > + PERF_REG_POWERPC_MMCR2, > + PERF_REG_EXTENDED_MAX, > + /* Max regs without extended arch regs */ > + PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1, > +
Unnecesasy line.
> }; > +#define PERF_REG_PMU_MASK ((1ULL << PERF_REG_POWERPC_MAX) - 1) > +#define PERF_REG_EXTENDED_MASK (((1ULL << (PERF_REG_EXTENDED_MAX))\ > + - 1) - PERF_REG_PMU_MASK) > + > #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */ > diff --git a/tools/perf/arch/powerpc/include/perf_regs.h b/tools/perf/arch/powerpc/include/perf_regs.h > index e18a3556f5e3..f7bbdb816f88 100644 > --- a/tools/perf/arch/powerpc/include/perf_regs.h > +++ b/tools/perf/arch/powerpc/include/perf_regs.h > @@ -64,7 +64,11 @@ static const char *reg_names[] = { > [PERF_REG_POWERPC_DAR] = "dar", > [PERF_REG_POWERPC_DSISR] = "dsisr", > [PERF_REG_POWERPC_SIER] = "sier", > - [PERF_REG_POWERPC_MMCRA] = "mmcra" > + [PERF_REG_POWERPC_MMCRA] = "mmcra", > + [PERF_REG_POWERPC_MMCR0] = "mmcr0", > + [PERF_REG_POWERPC_MMCR1] = "mmcr1", > + [PERF_REG_POWERPC_MMCR2] = "mmcr2", > +
Unnecesasy line.
Apart from those, for the series: Reviewed-and-Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
| |