This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access APIs
- From: Christoph Hellwig <hch at infradead dot org>
- To: Masami Hiramatsu <mhiramat at redhat dot com>
- Cc: Ingo Molnar <mingo at elte dot hu>, Steven Rostedt <rostedt at goodmis dot org>, lkml <linux-kernel at vger dot kernel dot org>, systemtap <systemtap at sources dot redhat dot com>, kvm <kvm at vger dot kernel dot org>, DLE <dle-develop at lists dot sourceforge dot net>, Ananth N Mavinakayanahalli <ananth at in dot ibm dot com>, Frederic Weisbecker <fweisbec at gmail dot com>, Roland McGrath <roland at redhat dot com>, linux-arch at vger dot kernel dot org
- Date: Sat, 30 May 2009 04:13:03 -0400
- Subject: Re: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access APIs
- References: <20090529000326.17532.70868.stgit@localhost.localdomain> <20090529000347.17532.34038.stgit@localhost.localdomain>
You might want to run this past linux-arch to make sure this is suitable
for other architectures.
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -7,6 +7,7 @@
>
> #ifdef __KERNEL__
> #include <asm/segment.h>
> +#include <asm/page_types.h>
> #endif
I really wonder if we should split asm/ptrace.h into one file
just defining pt_regs both for userspace and the kernel, and one with
all kinds of register access helpers and maybe another one for the
ptrace architecture interface.
Unforuntately we would have to keep the ptrace.h name for the one
carrying pt_regs as it's exposed to userland.
> +static inline unsigned long get_register(struct pt_regs *regs, unsigned offset)
> +{
I woner if all these names aren't a bit generic. Shoud we maybe add a
regs_ prefix to make it clear it operates on a pt_regs register set?
Also some kerneldoc documentation for all these helpers would be nice.
> +/* Get Nth argument at function call */
> +static inline unsigned long get_argument_nth(struct pt_regs *regs, unsigned n)
> +{
> +#ifdef CONFIG_X86_32
> +#define NR_REGPARMS 3
I think completely separate version for 32 vs 64 bit for this one would
be cleaner.
> + if (n < NR_REGPARMS) {
> + switch (n) {
> + case 0: return regs->ax;
> + case 1: return regs->dx;
> + case 2: return regs->cx;
> + }
Normal kernel style would be
switch (n) {
case 0:
return regs->ax;
case 1:
return regs->dx;
case 2:
return regs->cx;
}
> +#define REG_OFFSET(r) offsetof(struct pt_regs, r)
> +#define REG_OFFSET_NAME(r) {.name = #r, .offset = REG_OFFSET(r)}
> +#define REG_OFFSET_END {.name = NULL, .offset = 0}
At least the REG_OFFSET macro seems superflous to me.