From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55386) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RqZm4-0000EE-BJ for qemu-devel@nongnu.org; Thu, 26 Jan 2012 19:34:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RqZm2-0007fP-Vo for qemu-devel@nongnu.org; Thu, 26 Jan 2012 19:34:40 -0500 Received: from ozlabs.org ([203.10.76.45]:43730) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RqZm2-0007f9-Ba for qemu-devel@nongnu.org; Thu, 26 Jan 2012 19:34:38 -0500 From: Rusty Russell In-Reply-To: References: <1326487969-12462-1-git-send-email-peter.maydell@linaro.org> <1326487969-12462-3-git-send-email-peter.maydell@linaro.org> <87fwf5ebjw.fsf@rustcorp.com.au> Date: Fri, 27 Jan 2012 11:03:19 +1030 Message-ID: <87vcnyc7cg.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Android-virt] [PATCH 02/12] arm: make the number of GIC interrupts configurable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org, android-virt@lists.cs.columbia.edu, patches@linaro.org On Wed, 25 Jan 2012 15:09:41 +0000, Peter Maydell wrote: > On 24 January 2012 08:42, Rusty Russell wrote: > > Reading through this, I see a lot of "- 32". =C2=A0Trivial patch follow= s, > > which applies to your rebasing branch: >=20 > (If you send patches as fresh new emails then they just apply > with git am without needing manual cleanup, appear with sensible > subjects in patchwork/patches.linaro, etc.) Indeed, but it's so conversaionally gauche :( I thought git am did the Right Thing, but it doesn't, and --scissors doesn't help either (it gets the wrong Subject line). Oh well, I'll do it that way in future. > > Subject: ARM: clean up GIC constants. > > From: Rusty Russell > > > > Interrupts numbers 0-31 are private to the processor interface, 32-1019= are > > general interrups. =C2=A0Add GIC_INTERNAL and substitute everywhere. > > > > Also, add a check that the total number of interrupts is divisible by > > 32 (required for reporting interupt numbers, see gic_dist_readb(), and > > is greater than 32. =C2=A0And remove a single stray tab. >=20 > I agree with Avi that the presence of "Also" in a git > commit message is generally a sign you should have submitted > more than one patch :-) Indeed, guilty as charged :) > > Signed-off-by: Rusty Russell > > --- > > =C2=A0hw/arm_gic.c | =C2=A0 48 ++++++++++++++++++++++++++--------------= -------- > > =C2=A01 files changed, 26 insertions(+), 22 deletions(-) > > > > diff --git a/hw/arm_gic.c b/hw/arm_gic.c > > index cf582a5..a29eacb 100644 > > --- a/hw/arm_gic.c > > +++ b/hw/arm_gic.c > > @@ -13,6 +13,8 @@ > > > > =C2=A0/* Maximum number of possible interrupts, determined by the GIC a= rchitecture */ > > =C2=A0#define GIC_MAXIRQ 1020 > > +/* First 32 are private to each CPU (SGIs and PPIs). */ > > +#define GIC_INTERNAL 32 > > =C2=A0//#define DEBUG_GIC > > > > =C2=A0#ifdef DEBUG_GIC > > @@ -74,7 +76,7 @@ typedef struct gic_irq_state > > =C2=A0#define GIC_CLEAR_TRIGGER(irq) s->irq_state[irq].trigger =3D 0 > > =C2=A0#define GIC_TEST_TRIGGER(irq) s->irq_state[irq].trigger > > =C2=A0#define GIC_GET_PRIORITY(irq, cpu) \ > > - =C2=A0(((irq) < 32) ? s->priority1[irq][cpu] : s->priority2[(irq) - 3= 2]) > > + =C2=A0(((irq) < GIC_INTERNAL) ? s->priority1[irq][cpu] : s->priority2= [(irq) - GIC_INTERNAL]) >=20 > This line is now >80 characters and needs folding. > (scripts/checkpatch.pl will catch this kind of nit.) Will do. > > @@ -812,11 +814,13 @@ static void gic_init(gic_state *s, int num_irq) > > =C2=A0 =C2=A0 s->num_cpu =3D num_cpu; > > =C2=A0#endif > > =C2=A0 =C2=A0 s->num_irq =3D num_irq + GIC_BASE_IRQ; > > - =C2=A0 =C2=A0if (s->num_irq > GIC_MAXIRQ) { > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0hw_error("requested %u interrupt lines exc= eeds GIC maximum %d\n", > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 num_irq, GIC_= MAXIRQ); > > + =C2=A0 =C2=A0if (s->num_irq > GIC_MAXIRQ > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0|| s->num_irq < GIC_INTERNAL > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0|| (s->num_irq % 32) !=3D 0) { >=20 > So I guess our implementation isn't likely to work properly for a > non-multiple-of-32 number of IRQs, but this isn't an architectural GIC > restriction. (In fact the GIC architecture spec allows the supported > interrupt IDs to not even be in a contiguous range, which we certainly > don't support...) Yes, I intuited it from here: static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset) { ... if (offset =3D=3D 4) return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5); If want want to support non-32-divisible # irqs, we need at least: ((s->num_irq + 31) / 32 - 1) Seemed easier to have an initialization-time assert than check everywhere else for overflows. > I also think it's architecturally permitted that not all the internal > (SPI/PPI) interrupts are implemented, ie that s->num_irq < 32 (you > have to read the GIC architecture manually quite closely to deduce > this, though). This made me read that part of the manual... interesting. > Anyway, if we would otherwise die horribly later on we should > catch these cases, but it would be good to have at least a comment > saying that these are implementation limitations rather than > architectural ones. Good point. If we add an "supported" bit to each irq, we could do weird things, but presumably ->num_irq would still correspond to ITLinesNumber. I don't want to put too much of an essay in there. How's this: /* ITLinesNumber is represented as (N - 32) / 1. See gic_dist_readb. */ if (s->num_irq < 32 || (s->num_irq % 32)) { hw_error("%u interrupt lines unsupported: not divisible by = 32\n", num_irq); =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 > Beefing up the parameter check should be a separate patch. Thanks, coming soon. I should be getting access to git.linaro.org RSN, so I can post there for easier merging. Thanks, Rusty.