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

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 08-Sep-2021, at 10:47 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Athira Rajeev <atrajeev@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@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.

Thanks
Athira
> 
> I guess we already got this wrong when we added the Power10 registers,
> SIER2/3 etc.
> 
> cheers