> On 20-Sep-2021, at 12:43 PM, Michael Ellerman <m...@ellerman.id.au> wrote:
>
> Athira Rajeev <atraj...@linux.vnet.ibm.com> writes:
>>> On 08-Sep-2021, at 10:47 AM, Michael Ellerman <m...@ellerman.id.au> wrote:
>>>
>>> Athira Rajeev <atraj...@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 <atraj...@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
