From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41577) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBigB-0004LP-SR for qemu-devel@nongnu.org; Wed, 12 Sep 2012 04:52:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TBig2-0007V4-J0 for qemu-devel@nongnu.org; Wed, 12 Sep 2012 04:52:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37558) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBig2-0007Uu-9X for qemu-devel@nongnu.org; Wed, 12 Sep 2012 04:52:06 -0400 Message-ID: <50504D2E.2080802@redhat.com> Date: Wed, 12 Sep 2012 11:51:58 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1347240563-6212-1-git-send-email-mmogilvi_qemu@miniinfo.net> <50504151.40704@redhat.com> <50504C69.60703@siemens.com> In-Reply-To: <50504C69.60703@siemens.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] KVM: fix i8259 interrupt high to low transition logic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, Jan Kiszka , "Maciej W. Rozycki" , Paolo Bonzini , Matthew Ogilvie On 09/12/2012 11:48 AM, Jan Kiszka wrote: > On 2012-09-12 10:01, Avi Kivity wrote: >> On 09/10/2012 04:29 AM, Matthew Ogilvie wrote: >>> Intel's definition of "edge triggered" means: "asserted with a >>> low-to-high transition at the time an interrupt is registered >>> and then kept high until the interrupt is served via one of the >>> EOI mechanisms or goes away unhandled." >>> >>> So the only difference between edge triggered and level triggered >>> is in the leading edge, with no difference in the trailing edge. >>> >>> This bug manifested itself when the guest was Microport UNIX >>> System V/386 v2.1 (ca. 1987), because it would sometimes mask >>> off IRQ14 in the slave IMR after it had already been asserted. >>> The master would still try to deliver an interrupt even though >>> IRQ2 had dropped again, resulting in a spurious interupt >>> (IRQ15) and a panicked UNIX kernel. >>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c >>> index adba28f..5cbba99 100644 >>> --- a/arch/x86/kvm/i8254.c >>> +++ b/arch/x86/kvm/i8254.c >>> @@ -302,8 +302,12 @@ static void pit_do_work(struct kthread_work *work) >>> } >>> spin_unlock(&ps->inject_lock); >>> if (inject) { >>> - kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1); >>> + /* Clear previous interrupt, then create a rising >>> + * edge to request another interupt, and leave it at >>> + * level=1 until time to inject another one. >>> + */ >>> kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0); >>> + kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1); >>> >>> /* >> >> I thought I understood this, now I'm not sure. How can this be correct? >> Real hardware doesn't act like this. >> >> What if the PIT is disabled after this? You're injecting a spurious >> interrupt then. > > Yes, the PIT has to raise the output as long as specified, i.e. > according to the datasheet. That's important now due to the corrections > to the PIC. We can then carefully check if there is room for > simplifications / optimizations. I also cannot imagine that the above > already fulfills these requirements. > > And if the PIT is disabled by the HPET, we need to clear the output > explicitly as we inject the IRQ#0 under a different source ID than > userspace HPET does (which will logically take over IRQ#0 control). The > kernel would otherwise OR both sources to an incorrect result. > I guess we need to double the hrtimer rate then in order to generate a square wave. It's getting ridiculous how accurate our model needs to be. -- error compiling committee.c: too many arguments to function