Re: [RFC PATCH v3 18/27] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

[yangxiaojuan]

Hi,Mark:

  Sorry for the late reply. I just saw the mail after I send the v4 patch. I 
sorted the mail into different folders from
the qemu-devel, so I didn't see the mail in time. Sorry again.

Xiaojuan

On 12/23/2021 06:21 PM, Mark Cave-Ayland wrote:
> On 22/12/2021 02:38, yangxiaojuan wrote:
> 
>> Hi, Mark
>>
>> On 12/18/2021 08:33 AM, Mark Cave-Ayland wrote:
>>> On 04/12/2021 12:07, Xiaojuan Yang wrote:
>>>
>>>> This patch realize the PCH-PIC interrupt controller.
>>>>
>>>> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>>>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>>>> ---
>>>>    hw/intc/Kconfig                     |   4 +
>>>>    hw/intc/loongarch_pch_pic.c         | 357 ++++++++++++++++++++++++++++
>>>>    hw/intc/meson.build                 |   1 +
>>>>    hw/intc/trace-events                |   5 +
>>>>    hw/loongarch/Kconfig                |   1 +
>>>>    include/hw/intc/loongarch_pch_pic.h |  61 +++++
>>>>    6 files changed, 429 insertions(+)
>>>>    create mode 100644 hw/intc/loongarch_pch_pic.c
>>>>    create mode 100644 include/hw/intc/loongarch_pch_pic.h
>>>>
>>>> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
>>>> index 511dcac537..96da13ad1d 100644
>>>> --- a/hw/intc/Kconfig
>>>> +++ b/hw/intc/Kconfig
>>>> @@ -76,3 +76,7 @@ config M68K_IRQC
>>>>      config LOONGARCH_IPI
>>>>        bool
>>>> +
>>>> +config LOONGARCH_PCH_PIC
>>>> +    bool
>>>> +    select UNIMP
>>>> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
>>>> new file mode 100644
>>>> index 0000000000..2ede29ceb0
>>>> --- /dev/null
>>>> +++ b/hw/intc/loongarch_pch_pic.c
>>>> @@ -0,0 +1,357 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * QEMU Loongson 7A1000 I/O interrupt controller.
>>>> + *
>>>> + * Copyright (C) 2021 Loongson Technology Corporation Limited
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "hw/sysbus.h"
>>>> +#include "hw/loongarch/loongarch.h"
>>>> +#include "hw/irq.h"
>>>> +#include "hw/intc/loongarch_pch_pic.h"
>>>> +#include "migration/vmstate.h"
>>>> +#include "trace.h"
>>>> +
>>>> +#define for_each_set_bit(bit, addr, size) \
>>>> +         for ((bit) = find_first_bit((addr), (size));            \
>>>> +              (bit) < (size);                                    \
>>>> +              (bit) = find_next_bit((addr), (size), (bit) + 1))
>>>> +
>>>> +static void pch_pic_update_irq(loongarch_pch_pic *s, uint64_t mask, int 
>>>> level)
>>>> +{
>>>> +    int i;
>>>> +    uint64_t val;
>>>> +    val = mask & s->intirr & (~s->int_mask);
>>>> +
>>>> +    for_each_set_bit(i, &val, 64) {
>>>> +        if (level == 1) {
>>>> +            if ((s->intisr & (0x1ULL << i)) == 0) {
>>>> +                s->intisr |= 1ULL << i;
>>>> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 1);
>>>> +            }
>>>> +        } else if (level == 0) {
>>>> +            if (s->intisr & (0x1ULL << i)) {
>>>> +                s->intisr &= ~(0x1ULL << i);
>>>> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 0);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +}
>>>
>>> The normal pattern would be to use something like:
>>>
>>> for (i = 0; i < 64; i++) {
>>>      if (level) {
>>>          s->intisr |= 1ULL << i;
>>>      } else {
>>>          s->intisr &= ~(0x1ULL << i);
>>>      }
>>>
>>>      qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], level);
>>> }
>>>
>>> Why is it necessary to check the previous value of (s->intisr & (0x1ULL << 
>>> i)) here?
>>
>> Here check the previous value to avoid Unnecessary write. It seems make 
>> things more complicated. I will modify
> 
> In general a *_update_irq() function should be fine to propagate the IRQ up 
> to the parent directly: I think this is fine in this case because you are 
> directly manipulating the parent_irq elements rather than using e.g. a 
> priority encoder within this device to raise an IRQ to the CPU. I'm presuming 
> this final prioritisation and delivery is done elsewhere?
> 
>>>> +static void pch_pic_irq_handler(void *opaque, int irq, int level)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>>> +
>>>> +    assert(irq < PCH_PIC_IRQ_NUM);
>>>> +    uint64_t mask = 1ULL << irq;
>>>> +
>>>> +    trace_pch_pic_irq_handler(s->intedge, irq, level);
>>>> +
>>>> +    if (s->intedge & mask) {
>>>> +        /* Edge triggered */
>>>> +        if (level) {
>>>> +            if ((s->last_intirr & mask) == 0) {
>>>> +                s->intirr |= mask;
>>>> +            }
>>>> +            s->last_intirr |= mask;
>>>> +        } else {
>>>> +            s->last_intirr &= ~mask;
>>>> +        }
>>>> +    } else {
>>>> +        /* Level triggered */
>>>> +        if (level) {
>>>> +            s->intirr |= mask;
>>>> +            s->last_intirr |= mask;
>>>> +        } else {
>>>> +            s->intirr &= ~mask;
>>>> +            s->last_intirr &= ~mask;
>>>> +        }
>>>> +
>>>> +    }
>>>> +    pch_pic_update_irq(s, mask, level);
>>>> +}
>>>> +
>>>> +static uint64_t loongarch_pch_pic_reg_read(void *opaque, hwaddr addr,
>>>> +                                           unsigned size)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>>> +    uint64_t val = 0;
>>>> +    uint32_t offset = addr & 0xfff;
>>>> +    int64_t offset_tmp;
>>>> +
>>>> +    if (size == 8) {
>>>> +        switch (offset) {
>>>> +        case PCH_PIC_INT_ID_OFFSET:
>>>> +            val = (PCH_PIC_INT_ID_NUM << 32) | PCH_PIC_INT_ID_VAL;
>>>> +            break;
>>>> +        case PCH_PIC_INT_MASK_OFFSET:
>>>> +            val =  s->int_mask;
>>>> +            break;
>>>> +        case PCH_PIC_INT_STATUS_OFFSET:
>>>> +            val = s->intisr & (~s->int_mask);
>>>> +            break;
>>>> +        case PCH_PIC_INT_EDGE_OFFSET:
>>>> +            val = s->intedge;
>>>> +            break;
>>>> +        case PCH_PIC_INT_POL_OFFSET:
>>>> +            val = s->int_polarity;
>>>> +            break;
>>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>>> +            val = s->htmsi_en;
>>>> +            break;
>>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>>> +            break;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    } else if (size == 4) {
>>>> +        switch (offset) {
>>>> +        case PCH_PIC_INT_ID_OFFSET:
>>>> +            val = PCH_PIC_INT_ID_VAL;
>>>> +            break;
>>>> +        case PCH_PIC_INT_ID_OFFSET + 4:
>>>> +            val = PCH_PIC_INT_ID_NUM;
>>>> +            break;
>>>> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
>>>> +            val = ldl_p((void *)&s->int_mask +
>>>> +                        (offset - PCH_PIC_INT_MASK_OFFSET));
>>>> +            break;
>>>> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
>>>> +            val = ldl_p((void *)&s->intisr +
>>>> +                        (offset - PCH_PIC_INT_STATUS_OFFSET)) & 
>>>> (~s->int_mask);
>>>> +            break;
>>>> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
>>>> +            val = ldl_p((void *)&s->intedge +
>>>> +                        (offset - PCH_PIC_INT_EDGE_OFFSET));
>>>> +            break;
>>>> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
>>>> +            val = ldl_p((void *)&s->int_polarity +
>>>> +                        (offset - PCH_PIC_INT_POL_OFFSET));
>>>> +            break;
>>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>>> +            val = ldl_p((void *)&s->htmsi_en +
>>>> +                        (offset - PCH_PIC_HTMSI_EN_OFFSET));
>>>> +            break;
>>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>>> +            break;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    } else if (size == 1) {
>>>> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
>>>> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
>>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>>> +                val = s->htmsi_vector[offset_tmp];
>>>> +            }
>>>> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
>>>> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
>>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>>> +                val = s->route_entry[offset_tmp];
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    trace_loongarch_pch_pic_read(size, (uint32_t)addr, val);
>>>> +    return val;
>>>> +}
>>>> +
>>>> +static void loongarch_pch_pic_reg_write(void *opaque, hwaddr addr,
>>>> +                                        uint64_t data, unsigned size)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>>
>>> Perhaps use LoongArchPCHPIC for the camel case version?
>>
>> OK, all interrupt controller will use the camel case.
>>
>>>
>>>> +    int32_t offset_tmp;
>>>> +    uint32_t offset, old;
>>>> +    offset = addr & 0xfff;
>>>> +
>>>> +    trace_loongarch_pch_pic_write(size, (uint32_t)addr, data);
>>>> +
>>>> +    if (size == 8) {
>>>> +        switch (offset) {
>>>> +        case PCH_PIC_INT_MASK_OFFSET:
>>>> +            old = s->int_mask;
>>>> +            s->int_mask = data;
>>>> +            if (old & ~data) {
>>>> +                pch_pic_update_irq(s, (old & ~data), 1);
>>>> +            } else if (~old & data) {
>>>> +                pch_pic_update_irq(s, (~old & data), 0);
>>>> +            }
>>>> +            break;
>>>> +        case PCH_PIC_INT_STATUS_OFFSET:
>>>> +            s->intisr = data;
>>>> +            break;
>>>> +        case PCH_PIC_INT_EDGE_OFFSET:
>>>> +            s->intedge = data;
>>>> +            break;
>>>> +        case PCH_PIC_INT_CLEAR_OFFSET:
>>>> +            s->intirr &= (~(data & s->intedge));
>>>> +            pch_pic_update_irq(s, data, 0);
>>>> +            s->intisr &= (~data);
>>>> +            break;
>>>> +        case PCH_PIC_INT_POL_OFFSET:
>>>> +            s->int_polarity = data;
>>>> +            break;
>>>> +        case PCH_PIC_HTMSI_EN_OFFSET:
>>>> +            s->htmsi_en = data;
>>>> +            break;
>>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET:
>>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET:
>>>> +            break;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    } else if (size == 4) {
>>>> +        switch (offset) {
>>>> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
>>>> +            offset -= PCH_PIC_INT_MASK_OFFSET;
>>>> +            old = ldl_p((void *)&s->int_mask + offset);
>>>> +            stl_p((void *)&s->int_mask + offset, data);
>>>> +
>>>> +            if (old & ~data) {
>>>> +                pch_pic_update_irq(s, (old & ~data), 1);
>>>> +            } else if (~old & data) {
>>>> +                pch_pic_update_irq(s, (~old & data), 0);
>>>> +            }
>>>> +            break;
>>>> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
>>>> +            stl_p((void *)&s->intisr + (offset - 
>>>> PCH_PIC_INT_STATUS_OFFSET),
>>>> +                  data);
>>>> +            break;
>>>> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
>>>> +            stl_p((void *)&s->intedge + (offset - 
>>>> PCH_PIC_INT_EDGE_OFFSET),
>>>> +                  data);
>>>> +            break;
>>>> +        case PCH_PIC_INT_CLEAR_OFFSET...PCH_PIC_INT_CLEAR_END:
>>>> +            old = s->intirr & (~(data & s->intedge));
>>>> +            stl_p((void *)&s->intirr + (offset - 
>>>> PCH_PIC_INT_CLEAR_OFFSET),
>>>> +                  old);
>>>> +            pch_pic_update_irq(s, data, 0);
>>>> +            old = s->intisr & (~data);
>>>> +            stl_p((void *)&s->intisr + (offset - 
>>>> PCH_PIC_INT_CLEAR_OFFSET),
>>>> +                  old);
>>>> +            break;
>>>> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
>>>> +            stl_p((void *)&s->int_polarity + (offset - 
>>>> PCH_PIC_INT_POL_OFFSET),
>>>> +                  data);
>>>> +            break;
>>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>>> +            stl_p((void *)&s->htmsi_en + (offset - 
>>>> PCH_PIC_HTMSI_EN_OFFSET),
>>>> +                  data);
>>>> +            break;
>>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>>> +            break;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    } else if (size == 1) {
>>>> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
>>>> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
>>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>>> +                s->htmsi_vector[offset_tmp] = (uint8_t)(data & 0xff);
>>>> +            }
>>>> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
>>>> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
>>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>>> +                s->route_entry[offset_tmp] = (uint8_t)(data & 0xff);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps loongarch_pch_pic_ops = {
>>>> +    .read = loongarch_pch_pic_reg_read,
>>>> +    .write = loongarch_pch_pic_reg_write,
>>>> +    .valid = {
>>>> +        .min_access_size = 1,
>>>> +        .max_access_size = 8,
>>>> +    },
>>>> +    .impl = {
>>>> +        .min_access_size = 1,
>>>> +        .max_access_size = 8,
>>>> +    },
>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>> +};
>>>
>>> It feels like this should be split into 2 separate memory regions: as per 
>>> before set .min_access_size = 4 for PCH_PIC_INT_MASK_OFFSET ... 
>>> PCH_PIC_AUTO_CTRL1_END and use uint32_t for the underlying types in 
>>> loongarch_pch_pic.
>>>
>>> This leaves a separate memory region for accesses > 
>>> PCH_PIC_HTMSI_VEC_OFFSET which can be set with .min_access_size = 1 which 
>>> maps nicely to the underlying uint8_t types. Which means you should be able 
>>> to access the arrays directly instead of having to use ldl_p() and stl_p().
>>>
>>
>> yeah, here the PCH_PIC_HTMSI_VEC_OFFSET region is right in the middle, I 
>> want to use a separate memory region to overlap the pch-pic region, is it ok 
>> to do like this? Thanks
> 
> That should be fine. I'd also use a single container memory region so that 
> you only have to expose a single memory region with sysbus_init_mmio() as 
> below:
> 
>>> +static void loongarch_pch_pic_reset(DeviceState *d)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(d);
>>>> +    int i;
>>>> +
>>>> +    s->int_mask = -1ULL;
>>>> +    s->htmsi_en = 0x0;
>>>> +    s->intedge  = 0x0;
>>>> +    s->intclr   = 0x0;
>>>> +    s->auto_crtl0 = 0x0;
>>>> +    s->auto_crtl1 = 0x0;
>>>> +    for (i = 0; i < 64; i++) {
>>>> +        s->route_entry[i] = 0x1;
>>>> +        s->htmsi_vector[i] = 0x0;
>>>> +    }
>>>> +    s->intirr = 0x0;
>>>> +    s->intisr = 0x0;
>>>> +    s->last_intirr = 0x0;
>>>> +    s->int_polarity = 0x0;
>>>> +}
>>>> +
>>>> +static void loongarch_pch_pic_init(Object *obj)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(obj);
>>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>>> +    int i;
>>>> +
>>>> +    memory_region_init_io(&s->iomem, obj, &loongarch_pch_pic_ops,
>>>> +                          s, TYPE_LOONGARCH_PCH_PIC, 0x1000);
>>>> +    sysbus_init_mmio(sbd, &s->iomem);
> 
> Please adjust the example below for your particular case replacing the sizes 
> as appropriate but I would expect it to look something like:
> 
>     memory_region_init(&s->picmem, obj, TYPE_LOONGARCH_PCH_PIC, 0x1000);
> 
>     memory_region_init_io(&s->picintmem, obj, &loongarch_pch_pic_int_ops,
>                           s, "pic.int", 0xaaaa);
>     memory_region_add_subregion(&s->picmem, INT_OFFSET, &s->picintmem);
> 
>     memory_region_init_io(&s->picintmem, obj, &loongarch_pch_pic_htmsi_ops,
>                           s, "pic.htmsi", 0xbbbb);
>     memory_region_add_subregion(&s->picmem, HTMSI_OFFSET, &s->picintmem);
> 
>     memory_region_init_io(&s->picroutemem, obj, &loongarch_pch_pic_route_ops,
>                           s, "pic.route", 0xcccc);
>     memory_region_add_subregion(&s->picmem, ROUTE_OFFSET, &s->picroutemem);
> 
>     sysbus_init_mmio(sbd, &s->picmem);
> 
> You can then configure each of the *_ops memory regions with the correct 
> .min.access and .max.access sizes accordingly, and each region will show up 
> separately in the output of "info mtree".
> 
>>>> +    for (i = 0; i < PCH_PIC_IRQ_NUM; i++) {
>>>> +        sysbus_init_irq(sbd, &s->parent_irq[i]);
>>>> +    }
>>>> +    qdev_init_gpio_in(DEVICE(obj), pch_pic_irq_handler, PCH_PIC_IRQ_NUM);
>>>> +}
>>>> +
>>>> +static const VMStateDescription vmstate_loongarch_pch_pic = {
>>>> +    .name = TYPE_LOONGARCH_PCH_PIC,
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .fields = (VMStateField[]) {
>>>> +        VMSTATE_UINT64(int_mask, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(htmsi_en, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(intedge, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(intclr, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(auto_crtl0, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(auto_crtl1, loongarch_pch_pic),
>>>> +        VMSTATE_UINT8_ARRAY(route_entry, loongarch_pch_pic, 64),
>>>> +        VMSTATE_UINT8_ARRAY(htmsi_vector, loongarch_pch_pic, 64),
>>>> +        VMSTATE_UINT64(last_intirr, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(intirr, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(intisr, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(int_polarity, loongarch_pch_pic),
>>>> +        VMSTATE_END_OF_LIST()
>>>> +    }
>>>> +};
>>>> +
>>>> +static void loongarch_pch_pic_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +
>>>> +    dc->reset = loongarch_pch_pic_reset;
>>>> +    dc->vmsd = &vmstate_loongarch_pch_pic;
>>>> +}
>>>> +
>>>> +static const TypeInfo loongarch_pch_pic_info = {
>>>> +    .name          = TYPE_LOONGARCH_PCH_PIC,
>>>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>>>> +    .instance_size = sizeof(loongarch_pch_pic),
>>>> +    .instance_init = loongarch_pch_pic_init,
>>>> +    .class_init    = loongarch_pch_pic_class_init,
>>>> +};
>>>> +
>>>> +static void loongarch_pch_pic_register_types(void)
>>>> +{
>>>> +    type_register_static(&loongarch_pch_pic_info);
>>>> +}
>>>> +
>>>> +type_init(loongarch_pch_pic_register_types)
>>>> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
>>>> index 51f0c3988a..33ba63266e 100644
>>>> --- a/hw/intc/meson.build
>>>> +++ b/hw/intc/meson.build
>>>> @@ -58,3 +58,4 @@ specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
>>>>    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'))
>>>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>>>> index 124608e51f..52fedf82be 100644
>>>> --- a/hw/intc/trace-events
>>>> +++ b/hw/intc/trace-events
>>>> @@ -250,3 +250,8 @@ sh_intc_set(int id, int enable) "setting interrupt 
>>>> group %d to %d"
>>>>    # loongarch_ipi.c
>>>>    loongarch_ipi_read(unsigned size, uint32_t addr, unsigned long val) 
>>>> "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>>>>    loongarch_ipi_write(unsigned size, uint32_t addr, unsigned long val) 
>>>> "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>>>> +
>>>> +# loongarch_pch_pic.c
>>>> +pch_pic_irq_handler(uint32_t edge, int irq, int level) "edge 0x%02x irq 
>>>> %d level %d"
>>>> +loongarch_pch_pic_read(unsigned size, uint32_t addr, unsigned long val) 
>>>> "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>>>> +loongarch_pch_pic_write(unsigned size, uint32_t addr, unsigned long val) 
>>>> "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>>>> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
>>>> index 1591574397..c2b8046b94 100644
>>>> --- a/hw/loongarch/Kconfig
>>>> +++ b/hw/loongarch/Kconfig
>>>> @@ -2,3 +2,4 @@ config LOONGSON3_LS7A
>>>>        bool
>>>>        select PCI_EXPRESS_7A
>>>>        select LOONGARCH_IPI
>>>> +    select LOONGARCH_PCH_PIC
>>>> diff --git a/include/hw/intc/loongarch_pch_pic.h 
>>>> b/include/hw/intc/loongarch_pch_pic.h
>>>> new file mode 100644
>>>> index 0000000000..bc04ed28ef
>>>> --- /dev/null
>>>> +++ b/include/hw/intc/loongarch_pch_pic.h
>>>> @@ -0,0 +1,61 @@
>>>> +/* 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_PIC "loongarch_pch_pic"
>>>> +DECLARE_INSTANCE_CHECKER(struct loongarch_pch_pic, LOONGARCH_PCH_PIC,
>>>> +                         TYPE_LOONGARCH_PCH_PIC)
>>>> +
>>>> +#define PCH_PIC_IRQ_START               0
>>>> +#define PCH_PIC_IRQ_END                 63
>>>> +#define PCH_PIC_IRQ_NUM                 64
>>>> +#define PCH_PIC_INT_ID_VAL              0x7000000UL
>>>> +#define PCH_PIC_INT_ID_NUM              0x3f0001UL
>>>> +
>>>> +#define PCH_PIC_INT_ID_OFFSET           0x00
>>>> +#define PCH_PIC_INT_ID_END              0x07
>>>> +#define PCH_PIC_INT_MASK_OFFSET         0x20
>>>> +#define PCH_PIC_INT_MASK_END            0x27
>>>> +#define PCH_PIC_HTMSI_EN_OFFSET         0x40
>>>> +#define PCH_PIC_HTMSI_EN_END            0x47
>>>> +#define PCH_PIC_INT_EDGE_OFFSET         0x60
>>>> +#define PCH_PIC_INT_EDGE_END            0x67
>>>> +#define PCH_PIC_INT_CLEAR_OFFSET        0x80
>>>> +#define PCH_PIC_INT_CLEAR_END           0x87
>>>> +#define PCH_PIC_AUTO_CTRL0_OFFSET       0xc0
>>>> +#define PCH_PIC_AUTO_CTRL0_END          0xc7
>>>> +#define PCH_PIC_AUTO_CTRL1_OFFSET       0xe0
>>>> +#define PCH_PIC_AUTO_CTRL1_END          0xe8
>>>> +#define PCH_PIC_ROUTE_ENTRY_OFFSET      0x100
>>>> +#define PCH_PIC_ROUTE_ENTRY_END         0x13f
>>>> +#define PCH_PIC_HTMSI_VEC_OFFSET        0x200
>>>> +#define PCH_PIC_HTMSI_VEC_END           0x23f
>>>> +#define PCH_PIC_INT_STATUS_OFFSET       0x3a0
>>>> +#define PCH_PIC_INT_STATUS_END          0x3a7
>>>> +#define PCH_PIC_INT_POL_OFFSET          0x3e0
>>>> +#define PCH_PIC_INT_POL_END             0x3e7
>>>> +
>>>> +typedef struct loongarch_pch_pic {
>>>> +    SysBusDevice parent_obj;
>>>> +    qemu_irq parent_irq[64];
>>>> +    uint64_t int_mask; /*0x020 interrupt mask register*/
>>>> +    uint64_t htmsi_en;/*0x040 1=msi*/
>>>> +    uint64_t intedge; /*0x060 edge=1 level  =0*/
>>>> +    uint64_t intclr; /*0x080 for clean edge int,set 1 clean,set 0 is 
>>>> noused*/
>>>> +    uint64_t auto_crtl0; /*0x0c0*/
>>>> +    uint64_t auto_crtl1; /*0x0e0*/
>>>> +    uint8_t route_entry[64]; /*0x100 - 0x138*/
>>>> +    uint8_t htmsi_vector[64]; /*0x200 - 0x238*/
>>>> +    uint64_t last_intirr;    /* edge detection */
>>>> +    uint64_t intirr; /* 0x380 interrupt request register */
>>>> +    uint64_t intisr; /* 0x3a0 interrupt service register */
>>>> +    /*
>>>> +     * 0x3e0 interrupt level polarity selection
>>>> +     * register 0 for high level trigger
>>>> +     */
>>>> +    uint64_t int_polarity;
>>>> +    MemoryRegion iomem;
>>>> +} loongarch_pch_pic;
> 
> ATB,
> 
> Mark.