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

[Peter Maydell]

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.

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)

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.

>  #define NCPU 4
>
>  static inline int
> @@ -37,6 +37,7 @@ typedef struct a9mp_priv_state {
>     MemoryRegion ptimer_iomem;
>     MemoryRegion container;
>     DeviceState *mptimer;
> +    uint32_t num_irq;
>  } a9mp_priv_state;
>
>  static uint64_t a9_scu_read(void *opaque, target_phys_addr_t offset,
> @@ -152,8 +153,11 @@ static int a9mp_priv_init(SysBusDevice *dev)
>     if (s->num_cpu > NCPU) {
>         hw_error("a9mp_priv_init: num-cpu may not be more than %d\n", NCPU);
>     }
> +    if (s->num_irq > GIC_NIRQ) {
> +        hw_error("a9mp_priv_init: num-irq may not be more than %d\n", 
> GIC_NIRQ);
> +    }

No need to check this here -- just pass it through
to gic_init() and let gic_init() hw_error if needed.

> --- 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).

> -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).

-- PMM