able/vmware-svga
1
0
Fork 0
Code Issues Pull requests Packages Projects Releases Wiki Activity

Hopefully fix Metalkit refdriver SVGA_WaitForIRQ hangs

Browse source 
There's been a longstanding bug in the IRQ handling code
in the Metalkit version of the SVGA reference driver, which
occasionally caused tests to hang.

I looked at the disassembly of SVGA_WaitForIRQ and friends, and
I found one big problem and one smaller problem:

The 'switchContext' flag is used to tell the IRQ handler
whether it should branch to the saved context or not. This
value *must* be written before we halt, or we can deadlock.
Unfortunately, the value wasn't marked as 'volatile', and the
compiler was optimizing out this assignment entirely! This
means that 'flags' will never get set, and we'll be in an
infinite loop.

Given this bug, I'm not sure why WaitForIRQ ever worked.
It's possible that we were fine if the IRQ had already arrived
by the time WaitForIRQ was called, but we'd deadlock if we
actually ended up waiting. If this is the case, it also means
that fence-stress isn't doing a very good job of stressing the
IRQ code.

The other, much less severe problem: The stack frames for
WaitForIRQInternal and SaveContext may have been partially
overlapping, since gcc was allocating 0x10 bytes on the
stack before calling SaveContext. The proper solution is
probably just to rewrite all of this in assembly, but for
now I just increased the number of padding words on the
stack by adding extra NULL parameters to WaitForIRQInternal.
This commit is contained in:
Micah Dowty 2009-05-07 22:35:49 +00:00
parent 64070c7b8e
commit 99a767c5d9
2 changed files with 17 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

19
lib/refdriver/svga.c
View file

@ -1307,7 +1307,12 @@ SVGAWaitForIRQInternal(void *arg, ...)
{
uint32 flags = 0;
/*
* switchContext must be set to TRUE before we halt, or we'll deadlock.
* This variable is marked as volatile, plus we use a compiler memory barrier.
*/
gSVGA.irq.switchContext = TRUE;
asm volatile ("" ::: "memory");
Atomic_Exchange(gSVGA.irq.pending, flags);
@ -1315,7 +1320,12 @@ SVGAWaitForIRQInternal(void *arg, ...)
Intr_Halt();
}
/*
* Must set switchContext to FALSE before this stack frame is deallocated.
*/
gSVGA.irq.switchContext = FALSE;
asm volatile ("" ::: "memory");
return flags;
}
@ -1336,16 +1346,19 @@ SVGA_WaitForIRQ(void)
* halt.
*/
Intr_SaveContext(&gSVGA.irq.newContext);
Intr_SaveContext((IntrContext*) &gSVGA.irq.newContext);
do {
/*
* XXX: The arguments here are a kludge to prevent the interrupt
* stack frame from overlapping the stack frame referenced
* by Intr_SaveContext().
*
* Be sure to look at the generated assembly and compare the
* size of this argument list to the amount of stack space
* allocated for Intr_SaveContext.
*/
flags = SVGAWaitForIRQInternal(NULL, NULL, NULL, NULL);
flags = SVGAWaitForIRQInternal(NULL, NULL, NULL, NULL, NULL, NULL);
} while (flags == 0);

2
lib/refdriver/svga.h
View file

@ -65,7 +65,7 @@ typedef struct SVGADevice {
uint32 nextFence;
} fifo;
struct {
volatile struct {
uint32 pending;
uint32 switchContext;
IntrContext oldContext;