Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/2] powerpc/perf: Add support for outputting extended
 regs in perf intr_regs
To:     Anju T Sudhakar <anju@linux.vnet.ibm.com>
Cc:     linuxppc-dev@lists.ozlabs.org, mpe@ellerman.id.au, acme@kernel.org,
        linux-kernel@vger.kernel.org, maddy@linux.vnet.ibm.com,
        jolsa@kernel.org, Ravi Bangoria <ravi.bangoria@linux.ibm.com>
References: <20200429060415.25930-1-anju@linux.vnet.ibm.com>
 <20200429060415.25930-3-anju@linux.vnet.ibm.com>
From:   Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Message-ID: <3d8b3e54-8294-c5c1-f1ad-33e64a738ea7@linux.ibm.com>
Date:   Tue, 5 May 2020 16:01:15 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200429060415.25930-3-anju@linux.vnet.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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>