Re: [PATCH 2/2] perf tools: Introduce regs_query_register_offset() for arm64

[Hekuang]

hi

在 2017/1/25 3:09, Arnaldo Carvalho de Melo 写道:

Em Tue, Jan 24, 2017 at 06:25:18PM +, Will Deacon escreveu:

On Tue, Jan 24, 2017 at 10:30:15AM +, 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.

Signed-off-by: He Kuang 
  

It would've been nice to have been cc'd on this. In future, please at least
cc linux-arm-kernel for patches directly changing arm/arm64 code.
  

+   GPR_OFFSET_NAME(30),
+   {.name = "lr", .offset = offsetof(struct user_pt_regs, regs[30])},
+   REG_OFFSET_NAME(sp),
  

Don't sp and lr need the leading '%'?
  

+   REG_OFFSET_NAME(pc),
+   REG_OFFSET_NAME(pstate),
  

The AArch64 DWARF spec says that DWARF register 32 is "RESERVED" and
register 33 is the ELR, so these pc/pstate entries are wrong.
  

However, with those changes, I think this patch can simply be ignored and
mainline is doing the right thing.

Ok, thanks for checking, dropping this patch then.

- Arnaldo



The purpose of this patch is mainly on enable bpf prologue on arm64,
a new v2 version is sent and fix the problem mentioned by Will, thank
you for reviewing this.

Re: [PATCH 2/2] perf tools: Introduce regs_query_register_offset() for arm64

[Hekuang]

hi

在 2017/1/25 3:09, Arnaldo Carvalho de Melo 写道:

Em Tue, Jan 24, 2017 at 06:25:18PM +, Will Deacon escreveu:

On Tue, Jan 24, 2017 at 10:30:15AM +, 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.

Signed-off-by: He Kuang 
  

It would've been nice to have been cc'd on this. In future, please at least
cc linux-arm-kernel for patches directly changing arm/arm64 code.
  

+   GPR_OFFSET_NAME(30),
+   {.name = "lr", .offset = offsetof(struct user_pt_regs, regs[30])},
+   REG_OFFSET_NAME(sp),
  

Don't sp and lr need the leading '%'?
  

+   REG_OFFSET_NAME(pc),
+   REG_OFFSET_NAME(pstate),
  

The AArch64 DWARF spec says that DWARF register 32 is "RESERVED" and
register 33 is the ELR, so these pc/pstate entries are wrong.
  

However, with those changes, I think this patch can simply be ignored and
mainline is doing the right thing.

Ok, thanks for checking, dropping this patch then.

- Arnaldo



The purpose of this patch is mainly on enable bpf prologue on arm64,
a new v2 version is sent and fix the problem mentioned by Will, thank
you for reviewing this.

Re: [PATCH 2/2] perf tools: Introduce regs_query_register_offset() for arm64

[Arnaldo Carvalho de Melo]

Em Tue, Jan 24, 2017 at 06:25:18PM +, Will Deacon escreveu:
> On Tue, Jan 24, 2017 at 10:30:15AM +, 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.
> > 
> > Signed-off-by: He Kuang 
 
> It would've been nice to have been cc'd on this. In future, please at least
> cc linux-arm-kernel for patches directly changing arm/arm64 code.
 
> > +   GPR_OFFSET_NAME(30),
> > +   {.name = "lr", .offset = offsetof(struct user_pt_regs, regs[30])},
> > +   REG_OFFSET_NAME(sp),
 
> Don't sp and lr need the leading '%'?
 
> > +   REG_OFFSET_NAME(pc),
> > +   REG_OFFSET_NAME(pstate),
 
> The AArch64 DWARF spec says that DWARF register 32 is "RESERVED" and
> register 33 is the ELR, so these pc/pstate entries are wrong.
 
> However, with those changes, I think this patch can simply be ignored and
> mainline is doing the right thing.

Ok, thanks for checking, dropping this patch then.

- Arnaldo

Re: [PATCH 2/2] perf tools: Introduce regs_query_register_offset() for arm64

[Arnaldo Carvalho de Melo]

Em Tue, Jan 24, 2017 at 06:25:18PM +, Will Deacon escreveu:
> On Tue, Jan 24, 2017 at 10:30:15AM +, 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.
> > 
> > Signed-off-by: He Kuang 
 
> It would've been nice to have been cc'd on this. In future, please at least
> cc linux-arm-kernel for patches directly changing arm/arm64 code.
 
> > +   GPR_OFFSET_NAME(30),
> > +   {.name = "lr", .offset = offsetof(struct user_pt_regs, regs[30])},
> > +   REG_OFFSET_NAME(sp),
 
> Don't sp and lr need the leading '%'?
 
> > +   REG_OFFSET_NAME(pc),
> > +   REG_OFFSET_NAME(pstate),
 
> The AArch64 DWARF spec says that DWARF register 32 is "RESERVED" and
> register 33 is the ELR, so these pc/pstate entries are wrong.
 
> However, with those changes, I think this patch can simply be ignored and
> mainline is doing the right thing.

Ok, thanks for checking, dropping this patch then.

- Arnaldo

Re: [PATCH 2/2] perf tools: Introduce regs_query_register_offset() for arm64

[Will Deacon]

On Tue, Jan 24, 2017 at 10:30:15AM +, 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.
> 
> Signed-off-by: He Kuang 
> ---
>  tools/perf/arch/arm64/Makefile  |   1 +
>  tools/perf/arch/arm64/util/dwarf-regs.c | 123 
> ++--
>  2 files changed, 71 insertions(+), 53 deletions(-)

It would've been nice to have been cc'd on this. In future, please at least
cc linux-arm-kernel for patches directly changing arm/arm64 code.

> 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..6427ce0 100644
> --- a/tools/perf/arch/arm64/util/dwarf-regs.c
> +++ b/tools/perf/arch/arm64/util/dwarf-regs.c
> @@ -9,72 +9,89 @@
>   */
>  
>  #include 
> +#include  /* for struct user_pt_regs */
> +#include  /* for EINVAL */
> +#include  /* for strcmp */
>  #include 
>  
> -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) {.name = #r,  \
> + .offset = offsetof(struct user_pt_regs, r)}
> +#define REG_OFFSET_END {.name = NULL, .offset = 0}
> +#define GPR_OFFSET_NAME(r) \
> + {.name = "%x" #r, .offset = offsetof(struct user_pt_regs, regs[r])}
> +
> +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),
> + GPR_OFFSET_NAME(30),
> + {.name = "lr", .offset = offsetof(struct user_pt_regs, regs[30])},
> + REG_OFFSET_NAME(sp),

Don't sp and lr need the leading '%'?

> + REG_OFFSET_NAME(pc),
> + REG_OFFSET_NAME(pstate),

The AArch64 DWARF spec says that DWARF register 32 is "RESERVED" and
register 33 is the ELR, so these pc/pstate entries are wrong.

However, with those changes, I think this patch can simply be ignored and
mainline is doing the right thing.

Will

Re: [PATCH 2/2] perf tools: Introduce regs_query_register_offset() for arm64

[Will Deacon]

On Tue, Jan 24, 2017 at 10:30:15AM +, 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.
> 
> Signed-off-by: He Kuang 
> ---
>  tools/perf/arch/arm64/Makefile  |   1 +
>  tools/perf/arch/arm64/util/dwarf-regs.c | 123 
> ++--
>  2 files changed, 71 insertions(+), 53 deletions(-)

It would've been nice to have been cc'd on this. In future, please at least
cc linux-arm-kernel for patches directly changing arm/arm64 code.

> 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..6427ce0 100644
> --- a/tools/perf/arch/arm64/util/dwarf-regs.c
> +++ b/tools/perf/arch/arm64/util/dwarf-regs.c
> @@ -9,72 +9,89 @@
>   */
>  
>  #include 
> +#include  /* for struct user_pt_regs */
> +#include  /* for EINVAL */
> +#include  /* for strcmp */
>  #include 
>  
> -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) {.name = #r,  \
> + .offset = offsetof(struct user_pt_regs, r)}
> +#define REG_OFFSET_END {.name = NULL, .offset = 0}
> +#define GPR_OFFSET_NAME(r) \
> + {.name = "%x" #r, .offset = offsetof(struct user_pt_regs, regs[r])}
> +
> +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),
> + GPR_OFFSET_NAME(30),
> + {.name = "lr", .offset = offsetof(struct user_pt_regs, regs[30])},
> + REG_OFFSET_NAME(sp),

Don't sp and lr need the leading '%'?

> + REG_OFFSET_NAME(pc),
> + REG_OFFSET_NAME(pstate),

The AArch64 DWARF spec says that DWARF register 32 is "RESERVED" and
register 33 is the ELR, so these pc/pstate entries are wrong.

However, with those changes, I think this patch can simply be ignored and
mainline is doing the right thing.

Will