| Message ID | 1624200360-1429-2-git-send-email-atrajeev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/perf: Add instruction and data address registers to extended regs | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (7f030e9d57b8ff6025bde4162f42378e6081126a) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 40 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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? I guess we already got this wrong when we added the Power10 registers, SIER2/3 etc. cheers
> 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
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes: >> 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. 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. cheers
> On 20-Sep-2021, at 12:43 PM, Michael Ellerman <mpe@ellerman.id.au> wrote: > > Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes: >>> 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. > > 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
diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h b/arch/powerpc/include/uapi/asm/perf_regs.h index 578b3ee..cf5eee5 100644 --- a/arch/powerpc/include/uapi/asm/perf_regs.h +++ b/arch/powerpc/include/uapi/asm/perf_regs.h @@ -61,6 +61,8 @@ enum perf_event_powerpc_regs { PERF_REG_POWERPC_PMC4, PERF_REG_POWERPC_PMC5, PERF_REG_POWERPC_PMC6, + PERF_REG_POWERPC_SDAR, + PERF_REG_POWERPC_SIAR, /* Max regs without the extended regs */ PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1, }; @@ -72,16 +74,16 @@ enum perf_event_powerpc_regs { /* * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300 - * includes 9 SPRS from MMCR0 to PMC6 excluding the + * includes 11 SPRS from MMCR0 to SIAR excluding the * unsupported SPRS in PERF_EXCLUDE_REG_EXT_300. */ -#define PERF_REG_PMU_MASK_300 ((0xfffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300) +#define PERF_REG_PMU_MASK_300 ((0x3fffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300) /* * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 - * includes 12 SPRs from MMCR0 to PMC6. + * includes 14 SPRs from MMCR0 to SIAR. */ -#define PERF_REG_PMU_MASK_31 (0xfffULL << PERF_REG_POWERPC_MMCR0) +#define PERF_REG_PMU_MASK_31 (0x3fffULL << PERF_REG_POWERPC_MMCR0) -#define PERF_REG_EXTENDED_MAX (PERF_REG_POWERPC_PMC6 + 1) +#define PERF_REG_EXTENDED_MAX (PERF_REG_POWERPC_SIAR + 1) #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */ 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; } }
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(-)