From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VibS2-0005d6-Mc for qemu-devel@nongnu.org; Mon, 18 Nov 2013 21:54:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VibRw-0007wB-Qo for qemu-devel@nongnu.org; Mon, 18 Nov 2013 21:54:06 -0500 Received: from mail-pd0-f172.google.com ([209.85.192.172]:53070) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VibRw-0007tk-Gv for qemu-devel@nongnu.org; Mon, 18 Nov 2013 21:54:00 -0500 Received: by mail-pd0-f172.google.com with SMTP id g10so3102974pdj.17 for ; Mon, 18 Nov 2013 18:53:59 -0800 (PST) Date: Mon, 18 Nov 2013 18:53:56 -0800 From: Christoffer Dall Message-ID: <20131119025356.GA64526@lvm> References: <1380229386-24166-1-git-send-email-christoffer.dall@linaro.org> <1380229386-24166-4-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 v2 3/6] hw: arm_gic: Keep track of SGI sources 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 Mon, Oct 14, 2013 at 04:36:24PM +0100, Peter Maydell wrote: > On 26 September 2013 22:03, Christoffer Dall > wrote: > > Right now the arm gic emulation doesn't keep track of the source of an > > SGI (which apparently Linux guests don't use, or they're fine with > > assuming CPU 0 always). > > > > Add the necessary matrix on the GICState structure and maintain the data > > when setting and clearing the pending state of an IRQ. > > > > Note that we always choose to present the source as the lowest-numbered > > CPU in case multiple cores have signalled the same SGI number to a core > > on the system. > > Shouldn't the state you have in sgi_source[][] be surfaced as the > GICD_CPENDSGIR/GICD_SPENDSGIR registers [for a v2 GIC; they don't > exist in v1]? It might then be better to represent the state in > our data structures in the same layout as the registers. > Hmm, those registers actually don't represent which CPU is the *source* of a given SGI. I think the array I propose is quite reasonable... > > > > Signed-off-by: Christoffer Dall > > > > --- > > > > Changelog [v2]: > > - Fixed endless loop bug > > - Bump version_id and minimum_version_id on vmstate struct > > --- > > hw/intc/arm_gic.c | 41 ++++++++++++++++++++++++++++++++--------- > > hw/intc/arm_gic_common.c | 5 +++-- > > hw/intc/gic_internal.h | 3 +++ > > 3 files changed, 38 insertions(+), 11 deletions(-) > > > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > > index 7eaa55f..6470d37 100644 > > --- a/hw/intc/arm_gic.c > > +++ b/hw/intc/arm_gic.c > > @@ -97,6 +97,20 @@ void gic_set_pending_private(GICState *s, int cpu, int irq) > > gic_update(s); > > } > > > > +static void gic_clear_pending(GICState *s, int irq, int cm, uint8_t src) > > +{ > > I think that in the cases where we pass in 0 for src that the > irq can't be < GIC_NR_SGIS. > not quite sure what you mean by this comment, we pass in 0 for src in two cases: 1) when we are dealing with something else than an SGI (irq >= GIC_NR_SGIS), but we could pass whatever, the value doesn't make sense here 2) when we are dealing with an SGI, the source can be 0 up to s->num_cpu - 1. > > + unsigned cpu; > > + > > + GIC_CLEAR_PENDING(irq, cm); > > + if (irq < GIC_NR_SGIS) { > > + cpu = (unsigned)ffs(cm) - 1; > > If you used ctz32() rather than ffs() it would save you having to > subtract one all the time. Also, those unsigned casts are pretty > ugly: better to just make 'cpu' an int... > yeah, that was quite ridiculous. > > + while (cpu < NCPU) { > > + s->sgi_source[irq][cpu] &= ~(1 << src); > > + cpu = (unsigned)ffs(cm) - 1; > > + } > > ...this still seems to be an infinite loop: cm isn't modified > inside the loop so cpu will always have the same value each time. > > 610: eb fe jmp 610 > if only I had for_each_set_bit - does QEMU have something sane for this? Anyway, I tried again to fix this. Thanks for spotting my broken fix for my broken code. > > + } > > Are you sure the logic in this function is right? (ie that we > should only clear the sgi_source[][] bit for this source, and > not completely? If nothing else, I think the interrupt should > stay pending if some other source cpu still wants it to be > pending. That is, I think we need to track the pending status > separately for each (irq,target-cpu,source-cpu) separately for > SGIs. (I'm not totally sure I have this right though, the spec > is quite confusing.) > no, you're right, for SGIs we need to loop through the sgi_source array and make sure the irq is not pending on any CPUs from any CPUs. > > +} > > + > > > /* Maximum number of possible CPU interfaces, determined by GIC architecture */ > > #define NCPU 8 > > > > @@ -58,6 +59,7 @@ > > s->priority1[irq][cpu] : \ > > s->priority2[(irq) - GIC_INTERNAL]) > > #define GIC_TARGET(irq) s->irq_target[irq] > > +#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ? ffs(s->sgi_source[irq][cpu]) - 1 : 0) > > WARNING: line over 80 characters > #161: FILE: hw/intc/gic_internal.h:62: > +#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ? > ffs(s->sgi_source[irq][cpu]) - 1 : 0) > Thans, -Christoffer