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