[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Android-virt] [PATCH 02/12] arm: make the number of GI
From: |
Rusty Russell |
Subject: |
Re: [Qemu-devel] [Android-virt] [PATCH 02/12] arm: make the number of GIC interrupts configurable |
Date: |
Fri, 27 Jan 2012 11:03:19 +1030 |
User-agent: |
Notmuch/0.6.1-1 (http://notmuchmail.org) Emacs/23.3.1 (i686-pc-linux-gnu) |
On Wed, 25 Jan 2012 15:09:41 +0000, Peter Maydell <address@hidden> wrote:
> On 24 January 2012 08:42, Rusty Russell <address@hidden> wrote:
> > Reading through this, I see a lot of "- 32". Trivial patch follows,
> > which applies to your rebasing branch:
>
> (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 <address@hidden>
> >
> > Interrupts numbers 0-31 are private to the processor interface, 32-1019 are
> > general interrups. Add 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. And remove a single stray tab.
>
> 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 <address@hidden>
> > ---
> > hw/arm_gic.c | 48 ++++++++++++++++++++++++++----------------------
> > 1 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 @@
> >
> > /* Maximum number of possible interrupts, determined by the GIC
> > architecture */
> > #define GIC_MAXIRQ 1020
> > +/* First 32 are private to each CPU (SGIs and PPIs). */
> > +#define GIC_INTERNAL 32
> > //#define DEBUG_GIC
> >
> > #ifdef DEBUG_GIC
> > @@ -74,7 +76,7 @@ typedef struct gic_irq_state
> > #define GIC_CLEAR_TRIGGER(irq) s->irq_state[irq].trigger = 0
> > #define GIC_TEST_TRIGGER(irq) s->irq_state[irq].trigger
> > #define GIC_GET_PRIORITY(irq, cpu) \
> > - (((irq) < 32) ? s->priority1[irq][cpu] : s->priority2[(irq) - 32])
> > + (((irq) < GIC_INTERNAL) ? s->priority1[irq][cpu] : s->priority2[(irq) -
> > GIC_INTERNAL])
>
> 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)
> > s->num_cpu = num_cpu;
> > #endif
> > s->num_irq = num_irq + GIC_BASE_IRQ;
> > - if (s->num_irq > GIC_MAXIRQ) {
> > - hw_error("requested %u interrupt lines exceeds GIC maximum %d\n",
> > - num_irq, GIC_MAXIRQ);
> > + if (s->num_irq > GIC_MAXIRQ
> > + || s->num_irq < GIC_INTERNAL
> > + || (s->num_irq % 32) != 0) {
>
> 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 == 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);
> 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.
[Qemu-devel] [PATCH 06/12] hw/vexpress.c: Factor out daughterboard-specific initialization, Peter Maydell, 2012/01/13
[Qemu-devel] [PATCH 03/12] hw/arm_boot.c: Make SMP boards specify address to poll in bootup loop, Peter Maydell, 2012/01/13
Re: [Qemu-devel] [PATCH 03/12] hw/arm_boot.c: Make SMP boards specify address to poll in bootup loop, andrzej zaborowski, 2012/01/16
[Qemu-devel] [PATCH 08/12] hw/a15mpcore.c: Add Cortex-A15 private peripheral model, Peter Maydell, 2012/01/13
[Qemu-devel] [PATCH 01/12] vexpress, realview: Add (dummy) L2 cache controller, Peter Maydell, 2012/01/13