From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@qumranet.com>
Subject: Re: Re: [kvm-devel] [PATCH 2/5] SCI fixes (v2)
Date: Wed, 28 May 2008 16:06:08 +0300
Message-ID: <483D58C0.7060209@qumranet.com>
References: <1202137865-20232-1-git-send-email-aliguori@us.ibm.com>	 <1202137865-20232-3-git-send-email-aliguori@us.ibm.com>	 <1211857519.14708.30.camel@bling> <483C52D2.2080603@us.ibm.com>	 <483CFB6D.3070302@qumranet.com> <1211978270.14859.40.camel@bling>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Anthony Liguori <aliguori@us.ibm.com>,
	kvm-devel <kvm@vger.kernel.org>
To: Alex Williamson <alex.williamson@hp.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from bzq-179-150-194.static.bezeqint.net ([212.179.150.194]:33198
	"EHLO il.qumranet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752049AbYE1NGO (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 28 May 2008 09:06:14 -0400
In-Reply-To: <1211978270.14859.40.camel@bling>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.