Re: [PATCH v4 3/6] hw/loongarch/virt: Add generic function to init interrupt pin of CPU

[bibo mao]

On 2024/11/19 上午12:43, Igor Mammedov wrote:
> On Tue, 12 Nov 2024 10:17:35 +0800
> Bibo Mao <maobibo@loongson.cn> wrote:
>
> > Here generic function virt_init_cpu_irq() is added to init interrupt
> > pin of CPU object, IPI and extioi interrupt controllers are connected
> > to interrupt pin of CPU object.
> >
> > The generic function can be used to both cold-plug and hot-plug CPUs.
>
> this patch is heavily depends on cpu_index and specific order CPUs
> are created.
>
> > Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> > ---
> >   hw/loongarch/virt.c         | 78 ++++++++++++++++++++++++-------------
> >   include/hw/loongarch/virt.h |  2 +
> >   2 files changed, 53 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> > index b6b616d278..621380e2b3 100644
> > --- a/hw/loongarch/virt.c
> > +++ b/hw/loongarch/virt.c
> > @@ -58,6 +58,20 @@ static bool 
> > virt_is_veiointc_enabled(LoongArchVirtMachineState *lvms)
> >       return true;
> >   }
> >   
> > +static CPUState *virt_get_cpu(MachineState *ms, int index)
> > +{
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +    const CPUArchIdList *possible_cpus;
> > +
> > +    /* Init CPUs */
> > +    possible_cpus = mc->possible_cpu_arch_ids(ms);
> > +    if (index < 0 || index >= possible_cpus->len) {
> > +        return NULL;
> > +    }
> > +
> > +    return possible_cpus->cpus[index].cpu;
> > +}
>
> instead of adding this helper I'd suggest to try reusing
> virt_find_cpu_slot() added in previous patch.
>
> > +
> >   static void virt_get_veiointc(Object *obj, Visitor *v, const char *name,
> >                                 void *opaque, Error **errp)
> >   {
> > @@ -365,7 +379,7 @@ static void create_fdt(LoongArchVirtMachineState *lvms)
> >   static void fdt_add_cpu_nodes(const LoongArchVirtMachineState *lvms)
> >   {
> >       int num;
> > -    const MachineState *ms = MACHINE(lvms);
> > +    MachineState *ms = MACHINE(lvms);
> >       int smp_cpus = ms->smp.cpus;
> >   
> >       qemu_fdt_add_subnode(ms->fdt, "/cpus");
> > @@ -375,7 +389,7 @@ static void fdt_add_cpu_nodes(const 
> > LoongArchVirtMachineState *lvms)
> >       /* cpu nodes */
> >       for (num = smp_cpus - 1; num >= 0; num--) {
>
> loops based on smp_cpus become broken as soon as you have
>   '-smp x, -device your-cpu,...
> since it doesn't take in account '-device' created CPUs.
> You likely need to replace such loops to iterate over possible_cpus
> (in a separate patch please)
>    
> >           char *nodename = g_strdup_printf("/cpus/cpu@%d", num);
> > -        LoongArchCPU *cpu = LOONGARCH_CPU(qemu_get_cpu(num));
> > +        LoongArchCPU *cpu = LOONGARCH_CPU(virt_get_cpu(ms, num));
> >           CPUState *cs = CPU(cpu);
> >   
> >           qemu_fdt_add_subnode(ms->fdt, nodename);
> > @@ -783,16 +797,42 @@ static void virt_devices_init(DeviceState *pch_pic,
> >       lvms->platform_bus_dev = create_platform_bus(pch_pic);
> >   }
> >   
> > +static void virt_init_cpu_irq(MachineState *ms, CPUState *cs)
> > +{
> > +    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
> > +    CPULoongArchState *env;
> > +    LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(ms);
> > +    int pin;
> > +
> > +    if (!lvms->ipi || !lvms->extioi) {
> > +        return;
> > +    }
> > +
> > +    env = &(cpu->env);
> > +    env->address_space_iocsr = &lvms->as_iocsr;
> > +    env->ipistate = lvms->ipi;
> > +    /* connect ipi irq to cpu irq, logic cpu index used here */
> > +    qdev_connect_gpio_out(lvms->ipi, cs->cpu_index,
> I'd try to avoid using cpu_index (basically internal CPU detail) when
> wiring components together. It would be better to implement this the way
> the real hw does it.

lapic is created when x86 cpu object is realized, there is no lapic on 
LoongArch. One mechanism need be used to notify irqchip driver to setup 
interrupt routing for multiple processors when CPU is added.

How about adding HOTPLUG interface in irqchip driver, notifying irqchip 
driver when cpu is added? The sample code is shown at website
20241128021024.662057-5-maobibo@loongson.cn/T/#u">https://lore.kernel.org/qemu-devel/20241128021024.662057-5-maobibo@loongson.cn/T/#u

If so, qdev_connect_gpio_out and cs->cpu_index will be removed, just 
hotplug_handler_plug() notification to irqchip driver is used such as
   hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), cs, &local_err);

Regards
Bibo Mao
> > +                              qdev_get_gpio_in(DEVICE(cs), IRQ_IPI));
> > +
> > +    /*
> > +     * connect ext irq to the cpu irq
> > +     * cpu_pin[9:2] <= intc_pin[7:0]
> > +     */
> > +    for (pin = 0; pin < LS3A_INTC_IP; pin++) {
> > +        qdev_connect_gpio_out(lvms->extioi, cs->cpu_index * LS3A_INTC_IP + pin,
> > +                              qdev_get_gpio_in(DEVICE(cs), pin + 2));
> > +    }
> > +}
> > +
> >   static void virt_irq_init(LoongArchVirtMachineState *lvms)
> >   {
> >       MachineState *ms = MACHINE(lvms);
> > -    DeviceState *pch_pic, *pch_msi, *cpudev;
> > +    DeviceState *pch_pic, *pch_msi;
> >       DeviceState *ipi, *extioi;
> >       SysBusDevice *d;
> > -    LoongArchCPU *lacpu;
> > -    CPULoongArchState *env;
> >       CPUState *cpu_state;
> > -    int cpu, pin, i, start, num;
> > +    int cpu, i, start, num;
> >       uint32_t cpuintc_phandle, eiointc_phandle, pch_pic_phandle, 
> > pch_msi_phandle;
> >   
> >       /*
> > @@ -843,6 +883,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
> >       ipi = qdev_new(TYPE_LOONGARCH_IPI);
> >       qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.cpus);
> >       sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), &error_fatal);
> > +    lvms->ipi = ipi;
> >   
> >       /* IPI iocsr memory region */
> >       memory_region_add_subregion(&lvms->system_iocsr, SMP_IPI_MAILBOX,
> > @@ -853,18 +894,6 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
> >       /* Add cpu interrupt-controller */
> >       fdt_add_cpuic_node(lvms, &cpuintc_phandle);
> >   
> > -    for (cpu = 0; cpu < ms->smp.cpus; cpu++) {
> > -        cpu_state = qemu_get_cpu(cpu);
> > -        cpudev = DEVICE(cpu_state);
> > -        lacpu = LOONGARCH_CPU(cpu_state);
> > -        env = &(lacpu->env);
> > -        env->address_space_iocsr = &lvms->as_iocsr;
> > -
> > -        /* connect ipi irq to cpu irq */
> > -        qdev_connect_gpio_out(ipi, cpu, qdev_get_gpio_in(cpudev, IRQ_IPI));
> > -        env->ipistate = ipi;
> > -    }
> > -
> >       /* Create EXTIOI device */
> >       extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
> >       qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.cpus);
> > @@ -872,6 +901,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
> >           qdev_prop_set_bit(extioi, "has-virtualization-extension", true);
> >       }
> >       sysbus_realize_and_unref(SYS_BUS_DEVICE(extioi), &error_fatal);
> > +    lvms->extioi = extioi;
> >       memory_region_add_subregion(&lvms->system_iocsr, APIC_BASE,
> >                       sysbus_mmio_get_region(SYS_BUS_DEVICE(extioi), 0));
> >       if (virt_is_veiointc_enabled(lvms)) {
> > @@ -879,16 +909,10 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
> >                       sysbus_mmio_get_region(SYS_BUS_DEVICE(extioi), 1));
> >       }
> >   
> > -    /*
> > -     * connect ext irq to the cpu irq
> > -     * cpu_pin[9:2] <= intc_pin[7:0]
> > -     */
> > +    /* Connect irq to cpu, including ipi and extioi irqchip */
> >       for (cpu = 0; cpu < ms->smp.cpus; cpu++) {
> > -        cpudev = DEVICE(qemu_get_cpu(cpu));
> > -        for (pin = 0; pin < LS3A_INTC_IP; pin++) {
> > -            qdev_connect_gpio_out(extioi, (cpu * 8 + pin),
> > -                                  qdev_get_gpio_in(cpudev, pin + 2));
> > -        }
> > +        cpu_state = virt_get_cpu(ms, cpu);
> > +        virt_init_cpu_irq(ms, cpu_state);
> >       }
> >   
> >       /* Add Extend I/O Interrupt Controller node */
> > diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
> > index 9ba47793ef..260e6bd7cf 100644
> > --- a/include/hw/loongarch/virt.h
> > +++ b/include/hw/loongarch/virt.h
> > @@ -60,6 +60,8 @@ struct LoongArchVirtMachineState {
> >       MemoryRegion iocsr_mem;
> >       AddressSpace as_iocsr;
> >       struct loongarch_boot_info bootinfo;
> > +    DeviceState *ipi;
> > +    DeviceState *extioi;
> >   };
> >   
> >   #define TYPE_LOONGARCH_VIRT_MACHINE  MACHINE_TYPE_NAME("virt")