Re: [PATCH 23/31] perf tools: Introduce arch_get_reg_info() for x86

["Wangnan (F)" <wangnan0@huawei.com>]

On 2015/9/1 4:43, Arnaldo Carvalho de Melo wrote:
> Em Sat, Aug 29, 2015 at 04:21:57AM +0000, Wang Nan escreveu:
>> From: He Kuang <hekuang@huawei.com>
>>
>> arch_get_reg_info() 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.
> Is this something like:
>
> /* Query offset/name of register from its name/offset */
> extern int regs_query_register_offset(const char *name);
>
> in ptrace? Can't we reuse that name and even code?

Unfortunately we can't reuse its code, because pt_regs is defined 
differently
in user and kernel side.

In arch/x86/kernel/ptrace.c we have:

struct pt_regs_offset {
         const char *name;
         int offset;
};

#define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct 
pt_regs, r)}
#define REG_OFFSET_END {.name = NULL, .offset = 0}

static const struct pt_regs_offset regoffset_table[] = {
#ifdef CONFIG_X86_64
...
     REG_OFFSET_NAME(r15),
     REG_OFFSET_NAME(r14),
     REG_OFFSET_NAME(r13),
...

The definition of REG_OFFSET_NAME relys on the field name and the string 
name of a
register are identical. This is true for kernel, but not true for userspace.

For example, for x86_64, 'struct pt_regs' is defined in 
arch/x86/include/asm/ptrace.h
for kernel, and the reigster name is 'ax, cx, dx, si, di ...'. In 
contract, which is
defined in arch/x86/include/uapi/asm/ptrace.h for user, and the register 
name becomes
'rax, rcx, rdx, rsi, rdi ...'.

Since logical of regs_query_register_offset() is very simple, changing 
REG_OFFSET_NAME()
makes it a totally different function.

But yes, let's reuse its name. And it may worth considering to reuse its 
code for other
archs.

Thank you.

> Was this that was done and only a rename was made?
>
> - Arnaldo
>
>> This patch replaces original string table by a 'struct reg_info' table,
>> which records offset of registers according to its name.
>>
>> For x86, since there are two sub-archs (x86_32 and x86_64) but we can
>> only get pt_regs for the arch we are currently on, this patch fills
>> offset with '-1' for another sub-arch. This introduces a limitation to
>> perf prologue that, we are unable to generate prologue on a x86_32
>> compiled perf for BPF programs targeted on x86_64 kernel. This
>> limitation is acceptable, because this is a very rare usecase.
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Signed-off-by: He Kuang <hekuang@huawei.com>
>> Cc: Alexei Starovoitov <ast@plumgrid.com>
>> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David Ahern <dsahern@gmail.com>
>> Cc: He Kuang <hekuang@huawei.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Kaixu Xia <xiakaixu@huawei.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Cc: Zefan Li <lizefan@huawei.com>
>> Cc: pi3orama@163.com
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Link: http://lkml.kernel.org/n/1436445342-1402-34-git-send-email-wangnan0@huawei.com
>> ---
>>   tools/perf/arch/x86/Makefile          |   1 +
>>   tools/perf/arch/x86/util/Build        |   2 +
>>   tools/perf/arch/x86/util/dwarf-regs.c | 104 ++++++++++++++++++++++++----------
>>   3 files changed, 78 insertions(+), 29 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile
>> index 21322e0..a84a6f6f 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_GET_REG_INFO := 1
>> diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build
>> index 2c55e1b..09429f6 100644
>> --- a/tools/perf/arch/x86/util/Build
>> +++ b/tools/perf/arch/x86/util/Build
>> @@ -3,6 +3,8 @@ libperf-y += tsc.o
>>   libperf-y += pmu.o
>>   libperf-y += kvm-stat.o
>>   
>> +# BPF_PROLOGUE also need dwarf-regs.o. However, if CONFIG_BPF_PROLOGUE
>> +# is true, CONFIG_DWARF must true.
>>   libperf-$(CONFIG_DWARF) += dwarf-regs.o
>>   
>>   libperf-$(CONFIG_LIBUNWIND)          += unwind-libunwind.o
>> diff --git a/tools/perf/arch/x86/util/dwarf-regs.c b/tools/perf/arch/x86/util/dwarf-regs.c
>> index be22dd4..9928caf 100644
>> --- a/tools/perf/arch/x86/util/dwarf-regs.c
>> +++ b/tools/perf/arch/x86/util/dwarf-regs.c
>> @@ -22,44 +22,67 @@
>>   
>>   #include <stddef.h>
>>   #include <dwarf-regs.h>
>> +#include <string.h>
>> +#include <linux/ptrace.h>
>> +#include <linux/kernel.h> /* for offsetof */
>> +#include <util/bpf-loader.h>
>> +
>> +struct reg_info {
>> +	const char	*name;		/* Reg string in debuginfo      */
>> +	int		offset;		/* Reg offset in struct pt_regs */
>> +};
>>   
>>   /*
>>    * Generic dwarf analysis helpers
>>    */
>> -
>> +/*
>> + * x86_64 compiling can't access pt_regs for x86_32, so fill offset
>> + * with -1.
>> + */
>> +#ifdef __x86_64__
>> +# define REG_INFO(n, f) { .name = n, .offset = -1, }
>> +#else
>> +# define REG_INFO(n, f) { .name = n, .offset = offsetof(struct pt_regs, f), }
>> +#endif
>>   #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 reg_info x86_32_regs_table[X86_32_MAX_REGS] = {
>> +	REG_INFO("%ax", eax),
>> +	REG_INFO("%cx", ecx),
>> +	REG_INFO("%dx", edx),
>> +	REG_INFO("%bx", ebx),
>> +	REG_INFO("$stack", esp),	/* Stack address instead of %sp */
>> +	REG_INFO("%bp", ebp),
>> +	REG_INFO("%si", esi),
>> +	REG_INFO("%di", edi),
>>   };
>>   
>> +#undef REG_INFO
>> +#ifdef __x86_64__
>> +# define REG_INFO(n, f) { .name = n, .offset = offsetof(struct pt_regs, f), }
>> +#else
>> +# define REG_INFO(n, f) { .name = n, .offset = -1, }
>> +#endif
>>   #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",
>> +struct reg_info x86_64_regs_table[X86_64_MAX_REGS] = {
>> +	REG_INFO("%ax",		rax),
>> +	REG_INFO("%dx",		rdx),
>> +	REG_INFO("%cx",		rcx),
>> +	REG_INFO("%bx",		rbx),
>> +	REG_INFO("%si",		rsi),
>> +	REG_INFO("%di",		rdi),
>> +	REG_INFO("%bp",		rbp),
>> +	REG_INFO("%sp",		rsp),
>> +	REG_INFO("%r8",		r8),
>> +	REG_INFO("%r9",		r9),
>> +	REG_INFO("%r10",	r10),
>> +	REG_INFO("%r11",	r11),
>> +	REG_INFO("%r12",	r12),
>> +	REG_INFO("%r13",	r13),
>> +	REG_INFO("%r14",	r14),
>> +	REG_INFO("%r15",	r15),
>>   };
>>   
>> -/* 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
>> @@ -71,5 +94,28 @@ const char *x86_64_regs_table[X86_64_MAX_REGS] = {
>>   /* 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) ? arch_regs_table[n].name : NULL;
>>   }
>> +
>> +#ifdef HAVE_BPF_PROLOGUE
>> +int arch_get_reg_info(const char *name, int *offset)
>> +{
>> +	int i;
>> +	struct reg_info *info;
>> +
>> +	if (!name || !offset)
>> +		return -1;
>> +
>> +	for (i = 0; i < ARCH_MAX_REGS; i++) {
>> +		info = &arch_regs_table[i];
>> +		if (strcmp(info->name, name) == 0) {
>> +			if (info->offset < 0)
>> +				return -1;
>> +			*offset = info->offset;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -1;
>> +}
>> +#endif
>> -- 
>> 2.1.0