Port-i386 archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[patches 2/2] struct switchframe/pcb improvements
Hi all,
Here is the second round of patches. (For context, see
<http://mail-index.netbsd.org/port-i386/2014/01/09/msg003212.html>.)
These patches should apply cleanly to NetBSD-current.
Summary of the patches:
8. i386: add %ebp to struct switchframe
9. i386: remove pcb_ebp member from struct pcb
10. i386: allow backtrace beyond Xsoftintr()
11. i386: include the softint level in the debug backtrace
Patch #8 will have a negative performance impact because more work
will be done for each context switch. I'm not sure how significant
the impact is; I did not take any measurements.
I believe patch #9 will offset the performance impact of patch #8,
resulting in no net loss of performance. But again, I did not take
any measurements so I'm not sure.
Patches #8 and #9 rearrange the memory used to manage LWPs, which will
affect any debugging tools designed to examine the memory. I'm
concerned about breaking compatibility with such tools. The patches
modify the 'gdb' and 'crash' utilities to cope with the rearranged
memory changes, but I did not test the utilities to see if they still
work.
I'd appreciate any feedback, especially recommendations for testing
these changes more thoroughly to gain confidence in their correctness
and completeness. I backported the patches to NetBSD 6.1 and ran the
atf tests, but I have not tried running the atf tests with the patches
applied to NetBSD-current.
Thanks,
Richard
======================================================================
commit 13aea385a0d95a68b2dcbdae52c3e5f3af59257d
Author: Richard Hansen <rhansen%bbn.com@localhost>
Date: Thu Sep 26 06:23:34 2013 +0000
i386: add %ebp to struct switchframe
Add a new sf_ebp member to struct switchframe for holding %ebp.
Adjust dumpsys(), cpu_switchto(), Xsoftintr(), and cpu_setfunc() to
match. Now these functions set up a new frame when called, which will
make it possible to get a more complete backtrace after a panic().
This change may affect tools that examine LWPs in kernel memory dumps.
If the tool does not utilize struct switchframe or was built against
an older version of <machine/frame.h>, the tool may be unable to
correctly parse crash dump files (or /dev/kmem).
diff --git a/src/external/gpl3/gdb/dist/gdb/i386nbsd-nat.c
b/src/external/gpl3/gdb/dist/gdb/i386nbsd-nat.c
index 9593e9e..0c07c4e 100644
--- a/src/external/gpl3/gdb/dist/gdb/i386nbsd-nat.c
+++ b/src/external/gpl3/gdb/dist/gdb/i386nbsd-nat.c
@@ -51,11 +51,11 @@ i386nbsd_supply_pcb (struct regcache *regcache, struct pcb
*pcb)
{
struct switchframe sf;
- /* The following is true for NetBSD 1.6.2 and after:
+ /* The following is true for NetBSD 1.6.2 to 6.99.28:
The pcb contains %esp and %ebp at the point of the context switch
in cpu_switch()/cpu_switchto(). At that point we have a stack frame as
- described by `struct switchframe', which for NetBSD (2.0 and later) has
+ described by `struct switchframe', which for NetBSD (2.0 to 6.99.28) has
the following layout:
%edi
@@ -69,7 +69,17 @@ i386nbsd_supply_pcb (struct regcache *regcache, struct pcb
*pcb)
For core dumps the pcb is saved by savectx()/dumpsys() and contains the
stack pointer and frame pointer. A new dumpsys() fakes a switchframe
whereas older code isn't reliable so use an iffy heuristic to detect this
- and use the frame pointer to recover enough state. */
+ and use the frame pointer to recover enough state.
+
+ NetBSD 6.99.29 added %ebp to `struct switchframe', resulting in
+ the following layout:
+
+ %edi
+ %esi
+ %ebx
+ %ebp
+ return address
+ */
/* The stack pointer shouldn't be zero. */
if (pcb->pcb_esp == 0)
diff --git a/src/sys/arch/i386/i386/locore.S b/src/sys/arch/i386/i386/locore.S
index fcc9d08..e6e1730 100644
--- a/src/sys/arch/i386/i386/locore.S
+++ b/src/sys/arch/i386/i386/locore.S
@@ -964,13 +964,16 @@ END(longjmp)
* Mimic cpu_switchto() for postmortem debugging.
*/
ENTRY(dumpsys)
- pushl %ebx # set up fake switchframe
- pushl %esi # and save context
+ pushl %ebp # set up fake switchframe
+ movl %esp,%ebp # and save context
+ pushl %ebx
+ pushl %esi
pushl %edi
movl %esp,_C_LABEL(dumppcb)+PCB_ESP
movl %ebp,_C_LABEL(dumppcb)+PCB_EBP
call _C_LABEL(dodumpsys) # dump!
addl $(3*4), %esp # unwind switchframe
+ popl %ebp
ret
END(dumpsys)
@@ -986,6 +989,8 @@ END(dumpsys)
* it for a new lwp.
*/
ENTRY(cpu_switchto)
+ pushl %ebp
+ movl %esp,%ebp
pushl %ebx
pushl %esi
pushl %edi
@@ -1001,9 +1006,9 @@ ENTRY(cpu_switchto)
0:
#endif
- movl 16(%esp),%esi # oldlwp
- movl 20(%esp),%edi # newlwp
- movl 24(%esp),%edx # returning
+ movl 8(%ebp),%esi # oldlwp
+ movl 12(%ebp),%edi # newlwp
+ movl 16(%ebp),%edx # returning
testl %esi,%esi
jz 1f
@@ -1101,6 +1106,7 @@ ENTRY(cpu_switchto)
popl %edi
popl %esi
popl %ebx
+ popl %ebp
ret
/* Check for restartable atomic sequences (RAS). */
diff --git a/src/sys/arch/i386/i386/spl.S b/src/sys/arch/i386/i386/spl.S
index c6f5b9f..d56797f 100644
--- a/src/sys/arch/i386/i386/spl.S
+++ b/src/sys/arch/i386/i386/spl.S
@@ -336,9 +336,42 @@ IDTVEC_END(doreti)
*
* %eax intrsource
* %esi address to return to
+ *
+ * If softint_dispatch() blocks, the call to softint_dispatch() will
+ * not return to this function. Instead, cpu_switchto() will be
+ * called, and cpu_switchto() will restore the stack (which is why the
+ * stack layout of this function and cpu_switchto() must agree) and
+ * jump to softintr_ret(). softintr_ret() will in turn return to
+ * %esi (as saved in the switchframe).
+ *
+ * The stack layout for this function is tightly coupled with the
+ * layout of 'struct switchframe'. The following are the important
+ * parts (the number preceding each line is the number of words
+ * relative to Xsoftintr()'s %ebp):
+ *
+ * +1 switchframe sf_eip: addr of softintr_ret() for cpu_switchto()
+ * 0 switchframe sf_ebp: previous frame's base pointer
+ * -1 switchframe sf_ebx: backup for local register
+ * -2 switchframe sf_esi: backup of address to return to
+ * -3 switchframe sf_edi: backup for local register
+ *
+ * Note that the sf_ebp member makes it possible to get a backtrace
+ * without any special work upon encountering Xsoftintr().
+ *
+ * If this stack layout is changed, the following must also change:
+ *
+ * - struct switchframe
+ * - any other function that depends on the layout of struct
+ * switchframe
+ *
+ * Note that softintr_ret() does not depend on the stack layout;
+ * cpu_switchto() takes care of restoring the stack before returning
+ * to softintr_ret().
*/
IDTVEC(softintr)
pushl $_C_LABEL(softintr_ret) /* set up struct switchframe */
+ pushl %ebp
+ movl %esp,%ebp
pushl %ebx
pushl %esi
pushl %edi
@@ -362,7 +395,9 @@ IDTVEC(softintr)
xchgl %esi,CPUVAR(CURLWP) /* must be globally visible */
popl %edi /* unwind switchframe */
popl %esi
- addl $8,%esp
+ addl $4,%esp
+ popl %ebp
+ addl $4,%esp
jmp *%esi /* back to splx/doreti */
IDTVEC_END(softintr)
diff --git a/src/sys/arch/i386/include/frame.h
b/src/sys/arch/i386/include/frame.h
index b4b99e1..874a4d9 100644
--- a/src/sys/arch/i386/include/frame.h
+++ b/src/sys/arch/i386/include/frame.h
@@ -136,11 +136,25 @@ struct intrframe {
/*
* Stack frame inside cpu_switchto()
+ *
+ * The sf_ebp member makes it possible to set up a standard call frame
+ * in cpu_switchto(), dumpsys(), Xsoftintr(), and Xspllower(), which
+ * makes it easier to get a nice backtrace after a panic().
+ *
+ * If the layout of this structure is changed, the following functions
+ * must also change:
+ *
+ * - cpu_switchto()
+ * - dumpsys()
+ * - Xsoftintr()
+ * - Xspllower()
+ * - cpu_setfunc()
*/
struct switchframe {
int sf_edi;
int sf_esi;
int sf_ebx;
+ int sf_ebp;
int sf_eip;
};
diff --git a/src/sys/arch/x86/x86/vm_machdep.c
b/src/sys/arch/x86/x86/vm_machdep.c
index 90e29de..ac2d793 100644
--- a/src/sys/arch/x86/x86/vm_machdep.c
+++ b/src/sys/arch/x86/x86/vm_machdep.c
@@ -235,6 +235,7 @@ cpu_lwp_fork(struct lwp *l1, struct lwp *l2, void *stack,
size_t stacksize,
*/
sf->sf_esi = (int)func;
sf->sf_ebx = (int)arg;
+ sf->sf_ebp = (int)l2;
sf->sf_eip = (int)lwp_trampoline;
pcb2->pcb_esp = (int)sf;
pcb2->pcb_ebp = (int)l2;
diff --git a/src/sys/sys/param.h b/src/sys/sys/param.h
index ee23338..0280d44 100644
--- a/src/sys/sys/param.h
+++ b/src/sys/sys/param.h
@@ -63,7 +63,7 @@
* 2.99.9 (299000900)
*/
-#define __NetBSD_Version__ 699002800 /* NetBSD 6.99.28 */
+#define __NetBSD_Version__ 699002900 /* NetBSD 6.99.29 */
#define __NetBSD_Prereq__(M,m,p) (((((M) * 100000000) + \
(m) * 1000000) + (p) * 100) <= __NetBSD_Version__)
commit 6b920b1893d33337eb86c45804686bb481a45385
Author: Richard Hansen <rhansen%bbn.com@localhost>
Date: Thu Oct 3 22:24:54 2013 +0000
i386: remove pcb_ebp member from struct pcb
Now that %ebp is stored in struct switchframe, remove it from struct
pcb.
This changes the layout of the process control block, which affects
tools that examine LWPs in crash dump files (or /dev/kmem). Adjust
the gdb and crash utilities to correctly parse the new memory layout.
diff --git a/src/external/gpl3/gdb/dist/gdb/i386nbsd-nat.c
b/src/external/gpl3/gdb/dist/gdb/i386nbsd-nat.c
index 0c07c4e..ddf7fa0 100644
--- a/src/external/gpl3/gdb/dist/gdb/i386nbsd-nat.c
+++ b/src/external/gpl3/gdb/dist/gdb/i386nbsd-nat.c
@@ -79,6 +79,8 @@ i386nbsd_supply_pcb (struct regcache *regcache, struct pcb
*pcb)
%ebx
%ebp
return address
+
+ NetBSD 6.99.30 removed %ebp from the pcb.
*/
/* The stack pointer shouldn't be zero. */
@@ -95,7 +97,7 @@ i386nbsd_supply_pcb (struct regcache *regcache, struct pcb
*pcb)
pcb->pcb_esp += sizeof (struct switchframe);
regcache_raw_supply (regcache, I386_EDI_REGNUM, &sf.sf_edi);
regcache_raw_supply (regcache, I386_ESI_REGNUM, &sf.sf_esi);
- regcache_raw_supply (regcache, I386_EBP_REGNUM, &pcb->pcb_ebp);
+ regcache_raw_supply (regcache, I386_EBP_REGNUM, &sf.sf_ebp);
regcache_raw_supply (regcache, I386_ESP_REGNUM, &pcb->pcb_esp);
regcache_raw_supply (regcache, I386_EBX_REGNUM, &sf.sf_ebx);
regcache_raw_supply (regcache, I386_EIP_REGNUM, &sf.sf_eip);
@@ -107,10 +109,10 @@ i386nbsd_supply_pcb (struct regcache *regcache, struct
pcb *pcb)
/* No, the pcb must have been last updated by savectx() in old
dumpsys(). Use the frame pointer to recover enough state. */
- read_memory (pcb->pcb_ebp, (gdb_byte *) &fp, sizeof(fp));
- read_memory (pcb->pcb_ebp + 4, (gdb_byte *) &pc, sizeof(pc));
+ read_memory (sf.sf_ebp, (gdb_byte *) &fp, sizeof(fp));
+ read_memory (sf.sf_ebp + 4, (gdb_byte *) &pc, sizeof(pc));
- regcache_raw_supply (regcache, I386_ESP_REGNUM, &pcb->pcb_ebp);
+ regcache_raw_supply (regcache, I386_ESP_REGNUM, &sf.sf_ebp);
regcache_raw_supply (regcache, I386_EBP_REGNUM, &fp);
regcache_raw_supply (regcache, I386_EIP_REGNUM, &pc);
}
diff --git a/src/sys/arch/i386/i386/genassym.cf
b/src/sys/arch/i386/i386/genassym.cf
index c762f61..dac7d1e 100644
--- a/src/sys/arch/i386/i386/genassym.cf
+++ b/src/sys/arch/i386/i386/genassym.cf
@@ -199,7 +199,6 @@ define IP6_SRC offsetof(struct
ip6_hdr, ip6_src)
define IP6_DST offsetof(struct ip6_hdr, ip6_dst)
define PCB_CR3 offsetof(struct pcb, pcb_cr3)
-define PCB_EBP offsetof(struct pcb, pcb_ebp)
define PCB_ESP offsetof(struct pcb, pcb_esp)
define PCB_ESP0 offsetof(struct pcb, pcb_esp0)
define PCB_CR0 offsetof(struct pcb, pcb_cr0)
diff --git a/src/sys/arch/i386/i386/locore.S b/src/sys/arch/i386/i386/locore.S
index e6e1730..31464d6 100644
--- a/src/sys/arch/i386/i386/locore.S
+++ b/src/sys/arch/i386/i386/locore.S
@@ -970,7 +970,6 @@ ENTRY(dumpsys)
pushl %esi
pushl %edi
movl %esp,_C_LABEL(dumppcb)+PCB_ESP
- movl %ebp,_C_LABEL(dumppcb)+PCB_EBP
call _C_LABEL(dodumpsys) # dump!
addl $(3*4), %esp # unwind switchframe
popl %ebp
@@ -1015,11 +1014,9 @@ ENTRY(cpu_switchto)
/* Save old context. */
movl L_PCB(%esi),%eax
movl %esp,PCB_ESP(%eax)
- movl %ebp,PCB_EBP(%eax)
/* Switch to newlwp's stack. */
1: movl L_PCB(%edi),%ebx
- movl PCB_EBP(%ebx),%ebp
movl PCB_ESP(%ebx),%esp
/*
@@ -1146,7 +1143,6 @@ END(cpu_switchto)
ENTRY(savectx)
movl 4(%esp),%edx # edx = pcb
movl %esp,PCB_ESP(%edx)
- movl %ebp,PCB_EBP(%edx)
ret
END(savectx)
diff --git a/src/sys/arch/i386/i386/mptramp.S b/src/sys/arch/i386/i386/mptramp.S
index d643648..10dcfd8 100644
--- a/src/sys/arch/i386/i386/mptramp.S
+++ b/src/sys/arch/i386/i386/mptramp.S
@@ -251,7 +251,6 @@ mp_cont:
HALTT(0x19, %esi)
movl PCB_ESP(%esi),%esp
- movl PCB_EBP(%esi),%ebp
HALT(0x20)
/* Switch address space. */
diff --git a/src/sys/arch/i386/i386/spl.S b/src/sys/arch/i386/i386/spl.S
index d56797f..002aa90 100644
--- a/src/sys/arch/i386/i386/spl.S
+++ b/src/sys/arch/i386/i386/spl.S
@@ -382,7 +382,6 @@ IDTVEC(softintr)
movl L_PCB(%edi),%edx
movl L_PCB(%esi),%ecx
movl %esp,PCB_ESP(%ecx)
- movl %ebp,PCB_EBP(%ecx)
movl PCB_ESP0(%edx),%esp /* onto new stack */
sti
pushl IS_MAXLEVEL(%eax) /* ipl to run at */
diff --git a/src/sys/arch/i386/include/pcb.h b/src/sys/arch/i386/include/pcb.h
index f3f9c47..e079116 100644
--- a/src/sys/arch/i386/include/pcb.h
+++ b/src/sys/arch/i386/include/pcb.h
@@ -84,7 +84,6 @@
struct pcb {
int pcb_esp0; /* ring0 esp */
int pcb_esp; /* kernel esp */
- int pcb_ebp; /* kernel ebp */
int pcb_unused; /* unused */
int pcb_cr0; /* saved image of CR0 */
int pcb_cr2; /* page fault address (CR2) */
diff --git a/src/sys/arch/x86/include/db_machdep.h
b/src/sys/arch/x86/include/db_machdep.h
index d302fc1..bac90c9 100644
--- a/src/sys/arch/x86/include/db_machdep.h
+++ b/src/sys/arch/x86/include/db_machdep.h
@@ -21,14 +21,12 @@ struct db_variable;
#define tf_sp tf_rsp
#define tf_ip tf_rip
#define tf_bp tf_rbp
-#define pcb_bp pcb_rbp
#define pcb_sp pcb_rsp
#define x86_frame x86_64_frame
#else
#define tf_sp tf_esp
#define tf_ip tf_eip
#define tf_bp tf_ebp
-#define pcb_bp pcb_ebp
#define pcb_sp pcb_esp
#define x86_frame i386_frame
#endif
diff --git a/src/sys/arch/x86/x86/db_trace.c b/src/sys/arch/x86/x86/db_trace.c
index 5b8e14f..c8ba951 100644
--- a/src/sys/arch/x86/x86/db_trace.c
+++ b/src/sys/arch/x86/x86/db_trace.c
@@ -169,7 +169,14 @@ db_stack_trace_print(db_expr_t addr, bool have_addr,
db_expr_t count,
else
#endif
{
- db_read_bytes((db_addr_t)&pcb->pcb_bp,
+#ifdef __x86_64__
+ db_addr_t frameaddr = (db_addr_t)&pcb->pcb_rbp;
+#else
+ db_addr_t frameaddr = (db_addr_t)
+ ((char *)pcb->pcb_sp
+ + offsetof(struct switchframe, sf_ebp));
+#endif
+ db_read_bytes(frameaddr,
sizeof(frame), (char *)&frame);
db_read_bytes((db_addr_t)(frame + 1),
sizeof(callpc), (char *)&callpc);
diff --git a/src/sys/arch/x86/x86/vm_machdep.c
b/src/sys/arch/x86/x86/vm_machdep.c
index ac2d793..13e8839 100644
--- a/src/sys/arch/x86/x86/vm_machdep.c
+++ b/src/sys/arch/x86/x86/vm_machdep.c
@@ -238,7 +238,6 @@ cpu_lwp_fork(struct lwp *l1, struct lwp *l2, void *stack,
size_t stacksize,
sf->sf_ebp = (int)l2;
sf->sf_eip = (int)lwp_trampoline;
pcb2->pcb_esp = (int)sf;
- pcb2->pcb_ebp = (int)l2;
#endif
}
diff --git a/src/sys/sys/param.h b/src/sys/sys/param.h
index 0280d44..ca65166 100644
--- a/src/sys/sys/param.h
+++ b/src/sys/sys/param.h
@@ -63,7 +63,7 @@
* 2.99.9 (299000900)
*/
-#define __NetBSD_Version__ 699002900 /* NetBSD 6.99.29 */
+#define __NetBSD_Version__ 699003000 /* NetBSD 6.99.30 */
#define __NetBSD_Prereq__(M,m,p) (((((M) * 100000000) + \
(m) * 1000000) + (p) * 100) <= __NetBSD_Version__)
diff --git a/src/usr.sbin/crash/arch/x86.c b/src/usr.sbin/crash/arch/x86.c
index 94381fb..80582d0 100644
--- a/src/usr.sbin/crash/arch/x86.c
+++ b/src/usr.sbin/crash/arch/x86.c
@@ -66,7 +66,11 @@ db_mach_init(kvm_t *kd)
errx(EXIT_FAILURE, "cannot read dumppcb: %s", kvm_geterr(kd));
}
ddb_regs.tf_sp = pcb.pcb_sp;
- ddb_regs.tf_bp = pcb.pcb_bp;
+#ifdef __x86_64__
+ ddb_regs.tf_bp = pcb.pcb_rbp;
+#else
+ ddb_regs.tf_bp = pcb.pcb_sp + offsetof(struct switchframe, sf_ebp);
+#endif
if (ddb_regs.tf_bp != 0 && ddb_regs.tf_sp != 0) {
printf("Backtrace from time of crash is available.\n");
}
commit fa917c32b46c804a040eb3602cd36b3df956dc85
Author: Richard Hansen <rhansen%bbn.com@localhost>
Date: Thu Sep 26 06:23:34 2013 +0000
i386: allow backtrace beyond Xsoftintr()
Now that Xsoftintr() follows the normal calling conventions, allow a
backtrace to continue past Xsoftintr().
diff --git a/src/sys/arch/i386/i386/db_machdep.c
b/src/sys/arch/i386/i386/db_machdep.c
index 230ad32..db48f45 100644
--- a/src/sys/arch/i386/i386/db_machdep.c
+++ b/src/sys/arch/i386/i386/db_machdep.c
@@ -224,6 +224,9 @@ db_nextframe(
uintptr_t ptr;
switch (is_trap) {
+ case SOFTINTR:
+ (*pr)("--- softint ---\n");
+ /* FALLTHROUGH */
case NONE:
*ip = (db_addr_t)
db_get_value((int)*retaddr, 4, false);
@@ -256,7 +259,6 @@ db_nextframe(
case TRAP:
case SYSCALL:
case INTERRUPT:
- case SOFTINTR:
default:
/* The only argument to trap() or syscall() is the trapframe. */
switch (is_trap) {
@@ -277,11 +279,6 @@ db_nextframe(
*/
db_read_bytes((db_addr_t)argp, sizeof(tf), (char *)&tf);
break;
- case SOFTINTR:
- (*pr)("--- softint ---\n");
- tf.tf_eip = 0;
- tf.tf_ebp = 0;
- break;
}
*ip = (db_addr_t)tf.tf_eip;
fp = (struct i386_frame *)tf.tf_ebp;
commit 000d56b4aa3ee8062cfb4ce446046506f927495b
Author: Richard Hansen <rhansen%bbn.com@localhost>
Date: Thu Oct 3 21:37:58 2013 +0000
i386: include the softint level in the debug backtrace
diff --git a/src/sys/arch/i386/i386/db_machdep.c
b/src/sys/arch/i386/i386/db_machdep.c
index db48f45..e38139d 100644
--- a/src/sys/arch/i386/i386/db_machdep.c
+++ b/src/sys/arch/i386/i386/db_machdep.c
@@ -222,10 +222,16 @@ db_nextframe(
struct i386_frame *fp;
int traptype;
uintptr_t ptr;
+ long *const thisframe = *nextframe;
switch (is_trap) {
case SOFTINTR:
- (*pr)("--- softint ---\n");
+ /*
+ * see Xsoftintr() for the reasoning behind the magic
+ * value '-4'
+ */
+ (*pr)("--- softint (number %d) ---\n",
+ (int)db_get_value((db_addr_t)(thisframe - 4), 4, true));
/* FALLTHROUGH */
case NONE:
*ip = (db_addr_t)
diff --git a/src/sys/arch/i386/i386/spl.S b/src/sys/arch/i386/i386/spl.S
index 002aa90..ad47ed0 100644
--- a/src/sys/arch/i386/i386/spl.S
+++ b/src/sys/arch/i386/i386/spl.S
@@ -354,12 +354,15 @@ IDTVEC_END(doreti)
* -1 switchframe sf_ebx: backup for local register
* -2 switchframe sf_esi: backup of address to return to
* -3 switchframe sf_edi: backup for local register
+ * -4 softint level (when calling softint_dispatch()). Read by
+ * db_nextframe() when printing a backtrace.
*
* Note that the sf_ebp member makes it possible to get a backtrace
* without any special work upon encountering Xsoftintr().
*
* If this stack layout is changed, the following must also change:
*
+ * - db_nextframe()
* - struct switchframe
* - any other function that depends on the layout of struct
* switchframe
Home |
Main Index |
Thread Index |
Old Index