It was missed out in previous commit.

Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 hw/intc/loongarch_ipi.c | 347 ------------------------------------------------
 1 file changed, 347 deletions(-)

diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
deleted file mode 100644
index 44b3b9c138d6..000000000000
--- a/hw/intc/loongarch_ipi.c
+++ /dev/null
@@ -1,347 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * LoongArch ipi interrupt support
- *
- * Copyright (C) 2021 Loongson Technology Corporation Limited
- */
-
-#include "qemu/osdep.h"
-#include "hw/boards.h"
-#include "hw/sysbus.h"
-#include "hw/intc/loongarch_ipi.h"
-#include "hw/irq.h"
-#include "hw/qdev-properties.h"
-#include "qapi/error.h"
-#include "qemu/log.h"
-#include "exec/address-spaces.h"
-#include "migration/vmstate.h"
-#include "target/loongarch/cpu.h"
-#include "trace.h"
-
-static MemTxResult loongarch_ipi_readl(void *opaque, hwaddr addr,
-                                       uint64_t *data,
-                                       unsigned size, MemTxAttrs attrs)
-{
-    IPICore *s;
-    LoongArchIPI *ipi = opaque;
-    uint64_t ret = 0;
-    int index = 0;
-
-    s = &ipi->cpu[attrs.requester_id];
-    addr &= 0xff;
-    switch (addr) {
-    case CORE_STATUS_OFF:
-        ret = s->status;
-        break;
-    case CORE_EN_OFF:
-        ret = s->en;
-        break;
-    case CORE_SET_OFF:
-        ret = 0;
-        break;
-    case CORE_CLEAR_OFF:
-        ret = 0;
-        break;
-    case CORE_BUF_20 ... CORE_BUF_38 + 4:
-        index = (addr - CORE_BUF_20) >> 2;
-        ret = s->buf[index];
-        break;
-    default:
-        qemu_log_mask(LOG_UNIMP, "invalid read: %x", (uint32_t)addr);
-        break;
-    }
-
-    trace_loongarch_ipi_read(size, (uint64_t)addr, ret);
-    *data = ret;
-    return MEMTX_OK;
-}
-
-static void send_ipi_data(CPULoongArchState *env, uint64_t val, hwaddr addr,
-                          MemTxAttrs attrs)
-{
-    int i, mask = 0, data = 0;
-
-    /*
-     * bit 27-30 is mask for byte writing,
-     * if the mask is 0, we need not to do anything.
-     */
-    if ((val >> 27) & 0xf) {
-        data = address_space_ldl(env->address_space_iocsr, addr,
-                                 attrs, NULL);
-        for (i = 0; i < 4; i++) {
-            /* get mask for byte writing */
-            if (val & (0x1 << (27 + i))) {
-                mask |= 0xff << (i * 8);
-            }
-        }
-    }
-
-    data &= mask;
-    data |= (val >> 32) & ~mask;
-    address_space_stl(env->address_space_iocsr, addr,
-                      data, attrs, NULL);
-}
-
-static int archid_cmp(const void *a, const void *b)
-{
-   CPUArchId *archid_a = (CPUArchId *)a;
-   CPUArchId *archid_b = (CPUArchId *)b;
-
-   return archid_a->arch_id - archid_b->arch_id;
-}
-
-static CPUArchId *find_cpu_by_archid(MachineState *ms, uint32_t id)
-{
-    CPUArchId apic_id, *found_cpu;
-
-    apic_id.arch_id = id;
-    found_cpu = bsearch(&apic_id, ms->possible_cpus->cpus,
-        ms->possible_cpus->len, sizeof(*ms->possible_cpus->cpus),
-        archid_cmp);
-
-    return found_cpu;
-}
-
-static CPUState *ipi_getcpu(int arch_id)
-{
-    MachineState *machine = MACHINE(qdev_get_machine());
-    CPUArchId *archid;
-
-    archid = find_cpu_by_archid(machine, arch_id);
-    if (archid) {
-        return CPU(archid->cpu);
-    }
-
-    return NULL;
-}
-
-static MemTxResult mail_send(uint64_t val, MemTxAttrs attrs)
-{
-    uint32_t cpuid;
-    hwaddr addr;
-    CPUState *cs;
-
-    cpuid = extract32(val, 16, 10);
-    cs = ipi_getcpu(cpuid);
-    if (cs == NULL) {
-        return MEMTX_DECODE_ERROR;
-    }
-
-    /* override requester_id */
-    addr = SMP_IPI_MAILBOX + CORE_BUF_20 + (val & 0x1c);
-    attrs.requester_id = cs->cpu_index;
-    send_ipi_data(&LOONGARCH_CPU(cs)->env, val, addr, attrs);
-    return MEMTX_OK;
-}
-
-static MemTxResult any_send(uint64_t val, MemTxAttrs attrs)
-{
-    uint32_t cpuid;
-    hwaddr addr;
-    CPUState *cs;
-
-    cpuid = extract32(val, 16, 10);
-    cs = ipi_getcpu(cpuid);
-    if (cs == NULL) {
-        return MEMTX_DECODE_ERROR;
-    }
-
-    /* override requester_id */
-    addr = val & 0xffff;
-    attrs.requester_id = cs->cpu_index;
-    send_ipi_data(&LOONGARCH_CPU(cs)->env, val, addr, attrs);
-    return MEMTX_OK;
-}
-
-static MemTxResult loongarch_ipi_writel(void *opaque, hwaddr addr, uint64_t val,
-                                        unsigned size, MemTxAttrs attrs)
-{
-    LoongArchIPI *ipi = opaque;
-    IPICore *s;
-    int index = 0;
-    uint32_t cpuid;
-    uint8_t vector;
-    CPUState *cs;
-
-    s = &ipi->cpu[attrs.requester_id];
-    addr &= 0xff;
-    trace_loongarch_ipi_write(size, (uint64_t)addr, val);
-    switch (addr) {
-    case CORE_STATUS_OFF:
-        qemu_log_mask(LOG_GUEST_ERROR, "can not be written");
-        break;
-    case CORE_EN_OFF:
-        s->en = val;
-        break;
-    case CORE_SET_OFF:
-        s->status |= val;
-        if (s->status != 0 && (s->status & s->en) != 0) {
-            qemu_irq_raise(s->irq);
-        }
-        break;
-    case CORE_CLEAR_OFF:
-        s->status &= ~val;
-        if (s->status == 0 && s->en != 0) {
-            qemu_irq_lower(s->irq);
-        }
-        break;
-    case CORE_BUF_20 ... CORE_BUF_38 + 4:
-        index = (addr - CORE_BUF_20) >> 2;
-        s->buf[index] = val;
-        break;
-    case IOCSR_IPI_SEND:
-        cpuid = extract32(val, 16, 10);
-        /* IPI status vector */
-        vector = extract8(val, 0, 5);
-        cs = ipi_getcpu(cpuid);
-        if (cs == NULL) {
-            return MEMTX_DECODE_ERROR;
-        }
-
-        /* override requester_id */
-        attrs.requester_id = cs->cpu_index;
-        loongarch_ipi_writel(ipi, CORE_SET_OFF, BIT(vector), 4, attrs);
-        break;
-    default:
-        qemu_log_mask(LOG_UNIMP, "invalid write: %x", (uint32_t)addr);
-        break;
-    }
-
-    return MEMTX_OK;
-}
-
-static const MemoryRegionOps loongarch_ipi_ops = {
-    .read_with_attrs = loongarch_ipi_readl,
-    .write_with_attrs = loongarch_ipi_writel,
-    .impl.min_access_size = 4,
-    .impl.max_access_size = 4,
-    .valid.min_access_size = 4,
-    .valid.max_access_size = 8,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-/* mail send and any send only support writeq */
-static MemTxResult loongarch_ipi_writeq(void *opaque, hwaddr addr, uint64_t val,
-                                        unsigned size, MemTxAttrs attrs)
-{
-    MemTxResult ret = MEMTX_OK;
-
-    addr &= 0xfff;
-    switch (addr) {
-    case MAIL_SEND_OFFSET:
-        ret = mail_send(val, attrs);
-        break;
-    case ANY_SEND_OFFSET:
-        ret = any_send(val, attrs);
-        break;
-    default:
-       break;
-    }
-
-    return ret;
-}
-
-static const MemoryRegionOps loongarch_ipi64_ops = {
-    .write_with_attrs = loongarch_ipi_writeq,
-    .impl.min_access_size = 8,
-    .impl.max_access_size = 8,
-    .valid.min_access_size = 8,
-    .valid.max_access_size = 8,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-static void loongarch_ipi_realize(DeviceState *dev, Error **errp)
-{
-    LoongArchIPI *s = LOONGARCH_IPI(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-    int i;
-
-    if (s->num_cpu == 0) {
-        error_setg(errp, "num-cpu must be at least 1");
-        return;
-    }
-
-    memory_region_init_io(&s->ipi_iocsr_mem, OBJECT(dev), &loongarch_ipi_ops,
-                          s, "loongarch_ipi_iocsr", 0x48);
-
-    /* loongarch_ipi_iocsr performs re-entrant IO through ipi_send */
-    s->ipi_iocsr_mem.disable_reentrancy_guard = true;
-
-    sysbus_init_mmio(sbd, &s->ipi_iocsr_mem);
-
-    memory_region_init_io(&s->ipi64_iocsr_mem, OBJECT(dev),
-                          &loongarch_ipi64_ops,
-                          s, "loongarch_ipi64_iocsr", 0x118);
-    sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
-
-    s->cpu = g_new0(IPICore, s->num_cpu);
-    if (s->cpu == NULL) {
-        error_setg(errp, "Memory allocation for ExtIOICore faile");
-        return;
-    }
-
-    for (i = 0; i < s->num_cpu; i++) {
-        qdev_init_gpio_out(dev, &s->cpu[i].irq, 1);
-    }
-}
-
-static const VMStateDescription vmstate_ipi_core = {
-    .name = "ipi-single",
-    .version_id = 2,
-    .minimum_version_id = 2,
-    .fields = (const VMStateField[]) {
-        VMSTATE_UINT32(status, IPICore),
-        VMSTATE_UINT32(en, IPICore),
-        VMSTATE_UINT32(set, IPICore),
-        VMSTATE_UINT32(clear, IPICore),
-        VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static const VMStateDescription vmstate_loongarch_ipi = {
-    .name = TYPE_LOONGARCH_IPI,
-    .version_id = 2,
-    .minimum_version_id = 2,
-    .fields = (const VMStateField[]) {
-        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, LoongArchIPI, num_cpu,
-                         vmstate_ipi_core, IPICore),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static Property ipi_properties[] = {
-    DEFINE_PROP_UINT32("num-cpu", LoongArchIPI, num_cpu, 1),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void loongarch_ipi_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    dc->realize = loongarch_ipi_realize;
-    device_class_set_props(dc, ipi_properties);
-    dc->vmsd = &vmstate_loongarch_ipi;
-}
-
-static void loongarch_ipi_finalize(Object *obj)
-{
-    LoongArchIPI *s = LOONGARCH_IPI(obj);
-
-    g_free(s->cpu);
-}
-
-static const TypeInfo loongarch_ipi_info = {
-    .name          = TYPE_LOONGARCH_IPI,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(LoongArchIPI),
-    .class_init    = loongarch_ipi_class_init,
-    .instance_finalize = loongarch_ipi_finalize,
-};
-
-static void loongarch_ipi_register_types(void)
-{
-    type_register_static(&loongarch_ipi_info);
-}
-
-type_init(loongarch_ipi_register_types)

-- 
2.43.0

On 2024/6/5 上午10:15, Jiaxun Yang wrote:
> It was missed out in previous commit.
> 
> Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   hw/intc/loongarch_ipi.c | 347 ------------------------------------------------
>   1 file changed, 347 deletions(-)
> 
> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
> deleted file mode 100644
> index 44b3b9c138d6..000000000000
> --- a/hw/intc/loongarch_ipi.c
> +++ /dev/null
> @@ -1,347 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * LoongArch ipi interrupt support
> - *
> - * Copyright (C) 2021 Loongson Technology Corporation Limited
> - */
> -
> -#include "qemu/osdep.h"
> -#include "hw/boards.h"
> -#include "hw/sysbus.h"
> -#include "hw/intc/loongarch_ipi.h"
> -#include "hw/irq.h"
> -#include "hw/qdev-properties.h"
> -#include "qapi/error.h"
> -#include "qemu/log.h"
> -#include "exec/address-spaces.h"
> -#include "migration/vmstate.h"
> -#include "target/loongarch/cpu.h"
> -#include "trace.h"
> -
> -static MemTxResult loongarch_ipi_readl(void *opaque, hwaddr addr,
> -                                       uint64_t *data,
> -                                       unsigned size, MemTxAttrs attrs)
> -{
> -    IPICore *s;
> -    LoongArchIPI *ipi = opaque;
> -    uint64_t ret = 0;
> -    int index = 0;
> -
> -    s = &ipi->cpu[attrs.requester_id];
> -    addr &= 0xff;
> -    switch (addr) {
> -    case CORE_STATUS_OFF:
> -        ret = s->status;
> -        break;
> -    case CORE_EN_OFF:
> -        ret = s->en;
> -        break;
> -    case CORE_SET_OFF:
> -        ret = 0;
> -        break;
> -    case CORE_CLEAR_OFF:
> -        ret = 0;
> -        break;
> -    case CORE_BUF_20 ... CORE_BUF_38 + 4:
> -        index = (addr - CORE_BUF_20) >> 2;
> -        ret = s->buf[index];
> -        break;
> -    default:
> -        qemu_log_mask(LOG_UNIMP, "invalid read: %x", (uint32_t)addr);
> -        break;
> -    }
> -
> -    trace_loongarch_ipi_read(size, (uint64_t)addr, ret);
> -    *data = ret;
> -    return MEMTX_OK;
> -}
> -
> -static void send_ipi_data(CPULoongArchState *env, uint64_t val, hwaddr addr,
> -                          MemTxAttrs attrs)
> -{
> -    int i, mask = 0, data = 0;
> -
> -    /*
> -     * bit 27-30 is mask for byte writing,
> -     * if the mask is 0, we need not to do anything.
> -     */
> -    if ((val >> 27) & 0xf) {
> -        data = address_space_ldl(env->address_space_iocsr, addr,
> -                                 attrs, NULL);
> -        for (i = 0; i < 4; i++) {
> -            /* get mask for byte writing */
> -            if (val & (0x1 << (27 + i))) {
> -                mask |= 0xff << (i * 8);
> -            }
> -        }
> -    }
> -
> -    data &= mask;
> -    data |= (val >> 32) & ~mask;
> -    address_space_stl(env->address_space_iocsr, addr,
> -                      data, attrs, NULL);
> -}
> -
> -static int archid_cmp(const void *a, const void *b)
> -{
> -   CPUArchId *archid_a = (CPUArchId *)a;
> -   CPUArchId *archid_b = (CPUArchId *)b;
> -
> -   return archid_a->arch_id - archid_b->arch_id;
> -}
> -
> -static CPUArchId *find_cpu_by_archid(MachineState *ms, uint32_t id)
> -{
> -    CPUArchId apic_id, *found_cpu;
> -
> -    apic_id.arch_id = id;
> -    found_cpu = bsearch(&apic_id, ms->possible_cpus->cpus,
> -        ms->possible_cpus->len, sizeof(*ms->possible_cpus->cpus),
> -        archid_cmp);
> -
> -    return found_cpu;
> -}
> -
> -static CPUState *ipi_getcpu(int arch_id)
> -{
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -    CPUArchId *archid;
> -
> -    archid = find_cpu_by_archid(machine, arch_id);
> -    if (archid) {
> -        return CPU(archid->cpu);
> -    }
> -
> -    return NULL;
> -}
> -
> -static MemTxResult mail_send(uint64_t val, MemTxAttrs attrs)
> -{
> -    uint32_t cpuid;
> -    hwaddr addr;
> -    CPUState *cs;
> -
> -    cpuid = extract32(val, 16, 10);
> -    cs = ipi_getcpu(cpuid);
> -    if (cs == NULL) {
> -        return MEMTX_DECODE_ERROR;
> -    }
> -
> -    /* override requester_id */
> -    addr = SMP_IPI_MAILBOX + CORE_BUF_20 + (val & 0x1c);
> -    attrs.requester_id = cs->cpu_index;
> -    send_ipi_data(&LOONGARCH_CPU(cs)->env, val, addr, attrs);
> -    return MEMTX_OK;
> -}
> -
> -static MemTxResult any_send(uint64_t val, MemTxAttrs attrs)
> -{
> -    uint32_t cpuid;
> -    hwaddr addr;
> -    CPUState *cs;
> -
> -    cpuid = extract32(val, 16, 10);
> -    cs = ipi_getcpu(cpuid);
> -    if (cs == NULL) {
> -        return MEMTX_DECODE_ERROR;
> -    }
> -
> -    /* override requester_id */
> -    addr = val & 0xffff;
> -    attrs.requester_id = cs->cpu_index;
> -    send_ipi_data(&LOONGARCH_CPU(cs)->env, val, addr, attrs);
> -    return MEMTX_OK;
> -}
> -
> -static MemTxResult loongarch_ipi_writel(void *opaque, hwaddr addr, uint64_t val,
> -                                        unsigned size, MemTxAttrs attrs)
> -{
> -    LoongArchIPI *ipi = opaque;
> -    IPICore *s;
> -    int index = 0;
> -    uint32_t cpuid;
> -    uint8_t vector;
> -    CPUState *cs;
> -
> -    s = &ipi->cpu[attrs.requester_id];
> -    addr &= 0xff;
> -    trace_loongarch_ipi_write(size, (uint64_t)addr, val);
> -    switch (addr) {
> -    case CORE_STATUS_OFF:
> -        qemu_log_mask(LOG_GUEST_ERROR, "can not be written");
> -        break;
> -    case CORE_EN_OFF:
> -        s->en = val;
> -        break;
> -    case CORE_SET_OFF:
> -        s->status |= val;
> -        if (s->status != 0 && (s->status & s->en) != 0) {
> -            qemu_irq_raise(s->irq);
> -        }
> -        break;
> -    case CORE_CLEAR_OFF:
> -        s->status &= ~val;
> -        if (s->status == 0 && s->en != 0) {
> -            qemu_irq_lower(s->irq);
> -        }
> -        break;
> -    case CORE_BUF_20 ... CORE_BUF_38 + 4:
> -        index = (addr - CORE_BUF_20) >> 2;
> -        s->buf[index] = val;
> -        break;
> -    case IOCSR_IPI_SEND:
> -        cpuid = extract32(val, 16, 10);
> -        /* IPI status vector */
> -        vector = extract8(val, 0, 5);
> -        cs = ipi_getcpu(cpuid);
> -        if (cs == NULL) {
> -            return MEMTX_DECODE_ERROR;
> -        }
> -
> -        /* override requester_id */
> -        attrs.requester_id = cs->cpu_index;
> -        loongarch_ipi_writel(ipi, CORE_SET_OFF, BIT(vector), 4, attrs);
> -        break;
> -    default:
> -        qemu_log_mask(LOG_UNIMP, "invalid write: %x", (uint32_t)addr);
> -        break;
> -    }
> -
> -    return MEMTX_OK;
> -}
> -
> -static const MemoryRegionOps loongarch_ipi_ops = {
> -    .read_with_attrs = loongarch_ipi_readl,
> -    .write_with_attrs = loongarch_ipi_writel,
> -    .impl.min_access_size = 4,
> -    .impl.max_access_size = 4,
> -    .valid.min_access_size = 4,
> -    .valid.max_access_size = 8,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
> -/* mail send and any send only support writeq */
> -static MemTxResult loongarch_ipi_writeq(void *opaque, hwaddr addr, uint64_t val,
> -                                        unsigned size, MemTxAttrs attrs)
> -{
> -    MemTxResult ret = MEMTX_OK;
> -
> -    addr &= 0xfff;
> -    switch (addr) {
> -    case MAIL_SEND_OFFSET:
> -        ret = mail_send(val, attrs);
> -        break;
> -    case ANY_SEND_OFFSET:
> -        ret = any_send(val, attrs);
> -        break;
> -    default:
> -       break;
> -    }
> -
> -    return ret;
> -}
> -
> -static const MemoryRegionOps loongarch_ipi64_ops = {
> -    .write_with_attrs = loongarch_ipi_writeq,
> -    .impl.min_access_size = 8,
> -    .impl.max_access_size = 8,
> -    .valid.min_access_size = 8,
> -    .valid.max_access_size = 8,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
> -static void loongarch_ipi_realize(DeviceState *dev, Error **errp)
> -{
> -    LoongArchIPI *s = LOONGARCH_IPI(dev);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> -    int i;
> -
> -    if (s->num_cpu == 0) {
> -        error_setg(errp, "num-cpu must be at least 1");
> -        return;
> -    }
> -
> -    memory_region_init_io(&s->ipi_iocsr_mem, OBJECT(dev), &loongarch_ipi_ops,
> -                          s, "loongarch_ipi_iocsr", 0x48);
> -
> -    /* loongarch_ipi_iocsr performs re-entrant IO through ipi_send */
> -    s->ipi_iocsr_mem.disable_reentrancy_guard = true;
> -
> -    sysbus_init_mmio(sbd, &s->ipi_iocsr_mem);
> -
> -    memory_region_init_io(&s->ipi64_iocsr_mem, OBJECT(dev),
> -                          &loongarch_ipi64_ops,
> -                          s, "loongarch_ipi64_iocsr", 0x118);
> -    sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
It is different with existing implementation.

With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, however on 
loongarch ipi machine, there is no ipi_mmio_mem memory region.

So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory 
regions. In function sysbus_init_mmio(), memory region can not exceed
QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
region search speed also.

void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
{
     int n;

     assert(dev->num_mmio < QDEV_MAX_MMIO);
     n = dev->num_mmio++;
     dev->mmio[n].addr = -1;
     dev->mmio[n].memory = memory;
}

Can we revert this patch? We want to do production usable by real users 
rather than show pure technology.

Regards
Bibo Mao

> -
> -    s->cpu = g_new0(IPICore, s->num_cpu);
> -    if (s->cpu == NULL) {
> -        error_setg(errp, "Memory allocation for ExtIOICore faile");
> -        return;
> -    }
> -
> -    for (i = 0; i < s->num_cpu; i++) {
> -        qdev_init_gpio_out(dev, &s->cpu[i].irq, 1);
> -    }
> -}
> -
> -static const VMStateDescription vmstate_ipi_core = {
> -    .name = "ipi-single",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> -    .fields = (const VMStateField[]) {
> -        VMSTATE_UINT32(status, IPICore),
> -        VMSTATE_UINT32(en, IPICore),
> -        VMSTATE_UINT32(set, IPICore),
> -        VMSTATE_UINT32(clear, IPICore),
> -        VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2),
> -        VMSTATE_END_OF_LIST()
> -    }
> -};
> -
> -static const VMStateDescription vmstate_loongarch_ipi = {
> -    .name = TYPE_LOONGARCH_IPI,
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> -    .fields = (const VMStateField[]) {
> -        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, LoongArchIPI, num_cpu,
> -                         vmstate_ipi_core, IPICore),
> -        VMSTATE_END_OF_LIST()
> -    }
> -};
> -
> -static Property ipi_properties[] = {
> -    DEFINE_PROP_UINT32("num-cpu", LoongArchIPI, num_cpu, 1),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void loongarch_ipi_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -
> -    dc->realize = loongarch_ipi_realize;
> -    device_class_set_props(dc, ipi_properties);
> -    dc->vmsd = &vmstate_loongarch_ipi;
> -}
> -
> -static void loongarch_ipi_finalize(Object *obj)
> -{
> -    LoongArchIPI *s = LOONGARCH_IPI(obj);
> -
> -    g_free(s->cpu);
> -}
> -
> -static const TypeInfo loongarch_ipi_info = {
> -    .name          = TYPE_LOONGARCH_IPI,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(LoongArchIPI),
> -    .class_init    = loongarch_ipi_class_init,
> -    .instance_finalize = loongarch_ipi_finalize,
> -};
> -
> -static void loongarch_ipi_register_types(void)
> -{
> -    type_register_static(&loongarch_ipi_info);
> -}
> -
> -type_init(loongarch_ipi_register_types)
> 

Hi Bibo,

On 26/6/24 06:11, maobibo wrote:
> 
> 
> On 2024/6/5 上午10:15, Jiaxun Yang wrote:
>> It was missed out in previous commit.
>>
>> Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> ---
>>   hw/intc/loongarch_ipi.c | 347 
>> ------------------------------------------------
>>   1 file changed, 347 deletions(-)


>> -static void loongarch_ipi_realize(DeviceState *dev, Error **errp)
>> -{
>> -    LoongArchIPI *s = LOONGARCH_IPI(dev);
>> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> -    int i;
>> -
>> -    if (s->num_cpu == 0) {
>> -        error_setg(errp, "num-cpu must be at least 1");
>> -        return;
>> -    }
>> -
>> -    memory_region_init_io(&s->ipi_iocsr_mem, OBJECT(dev), 
>> &loongarch_ipi_ops,
>> -                          s, "loongarch_ipi_iocsr", 0x48);
>> -
>> -    /* loongarch_ipi_iocsr performs re-entrant IO through ipi_send */
>> -    s->ipi_iocsr_mem.disable_reentrancy_guard = true;
>> -
>> -    sysbus_init_mmio(sbd, &s->ipi_iocsr_mem);
>> -
>> -    memory_region_init_io(&s->ipi64_iocsr_mem, OBJECT(dev),
>> -                          &loongarch_ipi64_ops,
>> -                          s, "loongarch_ipi64_iocsr", 0x118);
>> -    sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
> It is different with existing implementation.
> 
> With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, however on 
> loongarch ipi machine, there is no ipi_mmio_mem memory region.
> 
> So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory 
> regions. In function sysbus_init_mmio(), memory region can not exceed
> QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
> region search speed also.
> 
> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
> {
>      int n;
> 
>      assert(dev->num_mmio < QDEV_MAX_MMIO);
>      n = dev->num_mmio++;
>      dev->mmio[n].addr = -1;
>      dev->mmio[n].memory = memory;
> }
> 
> Can we revert this patch? We want to do production usable by real users 
> rather than show pure technology.

Since commit b4a12dfc2132 this file is not built/tested anymore:

-specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: 
files('loongarch_ipi.c'))
+specific_ss.add(when: 'CONFIG_LOONGSON_IPI', if_true: 
files('loongson_ipi.c'))

We don't want to maintain dead code.

Regards,

Phil.

在 2024/6/26 下午8:10, Philippe Mathieu-Daudé 写道:
> Hi Bibo,
>
> On 26/6/24 06:11, maobibo wrote:
>>
>>
>> On 2024/6/5 上午10:15, Jiaxun Yang wrote:
>>> It was missed out in previous commit.
>>>
>>> Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>> ---
>>>   hw/intc/loongarch_ipi.c | 347 
>>> ------------------------------------------------
>>>   1 file changed, 347 deletions(-)
>
>
>>> -static void loongarch_ipi_realize(DeviceState *dev, Error **errp)
>>> -{
>>> -    LoongArchIPI *s = LOONGARCH_IPI(dev);
>>> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>> -    int i;
>>> -
>>> -    if (s->num_cpu == 0) {
>>> -        error_setg(errp, "num-cpu must be at least 1");
>>> -        return;
>>> -    }
>>> -
>>> -    memory_region_init_io(&s->ipi_iocsr_mem, OBJECT(dev), 
>>> &loongarch_ipi_ops,
>>> -                          s, "loongarch_ipi_iocsr", 0x48);
>>> -
>>> -    /* loongarch_ipi_iocsr performs re-entrant IO through ipi_send */
>>> -    s->ipi_iocsr_mem.disable_reentrancy_guard = true;
>>> -
>>> -    sysbus_init_mmio(sbd, &s->ipi_iocsr_mem);
>>> -
>>> -    memory_region_init_io(&s->ipi64_iocsr_mem, OBJECT(dev),
>>> -                          &loongarch_ipi64_ops,
>>> -                          s, "loongarch_ipi64_iocsr", 0x118);
>>> -    sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
>> It is different with existing implementation.
>>
>> With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, however 
>> on loongarch ipi machine, there is no ipi_mmio_mem memory region.
>>
>> So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory 
>> regions. In function sysbus_init_mmio(), memory region can not exceed
>> QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
>> region search speed also.
>>
>> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>> {
>>      int n;
>>
>>      assert(dev->num_mmio < QDEV_MAX_MMIO);
>>      n = dev->num_mmio++;
>>      dev->mmio[n].addr = -1;
>>      dev->mmio[n].memory = memory;
>> }
>>
>> Can we revert this patch? We want to do production usable by real 
>> users rather than show pure technology.
>
> Since commit b4a12dfc2132 this file is not built/tested anymore:
>
> -specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: 
> files('loongarch_ipi.c'))
> +specific_ss.add(when: 'CONFIG_LOONGSON_IPI', if_true: 
> files('loongson_ipi.c'))
>
> We don't want to maintain dead code.
>
Hi,  Philippe

It is commmit 49eba52a5 that causes Loongarch to fail to start.

What bibao means is that LoongArch and mips do not share "lloongson_ipi.c".
This avoids mutual influence.


My understanding of the next sentence is as follows.

Nowadays, most of the open source operating systems in China use the 
latest QEMU.
e.g. OpenEuler/OpenAnolis/OpenCloudOS, etc. These operating systems have 
a large
  number of real users. so we need to maintain the stability of the 
LoongArch architecture
of the QEMU community as much as possible. This will reduce maintenance 
costs.

Therefore, we would like to restore the 'loongarch_ipi.c' file. what do 
you think?

Thanks.
Song Gao

> Regards,
>
> Phil.

On 27/6/24 04:44, gaosong wrote:
> 在 2024/6/26 下午8:10, Philippe Mathieu-Daudé 写道:
>> Hi Bibo,
>>
>> On 26/6/24 06:11, maobibo wrote:
>>>
>>>
>>> On 2024/6/5 上午10:15, Jiaxun Yang wrote:
>>>> It was missed out in previous commit.
>>>>
>>>> Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
>>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>> ---
>>>>   hw/intc/loongarch_ipi.c | 347 
>>>> ------------------------------------------------
>>>>   1 file changed, 347 deletions(-)
>>
>>
>>>> -static void loongarch_ipi_realize(DeviceState *dev, Error **errp)
>>>> -{
>>>> -    LoongArchIPI *s = LOONGARCH_IPI(dev);
>>>> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>> -    int i;
>>>> -
>>>> -    if (s->num_cpu == 0) {
>>>> -        error_setg(errp, "num-cpu must be at least 1");
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    memory_region_init_io(&s->ipi_iocsr_mem, OBJECT(dev), 
>>>> &loongarch_ipi_ops,
>>>> -                          s, "loongarch_ipi_iocsr", 0x48);
>>>> -
>>>> -    /* loongarch_ipi_iocsr performs re-entrant IO through ipi_send */
>>>> -    s->ipi_iocsr_mem.disable_reentrancy_guard = true;
>>>> -
>>>> -    sysbus_init_mmio(sbd, &s->ipi_iocsr_mem);
>>>> -
>>>> -    memory_region_init_io(&s->ipi64_iocsr_mem, OBJECT(dev),
>>>> -                          &loongarch_ipi64_ops,
>>>> -                          s, "loongarch_ipi64_iocsr", 0x118);
>>>> -    sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
>>> It is different with existing implementation.
>>>
>>> With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, however 
>>> on loongarch ipi machine, there is no ipi_mmio_mem memory region.
>>>
>>> So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory 
>>> regions. In function sysbus_init_mmio(), memory region can not exceed
>>> QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
>>> region search speed also.
>>>
>>> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>>> {
>>>      int n;
>>>
>>>      assert(dev->num_mmio < QDEV_MAX_MMIO);
>>>      n = dev->num_mmio++;
>>>      dev->mmio[n].addr = -1;
>>>      dev->mmio[n].memory = memory;
>>> }
>>>
>>> Can we revert this patch? We want to do production usable by real 
>>> users rather than show pure technology.
>>
>> Since commit b4a12dfc2132 this file is not built/tested anymore:
>>
>> -specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: 
>> files('loongarch_ipi.c'))
>> +specific_ss.add(when: 'CONFIG_LOONGSON_IPI', if_true: 
>> files('loongson_ipi.c'))
>>
>> We don't want to maintain dead code.
>>
> Hi,  Philippe
> 
> It is commmit 49eba52a5 that causes Loongarch to fail to start.
> 
> What bibao means is that LoongArch and mips do not share "lloongson_ipi.c".
> This avoids mutual influence.
> 
> 
> My understanding of the next sentence is as follows.
> 
> Nowadays, most of the open source operating systems in China use the 
> latest QEMU.
> e.g. OpenEuler/OpenAnolis/OpenCloudOS, etc. These operating systems have 
> a large
>   number of real users. so we need to maintain the stability of the 
> LoongArch architecture
> of the QEMU community as much as possible. This will reduce maintenance 
> costs.

I'm glad there is a such large number of users :)

> Therefore, we would like to restore the 'loongarch_ipi.c' file. what do 
> you think?

My preference on "reducing maintenance cost" is code reuse instead of
duplication.

Before reverting, lets try to fix the issue. I suggested a v2:
https://lore.kernel.org/qemu-devel/20240627125819.62779-2-philmd@linaro.org

That said, both current patch and the suggested fix pass our
Avocado CI test suite (running tests/avocado/machine_loongarch.py).

Is your use case not covered? Could you expand the CI tests so
we don't hit this problem again? (Also we could reproduce and
fix more easily).

Thanks,

Phil.

Hi Philippe,

On 2024/6/27 下午9:02, Philippe Mathieu-Daudé wrote:
> On 27/6/24 04:44, gaosong wrote:
>> 在 2024/6/26 下午8:10, Philippe Mathieu-Daudé 写道:
>>> Hi Bibo,
>>>
>>> On 26/6/24 06:11, maobibo wrote:
>>>>
>>>>
>>>> On 2024/6/5 上午10:15, Jiaxun Yang wrote:
>>>>> It was missed out in previous commit.
>>>>>
>>>>> Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
>>>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>> ---
>>>>>   hw/intc/loongarch_ipi.c | 347 
>>>>> ------------------------------------------------
>>>>>   1 file changed, 347 deletions(-)
>>>
>>>
>>>>> -static void loongarch_ipi_realize(DeviceState *dev, Error **errp)
>>>>> -{
>>>>> -    LoongArchIPI *s = LOONGARCH_IPI(dev);
>>>>> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>>> -    int i;
>>>>> -
>>>>> -    if (s->num_cpu == 0) {
>>>>> -        error_setg(errp, "num-cpu must be at least 1");
>>>>> -        return;
>>>>> -    }
>>>>> -
>>>>> -    memory_region_init_io(&s->ipi_iocsr_mem, OBJECT(dev), 
>>>>> &loongarch_ipi_ops,
>>>>> -                          s, "loongarch_ipi_iocsr", 0x48);
>>>>> -
>>>>> -    /* loongarch_ipi_iocsr performs re-entrant IO through ipi_send */
>>>>> -    s->ipi_iocsr_mem.disable_reentrancy_guard = true;
>>>>> -
>>>>> -    sysbus_init_mmio(sbd, &s->ipi_iocsr_mem);
>>>>> -
>>>>> -    memory_region_init_io(&s->ipi64_iocsr_mem, OBJECT(dev),
>>>>> -                          &loongarch_ipi64_ops,
>>>>> -                          s, "loongarch_ipi64_iocsr", 0x118);
>>>>> -    sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
>>>> It is different with existing implementation.
>>>>
>>>> With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, 
>>>> however on loongarch ipi machine, there is no ipi_mmio_mem memory 
>>>> region.
>>>>
>>>> So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory 
>>>> regions. In function sysbus_init_mmio(), memory region can not exceed
>>>> QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
>>>> region search speed also.
>>>>
>>>> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>>>> {
>>>>      int n;
>>>>
>>>>      assert(dev->num_mmio < QDEV_MAX_MMIO);
>>>>      n = dev->num_mmio++;
>>>>      dev->mmio[n].addr = -1;
>>>>      dev->mmio[n].memory = memory;
>>>> }
>>>>
>>>> Can we revert this patch? We want to do production usable by real 
>>>> users rather than show pure technology.
>>>
>>> Since commit b4a12dfc2132 this file is not built/tested anymore:
>>>
>>> -specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: 
>>> files('loongarch_ipi.c'))
>>> +specific_ss.add(when: 'CONFIG_LOONGSON_IPI', if_true: 
>>> files('loongson_ipi.c'))
>>>
>>> We don't want to maintain dead code.
>>>
>> Hi,  Philippe
>>
>> It is commmit 49eba52a5 that causes Loongarch to fail to start.
>>
>> What bibao means is that LoongArch and mips do not share 
>> "lloongson_ipi.c".
>> This avoids mutual influence.
>>
>>
>> My understanding of the next sentence is as follows.
>>
>> Nowadays, most of the open source operating systems in China use the 
>> latest QEMU.
>> e.g. OpenEuler/OpenAnolis/OpenCloudOS, etc. These operating systems 
>> have a large
>>   number of real users. so we need to maintain the stability of the 
>> LoongArch architecture
>> of the QEMU community as much as possible. This will reduce 
>> maintenance costs.
> 
> I'm glad there is a such large number of users :)
> 
>> Therefore, we would like to restore the 'loongarch_ipi.c' file. what 
>> do you think?
> 
> My preference on "reducing maintenance cost" is code reuse instead of
> duplication.
> 
> Before reverting, lets try to fix the issue. I suggested a v2:
> https://lore.kernel.org/qemu-devel/20240627125819.62779-2-philmd@linaro.org
Sorry for late reply.

How about split loongson_ipi.c into 
loongson_ipi_base.c/loongson_ipi_loongson.c/loongson_ipi_loongarch.c,

File loongson_ipi_base.c contains the common code, loongson_ipi_xxx.c 
contains arch specific. Soon we will submit irqchip in kernel function,
it will be much different for architectures, since ioctl command is 
different for two architectures to save/restore ipi registers.

Regards
Bibo Mao

> 
> That said, both current patch and the suggested fix pass our
> Avocado CI test suite (running tests/avocado/machine_loongarch.py).
> 
> Is your use case not covered? Could you expand the CI tests so
> we don't hit this problem again? (Also we could reproduce and
> fix more easily).
> 
> Thanks,
> 
> Phil.

在2024年7月1日七月 上午2:35，maobibo写道：
[...]
>
> How about split loongson_ipi.c into 
> loongson_ipi_base.c/loongson_ipi_loongson.c/loongson_ipi_loongarch.c,
>
> File loongson_ipi_base.c contains the common code, loongson_ipi_xxx.c 
> contains arch specific. Soon we will submit irqchip in kernel function,
> it will be much different for architectures, since ioctl command is 
> different for two architectures to save/restore ipi registers.

MIPS's in kernel IPI IOCTL interface is non-existent so far, so if you are going
to design something, I think it will be adopted by MIPS if necessary. There is still
no need to create divergence in between.

That being said, You are more than welcomed to draft a patch so we can discuss based
on that.

Thanks
>
> Regards
> Bibo Mao

-- 
- Jiaxun

On 2024/7/1 下午3:08, Jiaxun Yang wrote:
> 
> 
> 在2024年7月1日七月 上午2:35，maobibo写道：
> [...]
>>
>> How about split loongson_ipi.c into
>> loongson_ipi_base.c/loongson_ipi_loongson.c/loongson_ipi_loongarch.c,
>>
>> File loongson_ipi_base.c contains the common code, loongson_ipi_xxx.c
>> contains arch specific. Soon we will submit irqchip in kernel function,
>> it will be much different for architectures, since ioctl command is
>> different for two architectures to save/restore ipi registers.
> 
> MIPS's in kernel IPI IOCTL interface is non-existent so far, so if you are going
> to design something, I think it will be adopted by MIPS if necessary. There is still
> no need to create divergence in between.
> 
> That being said, You are more than welcomed to draft a patch so we can discuss based
> on that.
Sure, will do.

Regards
Bibo Mao
> 
> Thanks
>>
>> Regards
>> Bibo Mao
> 

Also this patch is problematic on LoongArch.

The original patch is to search physical cpuid rather than logic cpuid.

We want to make ipi module better and better, however now it comes back 
to initial state at the beginning :(

commit 03ca348b6b9038ce284916b36c19f700ac0ce7a6
Author: Jiaxun Yang <jiaxun.yang@flygoat.com>
Date:   Wed Jun 5 10:04:27 2024

     hw/intc/loongson_ipi: Replace ipi_getcpu with cpu_by_arch_id

     cpu_by_arch_id is doing the same thing as our ipi_getcpu logic.

     Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
     Reviewed-by: Song Gao <gaosong@loongson.cn>
     Message-ID: <20240605-loongson3-ipi-v3-4-ddd2c0e03fa3@flygoat.com>
     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Regards
Bibo Mao

On 2024/7/1 上午9:35, maobibo wrote:
> Hi Philippe,
> 
> On 2024/6/27 下午9:02, Philippe Mathieu-Daudé wrote:
>> On 27/6/24 04:44, gaosong wrote:
>>> 在 2024/6/26 下午8:10, Philippe Mathieu-Daudé 写道:
>>>> Hi Bibo,
>>>>
>>>> On 26/6/24 06:11, maobibo wrote:
>>>>>
>>>>>
>>>>> On 2024/6/5 上午10:15, Jiaxun Yang wrote:
>>>>>> It was missed out in previous commit.
>>>>>>
>>>>>> Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
>>>>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>>> ---
>>>>>>   hw/intc/loongarch_ipi.c | 347 
>>>>>> ------------------------------------------------
>>>>>>   1 file changed, 347 deletions(-)
>>>>
>>>>
>>>>>> -static void loongarch_ipi_realize(DeviceState *dev, Error **errp)
>>>>>> -{
>>>>>> -    LoongArchIPI *s = LOONGARCH_IPI(dev);
>>>>>> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>>>> -    int i;
>>>>>> -
>>>>>> -    if (s->num_cpu == 0) {
>>>>>> -        error_setg(errp, "num-cpu must be at least 1");
>>>>>> -        return;
>>>>>> -    }
>>>>>> -
>>>>>> -    memory_region_init_io(&s->ipi_iocsr_mem, OBJECT(dev), 
>>>>>> &loongarch_ipi_ops,
>>>>>> -                          s, "loongarch_ipi_iocsr", 0x48);
>>>>>> -
>>>>>> -    /* loongarch_ipi_iocsr performs re-entrant IO through 
>>>>>> ipi_send */
>>>>>> -    s->ipi_iocsr_mem.disable_reentrancy_guard = true;
>>>>>> -
>>>>>> -    sysbus_init_mmio(sbd, &s->ipi_iocsr_mem);
>>>>>> -
>>>>>> -    memory_region_init_io(&s->ipi64_iocsr_mem, OBJECT(dev),
>>>>>> -                          &loongarch_ipi64_ops,
>>>>>> -                          s, "loongarch_ipi64_iocsr", 0x118);
>>>>>> -    sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
>>>>> It is different with existing implementation.
>>>>>
>>>>> With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, 
>>>>> however on loongarch ipi machine, there is no ipi_mmio_mem memory 
>>>>> region.
>>>>>
>>>>> So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory 
>>>>> regions. In function sysbus_init_mmio(), memory region can not exceed
>>>>> QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
>>>>> region search speed also.
>>>>>
>>>>> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>>>>> {
>>>>>      int n;
>>>>>
>>>>>      assert(dev->num_mmio < QDEV_MAX_MMIO);
>>>>>      n = dev->num_mmio++;
>>>>>      dev->mmio[n].addr = -1;
>>>>>      dev->mmio[n].memory = memory;
>>>>> }
>>>>>
>>>>> Can we revert this patch? We want to do production usable by real 
>>>>> users rather than show pure technology.
>>>>
>>>> Since commit b4a12dfc2132 this file is not built/tested anymore:
>>>>
>>>> -specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: 
>>>> files('loongarch_ipi.c'))
>>>> +specific_ss.add(when: 'CONFIG_LOONGSON_IPI', if_true: 
>>>> files('loongson_ipi.c'))
>>>>
>>>> We don't want to maintain dead code.
>>>>
>>> Hi,  Philippe
>>>
>>> It is commmit 49eba52a5 that causes Loongarch to fail to start.
>>>
>>> What bibao means is that LoongArch and mips do not share 
>>> "lloongson_ipi.c".
>>> This avoids mutual influence.
>>>
>>>
>>> My understanding of the next sentence is as follows.
>>>
>>> Nowadays, most of the open source operating systems in China use the 
>>> latest QEMU.
>>> e.g. OpenEuler/OpenAnolis/OpenCloudOS, etc. These operating systems 
>>> have a large
>>>   number of real users. so we need to maintain the stability of the 
>>> LoongArch architecture
>>> of the QEMU community as much as possible. This will reduce 
>>> maintenance costs.
>>
>> I'm glad there is a such large number of users :)
>>
>>> Therefore, we would like to restore the 'loongarch_ipi.c' file. what 
>>> do you think?
>>
>> My preference on "reducing maintenance cost" is code reuse instead of
>> duplication.
>>
>> Before reverting, lets try to fix the issue. I suggested a v2:
>> https://lore.kernel.org/qemu-devel/20240627125819.62779-2-philmd@linaro.org 
>>
> Sorry for late reply.
> 
> How about split loongson_ipi.c into 
> loongson_ipi_base.c/loongson_ipi_loongson.c/loongson_ipi_loongarch.c,
> 
> File loongson_ipi_base.c contains the common code, loongson_ipi_xxx.c 
> contains arch specific. Soon we will submit irqchip in kernel function,
> it will be much different for architectures, since ioctl command is 
> different for two architectures to save/restore ipi registers.
> 
> Regards
> Bibo Mao
> 
>>
>> That said, both current patch and the suggested fix pass our
>> Avocado CI test suite (running tests/avocado/machine_loongarch.py).
>>
>> Is your use case not covered? Could you expand the CI tests so
>> we don't hit this problem again? (Also we could reproduce and
>> fix more easily).
>>
>> Thanks,
>>
>> Phil.
> 

在2024年7月1日七月 上午7:44，maobibo写道：
> Also this patch is problematic on LoongArch.
>
> The original patch is to search physical cpuid rather than logic cpuid.
>
> We want to make ipi module better and better, however now it comes back 
> to initial state at the beginning :(

Isn't arch_id the "physical id" you want? "cs->cpu_index" is the logical ID
for QEMU.

arch_id is setup by arch code, like APIC ID for x86.

I had come across the old ipi_getcpu  implementation, and I'm sure we were
looking at arch_id as well.

Thanks
- Jiaxun
>
> commit 03ca348b6b9038ce284916b36c19f700ac0ce7a6
> Author: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Date:   Wed Jun 5 10:04:27 2024
>
>      hw/intc/loongson_ipi: Replace ipi_getcpu with cpu_by_arch_id
>
>      cpu_by_arch_id is doing the same thing as our ipi_getcpu logic.
>
>      Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>      Reviewed-by: Song Gao <gaosong@loongson.cn>
>      Message-ID: <20240605-loongson3-ipi-v3-4-ddd2c0e03fa3@flygoat.com>
>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>
> Regards
> Bibo Mao
>
-- 
- Jiaxun

On 2024/7/1 下午3:01, Jiaxun Yang wrote:
> 
> 
> 在2024年7月1日七月 上午7:44，maobibo写道：
>> Also this patch is problematic on LoongArch.
>>
>> The original patch is to search physical cpuid rather than logic cpuid.
>>
>> We want to make ipi module better and better, however now it comes back
>> to initial state at the beginning :(
> 
> Isn't arch_id the "physical id" you want? "cs->cpu_index" is the logical ID
> for QEMU.
> 
> arch_id is setup by arch code, like APIC ID for x86.
> 
> I had come across the old ipi_getcpu  implementation, and I'm sure we were
> looking at arch_id as well.
So, where is implementation code for function get_arch_id() looking for 
vcpu with physical index?

Regards
Bibo Mao

> 
> Thanks
> - Jiaxun
>>
>> commit 03ca348b6b9038ce284916b36c19f700ac0ce7a6
>> Author: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> Date:   Wed Jun 5 10:04:27 2024
>>
>>       hw/intc/loongson_ipi: Replace ipi_getcpu with cpu_by_arch_id
>>
>>       cpu_by_arch_id is doing the same thing as our ipi_getcpu logic.
>>
>>       Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>       Reviewed-by: Song Gao <gaosong@loongson.cn>
>>       Message-ID: <20240605-loongson3-ipi-v3-4-ddd2c0e03fa3@flygoat.com>
>>       Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>>
>> Regards
>> Bibo Mao
>>

在2024年7月1日七月 上午8:22，maobibo写道：
> On 2024/7/1 下午3:01, Jiaxun Yang wrote:
>> 
>> 
>> 在2024年7月1日七月 上午7:44，maobibo写道：
>>> Also this patch is problematic on LoongArch.
>>>
>>> The original patch is to search physical cpuid rather than logic cpuid.
>>>
>>> We want to make ipi module better and better, however now it comes back
>>> to initial state at the beginning :(
>> 
>> Isn't arch_id the "physical id" you want? "cs->cpu_index" is the logical ID
>> for QEMU.
>> 
>> arch_id is setup by arch code, like APIC ID for x86.
>> 
>> I had come across the old ipi_getcpu  implementation, and I'm sure we were
>> looking at arch_id as well.
> So, where is implementation code for function get_arch_id() looking for 
> vcpu with physical index?

Hi Bibo,

cpu_by_arch_id will be redirected to:

```
CPUState *cpu_by_arch_id(int64_t id)
{
    CPUState *cpu;

    CPU_FOREACH(cpu) {
        CPUClass *cc = CPU_GET_CLASS(cpu);

        if (cc->get_arch_id(cpu) == id) {
            return cpu;
        }
    }
    return NULL;
}
```

It iterates over all vcpus and return CPUStates with corresponding arch_id.

Whereas, for LoongArch's get_arch_id implementation:
```
static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
{
    LoongArchCPU *cpu = LOONGARCH_CPU(cs);

    return cpu->phy_id;
}
```
I believe it matches our intension here.

Thanks

>
> Regards
> Bibo Mao
>
>> 
>> Thanks
>> - Jiaxun
>>>
>>> commit 03ca348b6b9038ce284916b36c19f700ac0ce7a6
>>> Author: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>> Date:   Wed Jun 5 10:04:27 2024
>>>
>>>       hw/intc/loongson_ipi: Replace ipi_getcpu with cpu_by_arch_id
>>>
>>>       cpu_by_arch_id is doing the same thing as our ipi_getcpu logic.
>>>
>>>       Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>       Reviewed-by: Song Gao <gaosong@loongson.cn>
>>>       Message-ID: <20240605-loongson3-ipi-v3-4-ddd2c0e03fa3@flygoat.com>
>>>       Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>>
>>> Regards
>>> Bibo Mao
>>>

-- 
- Jiaxun

On 2024/7/1 下午4:29, Jiaxun Yang wrote:
> 
> 
> 在2024年7月1日七月 上午8:22，maobibo写道：
>> On 2024/7/1 下午3:01, Jiaxun Yang wrote:
>>>
>>>
>>> 在2024年7月1日七月 上午7:44，maobibo写道：
>>>> Also this patch is problematic on LoongArch.
>>>>
>>>> The original patch is to search physical cpuid rather than logic cpuid.
>>>>
>>>> We want to make ipi module better and better, however now it comes back
>>>> to initial state at the beginning :(
>>>
>>> Isn't arch_id the "physical id" you want? "cs->cpu_index" is the logical ID
>>> for QEMU.
>>>
>>> arch_id is setup by arch code, like APIC ID for x86.
>>>
>>> I had come across the old ipi_getcpu  implementation, and I'm sure we were
>>> looking at arch_id as well.
>> So, where is implementation code for function get_arch_id() looking for
>> vcpu with physical index?
> 
> Hi Bibo,
> 
> cpu_by_arch_id will be redirected to:
> 
> ```
> CPUState *cpu_by_arch_id(int64_t id)
> {
>      CPUState *cpu;
> 
>      CPU_FOREACH(cpu) {
>          CPUClass *cc = CPU_GET_CLASS(cpu);
> 
>          if (cc->get_arch_id(cpu) == id) {
>              return cpu;
>          }
>      }
>      return NULL;
> }
> ```
> 
> It iterates over all vcpus and return CPUStates with corresponding arch_id.
> 
> Whereas, for LoongArch's get_arch_id implementation:
> ```
> static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
> {
>      LoongArchCPU *cpu = LOONGARCH_CPU(cs);
> 
>      return cpu->phy_id;
> }
> ```
> I believe it matches our intension here.
yes, you are right.

Got it, I miss the architecture specific implementation, and thanks for 
pointing it out with patience.

Regards
> 
> Thanks
> 
>>
>> Regards
>> Bibo Mao
>>
>>>
>>> Thanks
>>> - Jiaxun
>>>>
>>>> commit 03ca348b6b9038ce284916b36c19f700ac0ce7a6
>>>> Author: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>> Date:   Wed Jun 5 10:04:27 2024
>>>>
>>>>        hw/intc/loongson_ipi: Replace ipi_getcpu with cpu_by_arch_id
>>>>
>>>>        cpu_by_arch_id is doing the same thing as our ipi_getcpu logic.
>>>>
>>>>        Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>        Reviewed-by: Song Gao <gaosong@loongson.cn>
>>>>        Message-ID: <20240605-loongson3-ipi-v3-4-ddd2c0e03fa3@flygoat.com>
>>>>        Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>
>>>>
>>>> Regards
>>>> Bibo Mao
>>>>
> 

在2024年6月26日六月 上午5:11，maobibo写道：
[...]
> It is different with existing implementation.
What do you mean? Isn't this the actual hardware behaviour?

>
> With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, however on 
> loongarch ipi machine, there is no ipi_mmio_mem memory region.
> So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory 
> regions. In function sysbus_init_mmio(), memory region can not exceed
> QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
> region search speed also.
Ouch, never thought about that before, but I think we can control the
registration of sysbus_mmio with a device property or even ifdef so LoongArch
machine won't be affected.

For MIPS loongson3-virt machine, our CPU number is capped, so that won't
be a problem.

I'm currently travelling without access to my PC, I'll prepare a patch
as soon as I gain access again. Feel free to send a patch before me with
this approach if you desperately want to fix it.

>
> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
> {
>      int n;
>
>      assert(dev->num_mmio < QDEV_MAX_MMIO);
>      n = dev->num_mmio++;
>      dev->mmio[n].addr = -1;
>      dev->mmio[n].memory = memory;
> }
>
> Can we revert this patch? We want to do production usable by real users 
> rather than show pure technology.

I don't really get your point, we have at leat 4 real users requesting SMP
support for loongson3-virt on gitlab issues, plus I need this to test
MIPS/Loongson64 SMP kernel.

If there is a problem with your use case, we can fix it. Why we do want to
remove the functionality when there is an easy fix?

It’s not only the features necessary for you that made QEMU an outstanding
project; it’s everything coming together that completes it.

Thanks
- Jiaxun
>
> Regards
> Bibo Mao
>
>> -
>> -    s->cpu = g_new0(IPICore, s->num_cpu);
>> -    if (s->cpu == NULL) {
>> -        error_setg(errp, "Memory allocation for ExtIOICore faile");
>> -        return;
>> -    }
>> -
>> -    for (i = 0; i < s->num_cpu; i++) {
>> -        qdev_init_gpio_out(dev, &s->cpu[i].irq, 1);
>> -    }
>> -}
>> -
>> -static const VMStateDescription vmstate_ipi_core = {
>> -    .name = "ipi-single",
>> -    .version_id = 2,
>> -    .minimum_version_id = 2,
>> -    .fields = (const VMStateField[]) {
>> -        VMSTATE_UINT32(status, IPICore),
>> -        VMSTATE_UINT32(en, IPICore),
>> -        VMSTATE_UINT32(set, IPICore),
>> -        VMSTATE_UINT32(clear, IPICore),
>> -        VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2),
>> -        VMSTATE_END_OF_LIST()
>> -    }
>> -};
>> -
>> -static const VMStateDescription vmstate_loongarch_ipi = {
>> -    .name = TYPE_LOONGARCH_IPI,
>> -    .version_id = 2,
>> -    .minimum_version_id = 2,
>> -    .fields = (const VMStateField[]) {
>> -        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, LoongArchIPI, num_cpu,
>> -                         vmstate_ipi_core, IPICore),
>> -        VMSTATE_END_OF_LIST()
>> -    }
>> -};
>> -
>> -static Property ipi_properties[] = {
>> -    DEFINE_PROP_UINT32("num-cpu", LoongArchIPI, num_cpu, 1),
>> -    DEFINE_PROP_END_OF_LIST(),
>> -};
>> -
>> -static void loongarch_ipi_class_init(ObjectClass *klass, void *data)
>> -{
>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>> -
>> -    dc->realize = loongarch_ipi_realize;
>> -    device_class_set_props(dc, ipi_properties);
>> -    dc->vmsd = &vmstate_loongarch_ipi;
>> -}
>> -
>> -static void loongarch_ipi_finalize(Object *obj)
>> -{
>> -    LoongArchIPI *s = LOONGARCH_IPI(obj);
>> -
>> -    g_free(s->cpu);
>> -}
>> -
>> -static const TypeInfo loongarch_ipi_info = {
>> -    .name          = TYPE_LOONGARCH_IPI,
>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>> -    .instance_size = sizeof(LoongArchIPI),
>> -    .class_init    = loongarch_ipi_class_init,
>> -    .instance_finalize = loongarch_ipi_finalize,
>> -};
>> -
>> -static void loongarch_ipi_register_types(void)
>> -{
>> -    type_register_static(&loongarch_ipi_info);
>> -}
>> -
>> -type_init(loongarch_ipi_register_types)
>>

-- 
- Jiaxun

On 2024/6/26 下午3:40, Jiaxun Yang wrote:
> 
> 
> 在2024年6月26日六月 上午5:11，maobibo写道：
> [...]
>> It is different with existing implementation.
> What do you mean? Isn't this the actual hardware behaviour?
> 
>>
>> With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, however on
>> loongarch ipi machine, there is no ipi_mmio_mem memory region.
>> So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory
>> regions. In function sysbus_init_mmio(), memory region can not exceed
>> QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
>> region search speed also.
> Ouch, never thought about that before, but I think we can control the
> registration of sysbus_mmio with a device property or even ifdef so LoongArch
> machine won't be affected.
> 
> For MIPS loongson3-virt machine, our CPU number is capped, so that won't
> be a problem.
> 
> I'm currently travelling without access to my PC, I'll prepare a patch
> as soon as I gain access again. Feel free to send a patch before me with
> this approach if you desperately want to fix it.
> 
>>
>> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>> {
>>       int n;
>>
>>       assert(dev->num_mmio < QDEV_MAX_MMIO);
>>       n = dev->num_mmio++;
>>       dev->mmio[n].addr = -1;
>>       dev->mmio[n].memory = memory;
>> }
>>
>> Can we revert this patch? We want to do production usable by real users
>> rather than show pure technology.
> 
> I don't really get your point, we have at leat 4 real users requesting SMP
> support for loongson3-virt on gitlab issues, plus I need this to test
> MIPS/Loongson64 SMP kernel.
> 
> If there is a problem with your use case, we can fix it. Why we do want to
> remove the functionality when there is an easy fix?
I do not think we have the ability to abstract hw and continuous 
maintenance for two different architecture, including you and me.

So I suggest that different files will be better for the present. After 
one year or later, if we have further understanding about system, it is 
ok to merge them into one file.

Regards
Bibo Mao
> 
> It’s not only the features necessary for you that made QEMU an outstanding
> project; it’s everything coming together that completes it.
> 
> Thanks
> - Jiaxun
>>
>> Regards
>> Bibo Mao
>>
>>> -
>>> -    s->cpu = g_new0(IPICore, s->num_cpu);
>>> -    if (s->cpu == NULL) {
>>> -        error_setg(errp, "Memory allocation for ExtIOICore faile");
>>> -        return;
>>> -    }
>>> -
>>> -    for (i = 0; i < s->num_cpu; i++) {
>>> -        qdev_init_gpio_out(dev, &s->cpu[i].irq, 1);
>>> -    }
>>> -}
>>> -
>>> -static const VMStateDescription vmstate_ipi_core = {
>>> -    .name = "ipi-single",
>>> -    .version_id = 2,
>>> -    .minimum_version_id = 2,
>>> -    .fields = (const VMStateField[]) {
>>> -        VMSTATE_UINT32(status, IPICore),
>>> -        VMSTATE_UINT32(en, IPICore),
>>> -        VMSTATE_UINT32(set, IPICore),
>>> -        VMSTATE_UINT32(clear, IPICore),
>>> -        VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2),
>>> -        VMSTATE_END_OF_LIST()
>>> -    }
>>> -};
>>> -
>>> -static const VMStateDescription vmstate_loongarch_ipi = {
>>> -    .name = TYPE_LOONGARCH_IPI,
>>> -    .version_id = 2,
>>> -    .minimum_version_id = 2,
>>> -    .fields = (const VMStateField[]) {
>>> -        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, LoongArchIPI, num_cpu,
>>> -                         vmstate_ipi_core, IPICore),
>>> -        VMSTATE_END_OF_LIST()
>>> -    }
>>> -};
>>> -
>>> -static Property ipi_properties[] = {
>>> -    DEFINE_PROP_UINT32("num-cpu", LoongArchIPI, num_cpu, 1),
>>> -    DEFINE_PROP_END_OF_LIST(),
>>> -};
>>> -
>>> -static void loongarch_ipi_class_init(ObjectClass *klass, void *data)
>>> -{
>>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>>> -
>>> -    dc->realize = loongarch_ipi_realize;
>>> -    device_class_set_props(dc, ipi_properties);
>>> -    dc->vmsd = &vmstate_loongarch_ipi;
>>> -}
>>> -
>>> -static void loongarch_ipi_finalize(Object *obj)
>>> -{
>>> -    LoongArchIPI *s = LOONGARCH_IPI(obj);
>>> -
>>> -    g_free(s->cpu);
>>> -}
>>> -
>>> -static const TypeInfo loongarch_ipi_info = {
>>> -    .name          = TYPE_LOONGARCH_IPI,
>>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>>> -    .instance_size = sizeof(LoongArchIPI),
>>> -    .class_init    = loongarch_ipi_class_init,
>>> -    .instance_finalize = loongarch_ipi_finalize,
>>> -};
>>> -
>>> -static void loongarch_ipi_register_types(void)
>>> -{
>>> -    type_register_static(&loongarch_ipi_info);
>>> -}
>>> -
>>> -type_init(loongarch_ipi_register_types)
>>>
> 

在2024年6月26日六月 上午8:51，maobibo写道：
> On 2024/6/26 下午3:40, Jiaxun Yang wrote:
>> 
>> 
>> 在2024年6月26日六月 上午5:11，maobibo写道：
>> [...]
>>> It is different with existing implementation.
>> What do you mean? Isn't this the actual hardware behaviour?
>> 
>>>
>>> With hw/intc/loongson_ipi.c, every vcpu has one ipi_mmio_mem, however on
>>> loongarch ipi machine, there is no ipi_mmio_mem memory region.
>>> So if machine has 256 vcpus, there will be 256 ipi_mmio_mem memory
>>> regions. In function sysbus_init_mmio(), memory region can not exceed
>>> QDEV_MAX_MMIO (32).  With so many memory regions, it slows down memory
>>> region search speed also.
>> Ouch, never thought about that before, but I think we can control the
>> registration of sysbus_mmio with a device property or even ifdef so LoongArch
>> machine won't be affected.
>> 
>> For MIPS loongson3-virt machine, our CPU number is capped, so that won't
>> be a problem.
>> 
>> I'm currently travelling without access to my PC, I'll prepare a patch
>> as soon as I gain access again. Feel free to send a patch before me with
>> this approach if you desperately want to fix it.
>> 
>>>
>>> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>>> {
>>>       int n;
>>>
>>>       assert(dev->num_mmio < QDEV_MAX_MMIO);
>>>       n = dev->num_mmio++;
>>>       dev->mmio[n].addr = -1;
>>>       dev->mmio[n].memory = memory;
>>> }
>>>
>>> Can we revert this patch? We want to do production usable by real users
>>> rather than show pure technology.
>> 
>> I don't really get your point, we have at leat 4 real users requesting SMP
>> support for loongson3-virt on gitlab issues, plus I need this to test
>> MIPS/Loongson64 SMP kernel.
>> 
>> If there is a problem with your use case, we can fix it. Why we do want to
>> remove the functionality when there is an easy fix?
> I do not think we have the ability to abstract hw and continuous 
> maintenance for two different architecture, including you and me.

After all it’s the same IP block, I fail to see any reason to implement it separately.

I perfectly understand that piece of hardware IP and I fix broken stuff in this driver time by time.

I understand how does it work on LoongArch systems so I make my best effort to cover LoongArch part.

There might be some edge cases that I missed, and I’ll be thankful for reports or assistance.

The approach of shared drivers for LoongArch/MIPS works so well in kernel development, for QEMU I believe it’s also desirable.

I’m not asking you or Loongson to maintain any MIPS/Loongson related features, I’ll take care of them if things go wrong.

>lp
> So I suggest that different files will be better for the present. After 
> one year or later, if we have further understanding about system, it is 
> ok to merge them into one file.

If there is anything not clear to you on both MIPS and LoongArch side please let me know, I’m happy to help.

We promote code reuse in QEMU to minimize maintenance burden.

Thanks

>
> Regards
> Bibo Mao
>> 
>> It’s not only the features necessary for you that made QEMU an outstanding
>> project; it’s everything coming together that completes it.
>> 
>> Thanks
>> - Jiaxun
>>>
>>> Regards
>>> Bibo Mao
>>>
>>>> -
>>>> -    s->cpu = g_new0(IPICore, s->num_cpu);
>>>> -    if (s->cpu == NULL) {
>>>> -        error_setg(errp, "Memory allocation for ExtIOICore faile");
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    for (i = 0; i < s->num_cpu; i++) {
>>>> -        qdev_init_gpio_out(dev, &s->cpu[i].irq, 1);
>>>> -    }
>>>> -}
>>>> -
>>>> -static const VMStateDescription vmstate_ipi_core = {
>>>> -    .name = "ipi-single",
>>>> -    .version_id = 2,
>>>> -    .minimum_version_id = 2,
>>>> -    .fields = (const VMStateField[]) {
>>>> -        VMSTATE_UINT32(status, IPICore),
>>>> -        VMSTATE_UINT32(en, IPICore),
>>>> -        VMSTATE_UINT32(set, IPICore),
>>>> -        VMSTATE_UINT32(clear, IPICore),
>>>> -        VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2),
>>>> -        VMSTATE_END_OF_LIST()
>>>> -    }
>>>> -};
>>>> -
>>>> -static const VMStateDescription vmstate_loongarch_ipi = {
>>>> -    .name = TYPE_LOONGARCH_IPI,
>>>> -    .version_id = 2,
>>>> -    .minimum_version_id = 2,
>>>> -    .fields = (const VMStateField[]) {
>>>> -        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, LoongArchIPI, num_cpu,
>>>> -                         vmstate_ipi_core, IPICore),
>>>> -        VMSTATE_END_OF_LIST()
>>>> -    }
>>>> -};
>>>> -
>>>> -static Property ipi_properties[] = {
>>>> -    DEFINE_PROP_UINT32("num-cpu", LoongArchIPI, num_cpu, 1),
>>>> -    DEFINE_PROP_END_OF_LIST(),
>>>> -};
>>>> -
>>>> -static void loongarch_ipi_class_init(ObjectClass *klass, void *data)
>>>> -{
>>>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> -
>>>> -    dc->realize = loongarch_ipi_realize;
>>>> -    device_class_set_props(dc, ipi_properties);
>>>> -    dc->vmsd = &vmstate_loongarch_ipi;
>>>> -}
>>>> -
>>>> -static void loongarch_ipi_finalize(Object *obj)
>>>> -{
>>>> -    LoongArchIPI *s = LOONGARCH_IPI(obj);
>>>> -
>>>> -    g_free(s->cpu);
>>>> -}
>>>> -
>>>> -static const TypeInfo loongarch_ipi_info = {
>>>> -    .name          = TYPE_LOONGARCH_IPI,
>>>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>>>> -    .instance_size = sizeof(LoongArchIPI),
>>>> -    .class_init    = loongarch_ipi_class_init,
>>>> -    .instance_finalize = loongarch_ipi_finalize,
>>>> -};
>>>> -
>>>> -static void loongarch_ipi_register_types(void)
>>>> -{
>>>> -    type_register_static(&loongarch_ipi_info);
>>>> -}
>>>> -
>>>> -type_init(loongarch_ipi_register_types)
>>>>
>>

-- 
- Jiaxun

On 5/6/24 04:15, Jiaxun Yang wrote:
> It was missed out in previous commit.
> 
> Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   hw/intc/loongarch_ipi.c | 347 ------------------------------------------------
>   1 file changed, 347 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

在 2024/6/5 上午10:15, Jiaxun Yang 写道:
> It was missed out in previous commit.
>
> Fixes: b4a12dfc2132 ("hw/intc/loongarch_ipi: Rename as loongson_ipi")
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
Reviewed-by: Song Gao <gaosong@loongson.cn>

Thanks.
Song Gao
>   hw/intc/loongarch_ipi.c | 347 ------------------------------------------------
>   1 file changed, 347 deletions(-)
>
> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
> deleted file mode 100644
> index 44b3b9c138d6..000000000000
> --- a/hw/intc/loongarch_ipi.c
> +++ /dev/null
> @@ -1,347 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * LoongArch ipi interrupt support
> - *
> - * Copyright (C) 2021 Loongson Technology Corporation Limited
> - */
> -
> -#include "qemu/osdep.h"
> -#include "hw/boards.h"
> -#include "hw/sysbus.h"
> -#include "hw/intc/loongarch_ipi.h"
> -#include "hw/irq.h"
> -#include "hw/qdev-properties.h"
> -#include "qapi/error.h"
> -#include "qemu/log.h"
> -#include "exec/address-spaces.h"
> -#include "migration/vmstate.h"
> -#include "target/loongarch/cpu.h"
> -#include "trace.h"
> -
> -static MemTxResult loongarch_ipi_readl(void *opaque, hwaddr addr,
> -                                       uint64_t *data,
> -                                       unsigned size, MemTxAttrs attrs)
> -{
> -    IPICore *s;
> -    LoongArchIPI *ipi = opaque;
> -    uint64_t ret = 0;
> -    int index = 0;
> -
> -    s = &ipi->cpu[attrs.requester_id];
> -    addr &= 0xff;
> -    switch (addr) {
> -    case CORE_STATUS_OFF:
> -        ret = s->status;
> -        break;
> -    case CORE_EN_OFF:
> -        ret = s->en;
> -        break;
> -    case CORE_SET_OFF:
> -        ret = 0;
> -        break;
> -    case CORE_CLEAR_OFF:
> -        ret = 0;
> -        break;
> -    case CORE_BUF_20 ... CORE_BUF_38 + 4:
> -        index = (addr - CORE_BUF_20) >> 2;
> -        ret = s->buf[index];
> -        break;
> -    default:
> -        qemu_log_mask(LOG_UNIMP, "invalid read: %x", (uint32_t)addr);
> -        break;
> -    }
> -
> -    trace_loongarch_ipi_read(size, (uint64_t)addr, ret);
> -    *data = ret;
> -    return MEMTX_OK;
> -}
> -
> -static void send_ipi_data(CPULoongArchState *env, uint64_t val, hwaddr addr,
> -                          MemTxAttrs attrs)
> -{
> -    int i, mask = 0, data = 0;
> -
> -    /*
> -     * bit 27-30 is mask for byte writing,
> -     * if the mask is 0, we need not to do anything.
> -     */
> -    if ((val >> 27) & 0xf) {
> -        data = address_space_ldl(env->address_space_iocsr, addr,
> -                                 attrs, NULL);
> -        for (i = 0; i < 4; i++) {
> -            /* get mask for byte writing */
> -            if (val & (0x1 << (27 + i))) {
> -                mask |= 0xff << (i * 8);
> -            }
> -        }
> -    }
> -
> -    data &= mask;
> -    data |= (val >> 32) & ~mask;
> -    address_space_stl(env->address_space_iocsr, addr,
> -                      data, attrs, NULL);
> -}
> -
> -static int archid_cmp(const void *a, const void *b)
> -{
> -   CPUArchId *archid_a = (CPUArchId *)a;
> -   CPUArchId *archid_b = (CPUArchId *)b;
> -
> -   return archid_a->arch_id - archid_b->arch_id;
> -}
> -
> -static CPUArchId *find_cpu_by_archid(MachineState *ms, uint32_t id)
> -{
> -    CPUArchId apic_id, *found_cpu;
> -
> -    apic_id.arch_id = id;
> -    found_cpu = bsearch(&apic_id, ms->possible_cpus->cpus,
> -        ms->possible_cpus->len, sizeof(*ms->possible_cpus->cpus),
> -        archid_cmp);
> -
> -    return found_cpu;
> -}
> -
> -static CPUState *ipi_getcpu(int arch_id)
> -{
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -    CPUArchId *archid;
> -
> -    archid = find_cpu_by_archid(machine, arch_id);
> -    if (archid) {
> -        return CPU(archid->cpu);
> -    }
> -
> -    return NULL;
> -}
> -
> -static MemTxResult mail_send(uint64_t val, MemTxAttrs attrs)
> -{
> -    uint32_t cpuid;
> -    hwaddr addr;
> -    CPUState *cs;
> -
> -    cpuid = extract32(val, 16, 10);
> -    cs = ipi_getcpu(cpuid);
> -    if (cs == NULL) {
> -        return MEMTX_DECODE_ERROR;
> -    }
> -
> -    /* override requester_id */
> -    addr = SMP_IPI_MAILBOX + CORE_BUF_20 + (val & 0x1c);
> -    attrs.requester_id = cs->cpu_index;
> -    send_ipi_data(&LOONGARCH_CPU(cs)->env, val, addr, attrs);
> -    return MEMTX_OK;
> -}
> -
> -static MemTxResult any_send(uint64_t val, MemTxAttrs attrs)
> -{
> -    uint32_t cpuid;
> -    hwaddr addr;
> -    CPUState *cs;
> -
> -    cpuid = extract32(val, 16, 10);
> -    cs = ipi_getcpu(cpuid);
> -    if (cs == NULL) {
> -        return MEMTX_DECODE_ERROR;
> -    }
> -
> -    /* override requester_id */
> -    addr = val & 0xffff;
> -    attrs.requester_id = cs->cpu_index;
> -    send_ipi_data(&LOONGARCH_CPU(cs)->env, val, addr, attrs);
> -    return MEMTX_OK;
> -}
> -
> -static MemTxResult loongarch_ipi_writel(void *opaque, hwaddr addr, uint64_t val,
> -                                        unsigned size, MemTxAttrs attrs)
> -{
> -    LoongArchIPI *ipi = opaque;
> -    IPICore *s;
> -    int index = 0;
> -    uint32_t cpuid;
> -    uint8_t vector;
> -    CPUState *cs;
> -
> -    s = &ipi->cpu[attrs.requester_id];
> -    addr &= 0xff;
> -    trace_loongarch_ipi_write(size, (uint64_t)addr, val);
> -    switch (addr) {
> -    case CORE_STATUS_OFF:
> -        qemu_log_mask(LOG_GUEST_ERROR, "can not be written");
> -        break;
> -    case CORE_EN_OFF:
> -        s->en = val;
> -        break;
> -    case CORE_SET_OFF:
> -        s->status |= val;
> -        if (s->status != 0 && (s->status & s->en) != 0) {
> -            qemu_irq_raise(s->irq);
> -        }
> -        break;
> -    case CORE_CLEAR_OFF:
> -        s->status &= ~val;
> -        if (s->status == 0 && s->en != 0) {
> -            qemu_irq_lower(s->irq);
> -        }
> -        break;
> -    case CORE_BUF_20 ... CORE_BUF_38 + 4:
> -        index = (addr - CORE_BUF_20) >> 2;
> -        s->buf[index] = val;
> -        break;
> -    case IOCSR_IPI_SEND:
> -        cpuid = extract32(val, 16, 10);
> -        /* IPI status vector */
> -        vector = extract8(val, 0, 5);
> -        cs = ipi_getcpu(cpuid);
> -        if (cs == NULL) {
> -            return MEMTX_DECODE_ERROR;
> -        }
> -
> -        /* override requester_id */
> -        attrs.requester_id = cs->cpu_index;
> -        loongarch_ipi_writel(ipi, CORE_SET_OFF, BIT(vector), 4, attrs);
> -        break;
> -    default:
> -        qemu_log_mask(LOG_UNIMP, "invalid write: %x", (uint32_t)addr);
> -        break;
> -    }
> -
> -    return MEMTX_OK;
> -}
> -
> -static const MemoryRegionOps loongarch_ipi_ops = {
> -    .read_with_attrs = loongarch_ipi_readl,
> -    .write_with_attrs = loongarch_ipi_writel,
> -    .impl.min_access_size = 4,
> -    .impl.max_access_size = 4,
> -    .valid.min_access_size = 4,
> -    .valid.max_access_size = 8,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
> -/* mail send and any send only support writeq */
> -static MemTxResult loongarch_ipi_writeq(void *opaque, hwaddr addr, uint64_t val,
> -                                        unsigned size, MemTxAttrs attrs)
> -{
> -    MemTxResult ret = MEMTX_OK;
> -
> -    addr &= 0xfff;
> -    switch (addr) {
> -    case MAIL_SEND_OFFSET:
> -        ret = mail_send(val, attrs);
> -        break;
> -    case ANY_SEND_OFFSET:
> -        ret = any_send(val, attrs);
> -        break;
> -    default:
> -       break;
> -    }
> -
> -    return ret;
> -}
> -
> -static const MemoryRegionOps loongarch_ipi64_ops = {
> -    .write_with_attrs = loongarch_ipi_writeq,
> -    .impl.min_access_size = 8,
> -    .impl.max_access_size = 8,
> -    .valid.min_access_size = 8,
> -    .valid.max_access_size = 8,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
> -static void loongarch_ipi_realize(DeviceState *dev, Error **errp)
> -{
> -    LoongArchIPI *s = LOONGARCH_IPI(dev);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> -    int i;
> -
> -    if (s->num_cpu == 0) {
> -        error_setg(errp, "num-cpu must be at least 1");
> -        return;
> -    }
> -
> -    memory_region_init_io(&s->ipi_iocsr_mem, OBJECT(dev), &loongarch_ipi_ops,
> -                          s, "loongarch_ipi_iocsr", 0x48);
> -
> -    /* loongarch_ipi_iocsr performs re-entrant IO through ipi_send */
> -    s->ipi_iocsr_mem.disable_reentrancy_guard = true;
> -
> -    sysbus_init_mmio(sbd, &s->ipi_iocsr_mem);
> -
> -    memory_region_init_io(&s->ipi64_iocsr_mem, OBJECT(dev),
> -                          &loongarch_ipi64_ops,
> -                          s, "loongarch_ipi64_iocsr", 0x118);
> -    sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
> -
> -    s->cpu = g_new0(IPICore, s->num_cpu);
> -    if (s->cpu == NULL) {
> -        error_setg(errp, "Memory allocation for ExtIOICore faile");
> -        return;
> -    }
> -
> -    for (i = 0; i < s->num_cpu; i++) {
> -        qdev_init_gpio_out(dev, &s->cpu[i].irq, 1);
> -    }
> -}
> -
> -static const VMStateDescription vmstate_ipi_core = {
> -    .name = "ipi-single",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> -    .fields = (const VMStateField[]) {
> -        VMSTATE_UINT32(status, IPICore),
> -        VMSTATE_UINT32(en, IPICore),
> -        VMSTATE_UINT32(set, IPICore),
> -        VMSTATE_UINT32(clear, IPICore),
> -        VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2),
> -        VMSTATE_END_OF_LIST()
> -    }
> -};
> -
> -static const VMStateDescription vmstate_loongarch_ipi = {
> -    .name = TYPE_LOONGARCH_IPI,
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> -    .fields = (const VMStateField[]) {
> -        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, LoongArchIPI, num_cpu,
> -                         vmstate_ipi_core, IPICore),
> -        VMSTATE_END_OF_LIST()
> -    }
> -};
> -
> -static Property ipi_properties[] = {
> -    DEFINE_PROP_UINT32("num-cpu", LoongArchIPI, num_cpu, 1),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void loongarch_ipi_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -
> -    dc->realize = loongarch_ipi_realize;
> -    device_class_set_props(dc, ipi_properties);
> -    dc->vmsd = &vmstate_loongarch_ipi;
> -}
> -
> -static void loongarch_ipi_finalize(Object *obj)
> -{
> -    LoongArchIPI *s = LOONGARCH_IPI(obj);
> -
> -    g_free(s->cpu);
> -}
> -
> -static const TypeInfo loongarch_ipi_info = {
> -    .name          = TYPE_LOONGARCH_IPI,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(LoongArchIPI),
> -    .class_init    = loongarch_ipi_class_init,
> -    .instance_finalize = loongarch_ipi_finalize,
> -};
> -
> -static void loongarch_ipi_register_types(void)
> -{
> -    type_register_static(&loongarch_ipi_info);
> -}
> -
> -type_init(loongarch_ipi_register_types)
>