From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56956) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtWP3-00022p-IN for qemu-devel@nongnu.org; Thu, 19 Dec 2013 00:44:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VtWOx-0004I6-0M for qemu-devel@nongnu.org; Thu, 19 Dec 2013 00:44:09 -0500 Received: from mail-pa0-f44.google.com ([209.85.220.44]:35955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtWOw-0004Ht-PM for qemu-devel@nongnu.org; Thu, 19 Dec 2013 00:44:02 -0500 Received: by mail-pa0-f44.google.com with SMTP id fa1so688007pad.17 for ; Wed, 18 Dec 2013 21:44:01 -0800 (PST) Date: Wed, 18 Dec 2013 21:44:00 -0800 From: Christoffer Dall Message-ID: <20131219054400.GR5711@cbox> References: <1384841896-19566-1-git-send-email-christoffer.dall@linaro.org> <1384841896-19566-2-git-send-email-christoffer.dall@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Patch Tracking , QEMU Developers , "kvmarm@lists.cs.columbia.edu" On Thu, Nov 28, 2013 at 04:17:43PM +0000, Peter Maydell wrote: > On 19 November 2013 06:18, Christoffer Dall wrote: > > For some reason only edge-triggered or enabled level-triggered > > interrupts would set the pending state of a raised IRQ. This is not in > > compliance with the specs, which indicate that the pending state is > > separate from the enabled state, which only controls if a pending > > interrupt is actually forwarded to the CPU interface. > > > > Therefore, simply always set the pending state on a rising edge, but > > only clear the pending state of falling edge if the interrupt is level > > triggered. > > > > Changelog [v2]: > > - Fix bisection issue, by not using gic_clear_pending yet. > > > > Signed-off-by: Christoffer Dall > > --- > > hw/intc/arm_gic.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > > index d431b7a..c7a24d5 100644 > > --- a/hw/intc/arm_gic.c > > +++ b/hw/intc/arm_gic.c > > @@ -128,11 +128,12 @@ static void gic_set_irq(void *opaque, int irq, int level) > > > > if (level) { > > GIC_SET_LEVEL(irq, cm); > > - if (GIC_TEST_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) { > > - DPRINTF("Set %d pending mask %x\n", irq, target); > > - GIC_SET_PENDING(irq, target); > > - } > > + DPRINTF("Set %d pending mask %x\n", irq, target); > > + GIC_SET_PENDING(irq, target); > > } else { > > + if (!GIC_TEST_TRIGGER(irq)) { > > + GIC_CLEAR_PENDING(irq, target); > > + } > > GIC_CLEAR_LEVEL(irq, cm); > > } > > gic_update(s); > > So I think this is a correct change in the sense that > it's fixing the behaviour of this function. However > we seem to get our pending behaviour for level triggered > interrupts wrong in several places: > * here > * gic_acknowledge_irq (which you fix in a later patch) > * gic_complete_irq, which currently says "enabled > level triggered still-raised interrupts should be > remarked as pending" > > This feels to me like a cluster of errors which have come > from somebody's misreading of the spec and which probably > combine to produce a coherent not-too-far-from-correct > result, and which we should therefore fix all at once rather > than only partially. > Fair enough, I'll try and combine these. -Christoffer