Re: [PATCH] x86: Use v8086_mode helper, trivial unification

Harvey Harrison
2008-01-18T01:16:04Z

Roland actually put on CC this time.

On Thu, 2008-01-17 at 19:59 -0500, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> >
> > Sorry, missed that detail in ptrace.h, I notice now.
> >
> > Is there some better way this could be organized, would the following
> > be an improvement, as opposed to two long ifdef sections?
> >
> > Patch will follow if you think it's a good idea.
>
> It is actually quite a bit easier to read.

I'll send along a patch along soon, any thoughts on how to order it in
the file?

>
> >
> > static inline unsigned long stack_pointer(struct pt_regs *regs)
> > {
> > #ifdef CONFIG_X86_32
> > return (unsigned long)regs;
> > #else
> > return regs->sp;
> > #endif
> > }
>
> This one is kind of strange. In particular, the 32-bit definition isn't
> exactly what one would expect. It makes me concerned that it actually
> refers to two different kinds of stack pointers?

This tripped up the kprobes unification as well, see the stack_addr()
helper that was introduced there. Would be good to figure this out
and put a big fat comment on it.

kprobes.c

#ifdef CONFIG_X86_64
#define stack_addr(regs) ((unsigned long *)regs->sp)
#else
/*
* "&regs->sp" looks wrong, but it's correct for x86_32. x86_32 CPUs
* don't save the ss and esp registers if the CPU is already in kernel
* mode when it traps. So for kprobes, regs->sp and regs->ss are not
* the [nonexistent] saved stack pointer and ss register, but rather
* the top 8 bytes of the pre-int3 stack. So &regs->sp happens to
* point to the top of the pre-int3 stack.
*/
#define stack_addr(regs) ((unsigned long *)&regs->sp)
#endif

> > /* still need a define here, as one is long and one is unsigned long.
> > * but this is another target for unification I guess. */
> > #define regs_return_value(regs) ((regs)->ax)
>
> Indeed...

I think this comes out of Roland's patches unifying some names eip/rip,
eax/rax, etc.

CC'd in case he felt like more work ;-)

Harvey