[PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of extended regs

[Athira Rajeev]

> On 20-Sep-2021, at 12:43 PM, Michael Ellerman <mpe at ellerman.id.au> wrote:
> 
> Athira Rajeev <atrajeev at linux.vnet.ibm.com> writes:
>>> On 08-Sep-2021, at 10:47 AM, Michael Ellerman <mpe at ellerman.id.au> wrote:
>>> 
>>> Athira Rajeev <atrajeev at linux.vnet.ibm.com> writes:
>>>> Patch adds support to include Sampled Instruction Address Register
>>>> (SIAR) and Sampled Data Address Register (SDAR) SPRs as part of extended
>>>> registers. Update the definition of PERF_REG_PMU_MASK_300/31 and
>>>> PERF_REG_EXTENDED_MAX to include these SPR's.
>>>> 
>>>> Signed-off-by: Athira Rajeev <atrajeev at linux.vnet.ibm.com>
>>>> ---
>>>> arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
>>>> arch/powerpc/perf/perf_regs.c             |  4 ++++
>>>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>>> 
>>> ...
>>>> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
>>>> index b931eed..51d31b6 100644
>>>> --- a/arch/powerpc/perf/perf_regs.c
>>>> +++ b/arch/powerpc/perf/perf_regs.c
>>>> @@ -90,7 +90,11 @@ static u64 get_ext_regs_value(int idx)
>>>> 		return mfspr(SPRN_SIER2);
>>>> 	case PERF_REG_POWERPC_SIER3:
>>>> 		return mfspr(SPRN_SIER3);
>>>> +	case PERF_REG_POWERPC_SDAR:
>>>> +		return mfspr(SPRN_SDAR);
>>>> #endif
>>>> +	case PERF_REG_POWERPC_SIAR:
>>>> +		return mfspr(SPRN_SIAR);
>>>> 	default: return 0;
>>>> 	}
>>> 
>>> This file is built for all powerpc configs that have PERF_EVENTS. Which
>>> includes CPUs that don't have SDAR or SIAR.
>>> 
>>> Don't we need checks in perf_reg_value() like we do for SIER?
>> 
>> Hi Michael,
>> 
>> Thanks for the review.
>> 
>> SIER is part of PERF_REG_PMU_MASK and hence check is needed to see if platform supports SIER.
>> Incase of extended regs, they are part of PERF_REG_EXTENDED_MASK and this mask is
>> filled with supported registers while registering the PMU ( ie during init_power9/10_pmu ). So these registers will be added
>> only for supported platforms. The validity of extended mask is also done in PMU common code 
>> ( In kernel/events/core.c with PERF_REG_EXTENDED_MASK check ). So an unsupported platform requesting for extended
>> registers won’t get it.
> 
> Right, I'd forgotten how that works.
> 
> But I think part of the reason I didn't remember is that
> PERF_REG_PMU_MASK_31 doesn't mention those regs by name, it's just a hex
> constant, ie:
> 
> -#define PERF_REG_PMU_MASK_31   (0xfffULL << PERF_REG_POWERPC_MMCR0)
> +#define PERF_REG_PMU_MASK_31   (0x3fffULL << PERF_REG_POWERPC_MMCR0)
> 
> Presumably you tested that the added 0x3 there sets the right bits for
> SDAR and SIAR, but it's 1) not obvious and 2) fragile.
> 
> So I'd like it better if we constructed the PERF_REG_PMU_MASK_31, and
> other similar masks, by or'ing together the actual register value
> constants.
> 
> eg. something like:
> 
> #define PERF_REG_PMU_MASK_31	\
> 	((1ul << PERF_REG_POWERPC_MMCR0) | (1ul << PERF_REG_POWERPC_MMCR1) | \
> 	(1ul << PERF_REG_POWERPC_MMCR2) | (1ul << PERF_REG_POWERPC_MMCR3) | \
> 	(1ul << PERF_REG_POWERPC_SIER2) | (1ul << PERF_REG_POWERPC_SIER3) | \
> 	(1ul << PERF_REG_POWERPC_PMC1) | (1ul << PERF_REG_POWERPC_PMC2) | \
> 	(1ul << PERF_REG_POWERPC_PMC3) | (1ul << PERF_REG_POWERPC_PMC4) | \
> 	(1ul << PERF_REG_POWERPC_PMC5) | (1ul << PERF_REG_POWERPC_PMC6))
> 
> 
> Also PERF_REG_EXTENDED_MAX should be part of the enum, just like
> PERF_REG_POWERPC_MAX.

Sure Michael,

I will address these changes in next version

Thanks
Athira
> 
> cheers