On Wed, 25 Jan 2012 15:09:41 +0000, Peter Maydell <peter.maydell at linaro.org> wrote:
> On 24 January 2012 08:42, Rusty Russell <rusty at rustcorp.com.au> 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 <rusty at rustcorp.com.au>
> >
> > 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 <rusty at rustcorp.com.au>
> > ---
> > ?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.
