[PATCH 0/8] hw/arm: Explicit number of GIC external IRQs for Cortex A9/A15 MPCore

[PATCH 0/8] hw/arm: Explicit number of GIC external IRQs for Cortex A9/A15 MPCore

[Philippe Mathieu-Daudé]

Some boards based on Cortex-A9MP / Cortex-A15MP do not explicit
the number of external GIC IRQs, using some (implicit) default value,
not always trivial to figure out. Change that by removing the default
value, requiring MPCore objects to be created with the "num-irq" set.

Philippe Mathieu-Daudé (8):
  hw/arm/exynos4210: Replace magic 32 by proper 'GIC_INTERNAL'
    definition
  hw/arm/exynos4210: Explicit number of GIC external IRQs
  hw/arm/realview: Explicit number of GIC external IRQs
  hw/arm/xilinx_zynq: Replace IRQ_OFFSET -> GIC_INTERNAL
  hw/arm/xilinx_zynq: Explicit number of GIC external IRQs
  hw/arm/vexpress: Explicit number of GIC external IRQs
  hw/arm/highbank: Explicit number of GIC external IRQs
  hw/cpu/arm_mpcore: Remove default values for GIC external IRQs

 hw/arm/exynos4210.c  | 16 +++++++++++++--
 hw/arm/highbank.c    | 15 ++++++++++----
 hw/arm/realview.c    | 17 ++++++++++++++--
 hw/arm/vexpress.c    | 13 ++++++++++--
 hw/arm/xilinx_zynq.c | 47 +++++++++++++++++++++++++-------------------
 hw/cpu/a15mpcore.c   | 13 ++++++------
 hw/cpu/a9mpcore.c    | 14 ++++++-------
 7 files changed, 91 insertions(+), 44 deletions(-)

-- 
2.47.1

[PATCH 1/8] hw/arm/exynos4210: Replace magic 32 by proper 'GIC_INTERNAL' definition

[Philippe Mathieu-Daudé]

The 32 IRQ lines skipped are the GIC internal ones.
Use the GIC_INTERNAL definition for clarity.
No logical change.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/exynos4210.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index dd0edc81d5c..99b05a175d6 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -393,8 +393,9 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
             }
         }
         if (irq_id) {
+            irq_id -= GIC_INTERNAL;
             qdev_connect_gpio_out(splitter, splitin,
-                                  qdev_get_gpio_in(extgicdev, irq_id - 32));
+                                  qdev_get_gpio_in(extgicdev, irq_id));
         }
     }
     for (; n < EXYNOS4210_MAX_INT_COMBINER_IN_IRQ; n++) {
@@ -413,6 +414,7 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
         }
 
         if (irq_id) {
+            irq_id -= GIC_INTERNAL;
             assert(splitcount < EXYNOS4210_NUM_SPLITTERS);
             splitter = DEVICE(&s->splitter[splitcount]);
             qdev_prop_set_uint16(splitter, "num-lines", 2);
@@ -421,7 +423,7 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
             s->irq_table[n] = qdev_get_gpio_in(splitter, 0);
             qdev_connect_gpio_out(splitter, 0, qdev_get_gpio_in(intcdev, n));
             qdev_connect_gpio_out(splitter, 1,
-                                  qdev_get_gpio_in(extgicdev, irq_id - 32));
+                                  qdev_get_gpio_in(extgicdev, irq_id));
         } else {
             s->irq_table[n] = qdev_get_gpio_in(intcdev, n);
         }
-- 
2.47.1

Re: [PATCH 1/8] hw/arm/exynos4210: Replace magic 32 by proper 'GIC_INTERNAL' definition

[Peter Maydell]

On Thu, 30 Jan 2025 at 18:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The 32 IRQ lines skipped are the GIC internal ones.
> Use the GIC_INTERNAL definition for clarity.
> No logical change.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/exynos4210.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index dd0edc81d5c..99b05a175d6 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -393,8 +393,9 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
>              }
>          }
>          if (irq_id) {
> +            irq_id -= GIC_INTERNAL;
>              qdev_connect_gpio_out(splitter, splitin,
> -                                  qdev_get_gpio_in(extgicdev, irq_id - 32));
> +                                  qdev_get_gpio_in(extgicdev, irq_id));
>          }
>      }
>      for (; n < EXYNOS4210_MAX_INT_COMBINER_IN_IRQ; n++) {
> @@ -413,6 +414,7 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
>          }
>
>          if (irq_id) {
> +            irq_id -= GIC_INTERNAL;
>              assert(splitcount < EXYNOS4210_NUM_SPLITTERS);
>              splitter = DEVICE(&s->splitter[splitcount]);
>              qdev_prop_set_uint16(splitter, "num-lines", 2);
> @@ -421,7 +423,7 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
>              s->irq_table[n] = qdev_get_gpio_in(splitter, 0);
>              qdev_connect_gpio_out(splitter, 0, qdev_get_gpio_in(intcdev, n));
>              qdev_connect_gpio_out(splitter, 1,
> -                                  qdev_get_gpio_in(extgicdev, irq_id - 32));
> +                                  qdev_get_gpio_in(extgicdev, irq_id));
>          } else {
>              s->irq_table[n] = qdev_get_gpio_in(intcdev, n);

A small nit, but I think I would prefer these as
irq_id - GIC_INTERNAL, rather than changing the value of the variable.
Otherwise the semantics of the value in the variable change
mid-way. That doesn't have a practical effect since the call
to qdev_get_gpio_in() is in both places that last use of the
variable, but I think it's a bit confusing.

thanks
-- PMM

[PATCH 2/8] hw/arm/exynos4210: Explicit number of GIC external IRQs

[Philippe Mathieu-Daudé]

When not specified, Cortex-A9MP configures its GIC with 64 external
IRQs (see commit a32134aad89 "arm:make the number of GIC interrupts
configurable"). Add the GIC_EXT_IRQS definition (with a comment)
to make that explicit.

Except explicitly setting a property value to its same implicit
value, there is no logical change intended.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/exynos4210.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 99b05a175d6..75d6e4d1ab9 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -103,6 +103,14 @@
 #define EXYNOS4210_PL330_BASE1_ADDR         0x12690000
 #define EXYNOS4210_PL330_BASE2_ADDR         0x12850000
 
+/*
+ * The Cortex-A9MP may have anything from 0 to 224 external interrupt
+ * IRQ lines (with another 32 internal). We default to 64+32, which
+ * is the number provided by the Cortex-A9MP test chip in the
+ * Realview PBX-A9 and Versatile Express A9 development boards.
+ */
+#define GIC_EXT_IRQS 64
+
 enum ExtGicId {
     EXT_GIC_ID_MDMA_LCD0 = 66,
     EXT_GIC_ID_PDMA0,
@@ -588,6 +596,8 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
 
     /* Private memory region and Internal GIC */
     qdev_prop_set_uint32(DEVICE(&s->a9mpcore), "num-cpu", EXYNOS4210_NCPUS);
+    qdev_prop_set_uint32(DEVICE(&s->a9mpcore), "num-irq",
+                         GIC_EXT_IRQS + GIC_INTERNAL);
     busdev = SYS_BUS_DEVICE(&s->a9mpcore);
     sysbus_realize(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, EXYNOS4210_SMP_PRIVATE_BASE_ADDR);
-- 
2.47.1

Re: [PATCH 2/8] hw/arm/exynos4210: Explicit number of GIC external IRQs

[Peter Maydell]

On Thu, 30 Jan 2025 at 18:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> When not specified, Cortex-A9MP configures its GIC with 64 external
> IRQs (see commit a32134aad89 "arm:make the number of GIC interrupts
> configurable"). Add the GIC_EXT_IRQS definition (with a comment)
> to make that explicit.
>
> Except explicitly setting a property value to its same implicit
> value, there is no logical change intended.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/exynos4210.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index 99b05a175d6..75d6e4d1ab9 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -103,6 +103,14 @@
>  #define EXYNOS4210_PL330_BASE1_ADDR         0x12690000
>  #define EXYNOS4210_PL330_BASE2_ADDR         0x12850000
>
> +/*
> + * The Cortex-A9MP may have anything from 0 to 224 external interrupt
> + * IRQ lines (with another 32 internal). We default to 64+32, which
> + * is the number provided by the Cortex-A9MP test chip in the
> + * Realview PBX-A9 and Versatile Express A9 development boards.
> + */

This isn't the A9MP test chip or the vexpress or realview
board, though. We should be setting this to whatever the
exynos4210's GIC is actually configured with, or else saying
we don't know what the right value is but this is the
one QEMU has always used (i.e. probably a TODO comment). Those
Arm devboards are irrelevant either way.

> +#define GIC_EXT_IRQS 64
> +
>  enum ExtGicId {
>      EXT_GIC_ID_MDMA_LCD0 = 66,
>      EXT_GIC_ID_PDMA0,
> @@ -588,6 +596,8 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
>
>      /* Private memory region and Internal GIC */
>      qdev_prop_set_uint32(DEVICE(&s->a9mpcore), "num-cpu", EXYNOS4210_NCPUS);
> +    qdev_prop_set_uint32(DEVICE(&s->a9mpcore), "num-irq",
> +                         GIC_EXT_IRQS + GIC_INTERNAL);
>      busdev = SYS_BUS_DEVICE(&s->a9mpcore);
>      sysbus_realize(busdev, &error_fatal);
>      sysbus_mmio_map(busdev, 0, EXYNOS4210_SMP_PRIVATE_BASE_ADDR);

thanks
-- PMM

Re: [PATCH 2/8] hw/arm/exynos4210: Explicit number of GIC external IRQs

[Philippe Mathieu-Daudé]

On 30/1/25 19:30, Peter Maydell wrote:
> On Thu, 30 Jan 2025 at 18:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> When not specified, Cortex-A9MP configures its GIC with 64 external
>> IRQs (see commit a32134aad89 "arm:make the number of GIC interrupts
>> configurable"). Add the GIC_EXT_IRQS definition (with a comment)
>> to make that explicit.
>>
>> Except explicitly setting a property value to its same implicit
>> value, there is no logical change intended.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/exynos4210.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
>> index 99b05a175d6..75d6e4d1ab9 100644
>> --- a/hw/arm/exynos4210.c
>> +++ b/hw/arm/exynos4210.c
>> @@ -103,6 +103,14 @@
>>   #define EXYNOS4210_PL330_BASE1_ADDR         0x12690000
>>   #define EXYNOS4210_PL330_BASE2_ADDR         0x12850000
>>
>> +/*
>> + * The Cortex-A9MP may have anything from 0 to 224 external interrupt
>> + * IRQ lines (with another 32 internal). We default to 64+32, which
>> + * is the number provided by the Cortex-A9MP test chip in the
>> + * Realview PBX-A9 and Versatile Express A9 development boards.
>> + */
> 
> This isn't the A9MP test chip or the vexpress or realview
> board, though. We should be setting this to whatever the
> exynos4210's GIC is actually configured with, or else saying
> we don't know what the right value is but this is the
> one QEMU has always used (i.e. probably a TODO comment). Those
> Arm devboards are irrelevant either way.

Historically this is how this board ended using this value though.

I'll reword the comment appropriately.

Regards,

Phil.

> 
>> +#define GIC_EXT_IRQS 64

[PATCH 3/8] hw/arm/realview: Explicit number of GIC external IRQs

[Philippe Mathieu-Daudé]

When not specified, Cortex-A9MP configures its GIC with 64 external
IRQs (see commit a32134aad89 "arm:make the number of GIC interrupts
configurable"). Add the GIC_EXT_IRQS definition (with a comment)
to make that explicit.

Except explicitly setting a property value to its same implicit
value, there is no logical change intended.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/realview.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 9900a98f3b8..4a62c83506b 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -35,6 +35,14 @@
 #define SMP_BOOT_ADDR 0xe0000000
 #define SMP_BOOTREG_ADDR 0x10000030
 
+/*
+ * The Cortex-A9MP may have anything from 0 to 224 external interrupt
+ * IRQ lines (with another 32 internal). We default to 64+32, which
+ * is the number provided by the Cortex-A9MP test chip in the
+ * Realview PBX-A9 and Versatile Express A9 development boards.
+ */
+#define GIC_EXT_IRQS 64
+
 /* Board init.  */
 
 static struct arm_boot_info realview_binfo = {
@@ -185,7 +193,12 @@ static void realview_init(MachineState *machine,
     sysbus_mmio_map(SYS_BUS_DEVICE(sysctl), 0, 0x10000000);
 
     if (is_mpcore) {
-        dev = qdev_new(is_pb ? TYPE_A9MPCORE_PRIV : "realview_mpcore");
+        if (is_pb) {
+            dev = qdev_new(TYPE_A9MPCORE_PRIV);
+            qdev_prop_set_uint32(dev, "num-irq", GIC_EXT_IRQS + GIC_INTERNAL);
+        } else {
+            dev = qdev_new("realview_mpcore");
+        }
         qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
         busdev = SYS_BUS_DEVICE(dev);
         sysbus_realize_and_unref(busdev, &error_fatal);
@@ -201,7 +214,7 @@ static void realview_init(MachineState *machine,
         /* For now just create the nIRQ GIC, and ignore the others.  */
         dev = sysbus_create_simple(TYPE_REALVIEW_GIC, gic_addr, cpu_irq[0]);
     }
-    for (n = 0; n < 64; n++) {
+    for (n = 0; n < GIC_EXT_IRQS; n++) {
         pic[n] = qdev_get_gpio_in(dev, n);
     }
 
-- 
2.47.1

Re: [PATCH 3/8] hw/arm/realview: Explicit number of GIC external IRQs

[Peter Maydell]

On Thu, 30 Jan 2025 at 18:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> When not specified, Cortex-A9MP configures its GIC with 64 external
> IRQs (see commit a32134aad89 "arm:make the number of GIC interrupts
> configurable"). Add the GIC_EXT_IRQS definition (with a comment)
> to make that explicit.
>
> Except explicitly setting a property value to its same implicit
> value, there is no logical change intended.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/realview.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 9900a98f3b8..4a62c83506b 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -35,6 +35,14 @@
>  #define SMP_BOOT_ADDR 0xe0000000
>  #define SMP_BOOTREG_ADDR 0x10000030
>
> +/*
> + * The Cortex-A9MP may have anything from 0 to 224 external interrupt
> + * IRQ lines (with another 32 internal). We default to 64+32, which
> + * is the number provided by the Cortex-A9MP test chip in the
> + * Realview PBX-A9 and Versatile Express A9 development boards.
> + */

On the other hand, this really *is* the Realview PBX-A9
development board. So we can just say that that's the right
number (and the vexpress is irrelevant, and the range of
settings the CPU itself can have isn't very important either).

(PS: there's no verb "to explicit" in English (ignoring some
obscure obsolete ones); French "expliciter" => English "to make
explicit"; or you in the case of the subject line here,
"specify explicitly" is probably most natural.)

> +#define GIC_EXT_IRQS 64
> +
>  /* Board init.  */
>
>  static struct arm_boot_info realview_binfo = {
> @@ -185,7 +193,12 @@ static void realview_init(MachineState *machine,
>      sysbus_mmio_map(SYS_BUS_DEVICE(sysctl), 0, 0x10000000);
>
>      if (is_mpcore) {
> -        dev = qdev_new(is_pb ? TYPE_A9MPCORE_PRIV : "realview_mpcore");
> +        if (is_pb) {
> +            dev = qdev_new(TYPE_A9MPCORE_PRIV);
> +            qdev_prop_set_uint32(dev, "num-irq", GIC_EXT_IRQS + GIC_INTERNAL);
> +        } else {
> +            dev = qdev_new("realview_mpcore");
> +        }
>          qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
>          busdev = SYS_BUS_DEVICE(dev);
>          sysbus_realize_and_unref(busdev, &error_fatal);
> @@ -201,7 +214,7 @@ static void realview_init(MachineState *machine,
>          /* For now just create the nIRQ GIC, and ignore the others.  */
>          dev = sysbus_create_simple(TYPE_REALVIEW_GIC, gic_addr, cpu_irq[0]);
>      }
> -    for (n = 0; n < 64; n++) {
> +    for (n = 0; n < GIC_EXT_IRQS; n++) {
>          pic[n] = qdev_get_gpio_in(dev, n);
>      }
>
> --
> 2.47.1

thanks
-- PMM

Re: [PATCH 3/8] hw/arm/realview: Explicit number of GIC external IRQs

[Philippe Mathieu-Daudé]

On 30/1/25 19:36, Peter Maydell wrote:
> On Thu, 30 Jan 2025 at 18:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> When not specified, Cortex-A9MP configures its GIC with 64 external
>> IRQs (see commit a32134aad89 "arm:make the number of GIC interrupts
>> configurable"). Add the GIC_EXT_IRQS definition (with a comment)
>> to make that explicit.
>>
>> Except explicitly setting a property value to its same implicit
>> value, there is no logical change intended.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/realview.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
>> index 9900a98f3b8..4a62c83506b 100644
>> --- a/hw/arm/realview.c
>> +++ b/hw/arm/realview.c
>> @@ -35,6 +35,14 @@
>>   #define SMP_BOOT_ADDR 0xe0000000
>>   #define SMP_BOOTREG_ADDR 0x10000030
>>
>> +/*
>> + * The Cortex-A9MP may have anything from 0 to 224 external interrupt
>> + * IRQ lines (with another 32 internal). We default to 64+32, which
>> + * is the number provided by the Cortex-A9MP test chip in the
>> + * Realview PBX-A9 and Versatile Express A9 development boards.
>> + */
> 
> On the other hand, this really *is* the Realview PBX-A9
> development board. So we can just say that that's the right
> number (and the vexpress is irrelevant, and the range of
> settings the CPU itself can have isn't very important either).
> 
> (PS: there's no verb "to explicit" in English (ignoring some
> obscure obsolete ones); French "expliciter" => English "to make
> explicit"; or you in the case of the subject line here,
> "specify explicitly" is probably most natural.)

TIL, thanks :)

[PATCH 4/8] hw/arm/xilinx_zynq: Replace IRQ_OFFSET -> GIC_INTERNAL

[Philippe Mathieu-Daudé]

We already have a definition to distinct GIC internal
IRQs versus external ones, use it. No logical changes.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/xilinx_zynq.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 8477b828745..18051458945 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -54,8 +54,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(ZynqMachineState, ZYNQ_MACHINE)
 #define FLASH_SIZE (64 * 1024 * 1024)
 #define FLASH_SECTOR_SIZE (128 * 1024)
 
-#define IRQ_OFFSET 32 /* pic interrupts start from index 32 */
-
 #define MPCORE_PERIPHBASE 0xF8F00000
 #define ZYNQ_BOARD_MIDR 0x413FC090
 
@@ -281,12 +279,12 @@ static void zynq_init(MachineState *machine)
         pic[n] = qdev_get_gpio_in(dev, n);
     }
 
-    n = zynq_init_spi_flashes(0xE0006000, pic[58 - IRQ_OFFSET], false, 0);
-    n = zynq_init_spi_flashes(0xE0007000, pic[81 - IRQ_OFFSET], false, n);
-    n = zynq_init_spi_flashes(0xE000D000, pic[51 - IRQ_OFFSET], true, n);
+    n = zynq_init_spi_flashes(0xE0006000, pic[58 - GIC_INTERNAL], false, 0);
+    n = zynq_init_spi_flashes(0xE0007000, pic[81 - GIC_INTERNAL], false, n);
+    n = zynq_init_spi_flashes(0xE000D000, pic[51 - GIC_INTERNAL], true, n);
 
-    sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - IRQ_OFFSET]);
-    sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - IRQ_OFFSET]);
+    sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - GIC_INTERNAL]);
+    sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - GIC_INTERNAL]);
 
     dev = qdev_new(TYPE_CADENCE_UART);
     busdev = SYS_BUS_DEVICE(dev);
@@ -295,7 +293,7 @@ static void zynq_init(MachineState *machine)
                           qdev_get_clock_out(slcr, "uart0_ref_clk"));
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, 0xE0000000);
-    sysbus_connect_irq(busdev, 0, pic[59 - IRQ_OFFSET]);
+    sysbus_connect_irq(busdev, 0, pic[59 - GIC_INTERNAL]);
     dev = qdev_new(TYPE_CADENCE_UART);
     busdev = SYS_BUS_DEVICE(dev);
     qdev_prop_set_chr(dev, "chardev", serial_hd(1));
@@ -303,15 +301,15 @@ static void zynq_init(MachineState *machine)
                           qdev_get_clock_out(slcr, "uart1_ref_clk"));
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, 0xE0001000);
-    sysbus_connect_irq(busdev, 0, pic[82 - IRQ_OFFSET]);
+    sysbus_connect_irq(busdev, 0, pic[82 - GIC_INTERNAL]);
 
     sysbus_create_varargs("cadence_ttc", 0xF8001000,
-            pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
+            pic[42-GIC_INTERNAL], pic[43-GIC_INTERNAL], pic[44-GIC_INTERNAL], NULL);
     sysbus_create_varargs("cadence_ttc", 0xF8002000,
-            pic[69-IRQ_OFFSET], pic[70-IRQ_OFFSET], pic[71-IRQ_OFFSET], NULL);
+            pic[69-GIC_INTERNAL], pic[70-GIC_INTERNAL], pic[71-GIC_INTERNAL], NULL);
 
-    gem_init(0xE000B000, pic[54 - IRQ_OFFSET]);
-    gem_init(0xE000C000, pic[77 - IRQ_OFFSET]);
+    gem_init(0xE000B000, pic[54 - GIC_INTERNAL]);
+    gem_init(0xE000C000, pic[77 - GIC_INTERNAL]);
 
     for (n = 0; n < 2; n++) {
         int hci_irq = n ? 79 : 56;
@@ -330,7 +328,7 @@ static void zynq_init(MachineState *machine)
         qdev_prop_set_uint64(dev, "capareg", ZYNQ_SDHCI_CAPABILITIES);
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
         sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, hci_addr);
-        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[hci_irq - IRQ_OFFSET]);
+        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[hci_irq - GIC_INTERNAL]);
 
         di = drive_get(IF_SD, 0, n);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
@@ -343,7 +341,7 @@ static void zynq_init(MachineState *machine)
     dev = qdev_new(TYPE_ZYNQ_XADC);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
-    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[39-IRQ_OFFSET]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[39-GIC_INTERNAL]);
 
     dev = qdev_new("pl330");
     object_property_set_link(OBJECT(dev), "memory",
@@ -363,15 +361,15 @@ static void zynq_init(MachineState *machine)
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, 0xF8003000);
-    sysbus_connect_irq(busdev, 0, pic[45-IRQ_OFFSET]); /* abort irq line */
+    sysbus_connect_irq(busdev, 0, pic[45-GIC_INTERNAL]); /* abort irq line */
     for (n = 0; n < ARRAY_SIZE(dma_irqs); ++n) { /* event irqs */
-        sysbus_connect_irq(busdev, n + 1, pic[dma_irqs[n] - IRQ_OFFSET]);
+        sysbus_connect_irq(busdev, n + 1, pic[dma_irqs[n] - GIC_INTERNAL]);
     }
 
     dev = qdev_new("xlnx.ps7-dev-cfg");
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(busdev, &error_fatal);
-    sysbus_connect_irq(busdev, 0, pic[40 - IRQ_OFFSET]);
+    sysbus_connect_irq(busdev, 0, pic[40 - GIC_INTERNAL]);
     sysbus_mmio_map(busdev, 0, 0xF8007000);
 
     /*
-- 
2.47.1

Re: [PATCH 4/8] hw/arm/xilinx_zynq: Replace IRQ_OFFSET -> GIC_INTERNAL

[Peter Maydell]

On Thu, 30 Jan 2025 at 18:26, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We already have a definition to distinct GIC internal
> IRQs versus external ones, use it. No logical changes.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/xilinx_zynq.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 5/8] hw/arm/xilinx_zynq: Explicit number of GIC external IRQs

[Philippe Mathieu-Daudé]

When not specified, Cortex-A9MP configures its GIC with 64 external
IRQs (see commit a32134aad89 "arm:make the number of GIC interrupts
configurable"). Add the GIC_EXT_IRQS definition (with a comment)
to make that explicit.

Except explicitly setting a property value to its same implicit
value, there is no logical change intended.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/xilinx_zynq.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 18051458945..dbb003e906a 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -57,6 +57,14 @@ OBJECT_DECLARE_SIMPLE_TYPE(ZynqMachineState, ZYNQ_MACHINE)
 #define MPCORE_PERIPHBASE 0xF8F00000
 #define ZYNQ_BOARD_MIDR 0x413FC090
 
+/*
+ * The Cortex-A9MP may have anything from 0 to 224 external interrupt
+ * IRQ lines (with another 32 internal). We default to 64+32, which
+ * is the number provided by the Cortex-A9MP test chip in the
+ * Realview PBX-A9 and Versatile Express A9 development boards.
+ */
+#define GIC_EXT_IRQS 64
+
 static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
@@ -205,7 +213,7 @@ static void zynq_init(MachineState *machine)
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
     DeviceState *dev, *slcr;
     SysBusDevice *busdev;
-    qemu_irq pic[64];
+    qemu_irq pic[GIC_EXT_IRQS];
     int n;
     unsigned int smp_cpus = machine->smp.cpus;
 
@@ -261,6 +269,7 @@ static void zynq_init(MachineState *machine)
 
     dev = qdev_new(TYPE_A9MPCORE_PRIV);
     qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
+    qdev_prop_set_uint32(dev, "num-irq", GIC_EXT_IRQS + GIC_INTERNAL);
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
@@ -275,7 +284,7 @@ static void zynq_init(MachineState *machine)
                            qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
     }
 
-    for (n = 0; n < 64; n++) {
+    for (n = 0; n < GIC_EXT_IRQS; n++) {
         pic[n] = qdev_get_gpio_in(dev, n);
     }
 
-- 
2.47.1

[PATCH 6/8] hw/arm/vexpress: Explicit number of GIC external IRQs

[Philippe Mathieu-Daudé]

When not specified, Cortex-A9MP configures its GIC with 64 external
IRQs, (see commit a32134aad89 "arm:make the number of GIC interrupts
configurable"), and Cortex-15MP to 128 (see commit  528622421eb
"hw/cpu/a15mpcore: Correct default value for num-irq").
The Versatile Express board however expects a fixed set of 64
interrupts (see the fixed IRQ length when this board was added in
commit 2055283bcc8 ("hw/vexpress: Add model of ARM Versatile Express
board"). Add the GIC_EXT_IRQS definition (with a comment) to make
that explicit.

Except explicitly setting a property value to its same implicit
value, there is no logical change intended.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/vexpress.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 42c67034061..8e801aa79cb 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -51,6 +51,14 @@
 #define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
 #define VEXPRESS_FLASH_SECT_SIZE (256 * 1024)
 
+/*
+ * The Cortex-A9MP/A15MP may have anything from 0 to 224 external interrupt
+ * IRQ lines (with another 32 internal). We default to 64+32, which
+ * is the number provided by the Cortex-A9MP test chip in the
+ * Realview PBX-A9 and Versatile Express A9 development boards.
+ */
+#define GIC_EXT_IRQS 64
+
 /* Number of virtio transports to create (0..8; limited by
  * number of available IRQ lines).
  */
@@ -241,6 +249,7 @@ static void init_cpus(MachineState *ms, const char *cpu_type,
      */
     dev = qdev_new(privdev);
     qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
+    qdev_prop_set_uint32(dev, "num-irq", GIC_EXT_IRQS + GIC_INTERNAL);
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, periphbase);
@@ -251,7 +260,7 @@ static void init_cpus(MachineState *ms, const char *cpu_type,
      * external interrupts starting from 32 (because there
      * are internal interrupts 0..31).
      */
-    for (n = 0; n < 64; n++) {
+    for (n = 0; n < GIC_EXT_IRQS; n++) {
         pic[n] = qdev_get_gpio_in(dev, n);
     }
 
@@ -543,7 +552,7 @@ static void vexpress_common_init(MachineState *machine)
     VexpressMachineClass *vmc = VEXPRESS_MACHINE_GET_CLASS(machine);
     VEDBoardInfo *daughterboard = vmc->daughterboard;
     DeviceState *dev, *sysctl, *pl041;
-    qemu_irq pic[64];
+    qemu_irq pic[GIC_EXT_IRQS];
     uint32_t sys_id;
     DriveInfo *dinfo;
     PFlashCFI01 *pflash0;
-- 
2.47.1

[PATCH 7/8] hw/arm/highbank: Explicit number of GIC external IRQs

[Philippe Mathieu-Daudé]

When not specified, Cortex-A9MP configures its GIC with 64 external
IRQs, (see commit a32134aad89 "arm:make the number of GIC interrupts
configurable"), and Cortex-15MP to 128 (see commit  528622421eb
"hw/cpu/a15mpcore: Correct default value for num-irq").
The Caldexa Highbank board however expects a fixed set of 128
interrupts (see the fixed IRQ length when this board was added in
commit 2488514cef2 ("arm: SoC model for Calxeda Highbank"). Add the
GIC_EXT_IRQS definition (with a comment) to make that explicit.

Except explicitly setting a property value to its same implicit
value, there is no logical change intended.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/highbank.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 495704d9726..d59f20b88e0 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -45,7 +45,14 @@
 #define MVBAR_ADDR              0x200
 #define BOARD_SETUP_ADDR        (MVBAR_ADDR + 8 * sizeof(uint32_t))
 
-#define NIRQ_GIC                160
+/*
+ * The Cortex-A9MP/A15MP may have anything from 0 to 224 external interrupt
+ * IRQ lines (with another 32 internal). We default to 128+32, which
+ * is the number provided by the Cortex-A15MP test chip in the
+ * Versatile Express A15 development board.
+ * Other boards may differ and should set this property appropriately.
+ */
+#define GIC_EXT_IRQS            128
 
 /* Board init.  */
 
@@ -180,7 +187,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
 {
     DeviceState *dev = NULL;
     SysBusDevice *busdev;
-    qemu_irq pic[128];
+    qemu_irq pic[GIC_EXT_IRQS];
     int n;
     unsigned int smp_cpus = machine->smp.cpus;
     qemu_irq cpu_irq[4];
@@ -260,7 +267,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         break;
     }
     qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
-    qdev_prop_set_uint32(dev, "num-irq", NIRQ_GIC);
+    qdev_prop_set_uint32(dev, "num-irq", GIC_EXT_IRQS + GIC_INTERNAL);
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
@@ -271,7 +278,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         sysbus_connect_irq(busdev, n + 3 * smp_cpus, cpu_vfiq[n]);
     }
 
-    for (n = 0; n < 128; n++) {
+    for (n = 0; n < GIC_EXT_IRQS; n++) {
         pic[n] = qdev_get_gpio_in(dev, n);
     }
 
-- 
2.47.1

[PATCH 8/8] hw/cpu/arm_mpcore: Remove default values for GIC external IRQs

[Philippe Mathieu-Daudé]

Implicit default values are often hard to figure out, better
be explicit. Now that all boards explicitly set the number of
GIC external IRQs, remove the default values (displaying an
error message if it is not set).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/cpu/a15mpcore.c | 13 ++++++-------
 hw/cpu/a9mpcore.c  | 14 +++++++-------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index 3b0897e54ee..372b615178f 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -58,6 +58,11 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
     bool has_el2 = false;
     Object *cpuobj;
 
+    if (!s->num_irq) {
+        error_setg(errp, "Property 'num-irq' not set");
+        return;
+    }
+
     gicdev = DEVICE(&s->gic);
     qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
@@ -146,13 +151,7 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
 
 static const Property a15mp_priv_properties[] = {
     DEFINE_PROP_UINT32("num-cpu", A15MPPrivState, num_cpu, 1),
-    /* The Cortex-A15MP may have anything from 0 to 224 external interrupt
-     * IRQ lines (with another 32 internal). We default to 128+32, which
-     * is the number provided by the Cortex-A15MP test chip in the
-     * Versatile Express A15 development board.
-     * Other boards may differ and should set this property appropriately.
-     */
-    DEFINE_PROP_UINT32("num-irq", A15MPPrivState, num_irq, 160),
+    DEFINE_PROP_UINT32("num-irq", A15MPPrivState, num_irq, 0),
 };
 
 static void a15mp_priv_class_init(ObjectClass *klass, void *data)
diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 9671585b5f9..c522f8d4b05 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -56,6 +56,12 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     CPUState *cpu0;
     Object *cpuobj;
 
+
+    if (!s->num_irq) {
+        error_setg(errp, "Property 'num-irq' not set");
+        return;
+    }
+
     cpu0 = qemu_get_cpu(0);
     cpuobj = OBJECT(cpu0);
     if (strcmp(object_get_typename(cpuobj), ARM_CPU_TYPE_NAME("cortex-a9"))) {
@@ -160,13 +166,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
 
 static const Property a9mp_priv_properties[] = {
     DEFINE_PROP_UINT32("num-cpu", A9MPPrivState, num_cpu, 1),
-    /* The Cortex-A9MP may have anything from 0 to 224 external interrupt
-     * IRQ lines (with another 32 internal). We default to 64+32, which
-     * is the number provided by the Cortex-A9MP test chip in the
-     * Realview PBX-A9 and Versatile Express A9 development boards.
-     * Other boards may differ and should set this property appropriately.
-     */
-    DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
+    DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 0),
 };
 
 static void a9mp_priv_class_init(ObjectClass *klass, void *data)
-- 
2.47.1

Re: [PATCH 8/8] hw/cpu/arm_mpcore: Remove default values for GIC external IRQs

[Peter Maydell]

On Thu, 30 Jan 2025 at 18:27, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Implicit default values are often hard to figure out, better
> be explicit. Now that all boards explicitly set the number of
> GIC external IRQs, remove the default values (displaying an
> error message if it is not set).

> diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
> index 3b0897e54ee..372b615178f 100644
> --- a/hw/cpu/a15mpcore.c
> +++ b/hw/cpu/a15mpcore.c
> @@ -58,6 +58,11 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
>      bool has_el2 = false;
>      Object *cpuobj;
>
> +    if (!s->num_irq) {
> +        error_setg(errp, "Property 'num-irq' not set");
> +        return;
> +    }
> +
>      gicdev = DEVICE(&s->gic);
>      qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
>      qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
> @@ -146,13 +151,7 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
>
>  static const Property a15mp_priv_properties[] = {
>      DEFINE_PROP_UINT32("num-cpu", A15MPPrivState, num_cpu, 1),
> -    /* The Cortex-A15MP may have anything from 0 to 224 external interrupt
> -     * IRQ lines (with another 32 internal). We default to 128+32, which
> -     * is the number provided by the Cortex-A15MP test chip in the
> -     * Versatile Express A15 development board.
> -     * Other boards may differ and should set this property appropriately.
> -     */
> -    DEFINE_PROP_UINT32("num-irq", A15MPPrivState, num_irq, 160),
> +    DEFINE_PROP_UINT32("num-irq", A15MPPrivState, num_irq, 0),

I think it's worth keeping at least some form of comment here
to document the valid range and that the value is internal + external
interrupts. Something like:

    /*
     * The Cortex-A15MP may have anything from 0 to 224 external interrupt
     * lines, plus always 32 internal IRQs. This property sets the total
     * of internal + external, so the valid range is from 32 to 256.
     * The board model must set this to whatever the configuration
     * used for the CPU on that board or SoC is.
     */

?

(I suppose we could also actually check the property really
is between 32 and 256, but a simple "did the board actually set
it?" check like you have here is fine.)

> @@ -160,13 +166,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>
>  static const Property a9mp_priv_properties[] = {
>      DEFINE_PROP_UINT32("num-cpu", A9MPPrivState, num_cpu, 1),
> -    /* The Cortex-A9MP may have anything from 0 to 224 external interrupt
> -     * IRQ lines (with another 32 internal). We default to 64+32, which
> -     * is the number provided by the Cortex-A9MP test chip in the
> -     * Realview PBX-A9 and Versatile Express A9 development boards.
> -     * Other boards may differ and should set this property appropriately.
> -     */

Similarly here.

thanks
-- PMM