| Message ID | 1434386579-6045-2-git-send-email-dave.long@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 06/16/15 09:17, Rob Herring wrote: > On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote: >> From: "David A. Long" <dave.long@linaro.org> >> >> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API >> feature and has identical definitions in four different arch ptrace.h >> include files. It seems unlikely that definition would ever need to be >> changed regardless of architecture so lets move it into >> include/linux/ptrace.h. >> >> Signed-off-by: David A. Long <dave.long@linaro.org> >> --- >> arch/arm/kernel/ptrace.c | 5 ----- >> arch/powerpc/kernel/ptrace.c | 5 ----- >> arch/sh/include/asm/ptrace.h | 5 ----- >> arch/x86/kernel/ptrace.c | 5 ----- >> include/linux/ptrace.h | 9 +++++++++ >> 5 files changed, 9 insertions(+), 20 deletions(-) >> >> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c >> index ef9119f..fb45cf1 100644 >> --- a/arch/arm/kernel/ptrace.c >> +++ b/arch/arm/kernel/ptrace.c >> @@ -59,11 +59,6 @@ >> #define BREAKINST_THUMB 0xde01 >> #endif >> >> -struct pt_regs_offset { >> - const char *name; >> - int offset; >> -}; >> - >> #define REG_OFFSET_NAME(r) \ >> {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)} >> #define REG_OFFSET_END {.name = NULL, .offset = 0} > > Can't you also move these? ARM is complicated with the "ARM_" > prefixing, but the others appear to be the same. Maybe you can remove > the prefix or redefine the macro for ARM. > > Rob > That would mandate that all the architecture-specific pt_regs structures would have to use a top-level named field for each named register. That seems to me like an unnecessary restriction when the point of regs_offset_table is to provide the offset of the register inside an arbitarily complex pt_regs struct. The often redundant definition of these two macros doesn't seem to me that high a price for that. -dl -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 06/19/15 00:19, Michael Ellerman wrote: > On Mon, 2015-06-15 at 12:42 -0400, David Long wrote: >> From: "David A. Long" <dave.long@linaro.org> >> >> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API >> feature and has identical definitions in four different arch ptrace.h >> include files. It seems unlikely that definition would ever need to be >> changed regardless of architecture so lets move it into >> include/linux/ptrace.h. >> >> Signed-off-by: David A. Long <dave.long@linaro.org> >> --- >> arch/powerpc/kernel/ptrace.c | 5 ----- > > Built and booted on powerpc, but is there an easy way to actually test the code > paths in question? > There is an easy way to "smoke test" it on all archiectures that also implement kprobes (which powerpc does). If I'm understanding the powerpc code correctly (WRT register naming conventions) just do the following: cd /sys/kernel/debug/tracing echo 'p do_fork %gpr0' > kprobe_events echo 1 > events/kprobes/enable ls cat trace echo 0 > events/kprobes/enable Every fork() call done on the system between those two echo commands (hence the "ls") should append a line to the trace file. For a more exhaustive test one could repeat this sequence for every register in the architecture. This should work the same on all architectures supporting kprobes. You just have to use the appropriate register names for your architecture after the "%". > Acked-by: Michael Ellerman <mpe@ellerman.id.au> > > cheers > > Thanks, -dl -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Please read the FAQ at http://www.tux.org/lkml/
On 06/22/15 23:32, Michael Ellerman wrote: > On Fri, 2015-06-19 at 10:12 -0400, David Long wrote: >> On 06/19/15 00:19, Michael Ellerman wrote: >>> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote: >>>> From: "David A. Long" <dave.long@linaro.org> >>>> >>>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API >>>> feature and has identical definitions in four different arch ptrace.h >>>> include files. It seems unlikely that definition would ever need to be >>>> changed regardless of architecture so lets move it into >>>> include/linux/ptrace.h. >>>> >>>> Signed-off-by: David A. Long <dave.long@linaro.org> >>>> --- >>>> arch/powerpc/kernel/ptrace.c | 5 ----- >>> >>> Built and booted on powerpc, but is there an easy way to actually test the code >>> paths in question? >>> >> >> There is an easy way to "smoke test" it on all archiectures that also >> implement kprobes (which powerpc does). If I'm understanding the >> powerpc code correctly (WRT register naming conventions) just do the >> following: >> >> cd /sys/kernel/debug/tracing >> echo 'p do_fork %gpr0' > kprobe_events >> echo 1 > events/kprobes/enable >> ls >> cat trace >> echo 0 > events/kprobes/enable >> >> Every fork() call done on the system between those two echo commands >> (hence the "ls") should append a line to the trace file. For a more >> exhaustive test one could repeat this sequence for every register in the >> architecture. > > OK, so I went the whole hog and did: > > $ echo 'p do_fork %gpr0 %gpr1 %gpr2 %gpr3 %gpr4 %gpr5 %gpr6 %gpr7 %gpr8 %gpr9 %gpr10 %gpr11 %gpr12 %gpr13 %gpr14 %gpr15 %gpr16 %gpr17 %gpr18 %gpr19 %gpr20 %gpr21 %gpr22 %gpr23 %gpr24 %gpr25 %gpr26 %gpr27 %gpr28 %gpr29 %gpr30 %gpr31 %nip %msr %ctr %link %xer %ccr %softe %trap %dar %dsisr' > kprobe_events > > And I get: > > bash-2057 [001] d... 535.433941: p_do_fork_0: (do_fork+0x8/0x490) arg1=0xc0000000000094d0 arg2=0xc0000001fbe9be30 arg3=0xc000000001133bb8 arg4=0x1200011 arg5=0x0 arg6=0x0 arg7=0x0 arg8=0x3fff7c885940 arg9=0x1 arg10=0xc0000001fbe9bea0 arg11=0x0 arg12=0xc01 arg13=0xc0000000000094c8 arg14=0xc00000000fdc0480 arg15=0x0 arg16=0x22000000 arg17=0x1016d6e8 arg18=0x0 arg19=0x44000000 arg20=0x0 arg21=0x10037c82208 arg22=0x1017b008 arg23=0x10143d18 arg24=0x10178854 arg25=0x10144f90 arg26=0x10037c821e8 arg27=0x0 arg28=0x0 arg29=0x0 arg30=0x0 arg31=0x809 arg32=0x3ffff788c010 arg33=0xc0000000000a7fe8 arg34=0x8000000000029033 arg35=0xc0000000000094c8 arg36=0xc0000000000094d0 arg37=0x0 arg38=0x42222844 arg39=0x1 arg40=0x700 arg41=0xc0000001fbe9bd50 arg42=0xc0000001fbe9bd30 > > Which is ugly as hell, but appears unchanged since before your patch. > Excellent. Many thanks. > I take it it's expected that the names are not decoded in the output? > Yes. > Also I wonder why we choose "gpr" when "r" is the more usual prefix on powerpc. > I guess we can add new aliases to the table. > Yeah I can't answer that, this is just what the preexisting code is written to do. I believe you could add aliases to the table, perhaps as a step in migrating to supporting only the more common naming. The reverse translation would have to return one or the other though. -dl -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 06/24/15 00:07, Michael Ellerman wrote: > On Tue, 2015-06-23 at 09:48 -0400, David Long wrote: >> On 06/22/15 23:32, Michael Ellerman wrote: >>> On Fri, 2015-06-19 at 10:12 -0400, David Long wrote: >>>> On 06/19/15 00:19, Michael Ellerman wrote: >>>>> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote: >>>>>> From: "David A. Long" <dave.long@linaro.org> >>>>>> >>>>>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API >>>>>> feature and has identical definitions in four different arch ptrace.h >>>>>> include files. It seems unlikely that definition would ever need to be >>>>>> changed regardless of architecture so lets move it into >>>>>> include/linux/ptrace.h. >>>>>> >>>>>> Signed-off-by: David A. Long <dave.long@linaro.org> >>>>>> --- >>>>>> arch/powerpc/kernel/ptrace.c | 5 ----- >>>>> >>>>> Built and booted on powerpc, but is there an easy way to actually test the code >>>>> paths in question? >>>> >>>> There is an easy way to "smoke test" it on all archiectures that also >>>> implement kprobes (which powerpc does). If I'm understanding the >>>> powerpc code correctly (WRT register naming conventions) just do the >>>> following: >>>> >>>> cd /sys/kernel/debug/tracing >>>> echo 'p do_fork %gpr0' > kprobe_events >>>> echo 1 > events/kprobes/enable >>>> ls >>>> cat trace >>>> echo 0 > events/kprobes/enable >>>> >>>> Every fork() call done on the system between those two echo commands >>>> (hence the "ls") should append a line to the trace file. For a more >>>> exhaustive test one could repeat this sequence for every register in the >>>> architecture. >>> >>> OK, so I went the whole hog and did: >>> >>> $ echo 'p do_fork %gpr0 %gpr1 %gpr2 %gpr3 %gpr4 %gpr5 %gpr6 %gpr7 %gpr8 %gpr9 %gpr10 %gpr11 %gpr12 %gpr13 %gpr14 %gpr15 %gpr16 %gpr17 %gpr18 %gpr19 %gpr20 %gpr21 %gpr22 %gpr23 %gpr24 %gpr25 %gpr26 %gpr27 %gpr28 %gpr29 %gpr30 %gpr31 %nip %msr %ctr %link %xer %ccr %softe %trap %dar %dsisr' > kprobe_events >>> >>> And I get: >>> >>> bash-2057 [001] d... 535.433941: p_do_fork_0: (do_fork+0x8/0x490) arg1=0xc0000000000094d0 arg2=0xc0000001fbe9be30 arg3=0xc000000001133bb8 arg4=0x1200011 arg5=0x0 arg6=0x0 arg7=0x0 arg8=0x3fff7c885940 arg9=0x1 arg10=0xc0000001fbe9bea0 arg11=0x0 arg12=0xc01 arg13=0xc0000000000094c8 arg14=0xc00000000fdc0480 arg15=0x0 arg16=0x22000000 arg17=0x1016d6e8 arg18=0x0 arg19=0x44000000 arg20=0x0 arg21=0x10037c82208 arg22=0x1017b008 arg23=0x10143d18 arg24=0x10178854 arg25=0x10144f90 arg26=0x10037c821e8 arg27=0x0 arg28=0x0 arg29=0x0 arg30=0x0 arg31=0x809 arg32=0x3ffff788c010 arg33=0xc0000000000a7fe8 arg34=0x8000000000029033 arg35=0xc0000000000094c8 arg36=0xc0000000000094d0 arg37=0x0 arg38=0x42222844 arg39=0x1 arg40=0x700 arg41=0xc0000001fbe9bd50 arg42=0xc0000001fbe9bd30 >>> >>> Which is ugly as hell, but appears unchanged since before your patch. >>> >> >> Excellent. Many thanks. > > No worries. > > Did I already send you an ack? Have another one in case: > > Acked-by: Michael Ellerman <mpe@ellerman.id.au> > > Thanks. >>> I take it it's expected that the names are not decoded in the output? >> >> Yes. > > In fact I don't see anywhere that uses the reverse decoding, ie. > regs_query_register_name(). > Neither did I. I assumed it was intended to support either future kernel code or custom debug modules. > cheers > > -dl -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 06/19/15 12:58, Kees Cook wrote: > On Fri, Jun 19, 2015 at 7:12 AM, David Long <dave.long@linaro.org> wrote: >> On 06/19/15 00:19, Michael Ellerman wrote: >>> >>> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote: >>>> >>>> From: "David A. Long" <dave.long@linaro.org> >>>> >>>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API >>>> feature and has identical definitions in four different arch ptrace.h >>>> include files. It seems unlikely that definition would ever need to be >>>> changed regardless of architecture so lets move it into >>>> include/linux/ptrace.h. >>>> >>>> Signed-off-by: David A. Long <dave.long@linaro.org> >>>> --- >>>> arch/powerpc/kernel/ptrace.c | 5 ----- >>> >>> >>> Built and booted on powerpc, but is there an easy way to actually test the >>> code >>> paths in question? >>> >> >> There is an easy way to "smoke test" it on all archiectures that also >> implement kprobes (which powerpc does). If I'm understanding the powerpc >> code correctly (WRT register naming conventions) just do the following: >> >> cd /sys/kernel/debug/tracing >> echo 'p do_fork %gpr0' > kprobe_events >> echo 1 > events/kprobes/enable >> ls >> cat trace >> echo 0 > events/kprobes/enable >> >> Every fork() call done on the system between those two echo commands (hence >> the "ls") should append a line to the trace file. For a more exhaustive >> test one could repeat this sequence for every register in the architecture. >> >> This should work the same on all architectures supporting kprobes. You just >> have to use the appropriate register names for your architecture after the >> "%". > > Is this something we could codify into the selftests directory? It > seems like a great thing to capture in a single place somewhere (the > register lists, that is). > e > -Kees > Due to the architecture-specific naming of registers this would have to be added to the architecture subdirectories. I only see powerpc and x86 subdirs at this time so extending that infrastructure would have to be part of this. Verifying the register contents would also require some change to the kernel, possibly a simple test module, which would have to be unique to each architecture. Without that we could only check for recognition of the register name, although maybe that's good enough. >> >>> Acked-by: Michael Ellerman <mpe@ellerman.id.au> >>> >>> cheers >>> >>> >> >> Thanks, >> -dl >> -dl -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 06/29/15 23:29, Michael Ellerman wrote: > On Wed, 2015-06-17 at 14:30 -0400, David Long wrote: >> On 06/16/15 09:17, Rob Herring wrote: >>> On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote: >>>> >>>> #define REG_OFFSET_NAME(r) \ >>>> {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)} >>>> #define REG_OFFSET_END {.name = NULL, .offset = 0} >>> >>> Can't you also move these? ARM is complicated with the "ARM_" >>> prefixing, but the others appear to be the same. Maybe you can remove >>> the prefix or redefine the macro for ARM. >> >> That would mandate that all the architecture-specific pt_regs structures >> would have to use a top-level named field for each named register. > > Why does it mandate that? > > See eg. powerpc where we use REG_OFFSET_NAME for the top-level named fields and > then a different macro for the array elements: > > #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)} > #define GPR_OFFSET_NAME(num) \ > {.name = STR(gpr##num), .offset = offsetof(struct pt_regs, gpr[num])} > > static const struct pt_regs_offset regoffset_table[] = { > GPR_OFFSET_NAME(0), > GPR_OFFSET_NAME(1), > GPR_OFFSET_NAME(2), > GPR_OFFSET_NAME(3), > ... > REG_OFFSET_NAME(nip), > REG_OFFSET_NAME(msr), > > > So I don't see why REG_OFFSET_NAME couldn't be common. > Sorry for the delay in responding to this. OK, so you're saying architectures that don't want this constraint can make their own macro. Seems to make this whole exercise slightly less useful, but whatever. I see three ways to go here: 1) Leave it as is. 2) Force all architectures to use a common definition. 3) Provide a common definition that all architectures (except "arm") currently using this functionality will use. I have a v2 patch to implement #3, ready to post. Do we think this is the way to go? I don't like #2 because I really don't want to rename all uses of the current register fields for arm since this is architecture-specific code to begin with and since it affects code in 39 arm source files. > cheers > Thanks, -dl -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 07/22/15 01:11, Michael Ellerman wrote: > On Wed, 2015-07-22 at 00:46 -0400, David Long wrote: >> On 06/29/15 23:29, Michael Ellerman wrote: >>> On Wed, 2015-06-17 at 14:30 -0400, David Long wrote: >>>> On 06/16/15 09:17, Rob Herring wrote: >>>>> On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote: >>>>>> >>>>>> #define REG_OFFSET_NAME(r) \ >>>>>> {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)} >>>>>> #define REG_OFFSET_END {.name = NULL, .offset = 0} >>>>> >>>>> Can't you also move these? ARM is complicated with the "ARM_" >>>>> prefixing, but the others appear to be the same. Maybe you can remove >>>>> the prefix or redefine the macro for ARM. >>>> >>>> That would mandate that all the architecture-specific pt_regs structures >>>> would have to use a top-level named field for each named register. >>> >>> Why does it mandate that? >>> >>> See eg. powerpc where we use REG_OFFSET_NAME for the top-level named fields and >>> then a different macro for the array elements: >>> >>> #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)} >>> #define GPR_OFFSET_NAME(num) \ >>> {.name = STR(gpr##num), .offset = offsetof(struct pt_regs, gpr[num])} >>> >>> static const struct pt_regs_offset regoffset_table[] = { >>> GPR_OFFSET_NAME(0), >>> GPR_OFFSET_NAME(1), >>> GPR_OFFSET_NAME(2), >>> GPR_OFFSET_NAME(3), >>> ... >>> REG_OFFSET_NAME(nip), >>> REG_OFFSET_NAME(msr), >>> >>> >>> So I don't see why REG_OFFSET_NAME couldn't be common. >>> >> >> Sorry for the delay in responding to this. >> >> OK, so you're saying architectures that don't want this constraint can >> make their own macro. Seems to make this whole exercise slightly less >> useful, but whatever. > > Well yeah. > > In fact of the 4 arches that use REG_OFFSET_NAME, 2 already have another macro > for specially named registers (powerpc & sh). > >> I see three ways to go here: >> >> 1) Leave it as is. >> 2) Force all architectures to use a common definition. >> 3) Provide a common definition that all architectures (except "arm") >> currently using this functionality will use. >> >> I have a v2 patch to implement #3, ready to post. Do we think this is >> the way to go? > > Yeah I think it is. How are you making it conditional? Just #ifndef REG_OFFSET_NAME? > I'm just defining a new macro for arm. The macro is only invoked in one arm file. Then the REG_OFFSET_NAME macro goes unused for this architecture. >> I don't like #2 because I really don't want to rename all >> uses of the current register fields for arm since this is >> architecture-specific code to begin with and since it affects code in 39 >> arm source files. > > I guess you're talking about renaming all the ARM_x regs to x. That would > likely cause problems because they're implemented as #defines, > eg. #define r0 uregs[0] would probably confuse your assembler. > Yeah, and I had not looked further to the implications of doing that but I see you've found where it is a genuine problem. > The clean thing to do would be to have the in-kernel struct pt_regs have actual > named members, but that would still be an intrusive change. > > cheers > > Thanks, -dl -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c index ef9119f..fb45cf1 100644 --- a/arch/arm/kernel/ptrace.c +++ b/arch/arm/kernel/ptrace.c @@ -59,11 +59,6 @@ #define BREAKINST_THUMB 0xde01 #endif -struct pt_regs_offset { - const char *name; - int offset; -}; - #define REG_OFFSET_NAME(r) \ {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)} #define REG_OFFSET_END {.name = NULL, .offset = 0} diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index f21897b..ab81733 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -52,11 +52,6 @@ #define PARAMETER_SAVE_AREA_OFFSET 48 /* bytes */ #endif -struct pt_regs_offset { - const char *name; - int offset; -}; - #define STR(s) #s /* convert to string */ #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)} #define GPR_OFFSET_NAME(num) \ diff --git a/arch/sh/include/asm/ptrace.h b/arch/sh/include/asm/ptrace.h index 2506c7d..677c72b 100644 --- a/arch/sh/include/asm/ptrace.h +++ b/arch/sh/include/asm/ptrace.h @@ -23,11 +23,6 @@ /* * kprobe-based event tracer support */ -struct pt_regs_offset { - const char *name; - int offset; -}; - #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)} #define REGS_OFFSET_NAME(num) \ {.name = __stringify(r##num), .offset = offsetof(struct pt_regs, regs[num])} diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index a7bc794..f135d35 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -53,11 +53,6 @@ enum x86_regset { REGSET_IOPERM32, }; -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} diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index 987a73a..b0b1ee6 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -383,4 +383,13 @@ extern int task_current_syscall(struct task_struct *target, long *callno, unsigned long args[6], unsigned int maxargs, unsigned long *sp, unsigned long *pc); +#ifdef CONFIG_HAVE_REGS_AND_STACK_ACCESS_API + +struct pt_regs_offset { + const char *name; + int offset; +}; + +#endif /* CONFIG_HAVE_REGS_AND_STACK_ACCESS_API */ + #endif