[RFC PATCH 0/3]perf/core: extend perf_reg and perf_sample_regs_intr

This patchset extend the perf sample regs infrastructure
to include architecture specific regs. Current perf_sample_regs_intr
exports only registers in the pt_regs to perf.data using
PERF_SAMPLE_REGS_INTR sample type. But sometimes we end up looking
for or prefer raw register values at the interrupt during debug.

This patchset extends the perf_regs to include arch specific struct,
and makes perf_sample_regs_intr in kernel/event/core.c as __weak__
function. This way, an arch specific implementation of
perf_sample_regs_intr() can update the arch specific data to
the perf_regs.

First patch defines a new structure arch_misc_reg and updates the same
in the struct perf_regs. Patch also modifies the perf_reg_value()
and perf_output_sample_regs() to take perf_regs parameter instead of pt_regs.

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Third patch adds arch specific perf_sample_regs_intr() in arch/powerpc
to hook up the arch_misc_regs to perf_regs.

This patchset depends on the recent posting by Anju T in
linuxppc-dev@lists.ozlabs.org to enable PERF_SAMPLE_REGS_INTR
support in arch/powerpc.

https://patchwork.ozlabs.org/patch/539242/
https://patchwork.ozlabs.org/patch/539243/
https://patchwork.ozlabs.org/patch/539244/

Would appreciate comments and feedback.

Signed-off-by: Madhavan Srinivasan <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Stephane Eranian <redacted>
Cc: Russell King <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Sukadev Bhattiprolu <redacted>

Madhavan Srinivasan (3):
  perf/core: extend perf_regs to include arch specific regs
  perf/powerpc: update macros and add regs to arch_misc_reg struct
  perf/powerpc: Functions to update arch_misc_regs

 arch/arm/include/asm/ptrace.h               |  2 ++
 arch/arm/kernel/perf_regs.c                 |  4 +++-
 arch/arm64/include/asm/ptrace.h             |  2 ++
 arch/arm64/kernel/perf_regs.c               |  4 +++-
 arch/powerpc/include/uapi/asm/perf_regs.h   | 10 ++++++++++
 arch/powerpc/include/uapi/asm/ptrace.h      | 11 +++++++++++
 arch/powerpc/perf/core-book3s.c             | 29 +++++++++++++++++++++++++++++
 arch/powerpc/perf/perf_regs.c               | 28 ++++++++++++++++++++++++++--
 arch/x86/include/asm/ptrace.h               |  2 ++
 arch/x86/kernel/perf_regs.c                 |  4 +++-
 include/linux/perf_regs.h                   |  5 +++--
 kernel/events/core.c                        |  8 ++++----
 tools/perf/arch/powerpc/include/perf_regs.h | 16 ++++++++++++++++
 13 files changed, 114 insertions(+), 11 deletions(-)

--
1.9.1

As a foundation patch, new structure called arch_misc_regs
added to perf_regs. And perf_reg_value() is modified to expect
perf_regs instead of pt_regs. This way, perf_reg_value() can decide
on the struct to pick based on the register idx.

Signed-off-by: Madhavan Srinivasan <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Stephane Eranian <redacted>
Cc: Russell King <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Sukadev Bhattiprolu <redacted>
---
 Will really appreciate comments and feedback for the patch

 arch/arm/include/asm/ptrace.h          | 2 ++
 arch/arm/kernel/perf_regs.c            | 4 +++-
 arch/arm64/include/asm/ptrace.h        | 2 ++
 arch/arm64/kernel/perf_regs.c          | 4 +++-
 arch/powerpc/include/uapi/asm/ptrace.h | 2 ++
 arch/powerpc/perf/perf_regs.c          | 4 +++-
 arch/x86/include/asm/ptrace.h          | 2 ++
 arch/x86/kernel/perf_regs.c            | 4 +++-
 include/linux/perf_regs.h              | 5 +++--
 kernel/events/core.c                   | 6 +++---
 10 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index 51622ba7c4a6..26cd8dbf1f02 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h

@@ -17,6 +17,8 @@ struct pt_regs {
 	unsigned long uregs[18];
 };
 
+struct arch_misc_regs {};
+
 #define user_mode(regs)	\
 	(((regs)->ARM_cpsr & 0xf) == 0)
 

diff --git a/arch/arm/kernel/perf_regs.c b/arch/arm/kernel/perf_regs.c
index 592dda3f21ff..18999abbadbe 100644
--- a/arch/arm/kernel/perf_regs.c
+++ b/arch/arm/kernel/perf_regs.c

@@ -6,8 +6,10 @@
 #include <asm/perf_regs.h>
 #include <asm/ptrace.h>
 
-u64 perf_reg_value(struct pt_regs *regs, int idx)
+u64 perf_reg_value(struct perf_regs *p_regs, int idx)
 {
+	struct pt_regs *regs = p_regs->regs;
+
 	if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM_MAX))
 		return 0;
 

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 536274ed292e..9a33ab69f19f 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h

@@ -118,6 +118,8 @@ struct pt_regs {
 	u64 syscallno;
 };
 
+struct arch_misc_regs {};
+
 #define arch_has_single_step()	(1)
 
 #ifdef CONFIG_COMPAT

diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
index 3f62b35fb6f1..8ee80d7c8604 100644
--- a/arch/arm64/kernel/perf_regs.c
+++ b/arch/arm64/kernel/perf_regs.c

@@ -7,8 +7,10 @@
 #include <asm/perf_regs.h>
 #include <asm/ptrace.h>
 
-u64 perf_reg_value(struct pt_regs *regs, int idx)
+u64 perf_reg_value(struct perf_regs *p_regs, int idx)
 {
+	struct pt_regs *regs = p_regs->regs;
+
 	if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX))
 		return 0;
 

diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index 8036b385417d..0025651ab8bc 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h

@@ -51,6 +51,8 @@ struct pt_regs {
 	unsigned long result;		/* Result of a system call */
 };
 
+struct arch_misc_regs {};
+
 #endif /* __ASSEMBLY__ */
 
 

diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
index 0520492d4078..f7995b46078a 100644
--- a/arch/powerpc/perf/perf_regs.c
+++ b/arch/powerpc/perf/perf_regs.c

@@ -61,8 +61,10 @@ static unsigned int pt_regs_offset[PERF_REG_POWERPC_MAX] = {
 	PT_REGS_OFFSET(PERF_REG_POWERPC_DSISR, dsisr),
 };
 
-u64 perf_reg_value(struct pt_regs *regs, int idx)
+u64 perf_reg_value(struct perf_regs *p_regs, int idx)
 {
+	struct pt_regs *regs = p_regs->regs;
+
 	if (WARN_ON_ONCE(idx >= PERF_REG_POWERPC_MAX))
 		return 0;
 	return regs_get_register(regs, pt_regs_offset[idx]);

diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index 6271281f947d..410a2eb885c1 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h

@@ -67,6 +67,8 @@ struct pt_regs {
 
 #endif /* !__i386__ */
 
+struct arch_misc_regs {};
+
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt_types.h>
 #endif

diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
index da8cb987b973..c6e41f7f648b 100644
--- a/arch/x86/kernel/perf_regs.c
+++ b/arch/x86/kernel/perf_regs.c

@@ -55,8 +55,10 @@ static unsigned int pt_regs_offset[PERF_REG_X86_MAX] = {
 #endif
 };
 
-u64 perf_reg_value(struct pt_regs *regs, int idx)
+u64 perf_reg_value(struct perf_regs *p_regs, int idx)
 {
+	struct pt_regs *regs = p_regs->regs;
+
 	if (WARN_ON_ONCE(idx >= ARRAY_SIZE(pt_regs_offset)))
 		return 0;
 

diff --git a/include/linux/perf_regs.h b/include/linux/perf_regs.h
index a5f98d53d732..7727a30e7d84 100644
--- a/include/linux/perf_regs.h
+++ b/include/linux/perf_regs.h

@@ -4,18 +4,19 @@
 struct perf_regs {
 	__u64		abi;
 	struct pt_regs	*regs;
+	struct arch_misc_regs *arch_regs;
 };
 
 #ifdef CONFIG_HAVE_PERF_REGS
 #include <asm/perf_regs.h>
-u64 perf_reg_value(struct pt_regs *regs, int idx);
+u64 perf_reg_value(struct perf_regs *p_regs, int idx);
 int perf_reg_validate(u64 mask);
 u64 perf_reg_abi(struct task_struct *task);
 void perf_get_regs_user(struct perf_regs *regs_user,
 			struct pt_regs *regs,
 			struct pt_regs *regs_user_copy);
 #else
-static inline u64 perf_reg_value(struct pt_regs *regs, int idx)
+static inline u64 perf_reg_value(struct perf_regs *p_regs, int idx)
 {
 	return 0;
 }

diff --git a/kernel/events/core.c b/kernel/events/core.c
index b11756f9b6dc..c04bdad3d365 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c

@@ -4934,7 +4934,7 @@ EXPORT_SYMBOL_GPL(perf_unregister_guest_info_callbacks);
 
 static void
 perf_output_sample_regs(struct perf_output_handle *handle,
-			struct pt_regs *regs, u64 mask)
+			struct perf_regs *regs, u64 mask)
 {
 	int bit;
 

@@ -5331,7 +5331,7 @@ void perf_output_sample(struct perf_output_handle *handle,
 		if (abi) {
 			u64 mask = event->attr.sample_regs_user;
 			perf_output_sample_regs(handle,
-						data->regs_user.regs,
+						&data->regs_user,
 						mask);
 		}
 	}

@@ -5363,7 +5363,7 @@ void perf_output_sample(struct perf_output_handle *handle,
 			u64 mask = event->attr.sample_regs_intr;
 
 			perf_output_sample_regs(handle,
-						data->regs_intr.regs,
+						&data->regs_intr,
 						mask);
 		}
 	}

-- 
1.9.1

Patch adds function to update the arch_misc_reg struct
and an arch specific perf_sample_regs_intr() to hook up
arch_misc_regs to perf_regs pointer. Also makes
perf_sample_regs_intr() in kernel/event/core.c as __weak__
function to make arch specific implementation to replace.

Signed-off-by: Madhavan Srinivasan <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Stephane Eranian <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Sukadev Bhattiprolu <redacted>
---
 Would really appreciate comments and feedback for the patch

 arch/powerpc/perf/core-book3s.c | 29 +++++++++++++++++++++++++++++
 kernel/events/core.c            |  2 +-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index b0382f3f1095..508c181c163f 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c

@@ -57,6 +57,7 @@ struct cpu_hw_events {
 	void				*bhrb_context;
 	struct	perf_branch_stack	bhrb_stack;
 	struct	perf_branch_entry	bhrb_entries[BHRB_MAX_ENTRIES];
+	struct arch_misc_regs arch_regs;
 };

 static DEFINE_PER_CPU(struct cpu_hw_events, cpu_hw_events);

@@ -1904,6 +1905,28 @@ ssize_t power_events_sysfs_show(struct device *dev,
 	return sprintf(page, "event=0x%02llx\n", pmu_attr->id);
 }

+void perf_sample_regs_intr(struct perf_regs *regs_intr, struct pt_regs *regs)
+{
+	struct cpu_hw_events *cpuhw;
+	cpuhw = this_cpu_ptr(&cpu_hw_events);
+
+	regs_intr->regs = regs;
+	regs_intr->arch_regs = &cpuhw->arch_regs;
+	regs_intr->abi  = perf_reg_abi(current);
+}
+
+static void power_arch_misc_regs(struct arch_misc_regs *regs)
+{
+	regs->pmc1 = mfspr(SPRN_PMC1);
+	regs->pmc2 = mfspr(SPRN_PMC2);
+	regs->pmc3 = mfspr(SPRN_PMC3);
+	regs->pmc4 = mfspr(SPRN_PMC4);
+	regs->pmc5 = mfspr(SPRN_PMC5);
+	regs->pmc6 = mfspr(SPRN_PMC6);
+	regs->mmcr0 = mfspr(SPRN_MMCR0);
+	regs->mmcr1 = mfspr(SPRN_MMCR1);
+}
+
 static struct pmu power_pmu = {
 	.pmu_enable	= power_pmu_enable,
 	.pmu_disable	= power_pmu_disable,

@@ -1985,6 +2008,12 @@ static void record_and_restart(struct perf_event *event, unsigned long val,
 			data.br_stack = &cpuhw->bhrb_stack;
 		}

+		if (event->attr.sample_type & PERF_SAMPLE_REGS_INTR) {
+			struct cpu_hw_events *cpuhw;
+			cpuhw = this_cpu_ptr(&cpu_hw_events);
+			power_arch_misc_regs(&cpuhw->arch_regs);
+		}
+
 		if (perf_event_overflow(event, &data, regs))
 			power_pmu_stop(event, 0);
 	}

diff --git a/kernel/events/core.c b/kernel/events/core.c
index c04bdad3d365..9567be72da5a 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c

@@ -4962,7 +4962,7 @@ static void perf_sample_regs_user(struct perf_regs *regs_user,
 	}
 }

-static void perf_sample_regs_intr(struct perf_regs *regs_intr,
+void __attribute__((weak)) perf_sample_regs_intr(struct perf_regs *regs_intr,
 				  struct pt_regs *regs)
 {
 	regs_intr->regs = regs;
--

1.9.1

Patch updates the arch_misc_regs struct (arch/powerpc/ptrace.h)
with arch/powerpc specific regs (PMU regs). Updates asm/perf_regs.h and
perf_regs.c with corresponding macros to export the arch_misc_reg values.

Signed-off-by: Madhavan Srinivasan <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@kernel.org>
CC: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Sukadev Bhattiprolu <redacted>
---
 Will really appreciate comments and feedback for the patch.

 arch/powerpc/include/uapi/asm/perf_regs.h   | 10 ++++++++++
 arch/powerpc/include/uapi/asm/ptrace.h      | 11 ++++++++++-
 arch/powerpc/perf/perf_regs.c               | 28 +++++++++++++++++++++++++---
 tools/perf/arch/powerpc/include/perf_regs.h | 16 ++++++++++++++++
 4 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h b/arch/powerpc/include/uapi/asm/perf_regs.h
index 30fb601bdbb2..2d170734f47d 100644
--- a/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/arch/powerpc/include/uapi/asm/perf_regs.h

@@ -49,6 +49,16 @@ enum perf_event_powerpc_regs {
 	PERF_REG_POWERPC_TRAP,
 	PERF_REG_POWERPC_DAR,
 	PERF_REG_POWERPC_DSISR,
+	PERF_REG_POWERPC_PMC1,
+	PERF_REG_POWERPC_PMC2,
+	PERF_REG_POWERPC_PMC3,
+	PERF_REG_POWERPC_PMC4,
+	PERF_REG_POWERPC_PMC5,
+	PERF_REG_POWERPC_PMC6,
+	PERF_REG_POWERPC_MMCR0,
+	PERF_REG_POWERPC_MMCR1,
+
 	PERF_REG_POWERPC_MAX,
+	PERF_REG_PT_REGS_END = PERF_REG_POWERPC_DSISR + 1,
 };
 #endif /* _ASM_POWERPC_PERF_REGS_H */

diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index 0025651ab8bc..5e9b3259ef78 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h

@@ -51,7 +51,16 @@ struct pt_regs {
 	unsigned long result;		/* Result of a system call */
 };

-struct arch_misc_regs {};
+struct arch_misc_regs {
+	unsigned long pmc1;
+	unsigned long pmc2;
+	unsigned long pmc3;
+	unsigned long pmc4;
+	unsigned long pmc5;
+	unsigned long pmc6;
+	unsigned long mmcr0;
+	unsigned long mmcr1;
+};

 #endif /* __ASSEMBLY__ */

diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
index f7995b46078a..48e479c91fb2 100644
--- a/arch/powerpc/perf/perf_regs.c
+++ b/arch/powerpc/perf/perf_regs.c

@@ -8,8 +8,11 @@
 #include <asm/perf_regs.h>

 #define PT_REGS_OFFSET(id, r) [id] = offsetof(struct pt_regs, r)
+#define ARCH_REGS_OFFSET(id, r) [id] = offsetof(struct arch_misc_regs, r)

 #define REG_RESERVED (~((1ULL << PERF_REG_POWERPC_MAX) - 1))
+#define MAX_ARCH_REG_OFFSET	(offsetof(struct arch_misc_regs, mmcr1))
+

 static unsigned int pt_regs_offset[PERF_REG_POWERPC_MAX] = {
 	PT_REGS_OFFSET(PERF_REG_POWERPC_GPR0, gpr[0]),

@@ -59,15 +62,34 @@ static unsigned int pt_regs_offset[PERF_REG_POWERPC_MAX] = {
 	PT_REGS_OFFSET(PERF_REG_POWERPC_TRAP, trap),
 	PT_REGS_OFFSET(PERF_REG_POWERPC_DAR, dar),
 	PT_REGS_OFFSET(PERF_REG_POWERPC_DSISR, dsisr),
+	ARCH_REGS_OFFSET(PERF_REG_POWERPC_PMC1, pmc1),
+	ARCH_REGS_OFFSET(PERF_REG_POWERPC_PMC2, pmc2),
+	ARCH_REGS_OFFSET(PERF_REG_POWERPC_PMC3, pmc3),
+	ARCH_REGS_OFFSET(PERF_REG_POWERPC_PMC4, pmc4),
+	ARCH_REGS_OFFSET(PERF_REG_POWERPC_PMC5, pmc5),
+	ARCH_REGS_OFFSET(PERF_REG_POWERPC_PMC6, pmc6),
+	ARCH_REGS_OFFSET(PERF_REG_POWERPC_MMCR0, mmcr0),
+	ARCH_REGS_OFFSET(PERF_REG_POWERPC_MMCR1, mmcr1),
 };

-u64 perf_reg_value(struct perf_regs *p_regs, int idx)
+static inline unsigned long
+arch_regs_get_register(struct arch_misc_regs *regs, unsigned int offset)
 {
-	struct pt_regs *regs = p_regs->regs;
+	if (unlikely(offset > MAX_ARCH_REG_OFFSET))
+		return 0;
+	return *(unsigned long *)((unsigned long)regs + offset);
+}

+u64 perf_reg_value(struct perf_regs *p_regs, int idx)
+{
 	if (WARN_ON_ONCE(idx >= PERF_REG_POWERPC_MAX))
 		return 0;
-	return regs_get_register(regs, pt_regs_offset[idx]);
+
+	if (idx >= PERF_REG_PT_REGS_END)
+		return arch_regs_get_register(p_regs->arch_regs,
+							pt_regs_offset[idx]);
+	else
+		return regs_get_register(p_regs->regs, pt_regs_offset[idx]);
 }

 int perf_reg_validate(u64 mask)

diff --git a/tools/perf/arch/powerpc/include/perf_regs.h b/tools/perf/arch/powerpc/include/perf_regs.h
index 47307ca19c28..e0d23b274644 100644
--- a/tools/perf/arch/powerpc/include/perf_regs.h
+++ b/tools/perf/arch/powerpc/include/perf_regs.h

@@ -106,6 +106,22 @@ static inline const char *perf_reg_name(int id)
 		return "dar";
 	case PERF_REG_POWERPC_DSISR:
 		return "dsisr";
+	case PERF_REG_POWERPC_PMC1:
+		return "pmc1";
+	case PERF_REG_POWERPC_PMC2:
+		return "pmc2";
+	case PERF_REG_POWERPC_PMC3:
+		return "pmc3";
+	case PERF_REG_POWERPC_PMC4:
+		return "pmc4";
+	case PERF_REG_POWERPC_PMC5:
+		return "pmc5";
+	case PERF_REG_POWERPC_PMC6:
+		return "pmc6";
+	case PERF_REG_POWERPC_MMCR0:
+		return "mmcr0";
+	case PERF_REG_POWERPC_MMCR1:
+		return "mmcr1";
 	default:
 		return NULL;
 	}
--

1.9.1

On Thu, Nov 05, 2015 at 02:16:15AM +0530, Madhavan Srinivasan wrote:

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Why; what's in those regs?

Hi,

On Wed, Nov 4, 2015 at 9:46 PM, Madhavan Srinivasan
[off-list ref] wrote:

This patchset extend the perf sample regs infrastructure
to include architecture specific regs. Current perf_sample_regs_intr
exports only registers in the pt_regs to perf.data using
PERF_SAMPLE_REGS_INTR sample type. But sometimes we end up looking
for or prefer raw register values at the interrupt during debug.

I don't quite understand this explanation here.
What do you mean by raw register values?
The content of pt_regs is also raw register values at interrupt.

The API does not say that the content of perf_sample_regs_intr can ONLY
contain names of registers coming from pt_regs. The meaning of each bit
is arch specific and you are free to map to whatever is relevant for your arch.
All the API says is that the values captured in the sampling buffer for these
registers are taken at PMU interrupt.

This patchset extends the perf_regs to include arch specific struct,
and makes perf_sample_regs_intr in kernel/event/core.c as __weak__
function. This way, an arch specific implementation of
perf_sample_regs_intr() can update the arch specific data to
the perf_regs.

First patch defines a new structure arch_misc_reg and updates the same
in the struct perf_regs. Patch also modifies the perf_reg_value()
and perf_output_sample_regs() to take perf_regs parameter instead of pt_regs.

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Third patch adds arch specific perf_sample_regs_intr() in arch/powerpc
to hook up the arch_misc_regs to perf_regs.

This patchset depends on the recent posting by Anju T in
linuxppc-dev@lists.ozlabs.org to enable PERF_SAMPLE_REGS_INTR
support in arch/powerpc.

https://patchwork.ozlabs.org/patch/539242/
https://patchwork.ozlabs.org/patch/539243/
https://patchwork.ozlabs.org/patch/539244/

Would appreciate comments and feedback.

Signed-off-by: Madhavan Srinivasan <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Stephane Eranian <redacted>
Cc: Russell King <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Sukadev Bhattiprolu <redacted>

Madhavan Srinivasan (3):
  perf/core: extend perf_regs to include arch specific regs
  perf/powerpc: update macros and add regs to arch_misc_reg struct
  perf/powerpc: Functions to update arch_misc_regs

 arch/arm/include/asm/ptrace.h               |  2 ++
 arch/arm/kernel/perf_regs.c                 |  4 +++-
 arch/arm64/include/asm/ptrace.h             |  2 ++
 arch/arm64/kernel/perf_regs.c               |  4 +++-
 arch/powerpc/include/uapi/asm/perf_regs.h   | 10 ++++++++++
 arch/powerpc/include/uapi/asm/ptrace.h      | 11 +++++++++++
 arch/powerpc/perf/core-book3s.c             | 29 +++++++++++++++++++++++++++++
 arch/powerpc/perf/perf_regs.c               | 28 ++++++++++++++++++++++++++--
 arch/x86/include/asm/ptrace.h               |  2 ++
 arch/x86/kernel/perf_regs.c                 |  4 +++-
 include/linux/perf_regs.h                   |  5 +++--
 kernel/events/core.c                        |  8 ++++----
 tools/perf/arch/powerpc/include/perf_regs.h | 16 ++++++++++++++++
 13 files changed, 114 insertions(+), 11 deletions(-)

--
1.9.1

Peter Zijlstra [peterz@infradead.org] wrote:
| On Thu, Nov 05, 2015 at 02:16:15AM +0530, Madhavan Srinivasan wrote:
| > Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
| > and adds offsetof macro for the same. It extends perf_reg_value()
| > to use reg idx to decide on struct to return value from.
| 
| Why; what's in those regs?

Those are PMU control registers/counters (in Patch 2) that are of
interest only in the context of a PMU interrupt and not relevant
to ptrace itself.

Could we add those registers to 'struct pt_regs' anyway?

We do have 'struct perf_regs' but that seems to be arch nuetral.
If architectures could override that, maybe we could add these
new registers there without touching 'struct pt_regs'.

Even so, lot of perf code depends on 'struct pt_regs'.

Sukadev

On Thursday 05 November 2015 06:37 PM, Peter Zijlstra wrote:

On Thu, Nov 05, 2015 at 02:16:15AM +0530, Madhavan Srinivasan wrote:

quoted

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Why; what's in those regs?

Was out and did not have access to mail, so missed to respond in time.

In current implementation of patch 2, have added 
few pmu control/status and counter registers,
which give additional information about the PMU context
for the sample.

These pmu registers are not relevant for ptrace and did not
want to overload pt_regs struct.

Adding these arch specific regs in perf_regs will break the "arch
neutral" part, and
other arch can also use this new struct in perf_regs to add more data
using perf_sample_regs_intr without extending the pt_regs.

Maddy

On Thursday 05 November 2015 08:12 PM, Stephane Eranian wrote:

Hi,

On Wed, Nov 4, 2015 at 9:46 PM, Madhavan Srinivasan
[off-list ref] wrote:

quoted

This patchset extend the perf sample regs infrastructure
to include architecture specific regs. Current perf_sample_regs_intr
exports only registers in the pt_regs to perf.data using
PERF_SAMPLE_REGS_INTR sample type. But sometimes we end up looking
for or prefer raw register values at the interrupt during debug.

I don't quite understand this explanation here.
What do you mean by raw register values?
The content of pt_regs is also raw register values at interrupt.

Was out and did not have access to mail, so missed to respond in time.

Yes. that is right. My bad. Did not effectively communicate
what I intended. Intend here is to capture more data related to
pmu context for the sample.

And in case of arch/powerpc, only a subset of pmu regs are
in the pt_regs structure. By adding these  additional pmu registers
(which are not part of pt_regs and i dont intend to overload
pt_regs struct with these regs, since it is not relevant to ptrace),
more interesting data about the PMU and its context is captured
for the sample.

The API does not say that the content of perf_sample_regs_intr can ONLY
contain names of registers coming from pt_regs. The meaning of each bit
is arch specific and you are free to map to whatever is relevant for your arch.
All the API says is that the values captured in the sampling buffer for these
registers are taken at PMU interrupt.

Yes thats right. But what I propose here to include an
arch specific struct in the perf_regs, so that more register values
can be fetched as part of perf_sample_regs_intr without
extending pt_regs.

Maddy

quoted

This patchset extends the perf_regs to include arch specific struct,
and makes perf_sample_regs_intr in kernel/event/core.c as __weak__
function. This way, an arch specific implementation of
perf_sample_regs_intr() can update the arch specific data to
the perf_regs.

First patch defines a new structure arch_misc_reg and updates the same
in the struct perf_regs. Patch also modifies the perf_reg_value()
and perf_output_sample_regs() to take perf_regs parameter instead of pt_regs.

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Third patch adds arch specific perf_sample_regs_intr() in arch/powerpc
to hook up the arch_misc_regs to perf_regs.

This patchset depends on the recent posting by Anju T in
linuxppc-dev@lists.ozlabs.org to enable PERF_SAMPLE_REGS_INTR
support in arch/powerpc.

https://patchwork.ozlabs.org/patch/539242/
https://patchwork.ozlabs.org/patch/539243/
https://patchwork.ozlabs.org/patch/539244/

Would appreciate comments and feedback.

Signed-off-by: Madhavan Srinivasan <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Stephane Eranian <redacted>
Cc: Russell King <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Sukadev Bhattiprolu <redacted>

Madhavan Srinivasan (3):
  perf/core: extend perf_regs to include arch specific regs
  perf/powerpc: update macros and add regs to arch_misc_reg struct
  perf/powerpc: Functions to update arch_misc_regs

 arch/arm/include/asm/ptrace.h               |  2 ++
 arch/arm/kernel/perf_regs.c                 |  4 +++-
 arch/arm64/include/asm/ptrace.h             |  2 ++
 arch/arm64/kernel/perf_regs.c               |  4 +++-
 arch/powerpc/include/uapi/asm/perf_regs.h   | 10 ++++++++++
 arch/powerpc/include/uapi/asm/ptrace.h      | 11 +++++++++++
 arch/powerpc/perf/core-book3s.c             | 29 +++++++++++++++++++++++++++++
 arch/powerpc/perf/perf_regs.c               | 28 ++++++++++++++++++++++++++--
 arch/x86/include/asm/ptrace.h               |  2 ++
 arch/x86/kernel/perf_regs.c                 |  4 +++-
 include/linux/perf_regs.h                   |  5 +++--
 kernel/events/core.c                        |  8 ++++----
 tools/perf/arch/powerpc/include/perf_regs.h | 16 ++++++++++++++++
 13 files changed, 114 insertions(+), 11 deletions(-)

--
1.9.1

On Friday 06 November 2015 08:28 AM, Sukadev Bhattiprolu wrote:

Peter Zijlstra [peterz@infradead.org] wrote:
| On Thu, Nov 05, 2015 at 02:16:15AM +0530, Madhavan Srinivasan wrote:
| > Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
| > and adds offsetof macro for the same. It extends perf_reg_value()
| > to use reg idx to decide on struct to return value from.
| 
| Why; what's in those regs?

Those are PMU control registers/counters (in Patch 2) that are of
interest only in the context of a PMU interrupt and not relevant
to ptrace itself.

Yes. Thats right.

Could we add those registers to 'struct pt_regs' anyway?

I would prefer not to. Since as you mentioned, these are
not relevant to ptrace. Currently patch 2, adds only few
pmu registers, but would like to include more.
 

We do have 'struct perf_regs' but that seems to be arch nuetral.
If architectures could override that, maybe we could add these
new registers there without touching 'struct pt_regs'.

Exactly, idea here is to capture more data using perf_sample_reg_intr
without extending pt_regs structure.

Maddy

Even so, lot of perf code depends on 'struct pt_regs'.

Sukadev

On Fri, Nov 06, 2015 at 12:57:17PM +0530, Madhavan Srinivasan wrote:

On Thursday 05 November 2015 06:37 PM, Peter Zijlstra wrote:

quoted

On Thu, Nov 05, 2015 at 02:16:15AM +0530, Madhavan Srinivasan wrote:

quoted

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Why; what's in those regs?

Was out and did not have access to mail, so missed to respond in time.

In current implementation of patch 2, have added 
few pmu control/status and counter registers,
which give additional information about the PMU context
for the sample.

Yes, I saw that, you still haven't answered the question though. What is
in those regs? Why is exposing that information like this the best
option.

On Fri, 2015-11-06 at 13:17 +0530, Madhavan Srinivasan wrote:

On Friday 06 November 2015 08:28 AM, Sukadev Bhattiprolu wrote:

quoted

Peter Zijlstra [peterz@infradead.org] wrote:

quoted

On Thu, Nov 05, 2015 at 02:16:15AM +0530, Madhavan Srinivasan wrote:

quoted

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Why; what's in those regs?

Those are PMU control registers/counters (in Patch 2) that are of
interest only in the context of a PMU interrupt and not relevant
to ptrace itself.

Yes. Thats right.

quoted

Could we add those registers to 'struct pt_regs' anyway?

I would prefer not to. Since as you mentioned, these are
not relevant to ptrace. Currently patch 2, adds only few
pmu registers, but would like to include more.

You can't just add them to pt_regs, it's part of the userspace ABI.

We could define a kernel internal version of pt_regs, but I don't think we want
to for this.

If we did that would bloat pt_regs for all users in the kernel, when we really
only want these regs for perf.

cheers

On Fri, 2015-11-06 at 10:24 +0100, Peter Zijlstra wrote:

On Fri, Nov 06, 2015 at 12:57:17PM +0530, Madhavan Srinivasan wrote:

quoted

On Thursday 05 November 2015 06:37 PM, Peter Zijlstra wrote:

quoted

On Thu, Nov 05, 2015 at 02:16:15AM +0530, Madhavan Srinivasan wrote:

quoted

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Why; what's in those regs?

Was out and did not have access to mail, so missed to respond in time.

In current implementation of patch 2, have added 
few pmu control/status and counter registers,
which give additional information about the PMU context
for the sample.

Yes, I saw that, you still haven't answered the question though. What is
in those regs? Why is exposing that information like this the best
option.

It's a perrenial request from our hardware PMU folks to be able to see the raw
values of the PMU registers.

I think partly it's so that they can verify that perf is doing what they want,
and some of it is that they're interested in some of the more obscure info that
isn't plumbed out through other perf interfaces.

We've used various internal hacks over the years to keep them happy. This is an
attempt to use a somewhat standard mechanism.

It would also be helpful for those of us working on the perf hardware backends,
to be able to verify that we're programming things correctly, without resorting
to debug printks etc.

Basically we want to sample regs at the time of the perf interrupt, so we
though PERF_SAMPLE_REGS_INTR made senes :)

But if you think this is the wrong mechanism within perf, then please let us
know.

I know perf's mission is to abstract as much of the arcane hardware details
into a generic interface and make PMUs actually useful for normal folks, and we
are committed to that, but it would also be useful to be able to get the raw
values for a different type of user.

Maddy's patch only exports PMC1-6 and MMCR0/1. I think we also need to export
some others, in particular MMCRA has a lot of stuff in it, half of which is not
even architected. So that would have to be exported as "POWER8_MMCRA". And then
there's the SIAR/SDAR/SIER which contain a bunch of info on sampled
instructions that is not currently plumbed out.

cheers

On Fri, Nov 06, 2015 at 09:04:00PM +1100, Michael Ellerman wrote:

It's a perrenial request from our hardware PMU folks to be able to see the raw
values of the PMU registers.

I think partly it's so that they can verify that perf is doing what they want,
and some of it is that they're interested in some of the more obscure info that
isn't plumbed out through other perf interfaces.

We've used various internal hacks over the years to keep them happy. This is an
attempt to use a somewhat standard mechanism.

It would also be helpful for those of us working on the perf hardware backends,
to be able to verify that we're programming things correctly, without resorting
to debug printks etc.

Basically we want to sample regs at the time of the perf interrupt, so we
though PERF_SAMPLE_REGS_INTR made senes :)

But if you think this is the wrong mechanism within perf, then please let us
know.

I know perf's mission is to abstract as much of the arcane hardware details
into a generic interface and make PMUs actually useful for normal folks, and we
are committed to that, but it would also be useful to be able to get the raw
values for a different type of user.

Maddy's patch only exports PMC1-6 and MMCR0/1. I think we also need to export
some others, in particular MMCRA has a lot of stuff in it, half of which is not
even architected. So that would have to be exported as "POWER8_MMCRA". And then
there's the SIAR/SDAR/SIER which contain a bunch of info on sampled
instructions that is not currently plumbed out.

OK, no objections then. But this is useful information and should be
included in the patch set.

On Friday 06 November 2015 03:34 PM, Michael Ellerman wrote:

On Fri, 2015-11-06 at 10:24 +0100, Peter Zijlstra wrote:

quoted

On Fri, Nov 06, 2015 at 12:57:17PM +0530, Madhavan Srinivasan wrote:

quoted

On Thursday 05 November 2015 06:37 PM, Peter Zijlstra wrote:

quoted

On Thu, Nov 05, 2015 at 02:16:15AM +0530, Madhavan Srinivasan wrote:

quoted

Second patch updates struct arch_misc_reg for arch/powerpc with pmu registers
and adds offsetof macro for the same. It extends perf_reg_value()
to use reg idx to decide on struct to return value from.

Why; what's in those regs?

Was out and did not have access to mail, so missed to respond in time.

In current implementation of patch 2, have added 
few pmu control/status and counter registers,
which give additional information about the PMU context
for the sample.

Yes, I saw that, you still haven't answered the question though. What is
in those regs? Why is exposing that information like this the best
option.

It's a perrenial request from our hardware PMU folks to be able to see the raw
values of the PMU registers.

I think partly it's so that they can verify that perf is doing what they want,
and some of it is that they're interested in some of the more obscure info that
isn't plumbed out through other perf interfaces.

We've used various internal hacks over the years to keep them happy. This is an
attempt to use a somewhat standard mechanism.

It would also be helpful for those of us working on the perf hardware backends,
to be able to verify that we're programming things correctly, without resorting
to debug printks etc.

Basically we want to sample regs at the time of the perf interrupt, so we
though PERF_SAMPLE_REGS_INTR made senes :)

But if you think this is the wrong mechanism within perf, then please let us
know.

I know perf's mission is to abstract as much of the arcane hardware details
into a generic interface and make PMUs actually useful for normal folks, and we
are committed to that, but it would also be useful to be able to get the raw
values for a different type of user.

Maddy's patch only exports PMC1-6 and MMCR0/1. I think we also need to export
some others, in particular MMCRA has a lot of stuff in it, half of which is not
even architected. So that would have to be exported as "POWER8_MMCRA". And then
there's the SIAR/SDAR/SIER which contain a bunch of info on sampled
instructions that is not currently plumbed out.

Sure. I will rework the patch to include the other regs also.

Maddy

cheers

On Friday 06 November 2015 03:55 PM, Peter Zijlstra wrote:

On Fri, Nov 06, 2015 at 09:04:00PM +1100, Michael Ellerman wrote:

quoted

It's a perrenial request from our hardware PMU folks to be able to see the raw
values of the PMU registers.

I think partly it's so that they can verify that perf is doing what they want,
and some of it is that they're interested in some of the more obscure info that
isn't plumbed out through other perf interfaces.

We've used various internal hacks over the years to keep them happy. This is an
attempt to use a somewhat standard mechanism.

It would also be helpful for those of us working on the perf hardware backends,
to be able to verify that we're programming things correctly, without resorting
to debug printks etc.

Basically we want to sample regs at the time of the perf interrupt, so we
though PERF_SAMPLE_REGS_INTR made senes :)

But if you think this is the wrong mechanism within perf, then please let us
know.

I know perf's mission is to abstract as much of the arcane hardware details
into a generic interface and make PMUs actually useful for normal folks, and we
are committed to that, but it would also be useful to be able to get the raw
values for a different type of user.

Maddy's patch only exports PMC1-6 and MMCR0/1. I think we also need to export
some others, in particular MMCRA has a lot of stuff in it, half of which is not
even architected. So that would have to be exported as "POWER8_MMCRA". And then
there's the SIAR/SDAR/SIER which contain a bunch of info on sampled
instructions that is not currently plumbed out.

OK, no objections then. But this is useful information and should be
included in the patch set.

Sure. Will add the information in the next version.

Maddy

On Fri, 2015-11-06 at 11:25 +0100, Peter Zijlstra wrote:

On Fri, Nov 06, 2015 at 09:04:00PM +1100, Michael Ellerman wrote:

quoted

It's a perrenial request from our hardware PMU folks to be able to see the raw
values of the PMU registers.

I think partly it's so that they can verify that perf is doing what they want,
and some of it is that they're interested in some of the more obscure info that
isn't plumbed out through other perf interfaces.

We've used various internal hacks over the years to keep them happy. This is an
attempt to use a somewhat standard mechanism.

It would also be helpful for those of us working on the perf hardware backends,
to be able to verify that we're programming things correctly, without resorting
to debug printks etc.

Basically we want to sample regs at the time of the perf interrupt, so we
though PERF_SAMPLE_REGS_INTR made senes :)

But if you think this is the wrong mechanism within perf, then please let us
know.

I know perf's mission is to abstract as much of the arcane hardware details
into a generic interface and make PMUs actually useful for normal folks, and we
are committed to that, but it would also be useful to be able to get the raw
values for a different type of user.

Maddy's patch only exports PMC1-6 and MMCR0/1. I think we also need to export
some others, in particular MMCRA has a lot of stuff in it, half of which is not
even architected. So that would have to be exported as "POWER8_MMCRA". And then
there's the SIAR/SDAR/SIER which contain a bunch of info on sampled
instructions that is not currently plumbed out.

OK, no objections then. But this is useful information and should be
included in the patch set.

Thanks, yeah definitely needs more explanation in the patch set.

cheers