[Qemu-devel] Re: [kvm] Re: [kvm-devel] [PATCH 2/5] SCI fixes (v2)

[Alex Williamson <alex.williamson@hp.com>]

Thanks Avi.  Adding qemu-devel as I think they need this fix too;
although I can only verify the fix on qemu using the kvm bios.bin (the
qemu one seems to put the SCI on interrupt 11, so the guest never sees
it anyway).  To recap, for qemu-devel, the patch in the subject, which
was incorporated to qemu svn back in March introduces a bug in the timer
overflow status reported to the guest and toggling of the SCI interrupt.
I've seen application hangs running Vista on qemu & kvm because of this.
The code path is further explained below.  Reverting the one liner
change, as shown in the patch below, solves the problem.  Thanks,

	Alex

On Wed, 2008-05-28 at 16:06 +0300, Avi Kivity wrote: 
> Alex Williamson wrote:
> > Yeah, it kinda seems like there is.  With this change, the timer expires
> > and we go through this path:
> >
> > pm_tmr_timer()
> > 	-> pm_update_sci()
> > 		-> get_pmsts()
> > 		-> qemu_set_irq() [but not for a TMFOF_EN]
> > 		-> qemu_mod_timer()
> > 		bump tmr_overlfow_time
> >
> > We bumped tmr_overflow_time in pm_update_sci after setting the timer to
> > expire on the old value.  Unless something goes horribly wrong with
> > timers, we'll always get the timer event before overflow time and
> > get_pmsts never adds in the TMROF_EN bit to the status flag.  We
> > therefore never toggle the SCI interrupt because of a timer overflow,
> > and we never report a timer overflow status to the guest.
> >
> > The author of this patch is correct that the timer in the original code
> > only goes off a couple times before we del_timer().  However, I think
> > the way it's supposed to work is that we set the timer overflow status,
> > toggle the SCI, then wait for the OSPM to come in through
> > pm_ioport_writew() to clear the timer overflow status, at which point we
> > call pm_update_sci() mod_timer and start it all over again.  At least
> > that's the way I see it working after removing this change.
> >
> > It doesn't make much sense to bump tmr_overflow_time so that we never
> > hit it, unless I'm completely misunderstanding the code.  Thanks,
> >   
> 
> You're right; a quick run with that patch reverted (and not) showed it 
> clearly.
> 
> I guess it was merged before we had sci working properly; otherwise I 
> can't explain how it worked then.
> 
> I reverted that patch.

acpi: Revert bogus ACPI timer overflow change

This caused us to never set the timer overflow status bit and send an
SCI interrupt to the guest due to an overflow.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
--

diff -r 1f1541286539 trunk/hw/acpi.c
--- a/trunk/hw/acpi.c	Sun May 25 15:01:05 2008 -0600
+++ b/trunk/hw/acpi.c	Wed May 28 14:33:03 2008 -0600
@@ -105,7 +105,6 @@ static void pm_update_sci(PIIX4PMState *
     if ((s->pmen & TMROF_EN) && !(pmsts & TMROF_EN)) {
         expire_time = muldiv64(s->tmr_overflow_time, ticks_per_sec, PM_FREQ);
         qemu_mod_timer(s->tmr_timer, expire_time);
-        s->tmr_overflow_time += 0x800000;
     } else {
         qemu_del_timer(s->tmr_timer);
     }