Re: [PATCH v4 1/3] hw/intc/loongarch_pch_msi: add irq number property

[Philippe Mathieu-Daudé]

On 30/12/22 10:59, Tianrui Zhao wrote:
> This patch adds irq number property for loongarch msi interrupt
> controller, and remove hard coding irq number macro.
>
> Signed-off-by: Tianrui Zhao <zhaotianrui@loongson.cn>
> ---
>   hw/intc/loongarch_pch_msi.c         | 33 ++++++++++++++++++++++++++---
>   hw/loongarch/virt.c                 | 13 +++++++-----
>   include/hw/intc/loongarch_pch_msi.h |  3 ++-
>   include/hw/pci-host/ls7a.h          |  1 -
>   4 files changed, 40 insertions(+), 10 deletions(-)


> +static void loongarch_pch_msi_realize(DeviceState *dev, Error **errp)
> +{
> +    LoongArchPCHMSI *s = LOONGARCH_PCH_MSI(dev);
> +
> +    if (!s->irq_num || s->irq_num  > PCH_MSI_IRQ_NUM) {

Here you check for s->irq_num != 0, ...

> +        error_setg(errp, "Invalid 'msi_irq_num'");
> +        return;
> +    }
> +
> +    s->pch_msi_irq = g_new(qemu_irq, s->irq_num);
> +    if (!s->pch_msi_irq) {

... so this check is unreachable / pointless: g_new() will never
return NULL but abort.

> +        error_report("loongarch_pch_msi: fail to alloc memory");
> +        exit(1);
> +    }
> +
> +    qdev_init_gpio_out(dev, s->pch_msi_irq, s->irq_num);
> +    qdev_init_gpio_in(dev, pch_msi_irq_handler, s->irq_num);
> +}

> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index 958be74fa1..1e58346aeb 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -496,7 +496,7 @@ static void loongarch_irq_init(LoongArchMachineState *lams)
>       LoongArchCPU *lacpu;
>       CPULoongArchState *env;
>       CPUState *cpu_state;
> -    int cpu, pin, i;
> +    int cpu, pin, i, start, num;
>   
>       ipi = qdev_new(TYPE_LOONGARCH_IPI);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), &error_fatal);
> @@ -576,14 +576,17 @@ static void loongarch_irq_init(LoongArchMachineState 
> *lams)
>       }
>   
>       pch_msi = qdev_new(TYPE_LOONGARCH_PCH_MSI);
> -    qdev_prop_set_uint32(pch_msi, "msi_irq_base", PCH_MSI_IRQ_START);
> +    start   =  PCH_PIC_IRQ_NUM;

Maybe name this 'msi_irq_base'

> +    num = EXTIOI_IRQS - start;

and 'msi_irq_num' for clarity?

> +    qdev_prop_set_uint32(pch_msi, "msi_irq_base", start);
> +    qdev_prop_set_uint32(pch_msi, "msi_irq_num", num);
>       d = SYS_BUS_DEVICE(pch_msi);
>       sysbus_realize_and_unref(d, &error_fatal);
>       sysbus_mmio_map(d, 0, VIRT_PCH_MSI_ADDR_LOW);
> -    for (i = 0; i < PCH_MSI_IRQ_NUM; i++) {
> -        /* Connect 192 pch_msi irqs to extioi */
> +    for (i = 0; i < num; i++) {
> +        /* Connect pch_msi irqs to extioi */
>           qdev_connect_gpio_out(DEVICE(d), i,
> -                              qdev_get_gpio_in(extioi, i + PCH_MSI_IRQ_START));
> +                              qdev_get_gpio_in(extioi, i + start));
>       }

Removing the unreachable check:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>