lkml.org 
[lkml]   [2009]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access APIs
    Hi Christoph,

    Thank you for review.

    Christoph Hellwig wrote:
    > You might want to run this past linux-arch to make sure this is suitable
    > for other architectures.

    Frankly, I'm not sure about linux-arch, could you explain it?
    Anyway, I'm interested in that idea.

    >> --- 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.

    Agreed, pt_regs is used more broadly than ptrace itself in kernel.

    > Unforuntately we would have to keep the ptrace.h name for the one
    > carrying pt_regs as it's exposed to userland.

    Perhaps, we should split pt_regs from ptrace.h, like as ptrace-regs.h.

    >> +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?

    Indeed.

    > Also some kerneldoc documentation for all these helpers would be nice.

    Sure.

    >> +/* 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.

    Agreed,

    >
    >> + 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;
    > }

    Oops, thanks,

    >
    >> +#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.
    >

    Exactly.

    Thank you again!

    --
    Masami Hiramatsu

    Software Engineer
    Hitachi Computer Products (America) Inc.
    Software Solutions Division

    e-mail: mhiramat@redhat.com



    \
     
     \ /
      Last update: 2009-05-30 16:51    [W:4.097 / U:0.316 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site