Re: [PATCH v3 1/4] hw/intc: Remove loongarch_ipi.c

[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 :(

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.