From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtWUo-0004Jb-ML for qemu-devel@nongnu.org; Thu, 19 Dec 2013 00:50:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VtWUi-0005vk-RC for qemu-devel@nongnu.org; Thu, 19 Dec 2013 00:50:06 -0500 Received: from mail-pb0-f51.google.com ([209.85.160.51]:39397) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtWUi-0005vg-Kj for qemu-devel@nongnu.org; Thu, 19 Dec 2013 00:50:00 -0500 Received: by mail-pb0-f51.google.com with SMTP id up15so683259pbc.38 for ; Wed, 18 Dec 2013 21:49:59 -0800 (PST) Date: Wed, 18 Dec 2013 21:49:58 -0800 From: Christoffer Dall Message-ID: <20131219054958.GS5711@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 05:43:54PM +0000, Peter Maydell wrote: > On 28 November 2013 16:17, Peter Maydell wrote: > > On 19 November 2013 06:18, Christoffer Dall wrote: > > 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. > > The other possibility is that it's a correct implementation > of 11MPCore GIC semantics -- the documentation of the > 11MPCore definitely says that level triggered interrupts > go from Pending to Active and can't be Active+Pending > unless software messes with the GIC state. So either > the docs are actively wrong for 11MPCore or it behaves > differently from GICv1 and v2 here (my guess would be > the latter, in which case we need to support both flavours). > A correct implementation? I don't see how, unless the pending/level fields are used in some obscure different way for the 11MPCore than for GICv2.0. For the 11MPCore, shouldn't it be: if (level) { GIC_SET_LEVEL(irq, cm); if (GIC_TEST_TRIGGER || !GIC_TEST_ACTIVE(irq, cm)) { GIC_SET_PENDING(irq, target); } } else { GIC_CLEAR_LEVEL(irq, cm) } and then the acknowledge could should check if the level is high and we are acking an active interrupt, make it pending instead of inactive? That being said, we are absolutely sure that support the 11MPCore is still needed? I would probably go the route of creating some structs with function pointers in them for things like the ack or raise etc. which have different semantics for the two versions and have separate functions to reduce the branching in each funciton. What do you think? -Christoffer