Re: [PATCH v1 1/2] hw/intc/loongarch_pch_msi: add irq number property

[Philippe Mathieu-Daudé]

On 15/12/22 07:50, 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         | 22 +++++++++++++++++++---
>   hw/loongarch/virt.c                 | 11 +++++++----
>   include/hw/intc/loongarch_pch_msi.h |  3 ++-
>   include/hw/pci-host/ls7a.h          |  1 -
>   4 files changed, 28 insertions(+), 9 deletions(-)


> @@ -49,6 +49,22 @@ static void pch_msi_irq_handler(void *opaque, int irq, int 
> level)
>       qemu_set_irq(s->pch_msi_irq[irq], level);
>   }
>   
> +static void loongarch_pch_msi_realize(DeviceState *dev, Error **errp)
> +{
> +    LoongArchPCHMSI *s = LOONGARCH_PCH_MSI(dev);
> +
> +    assert(s->irq_num > 0);

       if (!s->irq_num || s->irq_num  > PCH_MSI_IRQ_NUM) {
           error_setg(errp, "Invalid 'msi_irq_num'");
           return;
       }

> +    s->pch_msi_irq = g_malloc(sizeof(qemu_irq) * s->irq_num);

       s->pch_msi_irq = g_new(qemu_irq, s->irq_num);

> +    if (!s->pch_msi_irq) {
> +        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);
> +}

Missing g_free(s->pch_msi_irq) in loongarch_pch_msi_unrealize().

>   static void loongarch_pch_msi_init(Object *obj)
>   {
>       LoongArchPCHMSI *s = LOONGARCH_PCH_MSI(obj);
> @@ -59,12 +75,11 @@ static void loongarch_pch_msi_init(Object *obj)
>       sysbus_init_mmio(sbd, &s->msi_mmio);
>       msi_nonbroken = true;
>   
> -    qdev_init_gpio_out(DEVICE(obj), s->pch_msi_irq, PCH_MSI_IRQ_NUM);
> -    qdev_init_gpio_in(DEVICE(obj), pch_msi_irq_handler, PCH_MSI_IRQ_NUM);
>   }
>   
>   static Property loongarch_msi_properties[] = {
>       DEFINE_PROP_UINT32("msi_irq_base", LoongArchPCHMSI, irq_base, 0),
> +    DEFINE_PROP_UINT32("msi_irq_num",  LoongArchPCHMSI, irq_num, 0),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> @@ -72,6 +87,7 @@ static void loongarch_pch_msi_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
> +    dc->realize = loongarch_pch_msi_realize;

       dc->unrealize = loongarch_pch_msi_unrealize;

>       device_class_set_props(dc, loongarch_msi_properties);
>   }
>   
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index 958be74fa1..3547d5f711 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;
> +    num = 256 - start;

This part is confuse. So you don't need PCH_MSI_IRQ_START anymore?
What is this magic '256' value?

> +    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++) {
> +    for (i = 0; i < num; i++) {
>           /* Connect 192 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));
>       }