lkml.org 
[lkml]   [2020]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 2/2] powerpc/perf: Add support for outputting extended regs in perf intr_regs
From
Date
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>

\
 
 \ /
  Last update: 2020-05-05 12:32    [W:0.086 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site