Re: [PATCH v2 33/43] hw/intc: Add LoongArch ls7a msi interrupt controller support(PCH-MSI) - Mark Cave-Ayland

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
To: Xiaojuan Yang <yangxiaojuan@loongson.cn>, qemu-devel@nongnu.org
Cc: richard.henderson@linaro.org, gaosong@loongson.cn
Subject: Re: [PATCH v2 33/43] hw/intc: Add LoongArch ls7a msi interrupt controller support(PCH-MSI)
Date: Thu, 28 Apr 2022 09:16:21 +0100	[thread overview]
Message-ID: <491632d9-f60e-7a0d-daf6-4df4b6784b69@ilande.co.uk> (raw)
In-Reply-To: <20220425091027.2877892-34-yangxiaojuan@loongson.cn>

On 25/04/2022 10:10, Xiaojuan Yang wrote:

> This patch realize PCH-MSI interrupt controller.
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/intc/Kconfig                     |  5 ++
>   hw/intc/loongarch_pch_msi.c         | 75 +++++++++++++++++++++++++++++
>   hw/intc/meson.build                 |  1 +
>   hw/intc/trace-events                |  3 ++
>   hw/loongarch/Kconfig                |  1 +
>   include/hw/intc/loongarch_pch_msi.h | 20 ++++++++
>   6 files changed, 105 insertions(+)
>   create mode 100644 hw/intc/loongarch_pch_msi.c
>   create mode 100644 include/hw/intc/loongarch_pch_msi.h
> 
> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
> index 1fbba2e728..71c04c328e 100644
> --- a/hw/intc/Kconfig
> +++ b/hw/intc/Kconfig
> @@ -91,3 +91,8 @@ config LOONGARCH_IPI
>   config LOONGARCH_PCH_PIC
>       bool
>       select UNIMP
> +
> +config LOONGARCH_PCH_MSI
> +    select MSI_NONBROKEN
> +    bool
> +    select UNIMP
> diff --git a/hw/intc/loongarch_pch_msi.c b/hw/intc/loongarch_pch_msi.c
> new file mode 100644
> index 0000000000..57a894f3e5
> --- /dev/null
> +++ b/hw/intc/loongarch_pch_msi.c
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * QEMU Loongson 7A1000 msi interrupt controller.
> + *
> + * Copyright (C) 2021 Loongson Technology Corporation Limited
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "hw/irq.h"
> +#include "hw/intc/loongarch_pch_msi.h"
> +#include "hw/intc/loongarch_pch_pic.h"
> +#include "hw/pci/msi.h"
> +#include "hw/misc/unimp.h"
> +#include "migration/vmstate.h"
> +#include "trace.h"
> +
> +static uint64_t loongarch_msi_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return 0;
> +}
> +
> +static void loongarch_msi_mem_write(void *opaque, hwaddr addr,
> +                                    uint64_t val, unsigned size)
> +{
> +    LoongArchPCHMSI *s = LOONGARCH_PCH_MSI(opaque);
> +    int irq_num = val & 0xff;
> +
> +    trace_loongarch_msi_set_irq(irq_num);
> +    qemu_set_irq(s->pch_msi_irq[irq_num - PCH_PIC_IRQ_NUM], 1);
> +}

This really looks wrong to me: the PCI-MSI device has PCH_PIC_IRQ_NUM IRQs which are 
wired up to EIOINTC with an offset of PCH_MSI_IRQ_START already. What you're saying 
here is that you're manually setting the PCI-MSI IRQs using their EIOINTC IRQ number 
instead of their input IRQ number?

I think what you would want here is just:

    assert(irq_num < PCH_MSI_IRQ_NUM);
    trace_loongarch_msi_set_irq(irq_num);
    qemu_set_irq(s->pch_msi_irq[irq_num], 1);

Otherwise the PCH-MSI device has awareness of IRQ hierarchy outside of itself. 
Presumably this is some kind of testing interface?

> +static const MemoryRegionOps loongarch_pch_msi_ops = {
> +    .read  = loongarch_msi_mem_read,
> +    .write = loongarch_msi_mem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void pch_msi_irq_handler(void *opaque, int irq, int level)
> +{
> +    LoongArchPCHMSI *s = LOONGARCH_PCH_MSI(opaque);
> +
> +    qemu_set_irq(s->pch_msi_irq[irq], level);
> +}
> +
> +static void loongarch_pch_msi_init(Object *obj)
> +{
> +    LoongArchPCHMSI *s = LOONGARCH_PCH_MSI(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    int i;
> +
> +    memory_region_init_io(&s->msi_mmio, obj, &loongarch_pch_msi_ops,
> +                          s, TYPE_LOONGARCH_PCH_MSI, 0x8);
> +    sysbus_init_mmio(sbd, &s->msi_mmio);
> +    msi_nonbroken = true;
> +
> +    for (i = 0; i < PCH_MSI_IRQ_NUM; i++) {
> +        sysbus_init_irq(sbd, &s->pch_msi_irq[i]);
> +    }
> +    qdev_init_gpio_in(DEVICE(obj), pch_msi_irq_handler, PCH_MSI_IRQ_NUM);
> +}

Again I'd simply use GPIOs instead of sysbus_init_irq() here, similar to the example 
given in my reply to the PCH-PIC patch.

> +static const TypeInfo loongarch_pch_msi_info = {
> +    .name          = TYPE_LOONGARCH_PCH_MSI,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(LoongArchPCHMSI),
> +    .instance_init = loongarch_pch_msi_init,
> +};
> +
> +static void loongarch_pch_msi_register_types(void)
> +{
> +    type_register_static(&loongarch_pch_msi_info);
> +}
> +
> +type_init(loongarch_pch_msi_register_types)
> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> index 960ce81a92..77a30cec33 100644
> --- a/hw/intc/meson.build
> +++ b/hw/intc/meson.build
> @@ -64,3 +64,4 @@ specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
>   specific_ss.add(when: 'CONFIG_M68K_IRQC', if_true: files('m68k_irqc.c'))
>   specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: files('loongarch_ipi.c'))
>   specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_PIC', if_true: files('loongarch_pch_pic.c'))
> +specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_MSI', if_true: files('loongarch_pch_msi.c'))
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 1f343676ee..8bcc1b6992 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -300,3 +300,6 @@ loongarch_pch_pic_high_readw(unsigned size, uint32_t addr, unsigned long val) "s
>   loongarch_pch_pic_high_writew(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>   loongarch_pch_pic_readb(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>   loongarch_pch_pic_writeb(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
> +
> +# loongarch_pch_msi.c
> +loongarch_msi_set_irq(int irq_num) "set msi irq %d"
> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
> index 2df45f7e8f..d814fc6103 100644
> --- a/hw/loongarch/Kconfig
> +++ b/hw/loongarch/Kconfig
> @@ -4,3 +4,4 @@ config LOONGARCH_VIRT
>       select PCI_EXPRESS_GENERIC_BRIDGE
>       select LOONGARCH_IPI
>       select LOONGARCH_PCH_PIC
> +    select LOONGARCH_PCH_MSI
> diff --git a/include/hw/intc/loongarch_pch_msi.h b/include/hw/intc/loongarch_pch_msi.h
> new file mode 100644
> index 0000000000..f668bfca7a
> --- /dev/null
> +++ b/include/hw/intc/loongarch_pch_msi.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * LoongArch 7A1000 I/O interrupt controller definitions
> + *
> + * Copyright (C) 2021 Loongson Technology Corporation Limited
> + */
> +
> +#define TYPE_LOONGARCH_PCH_MSI "loongarch_pch_msi"
> +OBJECT_DECLARE_SIMPLE_TYPE(LoongArchPCHMSI, LOONGARCH_PCH_MSI)
> +
> +/* Msi irq start start from 64 to 255 */
> +#define PCH_MSI_IRQ_START   64
> +#define PCH_MSI_IRQ_END     255
> +#define PCH_MSI_IRQ_NUM     192
> +
> +struct LoongArchPCHMSI {
> +    SysBusDevice parent_obj;
> +    qemu_irq pch_msi_irq[PCH_MSI_IRQ_NUM];
> +    MemoryRegion msi_mmio;
> +};

ATB,

Mark.