> From: Arnaldo Carvalho de Melo [mailto:acme@redhat.com] > > Em Tue, Sep 01, 2015 at 11:47:41AM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu: > > > From: Wang Nan [mailto:wangnan0@huawei.com] > > > > > > regs_query_register_offset() is a helper function which converts > > > register name like "%rax" to offset of a register in 'struct pt_regs', > > > which is required by BPF prologue generator. Since the function is > > > identical, try to reuse the code in arch/x86/kernel/ptrace.c. > > > > > > Comment inside dwarf-regs.c list the differences between this > > > implementation and kernel code. > > > > Hmm, this also introduce a duplication of the code... > > It might be a good time to move them into arch/x86/lib/ and > > reuse it directly from perf code. > > It is strange to, having tried sharing stuff directly from the kernel, > to be now in a position where I advocate against it... > > Copy'n'pasting what I said in another message: > > ----- > We would go back to sharing stuff with the kernel, but this time around > we would be using something that everybody knows is being shared, which > doesn't elliminates the possibility that at some point changes made with > the kernel in mind would break the tools/ using code. > > Perhaps it is better to keep copying what we want and introduce > infrastructure to check for differences and warn us as soon as possible > so that we would do the copy, test if it doesn't break what we use, etc. > > I.e. we wouldn't be putting any new burden on the "kernel people", i.e. > the burden of having to check that changes they make don't break tools/ > living code, nor any out of the blue breakage on tools/ for tools/ > developers to fix when changes are made on the kernel "side" ----- > --- > > The "stop sharing directly stuff with the kernel" stance was taken after > a report from Linus about breakage due to tools/ using kernel files > directly and then a change made in some RCU files broke the tools/perf/ > build, even with tools/perf/ not using anything RCU related so far. > > Looking at tools/perf/MANIFEST, the file used to create a detached > tarball so that perf can be built outside the kernel sources there are > still some kernel source files listed, but those probably need to be > copied too... OK, so let this apply. Acked-by: Masami Hiramatsu And also we'll need a testcase for this. Thank you, > > - Arnaldo > > > Thank you, > > > > > > > > get_arch_regstr() switches to regoffset_table and the old string table > > > is dropped. > > > > > > Signed-off-by: Wang Nan > > > Signed-off-by: He Kuang > > > Cc: Alexei Starovoitov > > > Cc: Brendan Gregg > > > Cc: Daniel Borkmann > > > Cc: David Ahern > > > Cc: He Kuang > > > Cc: Jiri Olsa > > > Cc: Kaixu Xia > > > Cc: Masami Hiramatsu > > > Cc: Namhyung Kim > > > Cc: Paul Mackerras > > > Cc: Peter Zijlstra > > > Cc: Zefan Li > > > Cc: pi3orama@163.com > > > Cc: Arnaldo Carvalho de Melo > > > --- > > > tools/perf/arch/x86/Makefile | 1 + > > > tools/perf/arch/x86/util/Build | 1 + > > > tools/perf/arch/x86/util/dwarf-regs.c | 122 ++++++++++++++++++++++++---------- > > > 3 files changed, 90 insertions(+), 34 deletions(-) > > > > > > diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile > > > index 21322e0..09ba923 100644 > > > --- a/tools/perf/arch/x86/Makefile > > > +++ b/tools/perf/arch/x86/Makefile > > > @@ -2,3 +2,4 @@ ifndef NO_DWARF > > > PERF_HAVE_DWARF_REGS := 1 > > > endif > > > HAVE_KVM_STAT_SUPPORT := 1 > > > +PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1 > > > diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build > > > index 2c55e1b..d4d1f23 100644 > > > --- a/tools/perf/arch/x86/util/Build > > > +++ b/tools/perf/arch/x86/util/Build > > > @@ -4,6 +4,7 @@ libperf-y += pmu.o > > > libperf-y += kvm-stat.o > > > > > > libperf-$(CONFIG_DWARF) += dwarf-regs.o > > > +libperf-$(CONFIG_BPF_PROLOGUE) += dwarf-regs.o > > > > > > libperf-$(CONFIG_LIBUNWIND) += unwind-libunwind.o > > > libperf-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o > > > diff --git a/tools/perf/arch/x86/util/dwarf-regs.c b/tools/perf/arch/x86/util/dwarf-regs.c > > > index a08de0a..de5b936 100644 > > > --- a/tools/perf/arch/x86/util/dwarf-regs.c > > > +++ b/tools/perf/arch/x86/util/dwarf-regs.c > > > @@ -21,55 +21,109 @@ > > > */ > > > > > > #include > > > +#include /* for EINVAL */ > > > +#include /* for strcmp */ > > > +#include /* for struct pt_regs */ > > > +#include /* for offsetof */ > > > #include > > > > > > /* > > > - * Generic dwarf analysis helpers > > > + * See arch/x86/kernel/ptrace.c. > > > + * Different from it: > > > + * > > > + * - Since struct pt_regs is defined differently for user and kernel, > > > + * but we want to use 'ax, bx' instead of 'rax, rbx' (which is struct > > > + * field name of user's pt_regs), we make REG_OFFSET_NAME to accept > > > + * both string name and reg field name. > > > + * > > > + * - Since accessing x86_32's pt_regs from x86_64 building is difficult > > > + * and vise versa, we simply fill offset with -1, so > > > + * get_arch_regstr() still works but regs_query_register_offset() > > > + * returns error. > > > + * The only inconvenience caused by it now is that we are not allowed > > > + * to generate BPF prologue for a x86_64 kernel if perf is built for > > > + * x86_32. This is really a rare usecase. > > > + * > > > + * - Order is different from kernel's ptrace.c for get_arch_regstr(), which > > > + * is defined by dwarf. > > > */ > > > > > > -#define X86_32_MAX_REGS 8 > > > -const char *x86_32_regs_table[X86_32_MAX_REGS] = { > > > - "%ax", > > > - "%cx", > > > - "%dx", > > > - "%bx", > > > - "$stack", /* Stack address instead of %sp */ > > > - "%bp", > > > - "%si", > > > - "%di", > > > +struct pt_regs_offset { > > > + const char *name; > > > + int offset; > > > +}; > > > + > > > +#define REG_OFFSET_END {.name = NULL, .offset = 0} > > > + > > > +#ifdef __x86_64__ > > > +# define REG_OFFSET_NAME_64(n, r) {.name = n, .offset = offsetof(struct pt_regs, r)} > > > +# define REG_OFFSET_NAME_32(n, r) {.name = n, .offset = -1} > > > +#else > > > +# define REG_OFFSET_NAME_64(n, r) {.name = n, .offset = -1} > > > +# define REG_OFFSET_NAME_32(n, r) {.name = n, .offset = offsetof(struct pt_regs, r)} > > > +#endif > > > + > > > +static const struct pt_regs_offset x86_32_regoffset_table[] = { > > > + REG_OFFSET_NAME_32("%ax", eax), > > > + REG_OFFSET_NAME_32("%cx", ecx), > > > + REG_OFFSET_NAME_32("%dx", edx), > > > + REG_OFFSET_NAME_32("%bx", ebx), > > > + REG_OFFSET_NAME_32("$stack", esp), /* Stack address instead of %sp */ > > > + REG_OFFSET_NAME_32("%bp", ebp), > > > + REG_OFFSET_NAME_32("%si", esi), > > > + REG_OFFSET_NAME_32("%di", edi), > > > + REG_OFFSET_END, > > > }; > > > > > > -#define X86_64_MAX_REGS 16 > > > -const char *x86_64_regs_table[X86_64_MAX_REGS] = { > > > - "%ax", > > > - "%dx", > > > - "%cx", > > > - "%bx", > > > - "%si", > > > - "%di", > > > - "%bp", > > > - "%sp", > > > - "%r8", > > > - "%r9", > > > - "%r10", > > > - "%r11", > > > - "%r12", > > > - "%r13", > > > - "%r14", > > > - "%r15", > > > +static const struct pt_regs_offset x86_64_regoffset_table[] = { > > > + REG_OFFSET_NAME_64("%ax", rax), > > > + REG_OFFSET_NAME_64("%dx", rdx), > > > + REG_OFFSET_NAME_64("%cx", rcx), > > > + REG_OFFSET_NAME_64("%bx", rbx), > > > + REG_OFFSET_NAME_64("%si", rsi), > > > + REG_OFFSET_NAME_64("%di", rdi), > > > + REG_OFFSET_NAME_64("%bp", rbp), > > > + REG_OFFSET_NAME_64("%sp", rsp), > > > + REG_OFFSET_NAME_64("%r8", r8), > > > + REG_OFFSET_NAME_64("%r9", r9), > > > + REG_OFFSET_NAME_64("%r10", r10), > > > + REG_OFFSET_NAME_64("%r11", r11), > > > + REG_OFFSET_NAME_64("%r12", r12), > > > + REG_OFFSET_NAME_64("%r13", r13), > > > + REG_OFFSET_NAME_64("%r14", r14), > > > + REG_OFFSET_NAME_64("%r15", r15), > > > + REG_OFFSET_END, > > > }; > > > > > > /* TODO: switching by dwarf address size */ > > > #ifdef __x86_64__ > > > -#define ARCH_MAX_REGS X86_64_MAX_REGS > > > -#define arch_regs_table x86_64_regs_table > > > +#define regoffset_table x86_64_regoffset_table > > > #else > > > -#define ARCH_MAX_REGS X86_32_MAX_REGS > > > -#define arch_regs_table x86_32_regs_table > > > +#define regoffset_table x86_32_regoffset_table > > > #endif > > > > > > +/* 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) ? arch_regs_table[n] : NULL; > > > + return (n < ARCH_MAX_REGS) ? regoffset_table[n].name : NULL; > > > +} > > > + > > > +/* Reuse code from arch/x86/kernel/ptrace.c */ > > > +/** > > > + * regs_query_register_offset() - query register offset from its name > > > + * @name: the name of a register > > > + * > > > + * regs_query_register_offset() returns the offset of a register in struct > > > + * pt_regs from its name. If the name is invalid, this returns -EINVAL; > > > + */ > > > +int regs_query_register_offset(const char *name) > > > +{ > > > + 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.3.4 > > {.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I