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

[Avi Kivity <avi@qumranet.com>]

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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.