x86: Use v8086_mode helper, trivial unification

Use v8086_mode inline in fault_32.c, no functional change also ifdef the section for 32-bit only and add to fault_64.c

---
 arch/x86/mm/fault_32.c |    4 +++-
 arch/x86/mm/fault_64.c |   11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletions(-)

index 77915ac..6d8a0bb 100644
@@ -547,14 +547,16 @@ good_area:
 	else
 		tsk->min_flt++;
 
+#ifdef CONFIG_X86_32
 	/*
 	 * Did it hit the DOS screen memory VA from vm86 mode?
 	 */
-	if (regs->flags & VM_MASK) {
+	if (v8086_mode(regs)) {
 		unsigned long bit = (address - 0xA0000) >> PAGE_SHIFT;
 		if (bit < 32)
 			tsk->thread.screen_bitmap |= 1 << bit;
 	}
+#endif
 	up_read(&mm->mmap_sem);
 	return;
 

index 84531d8..8393275 100644
@@ -551,6 +551,17 @@ good_area:
 		tsk->maj_flt++;
 	else
 		tsk->min_flt++;
+
+#ifdef CONFIG_X86_32
+	/*
+	 * Did it hit the DOS screen memory VA from vm86 mode?
+	 */
+	if (v8086_mode(regs)) {
+		unsigned long bit = (address - 0xA0000) >> PAGE_SHIFT;
+		if (bit < 32)
+			tsk->thread.screen_bitmap |= 1 << bit;
+	}
+#endif
 	up_read(&mm->mmap_sem);
 	return;
 

1.5.4.rc2.1164.g6451

The #ifdef is unnecessary, since v8086_mode() is now a constant zero on x86-64. gcc will remove the if clause.

-hpa

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.

static inline int user_mode(struct pt_regs *regs)
{

#ifdef CONFIG_X86_32
	return (regs->cs & SEGMENT_RPL_MASK) == USER_RPL;
#else
	return !!(regs->cs & 3);

#endif
}

static inline int user_mode_vm(struct pt_regs *regs) {

#ifdef CONFIG_X86_32
	return ((regs->cs & SEGMENT_RPL_MASK) |
		(regs->flags & VM_MASK)) >= USER_RPL;
#else
	return user_mode(regs);

#endif
}

static inline int v8086_mode(struct pt_regs *regs) {

#ifdef CONFIG_X86_32
	return (regs->flags & VM_MASK);
#else
	return 0;

#endif
}

/* OK, maybe this should just be deleted, and use
regs->ip directly in code*/
static inline unsigned long instruction_pointer(struct pt_regs *regs)

{
	return regs->ip;
}

/* OK, maybe this should just be deleted, and use regs->bp directly in code*/
static inline unsigned long frame_pointer(struct pt_regs *regs)

{
	return regs->bp;
}

static inline unsigned long stack_pointer(struct pt_regs *regs) {

#ifdef CONFIG_X86_32
	return (unsigned long)regs;
#else
	return 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)

Harvey

It is actually quite a bit easier to read.

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?

Indeed...

-hpa

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

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

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

Funny, I thought those were mine :-/

-hpa

*Ahem* my apologies, they are in the x86.git lineup right before the ptrace work Roland did, guess it's time to get a bigger gitk font.

My bad,

Harvey

Roland actually put on CC this time.

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

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

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

It's indeed true that &pt_regs is truly the esp value for x86-32 kernel-mode trap frames. Because this nonobvious calculation is only right for a kernel mode pt_regs and not for a user-mode one, I think it would be better to use a name for the inline/macro that makes this quite clear, rather than one so generic as "stack_addr".

That was hpa, not me. Harmonizing the signedness of the struct members does seem to me like a reasonable and deobfuscatory thing to do. But I don't plan to get involved in doing it.

Thanks,
Roland

Indeed. This was certainly highly nonobvious in the current code.

-hpa

What do you think of kernel_sp()

Harvey

What do you think of:

/*
* (unsigned long)regs looks strange, 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 &regs happens to be esp. Valid only for kernel-mode
* pt_regs.
*/
static inline unsigned long stack_pointer(struct pt_regs *regs)
{

#ifdef CONFIG_X86_32
	return (unsigned long)regs;
#else
	return regs->sp;

#endif
}

Harvey

Please highlight the last sentence better. That's a user restriction and a highly important one.

-hpa

Choose a less generic name for such a special case. Add a comment explaining the odd use in X86_32.

Change the one user of stack_pointer.

---
 arch/x86/oprofile/backtrace.c |    2 +-
 include/asm-x86/ptrace.h      |    8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

index 0ca4815..e2095cb 100644
@@ -77,7 +77,7 @@ void
 x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct frame_head *head = (struct frame_head *)frame_pointer(regs);
-	unsigned long stack = stack_pointer(regs);
+	unsigned long stack = kernel_trap_sp(regs);
 
 	if (!user_mode_vm(regs)) {
 		if (depth)

index 79d5b8f..d9e04b4 100644
@@ -182,7 +182,13 @@ static inline int v8086_mode(struct pt_regs *regs)
 #endif
 }
 
-static inline unsigned long stack_pointer(struct pt_regs *regs)
+/*
+ * X86_32 CPUs don't save ss and esp if the CPU is already in kernel mode
+ * when it traps.  So regs will be the current sp.
+ *
+ * This is valid only for kernel mode traps.
+ */
+static inline unsigned long kernel_trap_sp(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
 	return (unsigned long)regs;

1.5.4.rc2.1164.g6451