[PATCH 2/2 v2] perf tools: Enable bpf prologue for arm64

Since HAVE_KPROBES can be enabled in arm64, this patch introduces
regs_query_register_offset() to convert register name to offset for
arm64, so the BPF prologue feature is ready to use.

This patch also changes the 'dwarfnum' to 'offset' in register table,
so the related functions are consistent with x86.

Signed-off-by: He Kuang <redacted>
---
 tools/perf/arch/arm64/Makefile          |   1 +
 tools/perf/arch/arm64/util/dwarf-regs.c | 124 ++++++++++++++++++--------------
 2 files changed, 72 insertions(+), 53 deletions(-)

diff --git a/tools/perf/arch/arm64/Makefile b/tools/perf/arch/arm64/Makefile
index 18b1351..eebe1ec 100644
--- a/tools/perf/arch/arm64/Makefile
+++ b/tools/perf/arch/arm64/Makefile

@@ -2,3 +2,4 @@ ifndef NO_DWARF
 PERF_HAVE_DWARF_REGS := 1
 endif
 PERF_HAVE_JITDUMP := 1
+PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1

diff --git a/tools/perf/arch/arm64/util/dwarf-regs.c b/tools/perf/arch/arm64/util/dwarf-regs.c
index d49efeb..5ec62a4 100644
--- a/tools/perf/arch/arm64/util/dwarf-regs.c
+++ b/tools/perf/arch/arm64/util/dwarf-regs.c

@@ -9,72 +9,90 @@
  */
 
 #include <stddef.h>
+#include <linux/ptrace.h> /* for struct user_pt_regs */
+#include <errno.h> /* for EINVAL */
+#include <string.h> /* for strcmp */
+#include <linux/ptrace.h> /* for struct user_pt_regs */
 #include <dwarf-regs.h>
 
-struct pt_regs_dwarfnum {
+struct pt_regs_offset {
 	const char *name;
-	unsigned int dwarfnum;
+	int offset;
 };
 
-#define STR(s) #s
-#define REG_DWARFNUM_NAME(r, num) {.name = r, .dwarfnum = num}
-#define GPR_DWARFNUM_NAME(num) \
-	{.name = STR(%x##num), .dwarfnum = num}
-#define REG_DWARFNUM_END {.name = NULL, .dwarfnum = 0}
-
 /*
  * Reference:
  * http://infocenter.arm.com/help/topic/com.arm.doc.ihi0057b/IHI0057B_aadwarf64.pdf
  */
-static const struct pt_regs_dwarfnum regdwarfnum_table[] = {
-	GPR_DWARFNUM_NAME(0),
-	GPR_DWARFNUM_NAME(1),
-	GPR_DWARFNUM_NAME(2),
-	GPR_DWARFNUM_NAME(3),
-	GPR_DWARFNUM_NAME(4),
-	GPR_DWARFNUM_NAME(5),
-	GPR_DWARFNUM_NAME(6),
-	GPR_DWARFNUM_NAME(7),
-	GPR_DWARFNUM_NAME(8),
-	GPR_DWARFNUM_NAME(9),
-	GPR_DWARFNUM_NAME(10),
-	GPR_DWARFNUM_NAME(11),
-	GPR_DWARFNUM_NAME(12),
-	GPR_DWARFNUM_NAME(13),
-	GPR_DWARFNUM_NAME(14),
-	GPR_DWARFNUM_NAME(15),
-	GPR_DWARFNUM_NAME(16),
-	GPR_DWARFNUM_NAME(17),
-	GPR_DWARFNUM_NAME(18),
-	GPR_DWARFNUM_NAME(19),
-	GPR_DWARFNUM_NAME(20),
-	GPR_DWARFNUM_NAME(21),
-	GPR_DWARFNUM_NAME(22),
-	GPR_DWARFNUM_NAME(23),
-	GPR_DWARFNUM_NAME(24),
-	GPR_DWARFNUM_NAME(25),
-	GPR_DWARFNUM_NAME(26),
-	GPR_DWARFNUM_NAME(27),
-	GPR_DWARFNUM_NAME(28),
-	GPR_DWARFNUM_NAME(29),
-	REG_DWARFNUM_NAME("%lr", 30),
-	REG_DWARFNUM_NAME("%sp", 31),
-	REG_DWARFNUM_END,
+#define REG_OFFSET_NAME(r, num) {.name = "%" #r,			\
+			.offset = offsetof(struct user_pt_regs, regs[num])}
+#define REG_OFFSET_END {.name = NULL, .offset = 0}
+#define GPR_OFFSET_NAME(r) \
+	{.name = "%x" #r, .offset = offsetof(struct user_pt_regs, regs[r])}
+
+/* This table is for reverse searching for the offset or register
+ * names in aarch64_regstr_tbl[].
+ */
+static const struct pt_regs_offset regoffset_table[] = {
+	GPR_OFFSET_NAME(0),
+	GPR_OFFSET_NAME(1),
+	GPR_OFFSET_NAME(2),
+	GPR_OFFSET_NAME(3),
+	GPR_OFFSET_NAME(4),
+	GPR_OFFSET_NAME(5),
+	GPR_OFFSET_NAME(6),
+	GPR_OFFSET_NAME(7),
+	GPR_OFFSET_NAME(8),
+	GPR_OFFSET_NAME(9),
+	GPR_OFFSET_NAME(10),
+	GPR_OFFSET_NAME(11),
+	GPR_OFFSET_NAME(12),
+	GPR_OFFSET_NAME(13),
+	GPR_OFFSET_NAME(14),
+	GPR_OFFSET_NAME(15),
+	GPR_OFFSET_NAME(16),
+	GPR_OFFSET_NAME(17),
+	GPR_OFFSET_NAME(18),
+	GPR_OFFSET_NAME(19),
+	GPR_OFFSET_NAME(20),
+	GPR_OFFSET_NAME(21),
+	GPR_OFFSET_NAME(22),
+	GPR_OFFSET_NAME(23),
+	GPR_OFFSET_NAME(24),
+	GPR_OFFSET_NAME(25),
+	GPR_OFFSET_NAME(26),
+	GPR_OFFSET_NAME(27),
+	GPR_OFFSET_NAME(28),
+	GPR_OFFSET_NAME(29),
+	REG_OFFSET_NAME(lr, 30),
+	REG_OFFSET_NAME(sp, 31),
+	REG_OFFSET_END,
 };
 
+/* Minus 1 for the ending REG_OFFSET_END */
+#define ARCH_MAX_REGS ((sizeof(regoffset_table) /		\
+			sizeof(regoffset_table[0])) - 1)
+
+/* Return architecture dependent register string (for kprobe-tracer) */
+const char *get_arch_regstr(unsigned int n)
+{
+	return (n < ARCH_MAX_REGS) ? regoffset_table[n].name : NULL;
+}
+
+/* Reuse code from arch/arm64/kernel/ptrace.c */
 /**
- * get_arch_regstr() - lookup register name from it's DWARF register number
- * @n:	the DWARF register number
+ * regs_query_register_offset() - query register offset from its name
+ * @name:	the name of a register
  *
- * get_arch_regstr() returns the name of the register in struct
- * regdwarfnum_table from it's DWARF register number. If the register is not
- * found in the table, this returns NULL;
+ * regs_query_register_offset() returns the offset of a register in struct
+ * user_pt_regs from its name. If the name is invalid, this returns -EINVAL;
  */
-const char *get_arch_regstr(unsigned int n)
+int regs_query_register_offset(const char *name)
 {
-	const struct pt_regs_dwarfnum *roff;
-	for (roff = regdwarfnum_table; roff->name != NULL; roff++)
-		if (roff->dwarfnum == n)
-			return roff->name;
-	return NULL;
+	const struct pt_regs_offset *roff;
+
+	for (roff = regoffset_table; roff->name != NULL; roff++)
+		if (!strcmp(roff->name, name))
+			return roff->offset;
+	return -EINVAL;
 }

-- 
1.8.5.2

On Wed, Jan 25, 2017 at 07:23:11AM +0000, He Kuang wrote:

Since HAVE_KPROBES can be enabled in arm64, this patch introduces
regs_query_register_offset() to convert register name to offset for
arm64, so the BPF prologue feature is ready to use.

This patch also changes the 'dwarfnum' to 'offset' in register table,
so the related functions are consistent with x86.

Wouldn't it be an awful lot simpler just to leave the code as-is, and
implement regs_query_register_offset in the same way that we implement
get_arch_regstr but return the dwarfnum?

I don't really see the point of all the refactoring.

Will

On Wed, 25 Jan 2017 13:32:01 +0000
Will Deacon [off-list ref] wrote:

On Wed, Jan 25, 2017 at 07:23:11AM +0000, He Kuang wrote:

quoted

Since HAVE_KPROBES can be enabled in arm64, this patch introduces
regs_query_register_offset() to convert register name to offset for
arm64, so the BPF prologue feature is ready to use.

This patch also changes the 'dwarfnum' to 'offset' in register table,
so the related functions are consistent with x86.

Wouldn't it be an awful lot simpler just to leave the code as-is, and
implement regs_query_register_offset in the same way that we implement
get_arch_regstr but return the dwarfnum?

No, since the offset is not same as dwarfnum.

With this style, the index of array becomes the dwarfnum (the index of
each register defined by DWARF) and the "offset" member means the
byte-offset of the register in (user_)pt_regs. Those should be different.

I don't really see the point of all the refactoring.

Also, from the maintenance point of view, this rewrite work makes
the code simply similar to x86 implementation, that will be easier to
maintain :)

Thank you,


-- 
Masami Hiramatsu [off-list ref]

On Wed, 25 Jan 2017 07:23:11 +0000
He Kuang [off-list ref] wrote:

Here is a duplicated line.

Other parts look good to me :)

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
except for the above line.

Thank you!

 #include <dwarf-regs.h>
 
-struct pt_regs_dwarfnum {
+struct pt_regs_offset {
 	const char *name;
-	unsigned int dwarfnum;
+	int offset;
 };
 
-#define STR(s) #s
-#define REG_DWARFNUM_NAME(r, num) {.name = r, .dwarfnum = num}
-#define GPR_DWARFNUM_NAME(num) \
-	{.name = STR(%x##num), .dwarfnum = num}
-#define REG_DWARFNUM_END {.name = NULL, .dwarfnum = 0}
-
 /*
  * Reference:
  * http://infocenter.arm.com/help/topic/com.arm.doc.ihi0057b/IHI0057B_aadwarf64.pdf
  */
-static const struct pt_regs_dwarfnum regdwarfnum_table[] = {
-	GPR_DWARFNUM_NAME(0),
-	GPR_DWARFNUM_NAME(1),
-	GPR_DWARFNUM_NAME(2),
-	GPR_DWARFNUM_NAME(3),
-	GPR_DWARFNUM_NAME(4),
-	GPR_DWARFNUM_NAME(5),
-	GPR_DWARFNUM_NAME(6),
-	GPR_DWARFNUM_NAME(7),
-	GPR_DWARFNUM_NAME(8),
-	GPR_DWARFNUM_NAME(9),
-	GPR_DWARFNUM_NAME(10),
-	GPR_DWARFNUM_NAME(11),
-	GPR_DWARFNUM_NAME(12),
-	GPR_DWARFNUM_NAME(13),
-	GPR_DWARFNUM_NAME(14),
-	GPR_DWARFNUM_NAME(15),
-	GPR_DWARFNUM_NAME(16),
-	GPR_DWARFNUM_NAME(17),
-	GPR_DWARFNUM_NAME(18),
-	GPR_DWARFNUM_NAME(19),
-	GPR_DWARFNUM_NAME(20),
-	GPR_DWARFNUM_NAME(21),
-	GPR_DWARFNUM_NAME(22),
-	GPR_DWARFNUM_NAME(23),
-	GPR_DWARFNUM_NAME(24),
-	GPR_DWARFNUM_NAME(25),
-	GPR_DWARFNUM_NAME(26),
-	GPR_DWARFNUM_NAME(27),
-	GPR_DWARFNUM_NAME(28),
-	GPR_DWARFNUM_NAME(29),
-	REG_DWARFNUM_NAME("%lr", 30),
-	REG_DWARFNUM_NAME("%sp", 31),
-	REG_DWARFNUM_END,
+#define REG_OFFSET_NAME(r, num) {.name = "%" #r,			\
+			.offset = offsetof(struct user_pt_regs, regs[num])}
+#define REG_OFFSET_END {.name = NULL, .offset = 0}
+#define GPR_OFFSET_NAME(r) \
+	{.name = "%x" #r, .offset = offsetof(struct user_pt_regs, regs[r])}
+
+/* This table is for reverse searching for the offset or register
+ * names in aarch64_regstr_tbl[].
+ */
+static const struct pt_regs_offset regoffset_table[] = {
+	GPR_OFFSET_NAME(0),
+	GPR_OFFSET_NAME(1),
+	GPR_OFFSET_NAME(2),
+	GPR_OFFSET_NAME(3),
+	GPR_OFFSET_NAME(4),
+	GPR_OFFSET_NAME(5),
+	GPR_OFFSET_NAME(6),
+	GPR_OFFSET_NAME(7),
+	GPR_OFFSET_NAME(8),
+	GPR_OFFSET_NAME(9),
+	GPR_OFFSET_NAME(10),
+	GPR_OFFSET_NAME(11),
+	GPR_OFFSET_NAME(12),
+	GPR_OFFSET_NAME(13),
+	GPR_OFFSET_NAME(14),
+	GPR_OFFSET_NAME(15),
+	GPR_OFFSET_NAME(16),
+	GPR_OFFSET_NAME(17),
+	GPR_OFFSET_NAME(18),
+	GPR_OFFSET_NAME(19),
+	GPR_OFFSET_NAME(20),
+	GPR_OFFSET_NAME(21),
+	GPR_OFFSET_NAME(22),
+	GPR_OFFSET_NAME(23),
+	GPR_OFFSET_NAME(24),
+	GPR_OFFSET_NAME(25),
+	GPR_OFFSET_NAME(26),
+	GPR_OFFSET_NAME(27),
+	GPR_OFFSET_NAME(28),
+	GPR_OFFSET_NAME(29),
+	REG_OFFSET_NAME(lr, 30),
+	REG_OFFSET_NAME(sp, 31),
+	REG_OFFSET_END,
 };
 
+/* Minus 1 for the ending REG_OFFSET_END */
+#define ARCH_MAX_REGS ((sizeof(regoffset_table) /		\
+			sizeof(regoffset_table[0])) - 1)
+
+/* Return architecture dependent register string (for kprobe-tracer) */
+const char *get_arch_regstr(unsigned int n)
+{
+	return (n < ARCH_MAX_REGS) ? regoffset_table[n].name : NULL;
+}
+
+/* Reuse code from arch/arm64/kernel/ptrace.c */
 /**
- * get_arch_regstr() - lookup register name from it's DWARF register number
- * @n:	the DWARF register number
+ * regs_query_register_offset() - query register offset from its name
+ * @name:	the name of a register
  *
- * get_arch_regstr() returns the name of the register in struct
- * regdwarfnum_table from it's DWARF register number. If the register is not
- * found in the table, this returns NULL;
+ * regs_query_register_offset() returns the offset of a register in struct
+ * user_pt_regs from its name. If the name is invalid, this returns -EINVAL;
  */
-const char *get_arch_regstr(unsigned int n)
+int regs_query_register_offset(const char *name)
 {
-	const struct pt_regs_dwarfnum *roff;
-	for (roff = regdwarfnum_table; roff->name != NULL; roff++)
-		if (roff->dwarfnum == n)
-			return roff->name;
-	return NULL;
+	const struct pt_regs_offset *roff;
+
+	for (roff = regoffset_table; roff->name != NULL; roff++)
+		if (!strcmp(roff->name, name))
+			return roff->offset;
+	return -EINVAL;
 }
-- 
1.8.5.2

-- 
Masami Hiramatsu [off-list ref]

On Thu, Jan 26, 2017 at 10:49:16AM +0900, Masami Hiramatsu wrote:

On Wed, 25 Jan 2017 13:32:01 +0000
Will Deacon [off-list ref] wrote:

quoted

On Wed, Jan 25, 2017 at 07:23:11AM +0000, He Kuang wrote:

quoted

Since HAVE_KPROBES can be enabled in arm64, this patch introduces
regs_query_register_offset() to convert register name to offset for
arm64, so the BPF prologue feature is ready to use.

This patch also changes the 'dwarfnum' to 'offset' in register table,
so the related functions are consistent with x86.

Wouldn't it be an awful lot simpler just to leave the code as-is, and
implement regs_query_register_offset in the same way that we implement
get_arch_regstr but return the dwarfnum?

No, since the offset is not same as dwarfnum.

With this style, the index of array becomes the dwarfnum (the index of
each register defined by DWARF) and the "offset" member means the
byte-offset of the register in (user_)pt_regs. Those should be different.

Ok, then do it as two patches then, rather than introduce functionality
along with the renaming.

quoted

I don't really see the point of all the refactoring.

Also, from the maintenance point of view, this rewrite work makes
the code simply similar to x86 implementation, that will be easier to
maintain :)

Right, apart from the two howling bugs in the version that was nearly merged
initially :p. I tend to err on the "if it ain't broke, don't fix it" side
of the argument but if you really want the refactoring lets keep it as a
separate change.

Will

Em Thu, Jan 26, 2017 at 04:52:12PM +0000, Will Deacon escreveu:

On Thu, Jan 26, 2017 at 10:49:16AM +0900, Masami Hiramatsu wrote:

quoted

On Wed, 25 Jan 2017 13:32:01 +0000
Will Deacon [off-list ref] wrote:

quoted

On Wed, Jan 25, 2017 at 07:23:11AM +0000, He Kuang wrote:

quoted

Since HAVE_KPROBES can be enabled in arm64, this patch introduces
regs_query_register_offset() to convert register name to offset for
arm64, so the BPF prologue feature is ready to use.

This patch also changes the 'dwarfnum' to 'offset' in register table,
so the related functions are consistent with x86.

Wouldn't it be an awful lot simpler just to leave the code as-is, and
implement regs_query_register_offset in the same way that we implement
get_arch_regstr but return the dwarfnum?

No, since the offset is not same as dwarfnum.

With this style, the index of array becomes the dwarfnum (the index of
each register defined by DWARF) and the "offset" member means the
byte-offset of the register in (user_)pt_regs. Those should be different.

Ok, then do it as two patches then, rather than introduce functionality
along with the renaming.

quoted

quoted

I don't really see the point of all the refactoring.

Also, from the maintenance point of view, this rewrite work makes
the code simply similar to x86 implementation, that will be easier to
maintain :)

Right, apart from the two howling bugs in the version that was nearly merged
initially :p. I tend to err on the "if it ain't broke, don't fix it" side
of the argument but if you really want the refactoring lets keep it as a
separate change.

So, He, can you do that? How do we proceed?

- Arnaldo

hi,

? 2017/1/27 3:31, Arnaldo Carvalho de Melo ??:

Em Thu, Jan 26, 2017 at 04:52:12PM +0000, Will Deacon escreveu:

quoted

On Thu, Jan 26, 2017 at 10:49:16AM +0900, Masami Hiramatsu wrote:

quoted

On Wed, 25 Jan 2017 13:32:01 +0000
Will Deacon [off-list ref] wrote:

quoted

On Wed, Jan 25, 2017 at 07:23:11AM +0000, He Kuang wrote:

quoted

Since HAVE_KPROBES can be enabled in arm64, this patch introduces
regs_query_register_offset() to convert register name to offset for
arm64, so the BPF prologue feature is ready to use.

This patch also changes the 'dwarfnum' to 'offset' in register table,
so the related functions are consistent with x86.

Wouldn't it be an awful lot simpler just to leave the code as-is, and
implement regs_query_register_offset in the same way that we implement
get_arch_regstr but return the dwarfnum?

No, since the offset is not same as dwarfnum.

With this style, the index of array becomes the dwarfnum (the index of
each register defined by DWARF) and the "offset" member means the
byte-offset of the register in (user_)pt_regs. Those should be different.

Ok, then do it as two patches then, rather than introduce functionality
along with the renaming.

quoted

quoted

I don't really see the point of all the refactoring.

Also, from the maintenance point of view, this rewrite work makes
the code simply similar to x86 implementation, that will be easier to
maintain :)

Right, apart from the two howling bugs in the version that was nearly merged
initially :p. I tend to err on the "if it ain't broke, don't fix it" side
of the argument but if you really want the refactoring lets keep it as a
separate change.

So, He, can you do that? How do we proceed?

- Arnaldo

I split the patch as Will suggested and resend them. Sorry for
late response, just back from Spring festival.