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

[Philippe Mathieu-Daudé]

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:
20240627125819.62779-2-philmd@linaro.org">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.