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

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

On Wed, 2008-05-28 at 09:27 +0300, Avi Kivity wrote:
> This is
> > commit ce35c9534137b71327466fa9abc243cbe2d7e8dc
> > Author: Avi Kivity <avi@qumranet.com>
> > Date:   Wed Jan 2 12:52:28 2008 +0200
> >
> >     kvm: qemu: fix power management timer overflow handling
> >     
> >     The PMSTS overflow bit needs to be set each time bit 23 of the pm 
> > timer
> >     is toggled.  This means we need to adjust the overflow time every time
> >     we have an overflow.
> >     
> >     Taken from qemu patch by TeLeMan in
> >     
> >     http://www.mail-archive.com/qemu-devel@nongnu.org/msg14680.html
> 
> And, like the explanation says, we have to advance the overflow time in 
> order to get an interrupt.  Is there something horribly broken?

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,

	Alex
 
-- 
Alex Williamson                             HP Open Source & Linux Org.