| Message ID | 20210930122055.1390-3-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 |
Hi Athira, > PERF_REG_PMU_MASK_300 and PERF_REG_PMU_MASK_31 defines the mask > value for extended registers. Current definition of these mask values > uses hex constant and does not use registers by name, making it less > readable. Patch refactor the macro values in perf tools side header file > by or'ing together the actual register value constants. > This looks like a good simplification. > -/* Exclude MMCR3, SIER2, SIER3 for CPU_FTR_ARCH_300 */ > -#define PERF_EXCLUDE_REG_EXT_300 (7ULL << PERF_REG_POWERPC_MMCR3) This file is uAPI - are we allowed to remove a define? Could a program built against these headers now fail to compile because we've removed it? > - > /* > * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300 > * includes 9 SPRS from MMCR0 to PMC6 excluding the > - * unsupported SPRS in PERF_EXCLUDE_REG_EXT_300. > + * unsupported SPRS MMCR3, SIER2 and SIER3. > */ > -#define PERF_REG_PMU_MASK_300 ((0xfffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300) > +#define PERF_REG_PMU_MASK_300 \ > + ((1ul << PERF_REG_POWERPC_MMCR0) | (1ul << PERF_REG_POWERPC_MMCR1) | \ > + (1ul << PERF_REG_POWERPC_MMCR2) | (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)) > > /* > * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 > * includes 12 SPRs from MMCR0 to PMC6. > */ > -#define PERF_REG_PMU_MASK_31 (0xfffULL << PERF_REG_POWERPC_MMCR0) > +#define PERF_REG_PMU_MASK_31 \ > + (PERF_REG_PMU_MASK_300 | (1ul << PERF_REG_POWERPC_MMCR3) | \ > + (1ul << PERF_REG_POWERPC_SIER2) | (1ul << PERF_REG_POWERPC_SIER3)) > > -#define PERF_REG_EXTENDED_MAX (PERF_REG_POWERPC_PMC6 + 1) Likewise for this define? I think this might also be an issue for some of your other patches which change an include/uapi/ file. Kind regards, Daniel > -- > 2.30.1 (Apple Git-130)
> On 01-Oct-2021, at 11:50 AM, Daniel Axtens <dja@axtens.net> wrote: > > Hi Athira, > >> PERF_REG_PMU_MASK_300 and PERF_REG_PMU_MASK_31 defines the mask >> value for extended registers. Current definition of these mask values >> uses hex constant and does not use registers by name, making it less >> readable. Patch refactor the macro values in perf tools side header file >> by or'ing together the actual register value constants. >> > This looks like a good simplification. > >> -/* Exclude MMCR3, SIER2, SIER3 for CPU_FTR_ARCH_300 */ >> -#define PERF_EXCLUDE_REG_EXT_300 (7ULL << PERF_REG_POWERPC_MMCR3) > > This file is uAPI - are we allowed to remove a define? Could a program > built against these headers now fail to compile because we've removed it? Hi Daniel, Thanks for the review. My bad, you are right. I will correct this in version3 by retaining this define and refactoring the macro. > >> - >> /* >> * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300 >> * includes 9 SPRS from MMCR0 to PMC6 excluding the >> - * unsupported SPRS in PERF_EXCLUDE_REG_EXT_300. >> + * unsupported SPRS MMCR3, SIER2 and SIER3. >> */ >> -#define PERF_REG_PMU_MASK_300 ((0xfffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300) >> +#define PERF_REG_PMU_MASK_300 \ >> + ((1ul << PERF_REG_POWERPC_MMCR0) | (1ul << PERF_REG_POWERPC_MMCR1) | \ >> + (1ul << PERF_REG_POWERPC_MMCR2) | (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)) >> >> /* >> * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 >> * includes 12 SPRs from MMCR0 to PMC6. >> */ >> -#define PERF_REG_PMU_MASK_31 (0xfffULL << PERF_REG_POWERPC_MMCR0) >> +#define PERF_REG_PMU_MASK_31 \ >> + (PERF_REG_PMU_MASK_300 | (1ul << PERF_REG_POWERPC_MMCR3) | \ >> + (1ul << PERF_REG_POWERPC_SIER2) | (1ul << PERF_REG_POWERPC_SIER3)) >> >> -#define PERF_REG_EXTENDED_MAX (PERF_REG_POWERPC_PMC6 + 1) > > Likewise for this define? > > I think this might also be an issue for some of your other patches which > change an include/uapi/ file. Though I am removing PERF_REG_EXTENDED_MAX define from end of this uapi file, it is moved along with enum definition of perf_event_powerpc_regs. So we should be good with moving this define from this place. Thanks Athira > > Kind regards, > Daniel > >> -- >> 2.30.1 (Apple Git-130)
Daniel Axtens <dja@axtens.net> writes: > Hi Athira, > >> PERF_REG_PMU_MASK_300 and PERF_REG_PMU_MASK_31 defines the mask >> value for extended registers. Current definition of these mask values >> uses hex constant and does not use registers by name, making it less >> readable. Patch refactor the macro values in perf tools side header file >> by or'ing together the actual register value constants. >> > This looks like a good simplification. > >> -/* Exclude MMCR3, SIER2, SIER3 for CPU_FTR_ARCH_300 */ >> -#define PERF_EXCLUDE_REG_EXT_300 (7ULL << PERF_REG_POWERPC_MMCR3) > > This file is uAPI - are we allowed to remove a define? Could a program > built against these headers now fail to compile because we've removed it? Yeah that's true. In this case I think I'd rather we remove it though, because: - it was never meant to be part of the uapi, it was just meant for use in the construction of PERF_REG_PMU_MASK_300, and is no longer needed for that. - it's only been in the header since v5.12, so I think the chance of anything using it is essentially zero. cheers
diff --git a/tools/arch/powerpc/include/uapi/asm/perf_regs.h b/tools/arch/powerpc/include/uapi/asm/perf_regs.h index 578b3ee86105..fb1d8a9b4393 100644 --- a/tools/arch/powerpc/include/uapi/asm/perf_regs.h +++ b/tools/arch/powerpc/include/uapi/asm/perf_regs.h @@ -61,27 +61,32 @@ enum perf_event_powerpc_regs { PERF_REG_POWERPC_PMC4, PERF_REG_POWERPC_PMC5, PERF_REG_POWERPC_PMC6, - /* Max regs without the extended regs */ + /* Max mask value for interrupt regs w/o extended regs */ PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1, + /* Max mask value for interrupt regs including extended regs */ + PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_PMC6 + 1, }; #define PERF_REG_PMU_MASK ((1ULL << PERF_REG_POWERPC_MAX) - 1) -/* Exclude MMCR3, SIER2, SIER3 for CPU_FTR_ARCH_300 */ -#define PERF_EXCLUDE_REG_EXT_300 (7ULL << PERF_REG_POWERPC_MMCR3) - /* * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300 * includes 9 SPRS from MMCR0 to PMC6 excluding the - * unsupported SPRS in PERF_EXCLUDE_REG_EXT_300. + * unsupported SPRS MMCR3, SIER2 and SIER3. */ -#define PERF_REG_PMU_MASK_300 ((0xfffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300) +#define PERF_REG_PMU_MASK_300 \ + ((1ul << PERF_REG_POWERPC_MMCR0) | (1ul << PERF_REG_POWERPC_MMCR1) | \ + (1ul << PERF_REG_POWERPC_MMCR2) | (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)) /* * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 * includes 12 SPRs from MMCR0 to PMC6. */ -#define PERF_REG_PMU_MASK_31 (0xfffULL << PERF_REG_POWERPC_MMCR0) +#define PERF_REG_PMU_MASK_31 \ + (PERF_REG_PMU_MASK_300 | (1ul << PERF_REG_POWERPC_MMCR3) | \ + (1ul << PERF_REG_POWERPC_SIER2) | (1ul << PERF_REG_POWERPC_SIER3)) -#define PERF_REG_EXTENDED_MAX (PERF_REG_POWERPC_PMC6 + 1) #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
PERF_REG_PMU_MASK_300 and PERF_REG_PMU_MASK_31 defines the mask value for extended registers. Current definition of these mask values uses hex constant and does not use registers by name, making it less readable. Patch refactor the macro values in perf tools side header file by or'ing together the actual register value constants. Suggested-by: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> --- .../arch/powerpc/include/uapi/asm/perf_regs.h | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-)