diff mbox series

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

Message ID 20241112021738.1952851-4-maobibo@loongson.cn
State New
Headers show
Series hw/loongarch/virt: Add cpu hotplug support | expand

Commit Message

bibo mao Nov. 12, 2024, 2:17 a.m. UTC
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.

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(-)

Comments

Igor Mammedov Nov. 18, 2024, 4:43 p.m. UTC | #1
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")
bibo mao Nov. 19, 2024, 10:02 a.m. UTC | #2
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.
yes, that is actually one problem with heavy dependency, I will try to 
remove the dependency.
> 
>>
>> 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)
yes, will do. possible_cpus can be used and virt_get_cpu() is unnecessary.

Interesting, I never create cpu like the method like this, will try this.
'-smp x, -device your-cpu,...'
>    
>>           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.
yes, will try to remove this and ipi device realize funciton. When ipi 
device is created, it will search possible_cpus and connect to interrupt 
pin of supported CPU.

The real hw is same with Interrupt Pin method :(, and there is no 
apic-bus or Processor System Bus like x86.

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")
Igor Mammedov Nov. 22, 2024, 1:45 p.m. UTC | #3
On Tue, 19 Nov 2024 18:02:54 +0800
bibo mao <maobibo@loongson.cn> wrote:

> 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.  
> yes, that is actually one problem with heavy dependency, I will try to 
> remove the dependency.
> >   
> >>
> >> 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)  
> yes, will do. possible_cpus can be used and virt_get_cpu() is unnecessary.
> 
> Interesting, I never create cpu like the method like this, will try this.
> '-smp x, -device your-cpu,...'

that's how target VM could be starred with if cpu were hotpluged on
migration source side.

'-smp x' basically shortcut to series of '-device cpu-foo',
with the only big difference is that the later is created after machine_init
while '-smp x' CPUs are created at machine_init time.

That's the reason to I'm pushing you to move all CPU wiring to plug handlers,
so eventually you would end up with only way of adding CPUs, regardless of
what creates them (-smp or -device/device_add)

Ideally/if possible you should be able to start VM with '-smp 0, -device cpu-foo'

> >      
> >>           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.  
> yes, will try to remove this and ipi device realize funciton. When ipi 
> device is created, it will search possible_cpus and connect to interrupt 
> pin of supported CPU.
> 
> The real hw is same with Interrupt Pin method :(, and there is no 
> apic-bus or Processor System Bus like x86.
> 
> 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")  
>
bibo mao Nov. 25, 2024, 1:54 a.m. UTC | #4
On 2024/11/22 下午9:45, Igor Mammedov wrote:
> On Tue, 19 Nov 2024 18:02:54 +0800
> bibo mao <maobibo@loongson.cn> wrote:
> 
>> 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.
>> yes, that is actually one problem with heavy dependency, I will try to
>> remove the dependency.
>>>    
>>>>
>>>> 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)
>> yes, will do. possible_cpus can be used and virt_get_cpu() is unnecessary.
>>
>> Interesting, I never create cpu like the method like this, will try this.
>> '-smp x, -device your-cpu,...'
> 
> that's how target VM could be starred with if cpu were hotpluged on
> migration source side.
> 
> '-smp x' basically shortcut to series of '-device cpu-foo',
> with the only big difference is that the later is created after machine_init
> while '-smp x' CPUs are created at machine_init time.
> 
> That's the reason to I'm pushing you to move all CPU wiring to plug handlers,
> so eventually you would end up with only way of adding CPUs, regardless of
> what creates them (-smp or -device/device_add)
Got it. I have further understanding with CPU object, I do not notice 
this before.

And thanks for your kindly help.

Regards
Bibo Mao
> 
> Ideally/if possible you should be able to start VM with '-smp 0, -device cpu-foo'
> 
>>>       
>>>>            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.
>> yes, will try to remove this and ipi device realize funciton. When ipi
>> device is created, it will search possible_cpus and connect to interrupt
>> pin of supported CPU.
>>
>> The real hw is same with Interrupt Pin method :(, and there is no
>> apic-bus or Processor System Bus like x86.
>>
>> 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")
>>
bibo mao Nov. 28, 2024, 9:02 a.m. UTC | #5
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
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")
>
diff mbox series

Patch

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;
+}
+
 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--) {
         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,
+                              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")