Skip to content

Commit

Permalink
interrupts could be recursive since lapic_eoi() called before rti
Browse files Browse the repository at this point in the history
so fast interrupts overflow the kernel stack
fix: cli() before lapic_eoi()
  • Loading branch information
rtm committed Aug 10, 2006
1 parent 8a8be1b commit 5be0039
Show file tree
Hide file tree
Showing 16 changed files with 194 additions and 28 deletions.
75 changes: 75 additions & 0 deletions Notes
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,78 @@ BUT now userfs doesn't do the final cat README

AND w/ cprintf("kbd overflow"), panic holding locks in scheduler
maybe also simulataneous panic("interrupt while holding a lock")

again (holding down x key):
kbd overflow
kbd oaaniicloowh
olding locks in scheduler
trap v 33 eip 100F5F c^CNext at t=32166285
(0) [0x0010033e] 0008:0010033e (unk. ctxt): jmp .+0xfffffffe (0x0010033e) ; ebfe
(1) [0x0010005c] 0008:0010005c (unk. ctxt): jmp .+0xfffffffe (0x0010005c) ; ebfe
cpu0 paniced due to holding locks in scheduler
cpu1 got panic("interrupt while holding a lock")
again in lapic_write.
while re-enabling an IRQ?

again:
cpu 0 panic("holding locks in scheduler")
but didn't trigger related panics earlier in scheduler or sched()
of course the panic is right after release() and thus sti()
so we may be seeing an interrupt that left locks held
cpu 1 unknown panic
why does it happen to both cpus at the same time?

again:
cpu 0 panic("holding locks in scheduler")
but trap() didn't see any held locks on return
cpu 1 no apparent panic

again:
cpu 0 panic: holding too many locks in scheduler
cpu 1 panic: kbd_intr returned while holding a lock

again:
cpu 0 panic: holding too man
la 10d70c lr 10027b
those don't seem to be locks...
only place non-constant lock is used is sleep()'s 2nd arg
maybe register not preserved across context switch?
it's in %esi...
sched() doesn't touch %esi
%esi is evidently callee-saved
something to do with interrupts? since ordinarily it works
cpu 1 panic: kbd_int returned while holding a lock
la 107340 lr 107300
console_lock and kbd_lock

maybe console_lock is often not released due to change
in use_console_lock (panic on other cpu)

again:
cpu 0: panic: h...
la 10D78C lr 102CA0
cpu 1: panic: acquire FL_IF (later than cpu 0)

but if sleep() were acquiring random locks, we'd see panics
in release, after sleep() returned.
actually when system is idle, maybe no-one sleeps at all.
just scheduler() and interrupts

questions:
does userfs use pipes? or fork?
no
does anything bad happen if process 1 exits? eg exit() in cat.c
looks ok
are there really no processes left?
lock_init() so we can have a magic number?

HMM maybe the variables at the end of struct cpu are being overwritten
nlocks, lastacquire, lastrelease
by cpu->stack?
adding junk buffers maybe causes crash to take longer...
when do we run on cpu stack?
just in scheduler()?
and interrupts from scheduler()

OH! recursive interrupts will use up any amount of cpu[].stack!
underflow and wrecks *previous* cpu's struct
8 changes: 7 additions & 1 deletion bio.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@
#include "buf.h"

struct buf buf[NBUF];
struct spinlock buf_table_lock = { "buf_table" };
struct spinlock buf_table_lock;

void
binit(void)
{
initlock(&buf_table_lock, "buf_table");
}

struct buf *
getblk()
Expand Down
31 changes: 21 additions & 10 deletions console.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,16 @@
#include "defs.h"
#include "spinlock.h"
#include "dev.h"
#include "param.h"

struct spinlock console_lock = { "console" };
struct spinlock console_lock;
int panicked = 0;
int use_console_lock = 0;

// per-cpu copy of output to help panic/lock debugging
char obuf[NCPU][1024];
uint obufi[NCPU];

/*
* copy console output to parallel port, which you can tell
* .bochsrc to copy to the stdout:
Expand All @@ -32,6 +37,10 @@ cons_putc(int c)
ushort *crt = (ushort *) 0xB8000; // base of CGA memory
int ind;

obuf[rcr4()][obufi[rcr4()]++] = c;
if(obufi[rcr4()] >= 1024)
obufi[rcr4()] = 0;

if(panicked){
cli();
for(;;)
Expand Down Expand Up @@ -101,11 +110,13 @@ printint(int xx, int base, int sgn)
void
cprintf(char *fmt, ...)
{
int i, state = 0, c;
int i, state = 0, c, locking = 0;
uint *ap = (uint *)(void*)&fmt + 1;

if(use_console_lock)
if(use_console_lock){
locking = 1;
acquire(&console_lock);
}

for(i = 0; fmt[i]; i++){
c = fmt[i] & 0xff;
Expand Down Expand Up @@ -140,7 +151,7 @@ cprintf(char *fmt, ...)
}
}

if(use_console_lock)
if(locking)
release(&console_lock);
}

Expand Down Expand Up @@ -293,7 +304,7 @@ static uchar *charcode[4] = {
char kbd_buf[KBD_BUF];
int kbd_r;
int kbd_w;
struct spinlock kbd_lock = { "kbd_lock" };
struct spinlock kbd_lock;

void
kbd_intr()
Expand All @@ -303,20 +314,17 @@ kbd_intr()

st = inb(KBSTATP);
if ((st & KBS_DIB) == 0){
lapic_eoi();
return;
}
data = inb(KBDATAP);

if (data == 0xE0) {
shift |= E0ESC;
lapic_eoi();
return;
} else if (data & 0x80) {
// Key released
data = (shift & E0ESC ? data : data & 0x7F);
shift &= ~(shiftcode[data] | E0ESC);
lapic_eoi();
return;
} else if (shift & E0ESC) {
// Last character was an E0 escape; or with 0x80
Expand Down Expand Up @@ -346,14 +354,17 @@ kbd_intr()
}

release(&kbd_lock);

lapic_eoi();
}

void
console_init()
{
initlock(&console_lock, "console");
initlock(&kbd_lock, "kbd");

devsw[CONSOLE].d_write = console_write;

ioapic_enable (IRQ_KBD, 1);

use_console_lock = 1;
}
5 changes: 5 additions & 0 deletions defs.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ void panic(char *s);
void kbd_intr(void);

// proc.c
void pinit(void);
struct proc;
struct jmpbuf;
void setupsegs(struct proc *);
Expand Down Expand Up @@ -67,6 +68,7 @@ void ioapic_enable (int irq, int cpu);

// spinlock.c
struct spinlock;
void initlock(struct spinlock *, char *);
void acquire(struct spinlock*);
void release(struct spinlock*);
int holding(struct spinlock*);
Expand All @@ -83,6 +85,7 @@ int pipe_write(struct pipe *p, char *addr, int n);
int pipe_read(struct pipe *p, char *addr, int n);

// fd.c
void fd_init(void);
int fd_ualloc(void);
struct fd * fd_alloc(void);
void fd_close(struct fd *);
Expand All @@ -97,13 +100,15 @@ void* ide_start_rw(int diskno, uint secno, void *dst, uint nsecs, int read);
int ide_finish(void *);

// bio.c
void binit(void);
struct buf;
struct buf *getblk(void);
struct buf *bread(uint, uint);
void bwrite(uint, struct buf *, uint);
void brelse(struct buf *);

// fs.c
void iinit(void);
struct inode * iget(uint dev, uint inum);
void ilock(struct inode *ip);
void iunlock(struct inode *ip);
Expand Down
6 changes: 6 additions & 0 deletions fd.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ struct devsw devsw[NDEV];

struct fd fds[NFD];

void
fd_init(void)
{
initlock(&fd_table_lock, "fd_table");
}

/*
* allocate a file descriptor number for curproc.
*/
Expand Down
8 changes: 7 additions & 1 deletion fs.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,16 @@
// these are inodes currently in use
// an entry is free if count == 0
struct inode inode[NINODE];
struct spinlock inode_table_lock = { "inode_table" };
struct spinlock inode_table_lock;

uint rootdev = 1;

void
iinit(void)
{
initlock(&inode_table_lock, "inode_table");
}

static uint
balloc(uint dev)
{
Expand Down
4 changes: 2 additions & 2 deletions ide.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ struct ide_request {
};
struct ide_request request[NREQUEST];
int head, tail;
struct spinlock ide_lock = { "ide" };
struct spinlock ide_lock;

int disk_channel;

Expand All @@ -46,6 +46,7 @@ ide_wait_ready(int check_error)
void
ide_init(void)
{
initlock(&ide_lock, "ide");
if (ncpu < 2) {
panic ("ide_init: disk interrupt is going to the second cpu\n");
}
Expand All @@ -61,7 +62,6 @@ ide_intr(void)
// cprintf("cpu%d: ide_intr\n", cpu());
wakeup(&request[tail]);
release(&ide_lock);
lapic_eoi();
}

int
Expand Down
3 changes: 2 additions & 1 deletion kalloc.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#include "proc.h"
#include "spinlock.h"

struct spinlock kalloc_lock = { "kalloc" };
struct spinlock kalloc_lock;

struct run {
struct run *next;
Expand All @@ -37,6 +37,7 @@ kinit(void)
uint mem;
char *start;

initlock(&kalloc_lock, "kalloc");
start = (char *) &end;
start = (char *) (((uint)start + PAGE) & ~(PAGE-1));
mem = 256; // XXX
Expand Down
18 changes: 13 additions & 5 deletions main.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ extern uchar _binary_user1_start[], _binary_user1_size[];
extern uchar _binary_usertests_start[], _binary_usertests_size[];
extern uchar _binary_userfs_start[], _binary_userfs_size[];

extern int use_console_lock;

// CPU 0 starts running C code here.
// This is called main0 not main so that it can have
// a void return type. Gcc can't handle functions named
Expand All @@ -27,28 +25,36 @@ main0(void)
int i;
struct proc *p;

lcr4(0); // xxx copy of cpu #

// clear BSS
memset(edata, 0, end - edata);

// Make sure interrupts stay disabled on all processors
// until each signals it is ready, by pretending to hold
// an extra lock.
for(i=0; i<NCPU; i++)
// xxx maybe replace w/ acquire remembering if FL_IF
for(i=0; i<NCPU; i++){
cpus[i].nlock++;
cpus[i].guard1 = 0xdeadbeef;
cpus[i].guard2 = 0xdeadbeef;
}

mp_init(); // collect info about this machine

use_console_lock = 1;

lapic_init(mp_bcpu());

cprintf("\n\ncpu%d: booting xv6\n\n", cpu());

pinit();
binit();
pic_init(); // initialize PIC
ioapic_init();
kinit(); // physical memory allocator
tvinit(); // trap vectors
idtinit(); // this CPU's idt register
fd_init();
iinit();

// create a fake process per CPU
// so each CPU always has a tss and a gdt
Expand Down Expand Up @@ -101,6 +107,8 @@ main0(void)
void
mpmain(void)
{
lcr4(1); // xxx copy of cpu #

cprintf("cpu%d: starting\n", cpu());
idtinit(); // CPU's idt
if(cpu() == 0)
Expand Down
1 change: 1 addition & 0 deletions mmu.h
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ struct gatedesc {

// Set up a normal interrupt/trap gate descriptor.
// - istrap: 1 for a trap (= exception) gate, 0 for an interrupt gate.
// interrupt gate clears FL_IF, trap gate leaves FL_IF alone
// - sel: Code segment selector for interrupt/trap handler
// - off: Offset in code segment for interrupt/trap handler
// - dpl: Descriptor Privilege Level -
Expand Down
2 changes: 1 addition & 1 deletion pipe.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pipe_alloc(struct fd **fd1, struct fd **fd2)
p->writeopen = 1;
p->writep = 0;
p->readp = 0;
memset(&p->lock, 0, sizeof(p->lock));
initlock(&p->lock, "pipe");
(*fd1)->type = FD_PIPE;
(*fd1)->readable = 1;
(*fd1)->writeable = 0;
Expand Down
Loading

0 comments on commit 5be0039

Please sign in to comment.