On 08-Jul-2020, at 5:34 PM, Michael Ellerman <mpe@ellerman.id.au> = wrote:

Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
From: Anju T Sudhakar <anju@linux.vnet.ibm.com>

Add extended regs to sample_reg_mask in the tool side to = use
with `-I?` option. Perf tools side uses extended mask = to display
the platform supported register names (with -I? = option) to the user
and also send this mask to the kernel = to capture the extended registers
in each sample. Hence = decide the mask value based on the processor
version.

Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
[Decide = extended mask at run time based on platform]
Signed-off-by: = Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Reviewed-by: = Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Will need an ack from perf tools = folks, who are not on Cc by the looks.

diff --git = a/tools/arch/powerpc/include/uapi/asm/perf_regs.h = b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
index = f599064..485b1d5 100644
--- = a/tools/arch/powerpc/include/uapi/asm/perf_regs.h
+++ = b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
@@ -48,6 = +48,18 @@ enum perf_event_powerpc_regs {
= PERF_REG_POWERPC_DSISR,
= PERF_REG_POWERPC_SIER,
PERF_REG_POWERPC_MMCRA,
- = PERF_REG_POWERPC_MAX,
+ /* Extended registers */
+ = PERF_REG_POWERPC_MMCR0,
+ = PERF_REG_POWERPC_MMCR1,
+ = PERF_REG_POWERPC_MMCR2,
+ /* Max = regs without the extended regs */
+ = PERF_REG_POWERPC_MAX =3D PERF_REG_POWERPC_MMCRA + 1,

I don't really understand this idea of a max that's not the = max.

Hi = Michael

};
+
+#define PERF_REG_PMU_MASK ((1ULL = << PERF_REG_POWERPC_MAX) - 1)
+
+/* = PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300 */
+#define PERF_REG_PMU_MASK_300   (((1ULL << = (PERF_REG_POWERPC_MMCR2 + 1)) - 1) \
+ - = PERF_REG_PMU_MASK)
+
#endif /* = _UAPI_ASM_POWERPC_PERF_REGS_H */
diff --git = a/tools/perf/arch/powerpc/include/perf_regs.h = b/tools/perf/arch/powerpc/include/perf_regs.h
index = e18a355..46ed00d 100644
--- = a/tools/perf/arch/powerpc/include/perf_regs.h
+++ = b/tools/perf/arch/powerpc/include/perf_regs.h
@@ -64,7 = +64,10 @@
[PERF_REG_POWERPC_DAR] =3D = "dar",
[PERF_REG_POWERPC_DSISR] =3D "dsisr",
= [PERF_REG_POWERPC_SIER] =3D "sier",
- = [PERF_REG_POWERPC_MMCRA] =3D "mmcra"
+ = [PERF_REG_POWERPC_MMCRA] =3D "mmcra",
+ = [PERF_REG_POWERPC_MMCR0] =3D "mmcr0",
+ = [PERF_REG_POWERPC_MMCR1] =3D "mmcr1",
+ = [PERF_REG_POWERPC_MMCR2] =3D "mmcr2",
};

static inline const char *perf_reg_name(int = id)
diff --git a/tools/perf/arch/powerpc/util/perf_regs.c = b/tools/perf/arch/powerpc/util/perf_regs.c
index = 0a52429..9179230 100644
--- = a/tools/perf/arch/powerpc/util/perf_regs.c
+++ = b/tools/perf/arch/powerpc/util/perf_regs.c
@@ -6,9 +6,14 = @@

#include "../../../util/perf_regs.h"
#include "../../../util/debug.h"
+#include = "../../../util/event.h"
+#include = "../../../util/header.h"
+#include = "../../../perf-sys.h"

#include = <linux/kernel.h>

+#define = PVR_POWER9 = = 0x004E
+
const struct sample_reg = sample_reg_masks[] =3D {
SMPL_REG(r0, = PERF_REG_POWERPC_R0),
SMPL_REG(r1, = PERF_REG_POWERPC_R1),
@@ -55,6 +60,9 @@
= SMPL_REG(dsisr, PERF_REG_POWERPC_DSISR),
= SMPL_REG(sier, PERF_REG_POWERPC_SIER),
= SMPL_REG(mmcra, PERF_REG_POWERPC_MMCRA),
+ = SMPL_REG(mmcr0, PERF_REG_POWERPC_MMCR0),
+ = SMPL_REG(mmcr1, PERF_REG_POWERPC_MMCR1),
+ = SMPL_REG(mmcr2, PERF_REG_POWERPC_MMCR2),
= SMPL_REG_END
};

@@ = -163,3 +171,50 @@ int arch_sdt_arg_parse_op(char *old_op, char = **new_op)

return SDT_ARG_VALID;
}
+
+uint64_t = arch__intr_reg_mask(void)
+{
+ struct = perf_event_attr attr =3D {
+ .type =             &n= bsp;     =3D PERF_TYPE_HARDWARE,
+ = = .config =             &n= bsp;   =3D PERF_COUNT_HW_CPU_CYCLES,
+ = .sample_type =            =3D = PERF_SAMPLE_REGS_INTR,
+ .precise_ip =             =3D= 1,
+ .disabled =             &n= bsp; =3D 1,
+ .exclude_kernel =         =3D 1,
+ = };
+ int fd, ret;
+ char = buffer[64];
+ u32 version;
+ u64 = extended_mask =3D 0;
+
+ /* Get = the PVR value to set the extended
+ * mask specific to = platform

Comment format is wrong, and punctuation please.

+ */
+ = get_cpuid(buffer, sizeof(buffer));
+ ret =3D = sscanf(buffer, "%u,", &version);

This is powerpc specific code, = why not just use mfspr(SPRN_PVR), rather
than redirecting via printf/sscanf.

+
+ = if (ret !=3D 1) {
+ pr_debug("Failed to get the = processor version, unable to output extended registers\n");
+ = = return PERF_REGS_MASK;
+ }
+
+ if (version =3D=3D PVR_POWER9)
+ = = extended_mask =3D PERF_REG_PMU_MASK_300;
+ else
+ = = return PERF_REGS_MASK;
+
+ = attr.sample_regs_intr =3D extended_mask;
+ = attr.sample_period =3D 1;
+ = event_attr_init(&attr);
+
+ /*
+ = * check if the = pmu supports perf extended regs, before
+ * returning the register = mask to sample.
+ */
+ fd =3D = sys_perf_event_open(&attr, 0, -1, -1, 0);
+ if (fd !=3D= -1) {
+ close(fd);
+ return (extended_mask | = PERF_REGS_MASK);
+ }
+ return = PERF_REGS_MASK;

I think this would read a bit better like:

mask =3D = PERF_REGS_MASK;

= if (version = =3D=3D PVR_POWER9)
= extended_mask = =3D PERF_REG_PMU_MASK_300;
       else
        return = mask;

       attr.sample_regs_intr= =3D extended_mask;
       attr.sample_period = =3D 1;
       event_attr_init(&= attr);

       /*
         * check = if the pmu supports perf extended regs, before
         * = returning the register mask to sample.
         */=
       fd =3D = sys_perf_event_open(&attr, 0, -1, -1, 0);
       if (fd !=3D -1) = {
          &nb= sp;    close(fd);
          &nb= sp;    mask |=3D extended_mask;
       }

return = mask;

cheers