Re: [Qemu-devel] [PATCH v3 9/9] arm: make number of a9mpcore GIC interrupts configurable

[Mark Langsdorf]

On 12/27/2011 03:59 PM, Peter Maydell wrote:
> On 27 December 2011 20:13, Mark Langsdorf <address@hidden> wrote:
>> Increase the maximum number of GIC interrupts for a9mp to 192, and
>> create a configurable property defaulting to 96 so that device
>> modelers can set the value appropriately for their SoC.
> 
>>  /* Configuration for arm_gic.c:
>>  * number of external IRQ lines, max number of CPUs, how to ID current CPU
>>  */
>> -#define GIC_NIRQ 96
>> +#define GIC_NIRQ 192
> 
> This (still) isn't the maximum number of GIC interrupts for
> an A9MP. You want 256.

I know, but I figured "more than anyone would be using" would
be sufficient.

> Moreover:
> now we have a per-cpu #define which is setting the compile-time
> maximum on a run-time parameter. That's pretty pointless --
> we should just have arm_gic.c have its own (purely internal)
> #define of the maximum number of interrupts it can permit,
> and the cpu-specific private peripheral code should only be
> setting the run-time parameter. You can drop the #define completely
> from a9mpcore.c &co. (don't forget to update the comment)

Okay.

> The GIC architectural limit on number of interrupts is 1020;
> that would (I think) imply a ~64K memory usage by the GIC,
> which we can live with I think.

I think you just said you wanted me to define the gic internal
maximum as 1020. I just want to be sure I understood you there.

>> --- a/hw/arm11mpcore.c
>> +++ b/hw/arm11mpcore.c
>> @@ -132,7 +132,7 @@ static int mpcore_priv_init(SysBusDevice *dev)
>>  {
>>     mpcore_priv_state *s = FROM_SYSBUSGIC(mpcore_priv_state, dev);
>>
>> -    gic_init(&s->gic, s->num_cpu);
>> +    gic_init(&s->gic, s->num_cpu, GIC_NIRQ);
> 
> 11MPCore, like A9MP, has a configurable number of interrupt lines
> (up to 256 including the 32 internal ones, also like A9MP).

I don't have 11mpcore to test with, but I'll make the change.

>> -static void gic_init(gic_state *s)
>> +static void gic_init(gic_state *s, int num_irq)
>>  #endif
>>  {
>>     int i;
>> @@ -808,7 +809,8 @@ static void gic_init(gic_state *s)
>>  #if NCPU > 1
>>     s->num_cpu = num_cpu;
>>  #endif
>> -    qdev_init_gpio_in(&s->busdev.qdev, gic_set_irq, GIC_NIRQ - 32);
>> +    s->num_irq = num_irq;
> 
> This is where you should be hw_error()ing if num_irq is
> too big.
> 
> I didn't see any changes to gic_load/gic_save, which means they
> will still be saving all GIC_NIRQ entries in each of the per-irq
> state arrays; this means that you've broken save/load compatibility.
> I think this can be fixed by having save/load only save/load the
> entries which are actually used (ie loop up to s->num_irq rather
> than GIC_NIRQ).

I had considered that, but wasn't sure enough about the pros and
cons. I'll make the changes and thanks for the feedback.

--Mark Langsdorf
Calxeda, Inc.