[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[microblaze-uclinux] Patch for stability under heavy IRQ load
Hi everyone,
In the course of debugging some issues for a PetaLogix customer, I've identifed
a few subtle kernel bugs in the signal handling code that were causing hangs/bad
behaviour under very high interrupt loads (1KHz and greater).
Several people have reported such issues in the past, but it took a week of
focussed debugging to track down exactly what was wrong.
To that end, I'd appreciated it if those who have experienced issues could
please try the attached patch, which will go cleanly over any recent CVS kernel.
It would also be helpful for any others to try it, and make sure it doesn't
break anything for you (shouldn't do, but the more testing the better!).
For those interested, the bug manifested itself in two ways:
1. If an interrupt occurred between the delivery of a signal to a process, and
the actual handling of that signal (two seperate things in the kernel), then the
signal handler would be called at an address that was 4 bytes "short" of the
proper address. This would typically cause a stack corruption, which sometimes
was fatal, and sometimes not.
The fix was to unify handling of return address offsets between interrupts and
system calls - by adding 4 to the "old PC" address on entry to a syscall, the
return from interrupts and system calls could be unified to have the same offset
- "rtbd r14, 0" and "rtid r14, 0".
2. In the syscall restart code, the syscall number was being saved back onto the
stack frame in the saved "r12" slot, however r12 was not actually being restored
in the syscall return path. It still worked due to a subtle interaction with
bug #1 above, again except under high IRQ loads when there was a narrow
interrupt window that could cause problems.
Cheers,
John
--
Dr John Williams, Research Fellow,
Embedded Systems Group / Reconfigurable Computing
School of ITEE, The University of Queensland, Brisbane, Australia
(p) +61 7 33652185 (f) +61 7 33654999 (m) +61 403969243
Index: include/asm-microblaze/processor.h
===================================================================
--- include/asm-microblaze/processor.h (revision 1353)
+++ include/asm-microblaze/processor.h (revision 1354)
@@ -66,10 +66,7 @@
extern inline void start_thread (struct pt_regs *regs,
unsigned long pc, unsigned long usp)
{
- /* Thread will be started as a return from syscall
- rtbd r14, 4.
- Therefore, we adjust start PC address by negative 4 */
- regs->pc = pc-4;
+ regs->pc = pc;
regs->gpr[GPR_SP] = usp;
regs->kernel_mode = 0;
}
Index: arch/microblaze/kernel/signal.c
===================================================================
--- arch/microblaze/kernel/signal.c (revision 1353)
+++ arch/microblaze/kernel/signal.c (revision 1354)
@@ -371,14 +371,13 @@
regs->gpr[GPR_ARG0] = signal; /* Arg 0: signum */
regs->gpr[GPR_ARG1] = &frame->sc; /* arg 1: sigcontext */
- /* Offset of 4 to handle microblaze rtbd r14, 4 */
- regs->pc = ((unsigned long)ka->sa.sa_handler)-4;
+ regs->pc = ((unsigned long)ka->sa.sa_handler);
set_fs(USER_DS);
#if DEBUG_SIG
- printk("SIG deliver (%s:%d): sp=%p pc=%08lx ra=%08lx\n",
- current->comm, current->pid, frame, regs->pc, );
+ printk("SIG deliver (%s:%d): sp=%p pc=%08lx(@0x%08x) \n",
+ current->comm, current->pid, frame, regs->pc,&(regs->pc) );
#endif
return;
@@ -449,14 +448,14 @@
regs->gpr[GPR_ARG0] = signal; /* arg 0: signum */
regs->gpr[GPR_ARG1] = &frame->info; /* arg 1: siginfo */
regs->gpr[GPR_ARG2] = &frame->uc; /* arg2: ucontext */
- /* Offset to handle microblaze rtbd r14, 4 */
- regs->pc =((unsigned long)ka->sa.sa_handler)-4;
+ regs->pc =((unsigned long)ka->sa.sa_handler);
+
set_fs(USER_DS);
#if DEBUG_SIG
- printk("SIG deliver (%s:%d): sp=%p pc=%08lx pr=%08lx\n",
- current->comm, current->pid, frame, regs->pc, regs->pr);
+ printk("SIG deliver (%s:%d): sp=%p pc=%08lx \n",
+ current->comm, current->pid, frame, regs->pc);
#endif
return;
@@ -486,11 +485,8 @@
case -ERESTARTNOINTR:
do_restart:
regs->gpr[12] = PT_REGS_SYSCALL (regs);
- /* offset of 8 bytes required = 4 for rtbd
- offset, plus 4 for size of
- "brki r14,8"
- instruction. */
- regs->pc -= 8;
+ /* Offset of 4 bytes to re-execute trap (brki) instruction */
+ regs->pc -= 4;
break;
}
}
Index: arch/microblaze/kernel/entry.S
===================================================================
--- arch/microblaze/kernel/entry.S (revision 1353)
+++ arch/microblaze/kernel/entry.S (revision 1354)
@@ -220,80 +220,90 @@
#define RESTORE_LINK(linkreg) \
lwi linkreg, r1, PTO+PT_PC; /* PC, before IRQ/trap /*
-/* Save a special reg into saved state frame */
-#define SAVE_SPECIAL(reg,OFFS) \
- mfs r11, r ## reg; \
- swi r11, r1, PTO+PT_ ## OFFS;
+/* Save a special reg into saved state frame
+ Clobbers r{clobber} in the process */
+#define SAVE_SPECIAL(reg,OFFS,clobber) \
+ mfs clobber, r ## reg; \
+ swi clobber, r1, PTO+PT_ ## OFFS;
-/* Restore a special reg from saved state frame */
-#define RESTORE_SPECIAL(reg,OFFS) \
- lwi r11, r1, PTO+PT_ ## OFFS; \
- mts r ## reg , r11;
+/* Restore a special reg from saved state frame
+ Clobbers r{clobber} */
+#define RESTORE_SPECIAL(reg,OFFS,clobber) \
+ lwi clobber, r1, PTO+PT_ ## OFFS; \
+ mts r ## reg , clobber;
-/* Save processor status word into saved state frame */
-#define SAVE_PSW \
- SAVE_SPECIAL(msr,PSW)
+/* Save processor status word into saved state frame
+ Uses r{clobber} as a clobber register */
+#define SAVE_PSW(clobber) \
+ SAVE_SPECIAL(msr,PSW,clobber)
-/* Restore processor status word from saved state frame */
-#define RESTORE_PSW \
- RESTORE_SPECIAL(msr,PSW)
+/* Restore processor status word from saved state frame
+ Uses r{clobber} as a clobber register */
+#define RESTORE_PSW(clobber) \
+ RESTORE_SPECIAL(msr,PSW,clobber)
/* Save system registers to the struct pt_regs pointed to by REG.
- r11 is clobbered. */
+ r14 is clobbered, but it's already saved on the frame. */
#define SAVE_SYS_REGS_FOR_IRQ \
SAVE_REG(17); \
SAVE_LINK(r14); \
- SAVE_PSW;
+ SAVE_PSW(r14);
/* Restore system registers from the struct pt_regs pointed to by EP.
clobber r14 to restore status register, it gets fixed immediately */
#define RESTORE_SYS_REGS_FOR_IRQ \
- RESTORE_REG(17); \
- RESTORE_PSW; \
- RESTORE_LINK(r14);
+ RESTORE_PSW(r14); \
+ RESTORE_LINK(r14); \
+ RESTORE_REG(17);
/* Save system registers to the struct pt_regs pointed to by REG.
- r11 is clobbered. */
+ r14 is clobbered, but not until after it's saved on the frame.
+ We add 4 to r14 (the return PC address, so we can return with an
+ rtbd r14,0. This unifies handling of return offsets in signal handler
+ and makes life a lot easier.
+ r12 must be saved in case we end up doing a syscall restart */
#define SAVE_SYS_REGS_FOR_TRAP \
+ SAVE_REG(12); \
SAVE_REG(17); \
+ addik r14, r14, 4; \
SAVE_LINK(r14); \
- SAVE_PSW;
+ SAVE_PSW(r14);
/* Restore system registers from the struct pt_regs pointed to by EP.
- clobber r11 to restore status register */
+ clobber r14 to restore status register, but is fixed immediately.
+ r12 is restored in case we are doing a syscall restart */
#define RESTORE_SYS_REGS_FOR_TRAP \
+ RESTORE_PSW(r14); \
+ RESTORE_LINK(r14); \
RESTORE_REG(17); \
- RESTORE_PSW; \
- RESTORE_LINK(r14);
+ RESTORE_REG(12);
/* Save system registers to the struct pt_regs pointed to by REG.
- r11 is clobbered. */
+ r14 is clobbered after being saved on the frame.. */
#define SAVE_SYS_REGS_FOR_DBTRAP \
SAVE_REG(17); \
SAVE_LINK(r14); \
- SAVE_PSW;
+ SAVE_PSW(r14);
/* Restore system registers from the struct pt_regs pointed to by EP.
- clobber r11 to restore status register */
+ clobber r14 to restore status register, but is fixed immediately */
#define RESTORE_SYS_REGS_FOR_DBTRAP \
- RESTORE_REG(17); \
- RESTORE_PSW; \
- RESTORE_LINK(r14);
+ RESTORE_PSW(r14); \
+ RESTORE_LINK(r14); \
+ RESTORE_REG(17);
/* Save system registers to the struct pt_regs pointed to by REG. This is a
NMI-specific version, because NMIs save the PC/PSW in a different place
- than other interrupt requests. r11 is clobbered. */
+ than other interrupt requests. r16 is clobbered after being saved on frame. */
#define SAVE_SYS_REGS_FOR_NMI \
SAVE_LINK(r16); \
- SAVE_PSW;
+ SAVE_PSW(r16);
/* Restore system registers from the struct pt_regs pointed to by EP. This is
a NMI-specific version, because NMIs save the PC/PSW in a different place
- than other interrupt requests. r11 is clobbered (it is used as a scratch
- register because the POP_STATE macro restores it, and this macro is usually
- used inside POP_STATE). */
+ than other interrupt requests. r16 is clobbered, but immediately restored */
#define RESTORE_SYS_REGS_FOR_NMI \
- RESTORE_PSW; \
+ RESTORE_PSW(r16); \
RESTORE_LINK(r16);
/* Push register state, except for the stack pointer, on the stack in the form
@@ -416,8 +426,8 @@
SAVE_SYS_REGS_FOR_TRAP
#define TRAP_STATE_RESTORER \
- RESTORE_CALL_CLOBBERED_REGS_NO_RVAL; \
- RESTORE_SYS_REGS_FOR_TRAP
+ RESTORE_SYS_REGS_FOR_TRAP \
+ RESTORE_CALL_CLOBBERED_REGS_NO_RVAL;
#else /* !TRAPS_PRESERVE_CALL_CLOBBERED_REGS */
@@ -432,17 +442,17 @@
returning from a system call, to avoid any internal values from leaking out
of the kernel. */
#define TRAP_STATE_RESTORER \
+ RESTORE_SYS_REGS_FOR_TRAP; \
ZERO_CALL_CLOBBERED_REGS_NO_ARGS_NO_RVAL; \
- RESTORE_ARG_REGS; \
- RESTORE_SYS_REGS_FOR_TRAP
+ RESTORE_ARG_REGS;
#else /* !TRAPS_ZERO_CALL_CLOBBERED_REGS */
/* When traps return, they just leave call-clobbered registers (except for arg
regs) with whatever value they have from the kernel. */
#define TRAP_STATE_RESTORER \
- RESTORE_ARG_REGS; \
- RESTORE_SYS_REGS_FOR_TRAP
+ RESTORE_SYS_REGS_FOR_TRAP; \
+ RESTORE_ARG_REGS;
#endif /* TRAPS_ZERO_CALL_CLOBBERED_REGS */
#endif /* TRAPS_PRESERVE_CALL_CLOBBERED_REGS */
@@ -451,11 +461,14 @@
#define TRAP_EXTRA_STATE_SAVER \
SAVE_RVAL_REGS; \
SAVE_CALL_SAVED_REGS
+
#define TRAP_EXTRA_STATE_RESTORER \
RESTORE_RVAL_REGS; \
RESTORE_CALL_SAVED_REGS
+
#define TRAP_FUNCALL_EXTRA_STATE_SAVER \
SAVE_RVAL_REGS
+
#define TRAP_FUNCALL_EXTRA_STATE_RESTORER \
RESTORE_RVAL_REGS
@@ -475,11 +488,10 @@
SAVE_SYS_REGS_FOR_DBTRAP
#define DBTRAP_STATE_RESTORER \
- RESTORE_CALL_SAVED_REGS; \
RESTORE_SYS_REGS_FOR_DBTRAP; \
- RESTORE_CALL_CLOBBERED_REGS
+ RESTORE_CALL_CLOBBERED_REGS \
+ RESTORE_CALL_SAVED_REGS;
-
/* Register saving/restoring for maskable interrupts. */
#define IRQ_STATE_SAVER \
SAVE_CALL_CLOBBERED_REGS; \
@@ -505,8 +517,8 @@
SAVE_SYS_REGS_FOR_NMI
#define NMI_STATE_RESTORER \
- RESTORE_CALL_CLOBBERED_REGS; \
- RESTORE_SYS_REGS_FOR_NMI
+ RESTORE_SYS_REGS_FOR_NMI \
+ RESTORE_CALL_CLOBBERED_REGS;
#define NMI_EXTRA_STATE_SAVER \
SAVE_CALL_SAVED_REGS
@@ -525,11 +537,11 @@
switch_thread itself. */
#define SWITCH_STATE_SAVER \
SAVE_CALL_SAVED_REGS \
- SAVE_PSW
+ SAVE_PSW(r11)
#define SWITCH_STATE_RESTORER \
- RESTORE_CALL_SAVED_REGS \
- RESTORE_PSW
+ RESTORE_PSW(r11) \
+ RESTORE_CALL_SAVED_REGS
/* Instructions to return from an IRQ */
#define IRQ_RETURN_INST \
@@ -539,9 +551,9 @@
/* Instructions to return from a trap */
/* Note we use rtbd to clear BIP bit on way out */
-/* Return offset is 4, to execute instruction after syscall brki insn*/
+/* Return offset is zero, because we incremented r14 on the way in */
#define TRAP_RETURN_INST \
- rtbd r14, 4; /* r14 used as trap link register */ \
+ rtbd r14, 0; /* r14 used as trap link register */ \
nop;
/* Instructions to return from a debug trap */
@@ -553,7 +565,7 @@
/* Code fragment to return from NMI */
#define NMI_RETURN_INST \
- rtbd r16, 8; /* r16 is NMI (brk) link register */ \
+ rtbd r16, 0; /* r16 is NMI (brk) link register */ \
nop;
/* Restore register state from the struct pt_regs on the stack, switch back