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

[Igor Mammedov <imammedo@redhat.com>]

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.


> +                              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")