Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 14 Sep 2016 12:57:40 +0000 (UTC)
From:      Bruce Evans <bde@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r305807 - in head/sys: amd64/amd64 i386/i386 x86/include
Message-ID:  <201609141257.u8ECveAs020350@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: bde
Date: Wed Sep 14 12:57:40 2016
New Revision: 305807
URL: https://svnweb.freebsd.org/changeset/base/305807

Log:
  Use the MI macro TRAPF_USERMODE() instead of open-coded checks for
  SEL_UPL and sometimes PSL_VM.  This is just a style change on amd64,
  but on i386 it fixes 1 unimportant place where the PSL_VM check was
  missing and starts fixing 1 important place where the PSL_VM check
  had a logic error.
  
  Fix logic errors in treating vm86 bioscall mode as kernel mode.  The
  main place checked all the necessary flags, but put the necessary
  parentheses for the PSL_VM and PCB_VM86CALL checks in the wrong
  place.  The broken case is only reached if a vm86 bioscall uses a
  %cs which is nonzero mod 4, but that is unusual -- most bios calls
  start with %cs = 0xc000 or 0xf000 and rarely change it.  Another
  place was missing the check for PCB_VM86CALL, but was only reachable
  if there are bugs virtualizing PSL_I.
  
  Add a macro TF_HAS_STACKREGS() and use this instead of converting
  open-coded checks of SEL_UPL, etc. to TRAPF_USERMODE() when we only
  care about whether the frame has stack registers.  This fixes 3
  places in my recent fix for register variables in vm86 mode where I
  messed up the PSL_VM check and cleans up other places.

Modified:
  head/sys/amd64/amd64/trap.c
  head/sys/i386/i386/db_trace.c
  head/sys/i386/i386/trap.c
  head/sys/x86/include/frame.h

Modified: head/sys/amd64/amd64/trap.c
==============================================================================
--- head/sys/amd64/amd64/trap.c	Wed Sep 14 12:07:34 2016	(r305806)
+++ head/sys/amd64/amd64/trap.c	Wed Sep 14 12:57:40 2016	(r305807)
@@ -236,7 +236,7 @@ trap(struct trapframe *frame)
 		 * interrupts disabled until they are accidentally
 		 * enabled later.
 		 */
-		if (ISPL(frame->tf_cs) == SEL_UPL)
+		if (TRAPF_USERMODE(frame))
 			uprintf(
 			    "pid %ld (%s): trap %d with interrupts disabled\n",
 			    (long)curproc->p_pid, curthread->td_name, type);
@@ -260,7 +260,7 @@ trap(struct trapframe *frame)
 
 	code = frame->tf_err;
 
-        if (ISPL(frame->tf_cs) == SEL_UPL) {
+	if (TRAPF_USERMODE(frame)) {
 		/* user trap */
 
 		td->td_pticks = 0;
@@ -787,7 +787,7 @@ trap_fatal(frame, eva)
 	else
 		msg = "UNKNOWN";
 	printf("\n\nFatal trap %d: %s while in %s mode\n", type, msg,
-	    ISPL(frame->tf_cs) == SEL_UPL ? "user" : "kernel");
+	    TRAPF_USERMODE(frame) ? "user" : "kernel");
 #ifdef SMP
 	/* two separate prints in case of a trap on an unmapped page */
 	printf("cpuid = %d; ", PCPU_GET(cpuid));
@@ -804,7 +804,7 @@ trap_fatal(frame, eva)
 	}
 	printf("instruction pointer	= 0x%lx:0x%lx\n",
 	       frame->tf_cs & 0xffff, frame->tf_rip);
-        if (ISPL(frame->tf_cs) == SEL_UPL) {
+	if (TF_HAS_STACKREGS(frame)) {
 		ss = frame->tf_ss & 0xffff;
 		esp = frame->tf_rsp;
 	} else {
@@ -934,7 +934,7 @@ amd64_syscall(struct thread *td, int tra
 	ksiginfo_t ksi;
 
 #ifdef DIAGNOSTIC
-	if (ISPL(td->td_frame->tf_cs) != SEL_UPL) {
+	if (!TRAPF_USERMODE(frame)) {
 		panic("syscall");
 		/* NOT REACHED */
 	}

Modified: head/sys/i386/i386/db_trace.c
==============================================================================
--- head/sys/i386/i386/db_trace.c	Wed Sep 14 12:07:34 2016	(r305806)
+++ head/sys/i386/i386/db_trace.c	Wed Sep 14 12:57:40 2016	(r305807)
@@ -82,8 +82,7 @@ struct db_variable *db_eregs = db_regs +
 static __inline int
 get_esp(struct trapframe *tf)
 {
-	return ((ISPL(tf->tf_cs) || kdb_frame->tf_eflags & PSL_VM) ?
-	    tf->tf_esp : (intptr_t)&tf->tf_esp);
+	return (TF_HAS_STACKREGS(tf) ? tf->tf_esp : (intptr_t)&tf->tf_esp);
 }
 
 static int
@@ -147,7 +146,7 @@ db_esp(struct db_variable *vp, db_expr_t
 
 	if (op == DB_VAR_GET)
 		*valuep = get_esp(kdb_frame);
-	else if (ISPL(kdb_frame->tf_cs))
+	else if (TF_HAS_STACKREGS(kdb_frame))
 		kdb_frame->tf_esp = *valuep;
 	return (1);
 }
@@ -180,9 +179,9 @@ db_ss(struct db_variable *vp, db_expr_t 
 		return (0);
 
 	if (op == DB_VAR_GET)
-		*valuep = (ISPL(kdb_frame->tf_cs) ||
-		    kdb_frame->tf_eflags & PSL_VM) ? kdb_frame->tf_ss : rss();
-	else if (ISPL(kdb_frame->tf_cs) || kdb_frame->tf_eflags & PSL_VM)
+		*valuep = TF_HAS_STACKREGS(kdb_frame) ? kdb_frame->tf_ss :
+		    rss();
+	else if (TF_HAS_STACKREGS(kdb_frame))
 		kdb_frame->tf_ss = *valuep;
 	return (1);
 }
@@ -439,7 +438,7 @@ db_backtrace(struct thread *td, struct t
 		 * Find where the trap frame actually ends.
 		 * It won't contain tf_esp or tf_ss unless crossing rings.
 		 */
-		if (ISPL(kdb_frame->tf_cs))
+		if (TF_HAS_STACKREGS(kdb_frame))
 			instr = (int)(kdb_frame + 1);
 		else
 			instr = (int)&kdb_frame->tf_esp;

Modified: head/sys/i386/i386/trap.c
==============================================================================
--- head/sys/i386/i386/trap.c	Wed Sep 14 12:07:34 2016	(r305806)
+++ head/sys/i386/i386/trap.c	Wed Sep 14 12:57:40 2016	(r305807)
@@ -267,7 +267,8 @@ trap(struct trapframe *frame)
 		 * interrupts disabled until they are accidentally
 		 * enabled later.
 		 */
-		if (ISPL(frame->tf_cs) == SEL_UPL || (frame->tf_eflags & PSL_VM))
+		if (TRAPF_USERMODE(frame) &&
+		    (curpcb->pcb_flags & PCB_VM86CALL) == 0)
 			uprintf(
 			    "pid %ld (%s): trap %d with interrupts disabled\n",
 			    (long)curproc->p_pid, curthread->td_name, type);
@@ -307,9 +308,7 @@ trap(struct trapframe *frame)
 			enable_intr();
 	}
 
-        if ((ISPL(frame->tf_cs) == SEL_UPL) ||
-	    ((frame->tf_eflags & PSL_VM) && 
-		!(curpcb->pcb_flags & PCB_VM86CALL))) {
+        if (TRAPF_USERMODE(frame) && (curpcb->pcb_flags & PCB_VM86CALL) == 0) {
 		/* user trap */
 
 		td->td_pticks = 0;
@@ -963,7 +962,7 @@ trap_fatal(frame, eva)
 	}
 	printf("instruction pointer	= 0x%x:0x%x\n",
 	       frame->tf_cs & 0xffff, frame->tf_eip);
-        if ((ISPL(frame->tf_cs) == SEL_UPL) || (frame->tf_eflags & PSL_VM)) {
+        if (TF_HAS_STACKREGS(frame)) {
 		ss = frame->tf_ss & 0xffff;
 		esp = frame->tf_esp;
 	} else {
@@ -1117,7 +1116,8 @@ syscall(struct trapframe *frame)
 	ksiginfo_t ksi;
 
 #ifdef DIAGNOSTIC
-	if (ISPL(frame->tf_cs) != SEL_UPL) {
+	if (!(TRAPF_USERMODE(frame) &&
+	    (curpcb->pcb_flags & PCB_VM86CALL) == 0)) {
 		panic("syscall");
 		/* NOT REACHED */
 	}

Modified: head/sys/x86/include/frame.h
==============================================================================
--- head/sys/x86/include/frame.h	Wed Sep 14 12:07:34 2016	(r305806)
+++ head/sys/x86/include/frame.h	Wed Sep 14 12:57:40 2016	(r305807)
@@ -64,7 +64,7 @@ struct trapframe {
 	int	tf_eip;
 	int	tf_cs;
 	int	tf_eflags;
-	/* below only when crossing rings (e.g. user to kernel) */
+	/* below only when crossing rings (user to kernel) */
 	int	tf_esp;
 	int	tf_ss;
 };
@@ -89,10 +89,10 @@ struct trapframe_vm86 {
 	int	tf_eip;
 	int	tf_cs;
 	int	tf_eflags;
-	/* below only when crossing rings (e.g. user to kernel) */
+	/* below only when crossing rings (user (including vm86) to kernel) */
 	int	tf_esp;
 	int	tf_ss;
-	/* below only when switching out of VM86 mode */
+	/* below only when crossing from vm86 mode to kernel */
 	int	tf_vm86_es;
 	int	tf_vm86_ds;
 	int	tf_vm86_fs;
@@ -136,6 +136,7 @@ struct trapframe {
 	register_t	tf_rip;
 	register_t	tf_cs;
 	register_t	tf_rflags;
+	/* below only when crossing rings (user to kernel) */
 	register_t	tf_rsp;
 	register_t	tf_ss;
 };
@@ -145,4 +146,13 @@ struct trapframe {
 #define	TF_HASFPXSTATE	0x4
 #endif /* __amd64__ */
 
+/*
+ * This alias for the MI TRAPF_USERMODE() should be used when we don't
+ * care about user mode itself, but need to know if a frame has stack
+ * registers.  The difference is only logical, but on i386 the logic
+ * for using TRAPF_USERMODE() is complicated by sometimes treating vm86
+ * bioscall mode (which is a special ring 3 user mode) as kernel mode.
+ */
+#define	TF_HAS_STACKREGS(tf)	TRAPF_USERMODE(tf)
+
 #endif /* _MACHINE_FRAME_H_ */



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201609141257.u8ECveAs020350>