[PATCH v4 0/6] hw/loongarch/virt: Add cpu hotplug support

[PATCH v4 0/6] hw/loongarch/virt: Add cpu hotplug support

[Bibo Mao]

LoongArch cpu hotplug is based on ACPI GED device, there is a little
change about ipi and extioi device, the value of num-cpu property is
maximum cpu number rather than present cpu number.

It can be verified with qemu command:
  qemu-system-loongarch64 -smp 2,maxcpus=16,sockets=4,cores=4,threads=1
and vcpu can be added or remove with hmp command:
  device_add la464-loongarch-cpu,socket-id=0,core-id=2,thread-id=0,id=cpu-2
  device_del cpu-2

---
v3 ... v4:
  1. For cold-plug CPUs, move socket-id/core-id/thread-id property
     setting from preplug function to CPU object creating loop, since
     there is topo information calculation already in CPU object creating
     loop.
  2. Init interrupt pin of CPU object in cpu plug interface for both
     cold-plug CPUs and hot-plug CPUs.
  3. Apply the patch based on latest qemu version.

v2 ... v3:
  1. Use qdev_realize_and_unref() with qdev_realize() and object_unref().
  2. Set vcpus_count with 1 since vcpu object is created for every thread.
  3. Remove property hw-id, use internal variable hw_id to differentiate
     cold-plug cpus and hot-plug cpus.
  4. Add generic function virt_init_cpu_irq() to init interrupt pin
     of CPU object, used by both cold-plug and hot-plug CPUs

v1 ... v2:
  1. Add new property hw-id, property hw-id is set for cold-added CPUs,
     and property socket-id/core-id/thread-id is set for hot-added CPUs.
     The two properties can be generated from each other.
  2. Use general hotplug api such as hotplug_handler_pre_plug etc
  3. Reorganize the patch order, split the patch set into 4 small
     patches.
---
Bibo Mao (6):
  hw/loongarch/virt: Add CPU topology support
  hw/loongarch/virt: Implement cpu plug interface
  hw/loongarch/virt: Add generic function to init interrupt pin of CPU
  hw/loongarch/virt: Init interrupt pin of CPU during plug interface
  hw/loongarch/virt: Update the ACPI table for hotplug cpu
  hw/loongarch/virt: Enable cpu hotplug feature on virt machine

 docs/system/loongarch/virt.rst |  31 +++
 hw/loongarch/Kconfig           |   1 +
 hw/loongarch/acpi-build.c      |  35 ++-
 hw/loongarch/virt.c            | 374 ++++++++++++++++++++++++++++-----
 include/hw/loongarch/virt.h    |   3 +
 target/loongarch/cpu.c         |  25 +++
 target/loongarch/cpu.h         |  17 ++
 7 files changed, 428 insertions(+), 58 deletions(-)


base-commit: 134b443512825bed401b6e141447b8cdc22d2efe
-- 
2.39.3

[PATCH v4 1/6] hw/loongarch/virt: Add CPU topology support

[Bibo Mao]

Add topological relationships for Loongarch VCPU and initialize
topology member variables. Also physical cpu id calculation
method comes from its topo information.

Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 docs/system/loongarch/virt.rst | 31 +++++++++++++++
 hw/loongarch/virt.c            | 73 ++++++++++++++++++++++++++++------
 target/loongarch/cpu.c         | 12 ++++++
 target/loongarch/cpu.h         | 16 ++++++++
 4 files changed, 119 insertions(+), 13 deletions(-)

diff --git a/docs/system/loongarch/virt.rst b/docs/system/loongarch/virt.rst
index 172fba079e..8daf60785f 100644
--- a/docs/system/loongarch/virt.rst
+++ b/docs/system/loongarch/virt.rst
@@ -28,6 +28,37 @@ The ``qemu-system-loongarch64`` provides emulation for virt
 machine. You can specify the machine type ``virt`` and
 cpu type ``la464``.
 
+CPU Topology
+------------
+
+The ``LA464`` type CPUs have the concept of Socket Core and Thread.
+
+For example:
+
+``-smp 1,maxcpus=M,sockets=S,cores=C,threads=T``
+
+The above parameters indicate that the machine has a maximum of ``M`` vCPUs and
+``S`` sockets, each socket has ``C`` cores, each core has ``T`` threads,
+and each thread corresponds to a vCPU.
+
+Then ``M`` ``S`` ``C`` ``T`` has the following relationship:
+
+``M = S * C * T``
+
+In the CPU topology relationship, When we know the ``socket_id`` ``core_id``
+and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
+
+``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``
+
+Similarly, when we know the ``arch_id`` of the CPU,
+we can also get its ``socket_id`` ``core_id`` and ``thread_id``:
+
+``socket_id = arch_id / (C * T)``
+
+``core_id = (arch_id / T) % C``
+
+``thread_id = arch_id % T``
+
 Boot options
 ------------
 
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 9a635d1d3d..1ed5130edf 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1143,9 +1143,9 @@ static void virt_init(MachineState *machine)
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
     int i;
     hwaddr base, size, ram_size = machine->ram_size;
-    const CPUArchIdList *possible_cpus;
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     CPUState *cpu;
+    Object *cpuobj;
 
     if (!cpu_model) {
         cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
@@ -1163,13 +1163,30 @@ static void virt_init(MachineState *machine)
     memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
 
     /* Init CPUs */
-    possible_cpus = mc->possible_cpu_arch_ids(machine);
-    for (i = 0; i < possible_cpus->len; i++) {
-        cpu = cpu_create(machine->cpu_type);
+    mc->possible_cpu_arch_ids(machine);
+    for (i = 0; i < machine->smp.cpus; i++) {
+        cpuobj = object_new(machine->cpu_type);
+        if (cpuobj == NULL) {
+            error_report("Fail to create object with type %s ",
+                         machine->cpu_type);
+            exit(EXIT_FAILURE);
+        }
+
+        cpu = CPU(cpuobj);
         cpu->cpu_index = i;
         machine->possible_cpus->cpus[i].cpu = cpu;
-        lacpu = LOONGARCH_CPU(cpu);
+        lacpu = LOONGARCH_CPU(cpuobj);
         lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
+        object_property_set_int(cpuobj, "socket-id",
+                                machine->possible_cpus->cpus[i].props.socket_id,
+                                NULL);
+        object_property_set_int(cpuobj, "core-id",
+                                machine->possible_cpus->cpus[i].props.core_id,
+                                NULL);
+        object_property_set_int(cpuobj, "thread-id",
+                                machine->possible_cpus->cpus[i].props.thread_id,
+                                NULL);
+        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
     }
     fdt_add_cpu_nodes(lvms);
     fdt_add_memory_nodes(machine);
@@ -1286,6 +1303,35 @@ static void virt_initfn(Object *obj)
     virt_flash_create(lvms);
 }
 
+static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo *topo)
+{
+    int arch_id, sock_vcpu_num, core_vcpu_num;
+
+    /*
+     * calculate total logical cpus across socket/core/thread.
+     * For more information on how to calculate the arch_id,
+     * you can refer to the CPU Topology chapter of the
+     * docs/system/loongarch/virt.rst document.
+     */
+    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
+    core_vcpu_num = topo->core_id * ms->smp.threads;
+
+    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
+    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;
+
+    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
+
+    return arch_id;
+}
+
+static void virt_get_topo_from_index(MachineState *ms,
+                                     LoongArchCPUTopo *topo, int index)
+{
+    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
+    topo->core_id = index / ms->smp.threads % ms->smp.cores;
+    topo->thread_id = index % ms->smp.threads;
+}
+
 static bool memhp_type_supported(DeviceState *dev)
 {
     /* we only support pc dimm now */
@@ -1385,8 +1431,9 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
 
 static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
 {
-    int n;
+    int n, arch_id;
     unsigned int max_cpus = ms->smp.max_cpus;
+    LoongArchCPUTopo topo;
 
     if (ms->possible_cpus) {
         assert(ms->possible_cpus->len == max_cpus);
@@ -1397,17 +1444,17 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
                                   sizeof(CPUArchId) * max_cpus);
     ms->possible_cpus->len = max_cpus;
     for (n = 0; n < ms->possible_cpus->len; n++) {
+        virt_get_topo_from_index(ms, &topo, n);
+        arch_id = virt_get_arch_id_from_topo(ms, &topo);
+        ms->possible_cpus->cpus[n].vcpus_count = 1;
         ms->possible_cpus->cpus[n].type = ms->cpu_type;
-        ms->possible_cpus->cpus[n].arch_id = n;
-
+        ms->possible_cpus->cpus[n].arch_id = arch_id;
         ms->possible_cpus->cpus[n].props.has_socket_id = true;
-        ms->possible_cpus->cpus[n].props.socket_id  =
-                                   n / (ms->smp.cores * ms->smp.threads);
+        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
         ms->possible_cpus->cpus[n].props.has_core_id = true;
-        ms->possible_cpus->cpus[n].props.core_id =
-                                   n / ms->smp.threads % ms->smp.cores;
+        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
         ms->possible_cpus->cpus[n].props.has_thread_id = true;
-        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
+        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
     }
     return ms->possible_cpus;
 }
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 57cc4f314b..a99e22094e 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -16,6 +16,7 @@
 #include "kvm/kvm_loongarch.h"
 #include "exec/exec-all.h"
 #include "cpu.h"
+#include "hw/qdev-properties.h"
 #include "internals.h"
 #include "fpu/softfloat-helpers.h"
 #include "cpu-csr.h"
@@ -725,6 +726,7 @@ static void loongarch_cpu_init(Object *obj)
     timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
                   &loongarch_constant_timer_cb, cpu);
 #endif
+    cpu->phy_id = UNSET_PHY_ID;
 #endif
 }
 
@@ -823,6 +825,14 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
 }
 #endif
 
+static Property loongarch_cpu_properties[] = {
+    DEFINE_PROP_INT32("socket-id", LoongArchCPU, socket_id, 0),
+    DEFINE_PROP_INT32("core-id", LoongArchCPU, core_id, 0),
+    DEFINE_PROP_INT32("thread-id", LoongArchCPU, thread_id, 0),
+    DEFINE_PROP_INT32("node-id", LoongArchCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void loongarch_cpu_class_init(ObjectClass *c, void *data)
 {
     LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
@@ -830,6 +840,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
     DeviceClass *dc = DEVICE_CLASS(c);
     ResettableClass *rc = RESETTABLE_CLASS(c);
 
+    device_class_set_props(dc, loongarch_cpu_properties);
     device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
                                     &lacc->parent_realize);
     resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
@@ -854,6 +865,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
 #ifdef CONFIG_TCG
     cc->tcg_ops = &loongarch_tcg_ops;
 #endif
+    dc->user_creatable = true;
 }
 
 static const gchar *loongarch32_gdb_arch_name(CPUState *cs)
diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index 86c86c6c95..7472df0521 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -19,6 +19,12 @@
 #include "cpu-csr.h"
 #include "cpu-qom.h"
 
+/*
+ * CPU can't have 0xFFFFFFFF physical ID, use that value to distinguish
+ * that physical ID hasn't been set yet
+ */
+#define UNSET_PHY_ID 0xFFFFFFFF
+
 #define IOCSRF_TEMP             0
 #define IOCSRF_NODECNT          1
 #define IOCSRF_MSI              2
@@ -390,6 +396,12 @@ typedef struct CPUArchState {
 #endif
 } CPULoongArchState;
 
+typedef struct LoongArchCPUTopo {
+    int32_t socket_id;  /* socket-id of this VCPU */
+    int32_t core_id;    /* core-id of this VCPU */
+    int32_t thread_id;  /* thread-id of this VCPU */
+} LoongArchCPUTopo;
+
 /**
  * LoongArchCPU:
  * @env: #CPULoongArchState
@@ -404,6 +416,10 @@ struct ArchCPU {
     uint32_t  phy_id;
     OnOffAuto lbt;
     OnOffAuto pmu;
+    int32_t socket_id;  /* socket-id of this VCPU */
+    int32_t core_id;    /* core-id of this VCPU */
+    int32_t thread_id;  /* thread-id of this VCPU */
+    int32_t node_id;    /* NUMA node of this VCPU */
 
     /* 'compatible' string for this CPU for Linux device trees */
     const char *dtb_compatible;
-- 
2.39.3

Re: [PATCH v4 1/6] hw/loongarch/virt: Add CPU topology support

[Igor Mammedov]

On Tue, 12 Nov 2024 10:17:33 +0800
Bibo Mao <maobibo@loongson.cn> wrote:

> Add topological relationships for Loongarch VCPU and initialize
> topology member variables. Also physical cpu id calculation
> method comes from its topo information.
> 
> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  docs/system/loongarch/virt.rst | 31 +++++++++++++++
>  hw/loongarch/virt.c            | 73 ++++++++++++++++++++++++++++------
>  target/loongarch/cpu.c         | 12 ++++++
>  target/loongarch/cpu.h         | 16 ++++++++
>  4 files changed, 119 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/system/loongarch/virt.rst b/docs/system/loongarch/virt.rst
> index 172fba079e..8daf60785f 100644
> --- a/docs/system/loongarch/virt.rst
> +++ b/docs/system/loongarch/virt.rst
> @@ -28,6 +28,37 @@ The ``qemu-system-loongarch64`` provides emulation for virt
>  machine. You can specify the machine type ``virt`` and
>  cpu type ``la464``.
>  
> +CPU Topology
> +------------
> +
> +The ``LA464`` type CPUs have the concept of Socket Core and Thread.
> +
> +For example:
> +
> +``-smp 1,maxcpus=M,sockets=S,cores=C,threads=T``
> +
> +The above parameters indicate that the machine has a maximum of ``M`` vCPUs and
> +``S`` sockets, each socket has ``C`` cores, each core has ``T`` threads,
> +and each thread corresponds to a vCPU.
> +
> +Then ``M`` ``S`` ``C`` ``T`` has the following relationship:
> +
> +``M = S * C * T``
> +
> +In the CPU topology relationship, When we know the ``socket_id`` ``core_id``
> +and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
> +
> +``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``

Is there a spec or some other reference where all of this is described?
(or is that a made up just for QEMU?)


> +
> +Similarly, when we know the ``arch_id`` of the CPU,
> +we can also get its ``socket_id`` ``core_id`` and ``thread_id``:
> +
> +``socket_id = arch_id / (C * T)``
> +
> +``core_id = (arch_id / T) % C``
> +
> +``thread_id = arch_id % T``
> +
>  Boot options
>  ------------
>  
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index 9a635d1d3d..1ed5130edf 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -1143,9 +1143,9 @@ static void virt_init(MachineState *machine)
>      LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
>      int i;
>      hwaddr base, size, ram_size = machine->ram_size;
> -    const CPUArchIdList *possible_cpus;
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      CPUState *cpu;
> +    Object *cpuobj;
>  
>      if (!cpu_model) {
>          cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
> @@ -1163,13 +1163,30 @@ static void virt_init(MachineState *machine)
>      memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
>  
>      /* Init CPUs */
> -    possible_cpus = mc->possible_cpu_arch_ids(machine);
I'd keep this, and use below, it makes line shorter


> -    for (i = 0; i < possible_cpus->len; i++) {
> -        cpu = cpu_create(machine->cpu_type);
> +    mc->possible_cpu_arch_ids(machine);
> +    for (i = 0; i < machine->smp.cpus; i++) {
> +        cpuobj = object_new(machine->cpu_type);
> +        if (cpuobj == NULL) {
> +            error_report("Fail to create object with type %s ",
> +                         machine->cpu_type);
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        cpu = CPU(cpuobj);

>          cpu->cpu_index = i;
this probably should be in _pre_plug handler,
also see
(a15d2728a9aa pc: Init CPUState->cpu_index with index in possible_cpus[])
for why x86 does it. 

>          machine->possible_cpus->cpus[i].cpu = cpu;
> -        lacpu = LOONGARCH_CPU(cpu);
> +        lacpu = LOONGARCH_CPU(cpuobj);

>          lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
Given above is derived from topo data set below, I'd move above above
to pre_plug time, and calculate/set it there based on topo data.
There is no point in setting both at the same place. 

> +        object_property_set_int(cpuobj, "socket-id",
> +                                machine->possible_cpus->cpus[i].props.socket_id,
> +                                NULL);
> +        object_property_set_int(cpuobj, "core-id",
> +                                machine->possible_cpus->cpus[i].props.core_id,
> +                                NULL);
> +        object_property_set_int(cpuobj, "thread-id",
> +                                machine->possible_cpus->cpus[i].props.thread_id,
> +                                NULL);
> +        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
>      }
>      fdt_add_cpu_nodes(lvms);
>      fdt_add_memory_nodes(machine);
> @@ -1286,6 +1303,35 @@ static void virt_initfn(Object *obj)
>      virt_flash_create(lvms);
>  }
>  
> +static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo *topo)
> +{
> +    int arch_id, sock_vcpu_num, core_vcpu_num;
> +
> +    /*
> +     * calculate total logical cpus across socket/core/thread.
> +     * For more information on how to calculate the arch_id,
> +     * you can refer to the CPU Topology chapter of the
> +     * docs/system/loongarch/virt.rst document.
> +     */
> +    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
> +    core_vcpu_num = topo->core_id * ms->smp.threads;
> +
> +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
> +    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;
> +
> +    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
> +
> +    return arch_id;
> +}
> +
> +static void virt_get_topo_from_index(MachineState *ms,
> +                                     LoongArchCPUTopo *topo, int index)
> +{
> +    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
> +    topo->core_id = index / ms->smp.threads % ms->smp.cores;
> +    topo->thread_id = index % ms->smp.threads;
> +}
> +
>  static bool memhp_type_supported(DeviceState *dev)
>  {
>      /* we only support pc dimm now */
> @@ -1385,8 +1431,9 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
>  
>  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>  {
> -    int n;
> +    int n, arch_id;
>      unsigned int max_cpus = ms->smp.max_cpus;
> +    LoongArchCPUTopo topo;
>  
>      if (ms->possible_cpus) {
>          assert(ms->possible_cpus->len == max_cpus);
> @@ -1397,17 +1444,17 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>                                    sizeof(CPUArchId) * max_cpus);
>      ms->possible_cpus->len = max_cpus;
>      for (n = 0; n < ms->possible_cpus->len; n++) {
> +        virt_get_topo_from_index(ms, &topo, n);
> +        arch_id = virt_get_arch_id_from_topo(ms, &topo);
> +        ms->possible_cpus->cpus[n].vcpus_count = 1;
>          ms->possible_cpus->cpus[n].type = ms->cpu_type;
> -        ms->possible_cpus->cpus[n].arch_id = n;
> -
> +        ms->possible_cpus->cpus[n].arch_id = arch_id;
>          ms->possible_cpus->cpus[n].props.has_socket_id = true;
> -        ms->possible_cpus->cpus[n].props.socket_id  =
> -                                   n / (ms->smp.cores * ms->smp.threads);
> +        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
>          ms->possible_cpus->cpus[n].props.has_core_id = true;
> -        ms->possible_cpus->cpus[n].props.core_id =
> -                                   n / ms->smp.threads % ms->smp.cores;
> +        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
>          ms->possible_cpus->cpus[n].props.has_thread_id = true;
> -        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
> +        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
>      }
>      return ms->possible_cpus;
>  }
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 57cc4f314b..a99e22094e 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -16,6 +16,7 @@
>  #include "kvm/kvm_loongarch.h"
>  #include "exec/exec-all.h"
>  #include "cpu.h"
> +#include "hw/qdev-properties.h"
>  #include "internals.h"
>  #include "fpu/softfloat-helpers.h"
>  #include "cpu-csr.h"
> @@ -725,6 +726,7 @@ static void loongarch_cpu_init(Object *obj)
>      timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
>                    &loongarch_constant_timer_cb, cpu);
>  #endif
> +    cpu->phy_id = UNSET_PHY_ID;
>  #endif
>  }
>  
> @@ -823,6 +825,14 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
>  }
>  #endif
>  
> +static Property loongarch_cpu_properties[] = {
> +    DEFINE_PROP_INT32("socket-id", LoongArchCPU, socket_id, 0),
> +    DEFINE_PROP_INT32("core-id", LoongArchCPU, core_id, 0),
> +    DEFINE_PROP_INT32("thread-id", LoongArchCPU, thread_id, 0),
> +    DEFINE_PROP_INT32("node-id", LoongArchCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>  static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>  {
>      LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
> @@ -830,6 +840,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>      DeviceClass *dc = DEVICE_CLASS(c);
>      ResettableClass *rc = RESETTABLE_CLASS(c);
>  
> +    device_class_set_props(dc, loongarch_cpu_properties);
>      device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
>                                      &lacc->parent_realize);
>      resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
> @@ -854,6 +865,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>  #ifdef CONFIG_TCG
>      cc->tcg_ops = &loongarch_tcg_ops;
>  #endif
> +    dc->user_creatable = true;
>  }
>  
>  static const gchar *loongarch32_gdb_arch_name(CPUState *cs)
> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
> index 86c86c6c95..7472df0521 100644
> --- a/target/loongarch/cpu.h
> +++ b/target/loongarch/cpu.h
> @@ -19,6 +19,12 @@
>  #include "cpu-csr.h"
>  #include "cpu-qom.h"
>  
> +/*
> + * CPU can't have 0xFFFFFFFF physical ID, use that value to distinguish
> + * that physical ID hasn't been set yet

pointer to CPU spec/doc here would be nice to have

> + */
> +#define UNSET_PHY_ID 0xFFFFFFFF
> +
>  #define IOCSRF_TEMP             0
>  #define IOCSRF_NODECNT          1
>  #define IOCSRF_MSI              2
> @@ -390,6 +396,12 @@ typedef struct CPUArchState {
>  #endif
>  } CPULoongArchState;
>  
> +typedef struct LoongArchCPUTopo {
> +    int32_t socket_id;  /* socket-id of this VCPU */
> +    int32_t core_id;    /* core-id of this VCPU */
> +    int32_t thread_id;  /* thread-id of this VCPU */
> +} LoongArchCPUTopo;
> +
>  /**
>   * LoongArchCPU:
>   * @env: #CPULoongArchState
> @@ -404,6 +416,10 @@ struct ArchCPU {
>      uint32_t  phy_id;
>      OnOffAuto lbt;
>      OnOffAuto pmu;
> +    int32_t socket_id;  /* socket-id of this VCPU */
> +    int32_t core_id;    /* core-id of this VCPU */
> +    int32_t thread_id;  /* thread-id of this VCPU */
> +    int32_t node_id;    /* NUMA node of this VCPU */
>  
>      /* 'compatible' string for this CPU for Linux device trees */
>      const char *dtb_compatible;

Re: [PATCH v4 1/6] hw/loongarch/virt: Add CPU topology support

[Igor Mammedov]

On Mon, 18 Nov 2024 17:10:29 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 12 Nov 2024 10:17:33 +0800
> Bibo Mao <maobibo@loongson.cn> wrote:
> 
> > Add topological relationships for Loongarch VCPU and initialize
> > topology member variables. Also physical cpu id calculation
> > method comes from its topo information.

hmm,

maybe split patch on 3 parts,

* initialize topo in possible CPUs
* add topo properties to CPU
* modify main cpu creation loop
  (I'd reorder that after 2/6, so you'd have pre_plug handler in
    place to move code to)

> > 
> > Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
> > Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> > ---
> >  docs/system/loongarch/virt.rst | 31 +++++++++++++++
> >  hw/loongarch/virt.c            | 73 ++++++++++++++++++++++++++++------
> >  target/loongarch/cpu.c         | 12 ++++++
> >  target/loongarch/cpu.h         | 16 ++++++++
> >  4 files changed, 119 insertions(+), 13 deletions(-)
> > 
> > diff --git a/docs/system/loongarch/virt.rst b/docs/system/loongarch/virt.rst
> > index 172fba079e..8daf60785f 100644
> > --- a/docs/system/loongarch/virt.rst
> > +++ b/docs/system/loongarch/virt.rst
> > @@ -28,6 +28,37 @@ The ``qemu-system-loongarch64`` provides emulation for virt
> >  machine. You can specify the machine type ``virt`` and
> >  cpu type ``la464``.
> >  
> > +CPU Topology
> > +------------
> > +
> > +The ``LA464`` type CPUs have the concept of Socket Core and Thread.
> > +
> > +For example:
> > +
> > +``-smp 1,maxcpus=M,sockets=S,cores=C,threads=T``
> > +
> > +The above parameters indicate that the machine has a maximum of ``M`` vCPUs and
> > +``S`` sockets, each socket has ``C`` cores, each core has ``T`` threads,
> > +and each thread corresponds to a vCPU.
> > +
> > +Then ``M`` ``S`` ``C`` ``T`` has the following relationship:
> > +
> > +``M = S * C * T``
> > +
> > +In the CPU topology relationship, When we know the ``socket_id`` ``core_id``
> > +and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
> > +
> > +``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``  
> 
> Is there a spec or some other reference where all of this is described?
> (or is that a made up just for QEMU?)
> 
> 
> > +
> > +Similarly, when we know the ``arch_id`` of the CPU,
> > +we can also get its ``socket_id`` ``core_id`` and ``thread_id``:
> > +
> > +``socket_id = arch_id / (C * T)``
> > +
> > +``core_id = (arch_id / T) % C``
> > +
> > +``thread_id = arch_id % T``
> > +
> >  Boot options
> >  ------------
> >  
> > diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> > index 9a635d1d3d..1ed5130edf 100644
> > --- a/hw/loongarch/virt.c
> > +++ b/hw/loongarch/virt.c
> > @@ -1143,9 +1143,9 @@ static void virt_init(MachineState *machine)
> >      LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
> >      int i;
> >      hwaddr base, size, ram_size = machine->ram_size;
> > -    const CPUArchIdList *possible_cpus;
> >      MachineClass *mc = MACHINE_GET_CLASS(machine);
> >      CPUState *cpu;
> > +    Object *cpuobj;
> >  
> >      if (!cpu_model) {
> >          cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
> > @@ -1163,13 +1163,30 @@ static void virt_init(MachineState *machine)
> >      memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
> >  
> >      /* Init CPUs */
> > -    possible_cpus = mc->possible_cpu_arch_ids(machine);  
> I'd keep this, and use below, it makes line shorter
> 
> 
> > -    for (i = 0; i < possible_cpus->len; i++) {
> > -        cpu = cpu_create(machine->cpu_type);
> > +    mc->possible_cpu_arch_ids(machine);
> > +    for (i = 0; i < machine->smp.cpus; i++) {
> > +        cpuobj = object_new(machine->cpu_type);
> > +        if (cpuobj == NULL) {
> > +            error_report("Fail to create object with type %s ",
> > +                         machine->cpu_type);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +
> > +        cpu = CPU(cpuobj);  
> 
> >          cpu->cpu_index = i;  
> this probably should be in _pre_plug handler,
> also see
> (a15d2728a9aa pc: Init CPUState->cpu_index with index in possible_cpus[])
> for why x86 does it. 
> 
> >          machine->possible_cpus->cpus[i].cpu = cpu;
> > -        lacpu = LOONGARCH_CPU(cpu);
> > +        lacpu = LOONGARCH_CPU(cpuobj);  
> 
> >          lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;  
> Given above is derived from topo data set below, I'd move above above
> to pre_plug time, and calculate/set it there based on topo data.
> There is no point in setting both at the same place. 
> 
> > +        object_property_set_int(cpuobj, "socket-id",
> > +                                machine->possible_cpus->cpus[i].props.socket_id,
> > +                                NULL);
> > +        object_property_set_int(cpuobj, "core-id",
> > +                                machine->possible_cpus->cpus[i].props.core_id,
> > +                                NULL);
> > +        object_property_set_int(cpuobj, "thread-id",
> > +                                machine->possible_cpus->cpus[i].props.thread_id,
> > +                                NULL);
> > +        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
> >      }
> >      fdt_add_cpu_nodes(lvms);
> >      fdt_add_memory_nodes(machine);
> > @@ -1286,6 +1303,35 @@ static void virt_initfn(Object *obj)
> >      virt_flash_create(lvms);
> >  }
> >  
> > +static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo *topo)
> > +{
> > +    int arch_id, sock_vcpu_num, core_vcpu_num;
> > +
> > +    /*
> > +     * calculate total logical cpus across socket/core/thread.
> > +     * For more information on how to calculate the arch_id,
> > +     * you can refer to the CPU Topology chapter of the
> > +     * docs/system/loongarch/virt.rst document.
> > +     */
> > +    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
> > +    core_vcpu_num = topo->core_id * ms->smp.threads;
> > +
> > +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
> > +    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;
> > +
> > +    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
> > +
> > +    return arch_id;
> > +}
> > +
> > +static void virt_get_topo_from_index(MachineState *ms,
> > +                                     LoongArchCPUTopo *topo, int index)
> > +{
> > +    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
> > +    topo->core_id = index / ms->smp.threads % ms->smp.cores;
> > +    topo->thread_id = index % ms->smp.threads;
> > +}
> > +
> >  static bool memhp_type_supported(DeviceState *dev)
> >  {
> >      /* we only support pc dimm now */
> > @@ -1385,8 +1431,9 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
> >  
> >  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >  {
> > -    int n;
> > +    int n, arch_id;
> >      unsigned int max_cpus = ms->smp.max_cpus;
> > +    LoongArchCPUTopo topo;
> >  
> >      if (ms->possible_cpus) {
> >          assert(ms->possible_cpus->len == max_cpus);
> > @@ -1397,17 +1444,17 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >                                    sizeof(CPUArchId) * max_cpus);
> >      ms->possible_cpus->len = max_cpus;
> >      for (n = 0; n < ms->possible_cpus->len; n++) {
> > +        virt_get_topo_from_index(ms, &topo, n);
> > +        arch_id = virt_get_arch_id_from_topo(ms, &topo);
> > +        ms->possible_cpus->cpus[n].vcpus_count = 1;
> >          ms->possible_cpus->cpus[n].type = ms->cpu_type;
> > -        ms->possible_cpus->cpus[n].arch_id = n;
> > -
> > +        ms->possible_cpus->cpus[n].arch_id = arch_id;
> >          ms->possible_cpus->cpus[n].props.has_socket_id = true;
> > -        ms->possible_cpus->cpus[n].props.socket_id  =
> > -                                   n / (ms->smp.cores * ms->smp.threads);
> > +        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
> >          ms->possible_cpus->cpus[n].props.has_core_id = true;
> > -        ms->possible_cpus->cpus[n].props.core_id =
> > -                                   n / ms->smp.threads % ms->smp.cores;
> > +        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
> >          ms->possible_cpus->cpus[n].props.has_thread_id = true;
> > -        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
> > +        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
> >      }
> >      return ms->possible_cpus;
> >  }
> > diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> > index 57cc4f314b..a99e22094e 100644
> > --- a/target/loongarch/cpu.c
> > +++ b/target/loongarch/cpu.c
> > @@ -16,6 +16,7 @@
> >  #include "kvm/kvm_loongarch.h"
> >  #include "exec/exec-all.h"
> >  #include "cpu.h"
> > +#include "hw/qdev-properties.h"
> >  #include "internals.h"
> >  #include "fpu/softfloat-helpers.h"
> >  #include "cpu-csr.h"
> > @@ -725,6 +726,7 @@ static void loongarch_cpu_init(Object *obj)
> >      timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
> >                    &loongarch_constant_timer_cb, cpu);
> >  #endif
> > +    cpu->phy_id = UNSET_PHY_ID;
> >  #endif
> >  }
> >  
> > @@ -823,6 +825,14 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
> >  }
> >  #endif
> >  
> > +static Property loongarch_cpu_properties[] = {
> > +    DEFINE_PROP_INT32("socket-id", LoongArchCPU, socket_id, 0),
> > +    DEFINE_PROP_INT32("core-id", LoongArchCPU, core_id, 0),
> > +    DEFINE_PROP_INT32("thread-id", LoongArchCPU, thread_id, 0),
> > +    DEFINE_PROP_INT32("node-id", LoongArchCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
> > +    DEFINE_PROP_END_OF_LIST()
> > +};
> > +
> >  static void loongarch_cpu_class_init(ObjectClass *c, void *data)
> >  {
> >      LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
> > @@ -830,6 +840,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
> >      DeviceClass *dc = DEVICE_CLASS(c);
> >      ResettableClass *rc = RESETTABLE_CLASS(c);
> >  
> > +    device_class_set_props(dc, loongarch_cpu_properties);
> >      device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
> >                                      &lacc->parent_realize);
> >      resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
> > @@ -854,6 +865,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
> >  #ifdef CONFIG_TCG
> >      cc->tcg_ops = &loongarch_tcg_ops;
> >  #endif
> > +    dc->user_creatable = true;
> >  }
> >  
> >  static const gchar *loongarch32_gdb_arch_name(CPUState *cs)
> > diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
> > index 86c86c6c95..7472df0521 100644
> > --- a/target/loongarch/cpu.h
> > +++ b/target/loongarch/cpu.h
> > @@ -19,6 +19,12 @@
> >  #include "cpu-csr.h"
> >  #include "cpu-qom.h"
> >  
> > +/*
> > + * CPU can't have 0xFFFFFFFF physical ID, use that value to distinguish
> > + * that physical ID hasn't been set yet  
> 
> pointer to CPU spec/doc here would be nice to have
> 
> > + */
> > +#define UNSET_PHY_ID 0xFFFFFFFF
> > +
> >  #define IOCSRF_TEMP             0
> >  #define IOCSRF_NODECNT          1
> >  #define IOCSRF_MSI              2
> > @@ -390,6 +396,12 @@ typedef struct CPUArchState {
> >  #endif
> >  } CPULoongArchState;
> >  
> > +typedef struct LoongArchCPUTopo {
> > +    int32_t socket_id;  /* socket-id of this VCPU */
> > +    int32_t core_id;    /* core-id of this VCPU */
> > +    int32_t thread_id;  /* thread-id of this VCPU */
> > +} LoongArchCPUTopo;
> > +
> >  /**
> >   * LoongArchCPU:
> >   * @env: #CPULoongArchState
> > @@ -404,6 +416,10 @@ struct ArchCPU {
> >      uint32_t  phy_id;
> >      OnOffAuto lbt;
> >      OnOffAuto pmu;
> > +    int32_t socket_id;  /* socket-id of this VCPU */
> > +    int32_t core_id;    /* core-id of this VCPU */
> > +    int32_t thread_id;  /* thread-id of this VCPU */
> > +    int32_t node_id;    /* NUMA node of this VCPU */
> >  
> >      /* 'compatible' string for this CPU for Linux device trees */
> >      const char *dtb_compatible;  
> 

Re: [PATCH v4 1/6] hw/loongarch/virt: Add CPU topology support

[bibo mao]

On 2024/11/19 上午12:22, Igor Mammedov wrote:
> On Mon, 18 Nov 2024 17:10:29 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
>> On Tue, 12 Nov 2024 10:17:33 +0800
>> Bibo Mao <maobibo@loongson.cn> wrote:
>>
>>> Add topological relationships for Loongarch VCPU and initialize
>>> topology member variables. Also physical cpu id calculation
>>> method comes from its topo information.
> 
> hmm,
> 
> maybe split patch on 3 parts,
> 
> * initialize topo in possible CPUs
> * add topo properties to CPU
> * modify main cpu creation loop
>    (I'd reorder that after 2/6, so you'd have pre_plug handler in
>      place to move code to)
> 
Sure, will do in this way.

Regards
Bibo Mao
>>>
>>> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>> ---
>>>   docs/system/loongarch/virt.rst | 31 +++++++++++++++
>>>   hw/loongarch/virt.c            | 73 ++++++++++++++++++++++++++++------
>>>   target/loongarch/cpu.c         | 12 ++++++
>>>   target/loongarch/cpu.h         | 16 ++++++++
>>>   4 files changed, 119 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/docs/system/loongarch/virt.rst b/docs/system/loongarch/virt.rst
>>> index 172fba079e..8daf60785f 100644
>>> --- a/docs/system/loongarch/virt.rst
>>> +++ b/docs/system/loongarch/virt.rst
>>> @@ -28,6 +28,37 @@ The ``qemu-system-loongarch64`` provides emulation for virt
>>>   machine. You can specify the machine type ``virt`` and
>>>   cpu type ``la464``.
>>>   
>>> +CPU Topology
>>> +------------
>>> +
>>> +The ``LA464`` type CPUs have the concept of Socket Core and Thread.
>>> +
>>> +For example:
>>> +
>>> +``-smp 1,maxcpus=M,sockets=S,cores=C,threads=T``
>>> +
>>> +The above parameters indicate that the machine has a maximum of ``M`` vCPUs and
>>> +``S`` sockets, each socket has ``C`` cores, each core has ``T`` threads,
>>> +and each thread corresponds to a vCPU.
>>> +
>>> +Then ``M`` ``S`` ``C`` ``T`` has the following relationship:
>>> +
>>> +``M = S * C * T``
>>> +
>>> +In the CPU topology relationship, When we know the ``socket_id`` ``core_id``
>>> +and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
>>> +
>>> +``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``
>>
>> Is there a spec or some other reference where all of this is described?
>> (or is that a made up just for QEMU?)
>>
>>
>>> +
>>> +Similarly, when we know the ``arch_id`` of the CPU,
>>> +we can also get its ``socket_id`` ``core_id`` and ``thread_id``:
>>> +
>>> +``socket_id = arch_id / (C * T)``
>>> +
>>> +``core_id = (arch_id / T) % C``
>>> +
>>> +``thread_id = arch_id % T``
>>> +
>>>   Boot options
>>>   ------------
>>>   
>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>>> index 9a635d1d3d..1ed5130edf 100644
>>> --- a/hw/loongarch/virt.c
>>> +++ b/hw/loongarch/virt.c
>>> @@ -1143,9 +1143,9 @@ static void virt_init(MachineState *machine)
>>>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
>>>       int i;
>>>       hwaddr base, size, ram_size = machine->ram_size;
>>> -    const CPUArchIdList *possible_cpus;
>>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>       CPUState *cpu;
>>> +    Object *cpuobj;
>>>   
>>>       if (!cpu_model) {
>>>           cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
>>> @@ -1163,13 +1163,30 @@ static void virt_init(MachineState *machine)
>>>       memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
>>>   
>>>       /* Init CPUs */
>>> -    possible_cpus = mc->possible_cpu_arch_ids(machine);
>> I'd keep this, and use below, it makes line shorter
>>
>>
>>> -    for (i = 0; i < possible_cpus->len; i++) {
>>> -        cpu = cpu_create(machine->cpu_type);
>>> +    mc->possible_cpu_arch_ids(machine);
>>> +    for (i = 0; i < machine->smp.cpus; i++) {
>>> +        cpuobj = object_new(machine->cpu_type);
>>> +        if (cpuobj == NULL) {
>>> +            error_report("Fail to create object with type %s ",
>>> +                         machine->cpu_type);
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +
>>> +        cpu = CPU(cpuobj);
>>
>>>           cpu->cpu_index = i;
>> this probably should be in _pre_plug handler,
>> also see
>> (a15d2728a9aa pc: Init CPUState->cpu_index with index in possible_cpus[])
>> for why x86 does it.
>>
>>>           machine->possible_cpus->cpus[i].cpu = cpu;
>>> -        lacpu = LOONGARCH_CPU(cpu);
>>> +        lacpu = LOONGARCH_CPU(cpuobj);
>>
>>>           lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
>> Given above is derived from topo data set below, I'd move above above
>> to pre_plug time, and calculate/set it there based on topo data.
>> There is no point in setting both at the same place.
>>
>>> +        object_property_set_int(cpuobj, "socket-id",
>>> +                                machine->possible_cpus->cpus[i].props.socket_id,
>>> +                                NULL);
>>> +        object_property_set_int(cpuobj, "core-id",
>>> +                                machine->possible_cpus->cpus[i].props.core_id,
>>> +                                NULL);
>>> +        object_property_set_int(cpuobj, "thread-id",
>>> +                                machine->possible_cpus->cpus[i].props.thread_id,
>>> +                                NULL);
>>> +        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
>>>       }
>>>       fdt_add_cpu_nodes(lvms);
>>>       fdt_add_memory_nodes(machine);
>>> @@ -1286,6 +1303,35 @@ static void virt_initfn(Object *obj)
>>>       virt_flash_create(lvms);
>>>   }
>>>   
>>> +static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo *topo)
>>> +{
>>> +    int arch_id, sock_vcpu_num, core_vcpu_num;
>>> +
>>> +    /*
>>> +     * calculate total logical cpus across socket/core/thread.
>>> +     * For more information on how to calculate the arch_id,
>>> +     * you can refer to the CPU Topology chapter of the
>>> +     * docs/system/loongarch/virt.rst document.
>>> +     */
>>> +    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
>>> +    core_vcpu_num = topo->core_id * ms->smp.threads;
>>> +
>>> +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
>>> +    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;
>>> +
>>> +    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
>>> +
>>> +    return arch_id;
>>> +}
>>> +
>>> +static void virt_get_topo_from_index(MachineState *ms,
>>> +                                     LoongArchCPUTopo *topo, int index)
>>> +{
>>> +    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
>>> +    topo->core_id = index / ms->smp.threads % ms->smp.cores;
>>> +    topo->thread_id = index % ms->smp.threads;
>>> +}
>>> +
>>>   static bool memhp_type_supported(DeviceState *dev)
>>>   {
>>>       /* we only support pc dimm now */
>>> @@ -1385,8 +1431,9 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
>>>   
>>>   static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>   {
>>> -    int n;
>>> +    int n, arch_id;
>>>       unsigned int max_cpus = ms->smp.max_cpus;
>>> +    LoongArchCPUTopo topo;
>>>   
>>>       if (ms->possible_cpus) {
>>>           assert(ms->possible_cpus->len == max_cpus);
>>> @@ -1397,17 +1444,17 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>                                     sizeof(CPUArchId) * max_cpus);
>>>       ms->possible_cpus->len = max_cpus;
>>>       for (n = 0; n < ms->possible_cpus->len; n++) {
>>> +        virt_get_topo_from_index(ms, &topo, n);
>>> +        arch_id = virt_get_arch_id_from_topo(ms, &topo);
>>> +        ms->possible_cpus->cpus[n].vcpus_count = 1;
>>>           ms->possible_cpus->cpus[n].type = ms->cpu_type;
>>> -        ms->possible_cpus->cpus[n].arch_id = n;
>>> -
>>> +        ms->possible_cpus->cpus[n].arch_id = arch_id;
>>>           ms->possible_cpus->cpus[n].props.has_socket_id = true;
>>> -        ms->possible_cpus->cpus[n].props.socket_id  =
>>> -                                   n / (ms->smp.cores * ms->smp.threads);
>>> +        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
>>>           ms->possible_cpus->cpus[n].props.has_core_id = true;
>>> -        ms->possible_cpus->cpus[n].props.core_id =
>>> -                                   n / ms->smp.threads % ms->smp.cores;
>>> +        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
>>>           ms->possible_cpus->cpus[n].props.has_thread_id = true;
>>> -        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
>>> +        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
>>>       }
>>>       return ms->possible_cpus;
>>>   }
>>> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
>>> index 57cc4f314b..a99e22094e 100644
>>> --- a/target/loongarch/cpu.c
>>> +++ b/target/loongarch/cpu.c
>>> @@ -16,6 +16,7 @@
>>>   #include "kvm/kvm_loongarch.h"
>>>   #include "exec/exec-all.h"
>>>   #include "cpu.h"
>>> +#include "hw/qdev-properties.h"
>>>   #include "internals.h"
>>>   #include "fpu/softfloat-helpers.h"
>>>   #include "cpu-csr.h"
>>> @@ -725,6 +726,7 @@ static void loongarch_cpu_init(Object *obj)
>>>       timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
>>>                     &loongarch_constant_timer_cb, cpu);
>>>   #endif
>>> +    cpu->phy_id = UNSET_PHY_ID;
>>>   #endif
>>>   }
>>>   
>>> @@ -823,6 +825,14 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
>>>   }
>>>   #endif
>>>   
>>> +static Property loongarch_cpu_properties[] = {
>>> +    DEFINE_PROP_INT32("socket-id", LoongArchCPU, socket_id, 0),
>>> +    DEFINE_PROP_INT32("core-id", LoongArchCPU, core_id, 0),
>>> +    DEFINE_PROP_INT32("thread-id", LoongArchCPU, thread_id, 0),
>>> +    DEFINE_PROP_INT32("node-id", LoongArchCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
>>> +    DEFINE_PROP_END_OF_LIST()
>>> +};
>>> +
>>>   static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>   {
>>>       LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
>>> @@ -830,6 +840,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>       DeviceClass *dc = DEVICE_CLASS(c);
>>>       ResettableClass *rc = RESETTABLE_CLASS(c);
>>>   
>>> +    device_class_set_props(dc, loongarch_cpu_properties);
>>>       device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
>>>                                       &lacc->parent_realize);
>>>       resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
>>> @@ -854,6 +865,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>   #ifdef CONFIG_TCG
>>>       cc->tcg_ops = &loongarch_tcg_ops;
>>>   #endif
>>> +    dc->user_creatable = true;
>>>   }
>>>   
>>>   static const gchar *loongarch32_gdb_arch_name(CPUState *cs)
>>> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
>>> index 86c86c6c95..7472df0521 100644
>>> --- a/target/loongarch/cpu.h
>>> +++ b/target/loongarch/cpu.h
>>> @@ -19,6 +19,12 @@
>>>   #include "cpu-csr.h"
>>>   #include "cpu-qom.h"
>>>   
>>> +/*
>>> + * CPU can't have 0xFFFFFFFF physical ID, use that value to distinguish
>>> + * that physical ID hasn't been set yet
>>
>> pointer to CPU spec/doc here would be nice to have
>>
>>> + */
>>> +#define UNSET_PHY_ID 0xFFFFFFFF
>>> +
>>>   #define IOCSRF_TEMP             0
>>>   #define IOCSRF_NODECNT          1
>>>   #define IOCSRF_MSI              2
>>> @@ -390,6 +396,12 @@ typedef struct CPUArchState {
>>>   #endif
>>>   } CPULoongArchState;
>>>   
>>> +typedef struct LoongArchCPUTopo {
>>> +    int32_t socket_id;  /* socket-id of this VCPU */
>>> +    int32_t core_id;    /* core-id of this VCPU */
>>> +    int32_t thread_id;  /* thread-id of this VCPU */
>>> +} LoongArchCPUTopo;
>>> +
>>>   /**
>>>    * LoongArchCPU:
>>>    * @env: #CPULoongArchState
>>> @@ -404,6 +416,10 @@ struct ArchCPU {
>>>       uint32_t  phy_id;
>>>       OnOffAuto lbt;
>>>       OnOffAuto pmu;
>>> +    int32_t socket_id;  /* socket-id of this VCPU */
>>> +    int32_t core_id;    /* core-id of this VCPU */
>>> +    int32_t thread_id;  /* thread-id of this VCPU */
>>> +    int32_t node_id;    /* NUMA node of this VCPU */
>>>   
>>>       /* 'compatible' string for this CPU for Linux device trees */
>>>       const char *dtb_compatible;
>>

Re: [PATCH v4 1/6] hw/loongarch/virt: Add CPU topology support

[bibo mao]

Hi Ignor,

On 2024/11/19 上午12:10, Igor Mammedov wrote:
> On Tue, 12 Nov 2024 10:17:33 +0800
> Bibo Mao <maobibo@loongson.cn> wrote:
> 
>> Add topological relationships for Loongarch VCPU and initialize
>> topology member variables. Also physical cpu id calculation
>> method comes from its topo information.
>>
>> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   docs/system/loongarch/virt.rst | 31 +++++++++++++++
>>   hw/loongarch/virt.c            | 73 ++++++++++++++++++++++++++++------
>>   target/loongarch/cpu.c         | 12 ++++++
>>   target/loongarch/cpu.h         | 16 ++++++++
>>   4 files changed, 119 insertions(+), 13 deletions(-)
>>
>> diff --git a/docs/system/loongarch/virt.rst b/docs/system/loongarch/virt.rst
>> index 172fba079e..8daf60785f 100644
>> --- a/docs/system/loongarch/virt.rst
>> +++ b/docs/system/loongarch/virt.rst
>> @@ -28,6 +28,37 @@ The ``qemu-system-loongarch64`` provides emulation for virt
>>   machine. You can specify the machine type ``virt`` and
>>   cpu type ``la464``.
>>   
>> +CPU Topology
>> +------------
>> +
>> +The ``LA464`` type CPUs have the concept of Socket Core and Thread.
>> +
>> +For example:
>> +
>> +``-smp 1,maxcpus=M,sockets=S,cores=C,threads=T``
>> +
>> +The above parameters indicate that the machine has a maximum of ``M`` vCPUs and
>> +``S`` sockets, each socket has ``C`` cores, each core has ``T`` threads,
>> +and each thread corresponds to a vCPU.
>> +
>> +Then ``M`` ``S`` ``C`` ``T`` has the following relationship:
>> +
>> +``M = S * C * T``
>> +
>> +In the CPU topology relationship, When we know the ``socket_id`` ``core_id``
>> +and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
>> +
>> +``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``
> 
> Is there a spec or some other reference where all of this is described?
> (or is that a made up just for QEMU?)
With hardware manual about cpuid register, it only says that it is 9-bit 
width now, however there is no detailed introduction about 
socket_id/core_id/thread_id about this register. So it can be treated as 
a made up for QEMU.
> 
> 
>> +
>> +Similarly, when we know the ``arch_id`` of the CPU,
>> +we can also get its ``socket_id`` ``core_id`` and ``thread_id``:
>> +
>> +``socket_id = arch_id / (C * T)``
>> +
>> +``core_id = (arch_id / T) % C``
>> +
>> +``thread_id = arch_id % T``
>> +
>>   Boot options
>>   ------------
>>   
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index 9a635d1d3d..1ed5130edf 100644
>> --- a/hw/loongarch/virt.c
>> +++ b/hw/loongarch/virt.c
>> @@ -1143,9 +1143,9 @@ static void virt_init(MachineState *machine)
>>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
>>       int i;
>>       hwaddr base, size, ram_size = machine->ram_size;
>> -    const CPUArchIdList *possible_cpus;
>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>       CPUState *cpu;
>> +    Object *cpuobj;
>>   
>>       if (!cpu_model) {
>>           cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
>> @@ -1163,13 +1163,30 @@ static void virt_init(MachineState *machine)
>>       memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
>>   
>>       /* Init CPUs */
>> -    possible_cpus = mc->possible_cpu_arch_ids(machine);
> I'd keep this, and use below, it makes line shorter
Sure, will modify it in next version.

> 
> 
>> -    for (i = 0; i < possible_cpus->len; i++) {
>> -        cpu = cpu_create(machine->cpu_type);
>> +    mc->possible_cpu_arch_ids(machine);
>> +    for (i = 0; i < machine->smp.cpus; i++) {
>> +        cpuobj = object_new(machine->cpu_type);
>> +        if (cpuobj == NULL) {
>> +            error_report("Fail to create object with type %s ",
>> +                         machine->cpu_type);
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        cpu = CPU(cpuobj);
> 
>>           cpu->cpu_index = i;
> this probably should be in _pre_plug handler,
> also see
> (a15d2728a9aa pc: Init CPUState->cpu_index with index in possible_cpus[])
> for why x86 does it.
> 
Will modify it in next version.

>>           machine->possible_cpus->cpus[i].cpu = cpu;
>> -        lacpu = LOONGARCH_CPU(cpu);
>> +        lacpu = LOONGARCH_CPU(cpuobj);
> 
>>           lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
> Given above is derived from topo data set below, I'd move above above
> to pre_plug time, and calculate/set it there based on topo data.
> There is no point in setting both at the same place.
> 
Will do.
>> +        object_property_set_int(cpuobj, "socket-id",
>> +                                machine->possible_cpus->cpus[i].props.socket_id,
>> +                                NULL);
>> +        object_property_set_int(cpuobj, "core-id",
>> +                                machine->possible_cpus->cpus[i].props.core_id,
>> +                                NULL);
>> +        object_property_set_int(cpuobj, "thread-id",
>> +                                machine->possible_cpus->cpus[i].props.thread_id,
>> +                                NULL);
>> +        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
>>       }
>>       fdt_add_cpu_nodes(lvms);
>>       fdt_add_memory_nodes(machine);
>> @@ -1286,6 +1303,35 @@ static void virt_initfn(Object *obj)
>>       virt_flash_create(lvms);
>>   }
>>   
>> +static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo *topo)
>> +{
>> +    int arch_id, sock_vcpu_num, core_vcpu_num;
>> +
>> +    /*
>> +     * calculate total logical cpus across socket/core/thread.
>> +     * For more information on how to calculate the arch_id,
>> +     * you can refer to the CPU Topology chapter of the
>> +     * docs/system/loongarch/virt.rst document.
>> +     */
>> +    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
>> +    core_vcpu_num = topo->core_id * ms->smp.threads;
>> +
>> +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
>> +    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;
>> +
>> +    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
>> +
>> +    return arch_id;
>> +}
>> +
>> +static void virt_get_topo_from_index(MachineState *ms,
>> +                                     LoongArchCPUTopo *topo, int index)
>> +{
>> +    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
>> +    topo->core_id = index / ms->smp.threads % ms->smp.cores;
>> +    topo->thread_id = index % ms->smp.threads;
>> +}
>> +
>>   static bool memhp_type_supported(DeviceState *dev)
>>   {
>>       /* we only support pc dimm now */
>> @@ -1385,8 +1431,9 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
>>   
>>   static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>   {
>> -    int n;
>> +    int n, arch_id;
>>       unsigned int max_cpus = ms->smp.max_cpus;
>> +    LoongArchCPUTopo topo;
>>   
>>       if (ms->possible_cpus) {
>>           assert(ms->possible_cpus->len == max_cpus);
>> @@ -1397,17 +1444,17 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>                                     sizeof(CPUArchId) * max_cpus);
>>       ms->possible_cpus->len = max_cpus;
>>       for (n = 0; n < ms->possible_cpus->len; n++) {
>> +        virt_get_topo_from_index(ms, &topo, n);
>> +        arch_id = virt_get_arch_id_from_topo(ms, &topo);
>> +        ms->possible_cpus->cpus[n].vcpus_count = 1;
>>           ms->possible_cpus->cpus[n].type = ms->cpu_type;
>> -        ms->possible_cpus->cpus[n].arch_id = n;
>> -
>> +        ms->possible_cpus->cpus[n].arch_id = arch_id;
>>           ms->possible_cpus->cpus[n].props.has_socket_id = true;
>> -        ms->possible_cpus->cpus[n].props.socket_id  =
>> -                                   n / (ms->smp.cores * ms->smp.threads);
>> +        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
>>           ms->possible_cpus->cpus[n].props.has_core_id = true;
>> -        ms->possible_cpus->cpus[n].props.core_id =
>> -                                   n / ms->smp.threads % ms->smp.cores;
>> +        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
>>           ms->possible_cpus->cpus[n].props.has_thread_id = true;
>> -        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
>> +        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
>>       }
>>       return ms->possible_cpus;
>>   }
>> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
>> index 57cc4f314b..a99e22094e 100644
>> --- a/target/loongarch/cpu.c
>> +++ b/target/loongarch/cpu.c
>> @@ -16,6 +16,7 @@
>>   #include "kvm/kvm_loongarch.h"
>>   #include "exec/exec-all.h"
>>   #include "cpu.h"
>> +#include "hw/qdev-properties.h"
>>   #include "internals.h"
>>   #include "fpu/softfloat-helpers.h"
>>   #include "cpu-csr.h"
>> @@ -725,6 +726,7 @@ static void loongarch_cpu_init(Object *obj)
>>       timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
>>                     &loongarch_constant_timer_cb, cpu);
>>   #endif
>> +    cpu->phy_id = UNSET_PHY_ID;
>>   #endif
>>   }
>>   
>> @@ -823,6 +825,14 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
>>   }
>>   #endif
>>   
>> +static Property loongarch_cpu_properties[] = {
>> +    DEFINE_PROP_INT32("socket-id", LoongArchCPU, socket_id, 0),
>> +    DEFINE_PROP_INT32("core-id", LoongArchCPU, core_id, 0),
>> +    DEFINE_PROP_INT32("thread-id", LoongArchCPU, thread_id, 0),
>> +    DEFINE_PROP_INT32("node-id", LoongArchCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
>> +    DEFINE_PROP_END_OF_LIST()
>> +};
>> +
>>   static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>   {
>>       LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
>> @@ -830,6 +840,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>       DeviceClass *dc = DEVICE_CLASS(c);
>>       ResettableClass *rc = RESETTABLE_CLASS(c);
>>   
>> +    device_class_set_props(dc, loongarch_cpu_properties);
>>       device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
>>                                       &lacc->parent_realize);
>>       resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
>> @@ -854,6 +865,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>   #ifdef CONFIG_TCG
>>       cc->tcg_ops = &loongarch_tcg_ops;
>>   #endif
>> +    dc->user_creatable = true;
>>   }
>>   
>>   static const gchar *loongarch32_gdb_arch_name(CPUState *cs)
>> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
>> index 86c86c6c95..7472df0521 100644
>> --- a/target/loongarch/cpu.h
>> +++ b/target/loongarch/cpu.h
>> @@ -19,6 +19,12 @@
>>   #include "cpu-csr.h"
>>   #include "cpu-qom.h"
>>   
>> +/*
>> + * CPU can't have 0xFFFFFFFF physical ID, use that value to distinguish
>> + * that physical ID hasn't been set yet
> 
> pointer to CPU spec/doc here would be nice to have
> 
Will add comments about CPU manual, the physical ID is 9-bit width at 
most now.

Regards
Bibo Mao
>> + */
>> +#define UNSET_PHY_ID 0xFFFFFFFF
>> +
>>   #define IOCSRF_TEMP             0
>>   #define IOCSRF_NODECNT          1
>>   #define IOCSRF_MSI              2
>> @@ -390,6 +396,12 @@ typedef struct CPUArchState {
>>   #endif
>>   } CPULoongArchState;
>>   
>> +typedef struct LoongArchCPUTopo {
>> +    int32_t socket_id;  /* socket-id of this VCPU */
>> +    int32_t core_id;    /* core-id of this VCPU */
>> +    int32_t thread_id;  /* thread-id of this VCPU */
>> +} LoongArchCPUTopo;
>> +
>>   /**
>>    * LoongArchCPU:
>>    * @env: #CPULoongArchState
>> @@ -404,6 +416,10 @@ struct ArchCPU {
>>       uint32_t  phy_id;
>>       OnOffAuto lbt;
>>       OnOffAuto pmu;
>> +    int32_t socket_id;  /* socket-id of this VCPU */
>> +    int32_t core_id;    /* core-id of this VCPU */
>> +    int32_t thread_id;  /* thread-id of this VCPU */
>> +    int32_t node_id;    /* NUMA node of this VCPU */
>>   
>>       /* 'compatible' string for this CPU for Linux device trees */
>>       const char *dtb_compatible;

Re: [PATCH v4 1/6] hw/loongarch/virt: Add CPU topology support

[Igor Mammedov]

On Tue, 19 Nov 2024 16:01:37 +0800
bibo mao <maobibo@loongson.cn> wrote:

> Hi Ignor,
> 
> On 2024/11/19 上午12:10, Igor Mammedov wrote:
> > On Tue, 12 Nov 2024 10:17:33 +0800
> > Bibo Mao <maobibo@loongson.cn> wrote:
> >   
> >> Add topological relationships for Loongarch VCPU and initialize
> >> topology member variables. Also physical cpu id calculation
> >> method comes from its topo information.
> >>
> >> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
> >> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >> ---
> >>   docs/system/loongarch/virt.rst | 31 +++++++++++++++
> >>   hw/loongarch/virt.c            | 73 ++++++++++++++++++++++++++++------
> >>   target/loongarch/cpu.c         | 12 ++++++
> >>   target/loongarch/cpu.h         | 16 ++++++++
> >>   4 files changed, 119 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/docs/system/loongarch/virt.rst b/docs/system/loongarch/virt.rst
> >> index 172fba079e..8daf60785f 100644
> >> --- a/docs/system/loongarch/virt.rst
> >> +++ b/docs/system/loongarch/virt.rst
> >> @@ -28,6 +28,37 @@ The ``qemu-system-loongarch64`` provides emulation for virt
> >>   machine. You can specify the machine type ``virt`` and
> >>   cpu type ``la464``.
> >>   
> >> +CPU Topology
> >> +------------
> >> +
> >> +The ``LA464`` type CPUs have the concept of Socket Core and Thread.
> >> +
> >> +For example:
> >> +
> >> +``-smp 1,maxcpus=M,sockets=S,cores=C,threads=T``
> >> +
> >> +The above parameters indicate that the machine has a maximum of ``M`` vCPUs and
> >> +``S`` sockets, each socket has ``C`` cores, each core has ``T`` threads,
> >> +and each thread corresponds to a vCPU.
> >> +
> >> +Then ``M`` ``S`` ``C`` ``T`` has the following relationship:
> >> +
> >> +``M = S * C * T``
> >> +
> >> +In the CPU topology relationship, When we know the ``socket_id`` ``core_id``
> >> +and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
> >> +
> >> +``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``  
> > 
> > Is there a spec or some other reference where all of this is described?
> > (or is that a made up just for QEMU?)  
> With hardware manual about cpuid register, it only says that it is 9-bit 
Is manual accessible to public/published somewhere?
What I'm basically asking is to add comments to registers involved
that point to specification that defines them in format
  (Spec name, revision, chapter [,reg name])
so whoever reads that code could go and compare it with specification

> width now, however there is no detailed introduction about 
> socket_id/core_id/thread_id about this register. So it can be treated as 
> a made up for QEMU.
I'd rather not make up thing unless there is no other way around.
arch_id doesn't have to be derived from topo parameters, and can be
separate from them (it's an ID by which a cpu can be addressed in hw)

How topology is encoded on real hw?

> > 
> >   
> >> +
> >> +Similarly, when we know the ``arch_id`` of the CPU,
> >> +we can also get its ``socket_id`` ``core_id`` and ``thread_id``:
> >> +
> >> +``socket_id = arch_id / (C * T)``
> >> +
> >> +``core_id = (arch_id / T) % C``
> >> +
> >> +``thread_id = arch_id % T``
> >> +
> >>   Boot options
> >>   ------------
> >>   
> >> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> >> index 9a635d1d3d..1ed5130edf 100644
> >> --- a/hw/loongarch/virt.c
> >> +++ b/hw/loongarch/virt.c
> >> @@ -1143,9 +1143,9 @@ static void virt_init(MachineState *machine)
> >>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
> >>       int i;
> >>       hwaddr base, size, ram_size = machine->ram_size;
> >> -    const CPUArchIdList *possible_cpus;
> >>       MachineClass *mc = MACHINE_GET_CLASS(machine);
> >>       CPUState *cpu;
> >> +    Object *cpuobj;
> >>   
> >>       if (!cpu_model) {
> >>           cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
> >> @@ -1163,13 +1163,30 @@ static void virt_init(MachineState *machine)
> >>       memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
> >>   
> >>       /* Init CPUs */
> >> -    possible_cpus = mc->possible_cpu_arch_ids(machine);  
> > I'd keep this, and use below, it makes line shorter  
> Sure, will modify it in next version.
> 
> > 
> >   
> >> -    for (i = 0; i < possible_cpus->len; i++) {
> >> -        cpu = cpu_create(machine->cpu_type);
> >> +    mc->possible_cpu_arch_ids(machine);
> >> +    for (i = 0; i < machine->smp.cpus; i++) {
> >> +        cpuobj = object_new(machine->cpu_type);
> >> +        if (cpuobj == NULL) {
> >> +            error_report("Fail to create object with type %s ",
> >> +                         machine->cpu_type);
> >> +            exit(EXIT_FAILURE);
> >> +        }
> >> +
> >> +        cpu = CPU(cpuobj);  
> >   
> >>           cpu->cpu_index = i;  
> > this probably should be in _pre_plug handler,
> > also see
> > (a15d2728a9aa pc: Init CPUState->cpu_index with index in possible_cpus[])
> > for why x86 does it.
> >   
> Will modify it in next version.
> 
> >>           machine->possible_cpus->cpus[i].cpu = cpu;
> >> -        lacpu = LOONGARCH_CPU(cpu);
> >> +        lacpu = LOONGARCH_CPU(cpuobj);  
> >   
> >>           lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;  
> > Given above is derived from topo data set below, I'd move above above
> > to pre_plug time, and calculate/set it there based on topo data.
> > There is no point in setting both at the same place.
> >   
> Will do.
> >> +        object_property_set_int(cpuobj, "socket-id",
> >> +                                machine->possible_cpus->cpus[i].props.socket_id,
> >> +                                NULL);
> >> +        object_property_set_int(cpuobj, "core-id",
> >> +                                machine->possible_cpus->cpus[i].props.core_id,
> >> +                                NULL);
> >> +        object_property_set_int(cpuobj, "thread-id",
> >> +                                machine->possible_cpus->cpus[i].props.thread_id,
> >> +                                NULL);
> >> +        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
> >>       }
> >>       fdt_add_cpu_nodes(lvms);
> >>       fdt_add_memory_nodes(machine);
> >> @@ -1286,6 +1303,35 @@ static void virt_initfn(Object *obj)
> >>       virt_flash_create(lvms);
> >>   }
> >>   
> >> +static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo *topo)
> >> +{
> >> +    int arch_id, sock_vcpu_num, core_vcpu_num;
> >> +
> >> +    /*
> >> +     * calculate total logical cpus across socket/core/thread.
> >> +     * For more information on how to calculate the arch_id,
> >> +     * you can refer to the CPU Topology chapter of the
> >> +     * docs/system/loongarch/virt.rst document.
> >> +     */
> >> +    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
> >> +    core_vcpu_num = topo->core_id * ms->smp.threads;
> >> +
> >> +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
> >> +    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;
> >> +
> >> +    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
> >> +
> >> +    return arch_id;
> >> +}
> >> +
> >> +static void virt_get_topo_from_index(MachineState *ms,
> >> +                                     LoongArchCPUTopo *topo, int index)
> >> +{
> >> +    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
> >> +    topo->core_id = index / ms->smp.threads % ms->smp.cores;
> >> +    topo->thread_id = index % ms->smp.threads;
> >> +}
> >> +
> >>   static bool memhp_type_supported(DeviceState *dev)
> >>   {
> >>       /* we only support pc dimm now */
> >> @@ -1385,8 +1431,9 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
> >>   
> >>   static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >>   {
> >> -    int n;
> >> +    int n, arch_id;
> >>       unsigned int max_cpus = ms->smp.max_cpus;
> >> +    LoongArchCPUTopo topo;
> >>   
> >>       if (ms->possible_cpus) {
> >>           assert(ms->possible_cpus->len == max_cpus);
> >> @@ -1397,17 +1444,17 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >>                                     sizeof(CPUArchId) * max_cpus);
> >>       ms->possible_cpus->len = max_cpus;
> >>       for (n = 0; n < ms->possible_cpus->len; n++) {
> >> +        virt_get_topo_from_index(ms, &topo, n);
> >> +        arch_id = virt_get_arch_id_from_topo(ms, &topo);
> >> +        ms->possible_cpus->cpus[n].vcpus_count = 1;
> >>           ms->possible_cpus->cpus[n].type = ms->cpu_type;
> >> -        ms->possible_cpus->cpus[n].arch_id = n;
> >> -
> >> +        ms->possible_cpus->cpus[n].arch_id = arch_id;
> >>           ms->possible_cpus->cpus[n].props.has_socket_id = true;
> >> -        ms->possible_cpus->cpus[n].props.socket_id  =
> >> -                                   n / (ms->smp.cores * ms->smp.threads);
> >> +        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
> >>           ms->possible_cpus->cpus[n].props.has_core_id = true;
> >> -        ms->possible_cpus->cpus[n].props.core_id =
> >> -                                   n / ms->smp.threads % ms->smp.cores;
> >> +        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
> >>           ms->possible_cpus->cpus[n].props.has_thread_id = true;
> >> -        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
> >> +        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
> >>       }
> >>       return ms->possible_cpus;
> >>   }
> >> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> >> index 57cc4f314b..a99e22094e 100644
> >> --- a/target/loongarch/cpu.c
> >> +++ b/target/loongarch/cpu.c
> >> @@ -16,6 +16,7 @@
> >>   #include "kvm/kvm_loongarch.h"
> >>   #include "exec/exec-all.h"
> >>   #include "cpu.h"
> >> +#include "hw/qdev-properties.h"
> >>   #include "internals.h"
> >>   #include "fpu/softfloat-helpers.h"
> >>   #include "cpu-csr.h"
> >> @@ -725,6 +726,7 @@ static void loongarch_cpu_init(Object *obj)
> >>       timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
> >>                     &loongarch_constant_timer_cb, cpu);
> >>   #endif
> >> +    cpu->phy_id = UNSET_PHY_ID;
> >>   #endif
> >>   }
> >>   
> >> @@ -823,6 +825,14 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
> >>   }
> >>   #endif
> >>   
> >> +static Property loongarch_cpu_properties[] = {
> >> +    DEFINE_PROP_INT32("socket-id", LoongArchCPU, socket_id, 0),
> >> +    DEFINE_PROP_INT32("core-id", LoongArchCPU, core_id, 0),
> >> +    DEFINE_PROP_INT32("thread-id", LoongArchCPU, thread_id, 0),
> >> +    DEFINE_PROP_INT32("node-id", LoongArchCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
> >> +    DEFINE_PROP_END_OF_LIST()
> >> +};
> >> +
> >>   static void loongarch_cpu_class_init(ObjectClass *c, void *data)
> >>   {
> >>       LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
> >> @@ -830,6 +840,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
> >>       DeviceClass *dc = DEVICE_CLASS(c);
> >>       ResettableClass *rc = RESETTABLE_CLASS(c);
> >>   
> >> +    device_class_set_props(dc, loongarch_cpu_properties);
> >>       device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
> >>                                       &lacc->parent_realize);
> >>       resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
> >> @@ -854,6 +865,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
> >>   #ifdef CONFIG_TCG
> >>       cc->tcg_ops = &loongarch_tcg_ops;
> >>   #endif
> >> +    dc->user_creatable = true;
> >>   }
> >>   
> >>   static const gchar *loongarch32_gdb_arch_name(CPUState *cs)
> >> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
> >> index 86c86c6c95..7472df0521 100644
> >> --- a/target/loongarch/cpu.h
> >> +++ b/target/loongarch/cpu.h
> >> @@ -19,6 +19,12 @@
> >>   #include "cpu-csr.h"
> >>   #include "cpu-qom.h"
> >>   
> >> +/*
> >> + * CPU can't have 0xFFFFFFFF physical ID, use that value to distinguish
> >> + * that physical ID hasn't been set yet  
> > 
> > pointer to CPU spec/doc here would be nice to have
> >   
> Will add comments about CPU manual, the physical ID is 9-bit width at 
> most now.
> 
> Regards
> Bibo Mao
> >> + */
> >> +#define UNSET_PHY_ID 0xFFFFFFFF
> >> +
> >>   #define IOCSRF_TEMP             0
> >>   #define IOCSRF_NODECNT          1
> >>   #define IOCSRF_MSI              2
> >> @@ -390,6 +396,12 @@ typedef struct CPUArchState {
> >>   #endif
> >>   } CPULoongArchState;
> >>   
> >> +typedef struct LoongArchCPUTopo {
> >> +    int32_t socket_id;  /* socket-id of this VCPU */
> >> +    int32_t core_id;    /* core-id of this VCPU */
> >> +    int32_t thread_id;  /* thread-id of this VCPU */
> >> +} LoongArchCPUTopo;
> >> +
> >>   /**
> >>    * LoongArchCPU:
> >>    * @env: #CPULoongArchState
> >> @@ -404,6 +416,10 @@ struct ArchCPU {
> >>       uint32_t  phy_id;
> >>       OnOffAuto lbt;
> >>       OnOffAuto pmu;
> >> +    int32_t socket_id;  /* socket-id of this VCPU */
> >> +    int32_t core_id;    /* core-id of this VCPU */
> >> +    int32_t thread_id;  /* thread-id of this VCPU */
> >> +    int32_t node_id;    /* NUMA node of this VCPU */
> >>   
> >>       /* 'compatible' string for this CPU for Linux device trees */
> >>       const char *dtb_compatible;  
> 

Re: [PATCH v4 1/6] hw/loongarch/virt: Add CPU topology support

[bibo mao]

On 2024/11/22 下午9:31, Igor Mammedov wrote:
> On Tue, 19 Nov 2024 16:01:37 +0800
> bibo mao <maobibo@loongson.cn> wrote:
> 
>> Hi Ignor,
>>
>> On 2024/11/19 上午12:10, Igor Mammedov wrote:
>>> On Tue, 12 Nov 2024 10:17:33 +0800
>>> Bibo Mao <maobibo@loongson.cn> wrote:
>>>    
>>>> Add topological relationships for Loongarch VCPU and initialize
>>>> topology member variables. Also physical cpu id calculation
>>>> method comes from its topo information.
>>>>
>>>> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>>> ---
>>>>    docs/system/loongarch/virt.rst | 31 +++++++++++++++
>>>>    hw/loongarch/virt.c            | 73 ++++++++++++++++++++++++++++------
>>>>    target/loongarch/cpu.c         | 12 ++++++
>>>>    target/loongarch/cpu.h         | 16 ++++++++
>>>>    4 files changed, 119 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/docs/system/loongarch/virt.rst b/docs/system/loongarch/virt.rst
>>>> index 172fba079e..8daf60785f 100644
>>>> --- a/docs/system/loongarch/virt.rst
>>>> +++ b/docs/system/loongarch/virt.rst
>>>> @@ -28,6 +28,37 @@ The ``qemu-system-loongarch64`` provides emulation for virt
>>>>    machine. You can specify the machine type ``virt`` and
>>>>    cpu type ``la464``.
>>>>    
>>>> +CPU Topology
>>>> +------------
>>>> +
>>>> +The ``LA464`` type CPUs have the concept of Socket Core and Thread.
>>>> +
>>>> +For example:
>>>> +
>>>> +``-smp 1,maxcpus=M,sockets=S,cores=C,threads=T``
>>>> +
>>>> +The above parameters indicate that the machine has a maximum of ``M`` vCPUs and
>>>> +``S`` sockets, each socket has ``C`` cores, each core has ``T`` threads,
>>>> +and each thread corresponds to a vCPU.
>>>> +
>>>> +Then ``M`` ``S`` ``C`` ``T`` has the following relationship:
>>>> +
>>>> +``M = S * C * T``
>>>> +
>>>> +In the CPU topology relationship, When we know the ``socket_id`` ``core_id``
>>>> +and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
>>>> +
>>>> +``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``
>>>
>>> Is there a spec or some other reference where all of this is described?
>>> (or is that a made up just for QEMU?)
>> With hardware manual about cpuid register, it only says that it is 9-bit
> Is manual accessible to public/published somewhere?
> What I'm basically asking is to add comments to registers involved
> that point to specification that defines them in format
>    (Spec name, revision, chapter [,reg name])
> so whoever reads that code could go and compare it with specification
> 
>> width now, however there is no detailed introduction about
>> socket_id/core_id/thread_id about this register. So it can be treated as
>> a made up for QEMU.
> I'd rather not make up thing unless there is no other way around.
> arch_id doesn't have to be derived from topo parameters, and can be
> separate from them (it's an ID by which a cpu can be addressed in hw)
> 
> How topology is encoded on real hw?
With real hw, the encoding order is socket, and then core, the last is 
thread id. On one socket there are 12 cores or 16 cores,  it uses 4 bit 
for core id representation; 2 bit us used 4 cores or 3 core on one socket.

Different HW platforms has different cores, so the bit width for core 
number is not fixed, the total width is 8-bit because of limitation of 
MSI/EXIIOT interrupt chip. Overall arch_id is a little different with 
cpu_index,

arch_id = socket_id * aligned_up(S, powerof(2)) + core_id * 
aligned_up(C, powerof(2))) + thread_id * aligned_up(T, powerof(2))

Regards
Bibo Mao
> 
>>>
>>>    
>>>> +
>>>> +Similarly, when we know the ``arch_id`` of the CPU,
>>>> +we can also get its ``socket_id`` ``core_id`` and ``thread_id``:
>>>> +
>>>> +``socket_id = arch_id / (C * T)``
>>>> +
>>>> +``core_id = (arch_id / T) % C``
>>>> +
>>>> +``thread_id = arch_id % T``
>>>> +
>>>>    Boot options
>>>>    ------------
>>>>    
>>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>>>> index 9a635d1d3d..1ed5130edf 100644
>>>> --- a/hw/loongarch/virt.c
>>>> +++ b/hw/loongarch/virt.c
>>>> @@ -1143,9 +1143,9 @@ static void virt_init(MachineState *machine)
>>>>        LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
>>>>        int i;
>>>>        hwaddr base, size, ram_size = machine->ram_size;
>>>> -    const CPUArchIdList *possible_cpus;
>>>>        MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>>        CPUState *cpu;
>>>> +    Object *cpuobj;
>>>>    
>>>>        if (!cpu_model) {
>>>>            cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
>>>> @@ -1163,13 +1163,30 @@ static void virt_init(MachineState *machine)
>>>>        memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
>>>>    
>>>>        /* Init CPUs */
>>>> -    possible_cpus = mc->possible_cpu_arch_ids(machine);
>>> I'd keep this, and use below, it makes line shorter
>> Sure, will modify it in next version.
>>
>>>
>>>    
>>>> -    for (i = 0; i < possible_cpus->len; i++) {
>>>> -        cpu = cpu_create(machine->cpu_type);
>>>> +    mc->possible_cpu_arch_ids(machine);
>>>> +    for (i = 0; i < machine->smp.cpus; i++) {
>>>> +        cpuobj = object_new(machine->cpu_type);
>>>> +        if (cpuobj == NULL) {
>>>> +            error_report("Fail to create object with type %s ",
>>>> +                         machine->cpu_type);
>>>> +            exit(EXIT_FAILURE);
>>>> +        }
>>>> +
>>>> +        cpu = CPU(cpuobj);
>>>    
>>>>            cpu->cpu_index = i;
>>> this probably should be in _pre_plug handler,
>>> also see
>>> (a15d2728a9aa pc: Init CPUState->cpu_index with index in possible_cpus[])
>>> for why x86 does it.
>>>    
>> Will modify it in next version.
>>
>>>>            machine->possible_cpus->cpus[i].cpu = cpu;
>>>> -        lacpu = LOONGARCH_CPU(cpu);
>>>> +        lacpu = LOONGARCH_CPU(cpuobj);
>>>    
>>>>            lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
>>> Given above is derived from topo data set below, I'd move above above
>>> to pre_plug time, and calculate/set it there based on topo data.
>>> There is no point in setting both at the same place.
>>>    
>> Will do.
>>>> +        object_property_set_int(cpuobj, "socket-id",
>>>> +                                machine->possible_cpus->cpus[i].props.socket_id,
>>>> +                                NULL);
>>>> +        object_property_set_int(cpuobj, "core-id",
>>>> +                                machine->possible_cpus->cpus[i].props.core_id,
>>>> +                                NULL);
>>>> +        object_property_set_int(cpuobj, "thread-id",
>>>> +                                machine->possible_cpus->cpus[i].props.thread_id,
>>>> +                                NULL);
>>>> +        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
>>>>        }
>>>>        fdt_add_cpu_nodes(lvms);
>>>>        fdt_add_memory_nodes(machine);
>>>> @@ -1286,6 +1303,35 @@ static void virt_initfn(Object *obj)
>>>>        virt_flash_create(lvms);
>>>>    }
>>>>    
>>>> +static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo *topo)
>>>> +{
>>>> +    int arch_id, sock_vcpu_num, core_vcpu_num;
>>>> +
>>>> +    /*
>>>> +     * calculate total logical cpus across socket/core/thread.
>>>> +     * For more information on how to calculate the arch_id,
>>>> +     * you can refer to the CPU Topology chapter of the
>>>> +     * docs/system/loongarch/virt.rst document.
>>>> +     */
>>>> +    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
>>>> +    core_vcpu_num = topo->core_id * ms->smp.threads;
>>>> +
>>>> +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
>>>> +    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;
>>>> +
>>>> +    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
>>>> +
>>>> +    return arch_id;
>>>> +}
>>>> +
>>>> +static void virt_get_topo_from_index(MachineState *ms,
>>>> +                                     LoongArchCPUTopo *topo, int index)
>>>> +{
>>>> +    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
>>>> +    topo->core_id = index / ms->smp.threads % ms->smp.cores;
>>>> +    topo->thread_id = index % ms->smp.threads;
>>>> +}
>>>> +
>>>>    static bool memhp_type_supported(DeviceState *dev)
>>>>    {
>>>>        /* we only support pc dimm now */
>>>> @@ -1385,8 +1431,9 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
>>>>    
>>>>    static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>    {
>>>> -    int n;
>>>> +    int n, arch_id;
>>>>        unsigned int max_cpus = ms->smp.max_cpus;
>>>> +    LoongArchCPUTopo topo;
>>>>    
>>>>        if (ms->possible_cpus) {
>>>>            assert(ms->possible_cpus->len == max_cpus);
>>>> @@ -1397,17 +1444,17 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>                                      sizeof(CPUArchId) * max_cpus);
>>>>        ms->possible_cpus->len = max_cpus;
>>>>        for (n = 0; n < ms->possible_cpus->len; n++) {
>>>> +        virt_get_topo_from_index(ms, &topo, n);
>>>> +        arch_id = virt_get_arch_id_from_topo(ms, &topo);
>>>> +        ms->possible_cpus->cpus[n].vcpus_count = 1;
>>>>            ms->possible_cpus->cpus[n].type = ms->cpu_type;
>>>> -        ms->possible_cpus->cpus[n].arch_id = n;
>>>> -
>>>> +        ms->possible_cpus->cpus[n].arch_id = arch_id;
>>>>            ms->possible_cpus->cpus[n].props.has_socket_id = true;
>>>> -        ms->possible_cpus->cpus[n].props.socket_id  =
>>>> -                                   n / (ms->smp.cores * ms->smp.threads);
>>>> +        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
>>>>            ms->possible_cpus->cpus[n].props.has_core_id = true;
>>>> -        ms->possible_cpus->cpus[n].props.core_id =
>>>> -                                   n / ms->smp.threads % ms->smp.cores;
>>>> +        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
>>>>            ms->possible_cpus->cpus[n].props.has_thread_id = true;
>>>> -        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
>>>> +        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
>>>>        }
>>>>        return ms->possible_cpus;
>>>>    }
>>>> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
>>>> index 57cc4f314b..a99e22094e 100644
>>>> --- a/target/loongarch/cpu.c
>>>> +++ b/target/loongarch/cpu.c
>>>> @@ -16,6 +16,7 @@
>>>>    #include "kvm/kvm_loongarch.h"
>>>>    #include "exec/exec-all.h"
>>>>    #include "cpu.h"
>>>> +#include "hw/qdev-properties.h"
>>>>    #include "internals.h"
>>>>    #include "fpu/softfloat-helpers.h"
>>>>    #include "cpu-csr.h"
>>>> @@ -725,6 +726,7 @@ static void loongarch_cpu_init(Object *obj)
>>>>        timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
>>>>                      &loongarch_constant_timer_cb, cpu);
>>>>    #endif
>>>> +    cpu->phy_id = UNSET_PHY_ID;
>>>>    #endif
>>>>    }
>>>>    
>>>> @@ -823,6 +825,14 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
>>>>    }
>>>>    #endif
>>>>    
>>>> +static Property loongarch_cpu_properties[] = {
>>>> +    DEFINE_PROP_INT32("socket-id", LoongArchCPU, socket_id, 0),
>>>> +    DEFINE_PROP_INT32("core-id", LoongArchCPU, core_id, 0),
>>>> +    DEFINE_PROP_INT32("thread-id", LoongArchCPU, thread_id, 0),
>>>> +    DEFINE_PROP_INT32("node-id", LoongArchCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
>>>> +    DEFINE_PROP_END_OF_LIST()
>>>> +};
>>>> +
>>>>    static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>>    {
>>>>        LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
>>>> @@ -830,6 +840,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>>        DeviceClass *dc = DEVICE_CLASS(c);
>>>>        ResettableClass *rc = RESETTABLE_CLASS(c);
>>>>    
>>>> +    device_class_set_props(dc, loongarch_cpu_properties);
>>>>        device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
>>>>                                        &lacc->parent_realize);
>>>>        resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
>>>> @@ -854,6 +865,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>>    #ifdef CONFIG_TCG
>>>>        cc->tcg_ops = &loongarch_tcg_ops;
>>>>    #endif
>>>> +    dc->user_creatable = true;
>>>>    }
>>>>    
>>>>    static const gchar *loongarch32_gdb_arch_name(CPUState *cs)
>>>> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
>>>> index 86c86c6c95..7472df0521 100644
>>>> --- a/target/loongarch/cpu.h
>>>> +++ b/target/loongarch/cpu.h
>>>> @@ -19,6 +19,12 @@
>>>>    #include "cpu-csr.h"
>>>>    #include "cpu-qom.h"
>>>>    
>>>> +/*
>>>> + * CPU can't have 0xFFFFFFFF physical ID, use that value to distinguish
>>>> + * that physical ID hasn't been set yet
>>>
>>> pointer to CPU spec/doc here would be nice to have
>>>    
>> Will add comments about CPU manual, the physical ID is 9-bit width at
>> most now.
>>
>> Regards
>> Bibo Mao
>>>> + */
>>>> +#define UNSET_PHY_ID 0xFFFFFFFF
>>>> +
>>>>    #define IOCSRF_TEMP             0
>>>>    #define IOCSRF_NODECNT          1
>>>>    #define IOCSRF_MSI              2
>>>> @@ -390,6 +396,12 @@ typedef struct CPUArchState {
>>>>    #endif
>>>>    } CPULoongArchState;
>>>>    
>>>> +typedef struct LoongArchCPUTopo {
>>>> +    int32_t socket_id;  /* socket-id of this VCPU */
>>>> +    int32_t core_id;    /* core-id of this VCPU */
>>>> +    int32_t thread_id;  /* thread-id of this VCPU */
>>>> +} LoongArchCPUTopo;
>>>> +
>>>>    /**
>>>>     * LoongArchCPU:
>>>>     * @env: #CPULoongArchState
>>>> @@ -404,6 +416,10 @@ struct ArchCPU {
>>>>        uint32_t  phy_id;
>>>>        OnOffAuto lbt;
>>>>        OnOffAuto pmu;
>>>> +    int32_t socket_id;  /* socket-id of this VCPU */
>>>> +    int32_t core_id;    /* core-id of this VCPU */
>>>> +    int32_t thread_id;  /* thread-id of this VCPU */
>>>> +    int32_t node_id;    /* NUMA node of this VCPU */
>>>>    
>>>>        /* 'compatible' string for this CPU for Linux device trees */
>>>>        const char *dtb_compatible;
>>

Re: [PATCH v4 1/6] hw/loongarch/virt: Add CPU topology support

[bibo mao]

On 2024/11/22 下午9:31, Igor Mammedov wrote:
> On Tue, 19 Nov 2024 16:01:37 +0800
> bibo mao <maobibo@loongson.cn> wrote:
> 
>> Hi Ignor,
>>
>> On 2024/11/19 上午12:10, Igor Mammedov wrote:
>>> On Tue, 12 Nov 2024 10:17:33 +0800
>>> Bibo Mao <maobibo@loongson.cn> wrote:
>>>    
>>>> Add topological relationships for Loongarch VCPU and initialize
>>>> topology member variables. Also physical cpu id calculation
>>>> method comes from its topo information.
>>>>
>>>> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>>> ---
>>>>    docs/system/loongarch/virt.rst | 31 +++++++++++++++
>>>>    hw/loongarch/virt.c            | 73 ++++++++++++++++++++++++++++------
>>>>    target/loongarch/cpu.c         | 12 ++++++
>>>>    target/loongarch/cpu.h         | 16 ++++++++
>>>>    4 files changed, 119 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/docs/system/loongarch/virt.rst b/docs/system/loongarch/virt.rst
>>>> index 172fba079e..8daf60785f 100644
>>>> --- a/docs/system/loongarch/virt.rst
>>>> +++ b/docs/system/loongarch/virt.rst
>>>> @@ -28,6 +28,37 @@ The ``qemu-system-loongarch64`` provides emulation for virt
>>>>    machine. You can specify the machine type ``virt`` and
>>>>    cpu type ``la464``.
>>>>    
>>>> +CPU Topology
>>>> +------------
>>>> +
>>>> +The ``LA464`` type CPUs have the concept of Socket Core and Thread.
>>>> +
>>>> +For example:
>>>> +
>>>> +``-smp 1,maxcpus=M,sockets=S,cores=C,threads=T``
>>>> +
>>>> +The above parameters indicate that the machine has a maximum of ``M`` vCPUs and
>>>> +``S`` sockets, each socket has ``C`` cores, each core has ``T`` threads,
>>>> +and each thread corresponds to a vCPU.
>>>> +
>>>> +Then ``M`` ``S`` ``C`` ``T`` has the following relationship:
>>>> +
>>>> +``M = S * C * T``
>>>> +
>>>> +In the CPU topology relationship, When we know the ``socket_id`` ``core_id``
>>>> +and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
>>>> +
>>>> +``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``
>>>
>>> Is there a spec or some other reference where all of this is described?
>>> (or is that a made up just for QEMU?)
>> With hardware manual about cpuid register, it only says that it is 9-bit
> Is manual accessible to public/published somewhere?
> What I'm basically asking is to add comments to registers involved
> that point to specification that defines them in format
>    (Spec name, revision, chapter [,reg name])
> so whoever reads that code could go and compare it with specification

Sure will add comments about this.
The spec can be located with chatper 7.4.12. CPU Identity (CPUID) at
   https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html

Regards
Bibo Mao

> 
>> width now, however there is no detailed introduction about
>> socket_id/core_id/thread_id about this register. So it can be treated as
>> a made up for QEMU.
> I'd rather not make up thing unless there is no other way around.
> arch_id doesn't have to be derived from topo parameters, and can be
> separate from them (it's an ID by which a cpu can be addressed in hw)
> 
> How topology is encoded on real hw?
> 
>>>
>>>    
>>>> +
>>>> +Similarly, when we know the ``arch_id`` of the CPU,
>>>> +we can also get its ``socket_id`` ``core_id`` and ``thread_id``:
>>>> +
>>>> +``socket_id = arch_id / (C * T)``
>>>> +
>>>> +``core_id = (arch_id / T) % C``
>>>> +
>>>> +``thread_id = arch_id % T``
>>>> +
>>>>    Boot options
>>>>    ------------
>>>>    
>>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>>>> index 9a635d1d3d..1ed5130edf 100644
>>>> --- a/hw/loongarch/virt.c
>>>> +++ b/hw/loongarch/virt.c
>>>> @@ -1143,9 +1143,9 @@ static void virt_init(MachineState *machine)
>>>>        LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
>>>>        int i;
>>>>        hwaddr base, size, ram_size = machine->ram_size;
>>>> -    const CPUArchIdList *possible_cpus;
>>>>        MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>>        CPUState *cpu;
>>>> +    Object *cpuobj;
>>>>    
>>>>        if (!cpu_model) {
>>>>            cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
>>>> @@ -1163,13 +1163,30 @@ static void virt_init(MachineState *machine)
>>>>        memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
>>>>    
>>>>        /* Init CPUs */
>>>> -    possible_cpus = mc->possible_cpu_arch_ids(machine);
>>> I'd keep this, and use below, it makes line shorter
>> Sure, will modify it in next version.
>>
>>>
>>>    
>>>> -    for (i = 0; i < possible_cpus->len; i++) {
>>>> -        cpu = cpu_create(machine->cpu_type);
>>>> +    mc->possible_cpu_arch_ids(machine);
>>>> +    for (i = 0; i < machine->smp.cpus; i++) {
>>>> +        cpuobj = object_new(machine->cpu_type);
>>>> +        if (cpuobj == NULL) {
>>>> +            error_report("Fail to create object with type %s ",
>>>> +                         machine->cpu_type);
>>>> +            exit(EXIT_FAILURE);
>>>> +        }
>>>> +
>>>> +        cpu = CPU(cpuobj);
>>>    
>>>>            cpu->cpu_index = i;
>>> this probably should be in _pre_plug handler,
>>> also see
>>> (a15d2728a9aa pc: Init CPUState->cpu_index with index in possible_cpus[])
>>> for why x86 does it.
>>>    
>> Will modify it in next version.
>>
>>>>            machine->possible_cpus->cpus[i].cpu = cpu;
>>>> -        lacpu = LOONGARCH_CPU(cpu);
>>>> +        lacpu = LOONGARCH_CPU(cpuobj);
>>>    
>>>>            lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
>>> Given above is derived from topo data set below, I'd move above above
>>> to pre_plug time, and calculate/set it there based on topo data.
>>> There is no point in setting both at the same place.
>>>    
>> Will do.
>>>> +        object_property_set_int(cpuobj, "socket-id",
>>>> +                                machine->possible_cpus->cpus[i].props.socket_id,
>>>> +                                NULL);
>>>> +        object_property_set_int(cpuobj, "core-id",
>>>> +                                machine->possible_cpus->cpus[i].props.core_id,
>>>> +                                NULL);
>>>> +        object_property_set_int(cpuobj, "thread-id",
>>>> +                                machine->possible_cpus->cpus[i].props.thread_id,
>>>> +                                NULL);
>>>> +        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
>>>>        }
>>>>        fdt_add_cpu_nodes(lvms);
>>>>        fdt_add_memory_nodes(machine);
>>>> @@ -1286,6 +1303,35 @@ static void virt_initfn(Object *obj)
>>>>        virt_flash_create(lvms);
>>>>    }
>>>>    
>>>> +static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo *topo)
>>>> +{
>>>> +    int arch_id, sock_vcpu_num, core_vcpu_num;
>>>> +
>>>> +    /*
>>>> +     * calculate total logical cpus across socket/core/thread.
>>>> +     * For more information on how to calculate the arch_id,
>>>> +     * you can refer to the CPU Topology chapter of the
>>>> +     * docs/system/loongarch/virt.rst document.
>>>> +     */
>>>> +    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
>>>> +    core_vcpu_num = topo->core_id * ms->smp.threads;
>>>> +
>>>> +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
>>>> +    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;
>>>> +
>>>> +    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
>>>> +
>>>> +    return arch_id;
>>>> +}
>>>> +
>>>> +static void virt_get_topo_from_index(MachineState *ms,
>>>> +                                     LoongArchCPUTopo *topo, int index)
>>>> +{
>>>> +    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
>>>> +    topo->core_id = index / ms->smp.threads % ms->smp.cores;
>>>> +    topo->thread_id = index % ms->smp.threads;
>>>> +}
>>>> +
>>>>    static bool memhp_type_supported(DeviceState *dev)
>>>>    {
>>>>        /* we only support pc dimm now */
>>>> @@ -1385,8 +1431,9 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
>>>>    
>>>>    static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>    {
>>>> -    int n;
>>>> +    int n, arch_id;
>>>>        unsigned int max_cpus = ms->smp.max_cpus;
>>>> +    LoongArchCPUTopo topo;
>>>>    
>>>>        if (ms->possible_cpus) {
>>>>            assert(ms->possible_cpus->len == max_cpus);
>>>> @@ -1397,17 +1444,17 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>                                      sizeof(CPUArchId) * max_cpus);
>>>>        ms->possible_cpus->len = max_cpus;
>>>>        for (n = 0; n < ms->possible_cpus->len; n++) {
>>>> +        virt_get_topo_from_index(ms, &topo, n);
>>>> +        arch_id = virt_get_arch_id_from_topo(ms, &topo);
>>>> +        ms->possible_cpus->cpus[n].vcpus_count = 1;
>>>>            ms->possible_cpus->cpus[n].type = ms->cpu_type;
>>>> -        ms->possible_cpus->cpus[n].arch_id = n;
>>>> -
>>>> +        ms->possible_cpus->cpus[n].arch_id = arch_id;
>>>>            ms->possible_cpus->cpus[n].props.has_socket_id = true;
>>>> -        ms->possible_cpus->cpus[n].props.socket_id  =
>>>> -                                   n / (ms->smp.cores * ms->smp.threads);
>>>> +        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
>>>>            ms->possible_cpus->cpus[n].props.has_core_id = true;
>>>> -        ms->possible_cpus->cpus[n].props.core_id =
>>>> -                                   n / ms->smp.threads % ms->smp.cores;
>>>> +        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
>>>>            ms->possible_cpus->cpus[n].props.has_thread_id = true;
>>>> -        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
>>>> +        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
>>>>        }
>>>>        return ms->possible_cpus;
>>>>    }
>>>> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
>>>> index 57cc4f314b..a99e22094e 100644
>>>> --- a/target/loongarch/cpu.c
>>>> +++ b/target/loongarch/cpu.c
>>>> @@ -16,6 +16,7 @@
>>>>    #include "kvm/kvm_loongarch.h"
>>>>    #include "exec/exec-all.h"
>>>>    #include "cpu.h"
>>>> +#include "hw/qdev-properties.h"
>>>>    #include "internals.h"
>>>>    #include "fpu/softfloat-helpers.h"
>>>>    #include "cpu-csr.h"
>>>> @@ -725,6 +726,7 @@ static void loongarch_cpu_init(Object *obj)
>>>>        timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
>>>>                      &loongarch_constant_timer_cb, cpu);
>>>>    #endif
>>>> +    cpu->phy_id = UNSET_PHY_ID;
>>>>    #endif
>>>>    }
>>>>    
>>>> @@ -823,6 +825,14 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
>>>>    }
>>>>    #endif
>>>>    
>>>> +static Property loongarch_cpu_properties[] = {
>>>> +    DEFINE_PROP_INT32("socket-id", LoongArchCPU, socket_id, 0),
>>>> +    DEFINE_PROP_INT32("core-id", LoongArchCPU, core_id, 0),
>>>> +    DEFINE_PROP_INT32("thread-id", LoongArchCPU, thread_id, 0),
>>>> +    DEFINE_PROP_INT32("node-id", LoongArchCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
>>>> +    DEFINE_PROP_END_OF_LIST()
>>>> +};
>>>> +
>>>>    static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>>    {
>>>>        LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
>>>> @@ -830,6 +840,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>>        DeviceClass *dc = DEVICE_CLASS(c);
>>>>        ResettableClass *rc = RESETTABLE_CLASS(c);
>>>>    
>>>> +    device_class_set_props(dc, loongarch_cpu_properties);
>>>>        device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
>>>>                                        &lacc->parent_realize);
>>>>        resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
>>>> @@ -854,6 +865,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>>>>    #ifdef CONFIG_TCG
>>>>        cc->tcg_ops = &loongarch_tcg_ops;
>>>>    #endif
>>>> +    dc->user_creatable = true;
>>>>    }
>>>>    
>>>>    static const gchar *loongarch32_gdb_arch_name(CPUState *cs)
>>>> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
>>>> index 86c86c6c95..7472df0521 100644
>>>> --- a/target/loongarch/cpu.h
>>>> +++ b/target/loongarch/cpu.h
>>>> @@ -19,6 +19,12 @@
>>>>    #include "cpu-csr.h"
>>>>    #include "cpu-qom.h"
>>>>    
>>>> +/*
>>>> + * CPU can't have 0xFFFFFFFF physical ID, use that value to distinguish
>>>> + * that physical ID hasn't been set yet
>>>
>>> pointer to CPU spec/doc here would be nice to have
>>>    
>> Will add comments about CPU manual, the physical ID is 9-bit width at
>> most now.
>>
>> Regards
>> Bibo Mao
>>>> + */
>>>> +#define UNSET_PHY_ID 0xFFFFFFFF
>>>> +
>>>>    #define IOCSRF_TEMP             0
>>>>    #define IOCSRF_NODECNT          1
>>>>    #define IOCSRF_MSI              2
>>>> @@ -390,6 +396,12 @@ typedef struct CPUArchState {
>>>>    #endif
>>>>    } CPULoongArchState;
>>>>    
>>>> +typedef struct LoongArchCPUTopo {
>>>> +    int32_t socket_id;  /* socket-id of this VCPU */
>>>> +    int32_t core_id;    /* core-id of this VCPU */
>>>> +    int32_t thread_id;  /* thread-id of this VCPU */
>>>> +} LoongArchCPUTopo;
>>>> +
>>>>    /**
>>>>     * LoongArchCPU:
>>>>     * @env: #CPULoongArchState
>>>> @@ -404,6 +416,10 @@ struct ArchCPU {
>>>>        uint32_t  phy_id;
>>>>        OnOffAuto lbt;
>>>>        OnOffAuto pmu;
>>>> +    int32_t socket_id;  /* socket-id of this VCPU */
>>>> +    int32_t core_id;    /* core-id of this VCPU */
>>>> +    int32_t thread_id;  /* thread-id of this VCPU */
>>>> +    int32_t node_id;    /* NUMA node of this VCPU */
>>>>    
>>>>        /* 'compatible' string for this CPU for Linux device trees */
>>>>        const char *dtb_compatible;
>>
> 

[PATCH v4 2/6] hw/loongarch/virt: Implement cpu plug interface

[Bibo Mao]

Add cpu hotplug interface, however cpu hotplug feature is still
disabled for the machine. When machine is on, all created vCPUs
go through hotplug interface, and there is no remaining vCPU which
can be hot-added after power on.

Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/virt.c    | 127 ++++++++++++++++++++++++++++++++++++++++-
 target/loongarch/cpu.c |  13 +++++
 target/loongarch/cpu.h |   1 +
 3 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 1ed5130edf..b6b616d278 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1174,7 +1174,6 @@ static void virt_init(MachineState *machine)
 
         cpu = CPU(cpuobj);
         cpu->cpu_index = i;
-        machine->possible_cpus->cpus[i].cpu = cpu;
         lacpu = LOONGARCH_CPU(cpuobj);
         lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
         object_property_set_int(cpuobj, "socket-id",
@@ -1332,6 +1331,123 @@ static void virt_get_topo_from_index(MachineState *ms,
     topo->thread_id = index % ms->smp.threads;
 }
 
+/* Find cpu slot in machine->possible_cpus by arch_id */
+static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id)
+{
+    int n;
+    for (n = 0; n < ms->possible_cpus->len; n++) {
+        if (ms->possible_cpus->cpus[n].arch_id == arch_id) {
+            return &ms->possible_cpus->cpus[n];
+        }
+    }
+
+    return NULL;
+}
+
+static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
+                              DeviceState *dev, Error **errp)
+{
+    LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
+    MachineState *ms = MACHINE(OBJECT(hotplug_dev));
+    LoongArchCPU *cpu = LOONGARCH_CPU(dev);
+    CPUArchId *cpu_slot;
+    Error *local_err = NULL;
+    int arch_id;
+
+    /* sanity check the cpu */
+    if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
+        error_setg(&local_err, "Invalid CPU type, expected cpu type: '%s'",
+                   ms->cpu_type);
+        goto out;
+    }
+
+    if (lvms->acpi_ged) {
+        hotplug_handler_pre_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev,
+                                 &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+           return;
+        }
+     }
+
+    if (cpu->phy_id == UNSET_PHY_ID) {
+        error_setg(&local_err, "CPU hotplug not supported");
+        goto out;
+    } else {
+        /* For cold-add cpu, find cpu slot from arch_id */
+        arch_id = cpu->phy_id;
+        cpu_slot = virt_find_cpu_slot(ms, arch_id);
+    }
+
+    numa_cpu_pre_plug(cpu_slot, dev, &local_err);
+    return;
+
+out:
+    error_propagate(errp, local_err);
+}
+
+static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
+{
+    LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
+    Error *local_err = NULL;
+    LoongArchCPU *cpu = LOONGARCH_CPU(dev);
+    CPUState *cs = CPU(dev);
+
+    if (!lvms->acpi_ged) {
+        error_setg(&local_err, "CPU hot unplug not supported without ACPI");
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (cs->cpu_index == 0) {
+        error_setg(&local_err,
+                   "hot-unplug of boot cpu(id%d=%d:%d:%d) not supported",
+                   cs->cpu_index, cpu->socket_id,
+                   cpu->core_id, cpu->thread_id);
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev,
+                                   &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
+static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
+                                DeviceState *dev, Error **errp)
+{
+    CPUArchId *cpu_slot;
+    Error *local_err = NULL;
+    LoongArchCPU *cpu = LOONGARCH_CPU(dev);
+    LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
+
+    hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
+    cpu_slot->cpu = NULL;
+    return;
+}
+
+static void virt_cpu_plug(HotplugHandler *hotplug_dev,
+                                DeviceState *dev, Error **errp)
+{
+    CPUArchId *cpu_slot;
+    LoongArchCPU *cpu = LOONGARCH_CPU(dev);
+    LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
+
+    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
+    cpu_slot->cpu = CPU(dev);
+    return;
+}
+
 static bool memhp_type_supported(DeviceState *dev)
 {
     /* we only support pc dimm now */
@@ -1350,6 +1466,8 @@ static void virt_device_pre_plug(HotplugHandler *hotplug_dev,
 {
     if (memhp_type_supported(dev)) {
         virt_mem_pre_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_LOONGARCH_CPU)) {
+        virt_cpu_pre_plug(hotplug_dev, dev, errp);
     }
 }
 
@@ -1368,6 +1486,8 @@ static void virt_device_unplug_request(HotplugHandler *hotplug_dev,
 {
     if (memhp_type_supported(dev)) {
         virt_mem_unplug_request(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_LOONGARCH_CPU)) {
+        virt_cpu_unplug_request(hotplug_dev, dev, errp);
     }
 }
 
@@ -1386,6 +1506,8 @@ static void virt_device_unplug(HotplugHandler *hotplug_dev,
 {
     if (memhp_type_supported(dev)) {
         virt_mem_unplug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_LOONGARCH_CPU)) {
+        virt_cpu_unplug(hotplug_dev, dev, errp);
     }
 }
 
@@ -1413,6 +1535,8 @@ static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
         }
     } else if (memhp_type_supported(dev)) {
         virt_mem_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_LOONGARCH_CPU)) {
+        virt_cpu_plug(hotplug_dev, dev, errp);
     }
 }
 
@@ -1422,6 +1546,7 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
     MachineClass *mc = MACHINE_GET_CLASS(machine);
 
     if (device_is_dynamic_sysbus(mc, dev) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_LOONGARCH_CPU) ||
         object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI) ||
         memhp_type_supported(dev)) {
         return HOTPLUG_HANDLER(machine);
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index a99e22094e..a5467811ab 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -613,6 +613,17 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
     lacc->parent_realize(dev, errp);
 }
 
+static void loongarch_cpu_unrealizefn(DeviceState *dev)
+{
+    LoongArchCPUClass *mcc = LOONGARCH_CPU_GET_CLASS(dev);
+
+#ifndef CONFIG_USER_ONLY
+    cpu_remove_sync(CPU(dev));
+#endif
+
+    mcc->parent_unrealize(dev);
+}
+
 static bool loongarch_get_lsx(Object *obj, Error **errp)
 {
     LoongArchCPU *cpu = LOONGARCH_CPU(obj);
@@ -843,6 +854,8 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
     device_class_set_props(dc, loongarch_cpu_properties);
     device_class_set_parent_realize(dc, loongarch_cpu_realizefn,
                                     &lacc->parent_realize);
+    device_class_set_parent_unrealize(dc, loongarch_cpu_unrealizefn,
+                                      &lacc->parent_unrealize);
     resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
                                        &lacc->parent_phases);
 
diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index 7472df0521..22e6d9baf5 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -438,6 +438,7 @@ struct LoongArchCPUClass {
     CPUClass parent_class;
 
     DeviceRealize parent_realize;
+    DeviceUnrealize parent_unrealize;
     ResettablePhases parent_phases;
 };
 
-- 
2.39.3

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

[Bibo Mao]

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

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

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

[Igor Mammedov]

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

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

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

[Igor Mammedov]

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

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

[bibo mao]

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

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

[PATCH v4 4/6] hw/loongarch/virt: Init interrupt pin of CPU during plug interface

[Bibo Mao]

Move CPU object creation after interrupt controller ipi and extioi,
and init interrupt pin of CPU in plug interface virt_cpu_plug().

So interrupt pin initialization of the cold-plug CPUs is the same
with that of hot-plug CPUs.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/virt.c | 84 ++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 621380e2b3..0e0c6c202b 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -831,8 +831,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
     DeviceState *pch_pic, *pch_msi;
     DeviceState *ipi, *extioi;
     SysBusDevice *d;
-    CPUState *cpu_state;
-    int cpu, i, start, num;
+    int i, start, num;
     uint32_t cpuintc_phandle, eiointc_phandle, pch_pic_phandle, pch_msi_phandle;
 
     /*
@@ -909,12 +908,6 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
                     sysbus_mmio_get_region(SYS_BUS_DEVICE(extioi), 1));
     }
 
-    /* Connect irq to cpu, including ipi and extioi irqchip */
-    for (cpu = 0; cpu < ms->smp.cpus; cpu++) {
-        cpu_state = virt_get_cpu(ms, cpu);
-        virt_init_cpu_irq(ms, cpu_state);
-    }
-
     /* Add Extend I/O Interrupt Controller node */
     fdt_add_eiointc_node(lvms, &cpuintc_phandle, &eiointc_phandle);
 
@@ -960,6 +953,44 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
     virt_devices_init(pch_pic, lvms, &pch_pic_phandle, &pch_msi_phandle);
 }
 
+static void virt_init_cpus(MachineState *machine)
+{
+    int i;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    Object *cpuobj;
+    CPUState *cpu;
+    LoongArchCPU *lacpu;
+    LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
+
+    /* Init CPUs */
+    mc->possible_cpu_arch_ids(machine);
+    for (i = 0; i < machine->smp.cpus; i++) {
+        cpuobj = object_new(machine->cpu_type);
+        if (cpuobj == NULL) {
+            error_report("Fail to create object with type %s ",
+                         machine->cpu_type);
+            exit(EXIT_FAILURE);
+        }
+
+        cpu = CPU(cpuobj);
+        cpu->cpu_index = i;
+        lacpu = LOONGARCH_CPU(cpuobj);
+        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
+        object_property_set_int(cpuobj, "socket-id",
+                                machine->possible_cpus->cpus[i].props.socket_id,
+                                NULL);
+        object_property_set_int(cpuobj, "core-id",
+                                machine->possible_cpus->cpus[i].props.core_id,
+                                NULL);
+        object_property_set_int(cpuobj, "thread-id",
+                                machine->possible_cpus->cpus[i].props.thread_id,
+                                NULL);
+        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
+    }
+
+    fdt_add_cpu_nodes(lvms);
+}
+
 static void virt_firmware_init(LoongArchVirtMachineState *lvms)
 {
     char *filename = MACHINE(lvms)->firmware;
@@ -1161,15 +1192,10 @@ static void fw_cfg_add_memory(MachineState *ms)
 
 static void virt_init(MachineState *machine)
 {
-    LoongArchCPU *lacpu;
     const char *cpu_model = machine->cpu_type;
     MemoryRegion *address_space_mem = get_system_memory();
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
-    int i;
     hwaddr base, size, ram_size = machine->ram_size;
-    MachineClass *mc = MACHINE_GET_CLASS(machine);
-    CPUState *cpu;
-    Object *cpuobj;
 
     if (!cpu_model) {
         cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
@@ -1186,32 +1212,6 @@ static void virt_init(MachineState *machine)
                           machine, "iocsr_misc", 0x428);
     memory_region_add_subregion(&lvms->system_iocsr, 0, &lvms->iocsr_mem);
 
-    /* Init CPUs */
-    mc->possible_cpu_arch_ids(machine);
-    for (i = 0; i < machine->smp.cpus; i++) {
-        cpuobj = object_new(machine->cpu_type);
-        if (cpuobj == NULL) {
-            error_report("Fail to create object with type %s ",
-                         machine->cpu_type);
-            exit(EXIT_FAILURE);
-        }
-
-        cpu = CPU(cpuobj);
-        cpu->cpu_index = i;
-        lacpu = LOONGARCH_CPU(cpuobj);
-        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
-        object_property_set_int(cpuobj, "socket-id",
-                                machine->possible_cpus->cpus[i].props.socket_id,
-                                NULL);
-        object_property_set_int(cpuobj, "core-id",
-                                machine->possible_cpus->cpus[i].props.core_id,
-                                NULL);
-        object_property_set_int(cpuobj, "thread-id",
-                                machine->possible_cpus->cpus[i].props.thread_id,
-                                NULL);
-        qdev_realize_and_unref(DEVICE(cpuobj), NULL, &error_fatal);
-    }
-    fdt_add_cpu_nodes(lvms);
     fdt_add_memory_nodes(machine);
     fw_cfg_add_memory(machine);
 
@@ -1269,6 +1269,9 @@ static void virt_init(MachineState *machine)
 
     /* Initialize the IO interrupt subsystem */
     virt_irq_init(lvms);
+
+    /* Init CPUs */
+    virt_init_cpus(machine);
     platform_bus_add_all_fdt_nodes(machine->fdt, "/platic",
                                    VIRT_PLATFORM_BUS_BASEADDRESS,
                                    VIRT_PLATFORM_BUS_SIZE,
@@ -1465,8 +1468,11 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
 {
     CPUArchId *cpu_slot;
     LoongArchCPU *cpu = LOONGARCH_CPU(dev);
+    MachineState *ms = MACHINE(hotplug_dev);
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
 
+    /* Connect irq to cpu, including ipi and extioi irqchip */
+    virt_init_cpu_irq(ms, CPU(cpu));
     cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
     cpu_slot->cpu = CPU(dev);
     return;
-- 
2.39.3

[PATCH v4 5/6] hw/loongarch/virt: Update the ACPI table for hotplug cpu

[Bibo Mao]

On LoongArch virt machine, ACPI GED hardware is used for CPU hotplug
handler, here CPU hotplug support feature is added based on GED handler,
also CPU scan and reject method is added about CPU device in DSDT table.

Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/Kconfig        |  1 +
 hw/loongarch/acpi-build.c   | 35 +++++++++++++++++++++++++++++++++--
 hw/loongarch/virt.c         | 10 ++++++++++
 include/hw/loongarch/virt.h |  1 +
 4 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
index fe1c6feac1..bb2838b7b5 100644
--- a/hw/loongarch/Kconfig
+++ b/hw/loongarch/Kconfig
@@ -17,6 +17,7 @@ config LOONGARCH_VIRT
     select LOONGARCH_EXTIOI
     select LS7A_RTC
     select SMBIOS
+    select ACPI_CPU_HOTPLUG
     select ACPI_PCI
     select ACPI_HW_REDUCED
     select FW_CFG_DMA
diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
index 50709bda0f..c220edec68 100644
--- a/hw/loongarch/acpi-build.c
+++ b/hw/loongarch/acpi-build.c
@@ -47,6 +47,22 @@
 #define ACPI_BUILD_DPRINTF(fmt, ...)
 #endif
 
+static void virt_madt_cpu_entry(int uid,
+                                const CPUArchIdList *apic_ids,
+                                GArray *entry, bool force_enabled)
+{
+    uint32_t flags, apic_id = apic_ids->cpus[uid].arch_id;
+
+    flags = apic_ids->cpus[uid].cpu || force_enabled ? 1 /* Enabled */ : 0;
+
+    /* Rev 1.0b, Table 5-13 Processor Local APIC Structure */
+    build_append_int_noprefix(entry, 0, 1);       /* Type */
+    build_append_int_noprefix(entry, 8, 1);       /* Length */
+    build_append_int_noprefix(entry, uid, 1);     /* ACPI Processor ID */
+    build_append_int_noprefix(entry, apic_id, 1); /* APIC ID */
+    build_append_int_noprefix(entry, flags, 4); /* Flags */
+}
+
 /* build FADT */
 static void init_common_fadt_data(AcpiFadtData *data)
 {
@@ -123,15 +139,17 @@ build_madt(GArray *table_data, BIOSLinker *linker,
     build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags */
 
     for (i = 0; i < arch_ids->len; i++) {
+        uint32_t flags;
+
         /* Processor Core Interrupt Controller Structure */
         arch_id = arch_ids->cpus[i].arch_id;
-
+        flags   = arch_ids->cpus[i].cpu ? 1 : 0;
         build_append_int_noprefix(table_data, 17, 1);    /* Type */
         build_append_int_noprefix(table_data, 15, 1);    /* Length */
         build_append_int_noprefix(table_data, 1, 1);     /* Version */
         build_append_int_noprefix(table_data, i, 4);     /* ACPI Processor ID */
         build_append_int_noprefix(table_data, arch_id, 4); /* Core ID */
-        build_append_int_noprefix(table_data, 1, 4);     /* Flags */
+        build_append_int_noprefix(table_data, flags, 4); /* Flags */
     }
 
     /* Extend I/O Interrupt Controller Structure */
@@ -334,6 +352,7 @@ build_la_ged_aml(Aml *dsdt, MachineState *machine)
 {
     uint32_t event;
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
+    CPUHotplugFeatures opts;
 
     build_ged_aml(dsdt, "\\_SB."GED_DEVICE,
                   HOTPLUG_HANDLER(lvms->acpi_ged),
@@ -346,6 +365,18 @@ build_la_ged_aml(Aml *dsdt, MachineState *machine)
                                  AML_SYSTEM_MEMORY,
                                  VIRT_GED_MEM_ADDR);
     }
+
+    if (event & ACPI_GED_CPU_HOTPLUG_EVT) {
+        opts.acpi_1_compatible = false;
+        opts.has_legacy_cphp = false;
+        opts.fw_unplugs_cpu = false;
+        opts.smi_path = NULL;
+
+        build_cpus_aml(dsdt, machine, opts, virt_madt_cpu_entry,
+                       VIRT_GED_CPUHP_ADDR, "\\_SB",
+                       AML_GED_EVT_CPU_SCAN_METHOD, AML_SYSTEM_MEMORY);
+    }
+
     acpi_dsdt_add_power_button(dsdt);
 }
 
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 0e0c6c202b..b49b15c0f6 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -666,11 +666,17 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
 {
     DeviceState *dev;
     MachineState *ms = MACHINE(lvms);
+    MachineClass *mc = MACHINE_GET_CLASS(lvms);
     uint32_t event = ACPI_GED_PWR_DOWN_EVT;
 
     if (ms->ram_slots) {
         event |= ACPI_GED_MEM_HOTPLUG_EVT;
     }
+
+    if (mc->has_hotpluggable_cpus) {
+        event |= ACPI_GED_CPU_HOTPLUG_EVT;
+    }
+
     dev = qdev_new(TYPE_ACPI_GED);
     qdev_prop_set_uint32(dev, "ged-event", event);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
@@ -682,6 +688,10 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
     /* ged regs used for reset and power down */
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, VIRT_GED_REG_ADDR);
 
+    if (mc->has_hotpluggable_cpus) {
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 3, VIRT_GED_CPUHP_ADDR);
+    }
+
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
                        qdev_get_gpio_in(pch_pic, VIRT_SCI_IRQ - VIRT_GSI_BASE));
     return dev;
diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
index 260e6bd7cf..79a85723c9 100644
--- a/include/hw/loongarch/virt.h
+++ b/include/hw/loongarch/virt.h
@@ -30,6 +30,7 @@
 #define VIRT_GED_EVT_ADDR       0x100e0000
 #define VIRT_GED_MEM_ADDR       (VIRT_GED_EVT_ADDR + ACPI_GED_EVT_SEL_LEN)
 #define VIRT_GED_REG_ADDR       (VIRT_GED_MEM_ADDR + MEMORY_HOTPLUG_IO_LEN)
+#define VIRT_GED_CPUHP_ADDR     (VIRT_GED_REG_ADDR + ACPI_GED_REG_COUNT)
 
 #define COMMAND_LINE_SIZE       512
 
-- 
2.39.3

Re: [PATCH v4 5/6] hw/loongarch/virt: Update the ACPI table for hotplug cpu

[Igor Mammedov]

On Tue, 12 Nov 2024 10:17:37 +0800
Bibo Mao <maobibo@loongson.cn> wrote:

> On LoongArch virt machine, ACPI GED hardware is used for CPU hotplug
> handler, here CPU hotplug support feature is added based on GED handler,
> also CPU scan and reject method is added about CPU device in DSDT table.
> 
> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  hw/loongarch/Kconfig        |  1 +
>  hw/loongarch/acpi-build.c   | 35 +++++++++++++++++++++++++++++++++--
>  hw/loongarch/virt.c         | 10 ++++++++++
>  include/hw/loongarch/virt.h |  1 +
>  4 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
> index fe1c6feac1..bb2838b7b5 100644
> --- a/hw/loongarch/Kconfig
> +++ b/hw/loongarch/Kconfig
> @@ -17,6 +17,7 @@ config LOONGARCH_VIRT
>      select LOONGARCH_EXTIOI
>      select LS7A_RTC
>      select SMBIOS
> +    select ACPI_CPU_HOTPLUG
>      select ACPI_PCI
>      select ACPI_HW_REDUCED
>      select FW_CFG_DMA
> diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
> index 50709bda0f..c220edec68 100644
> --- a/hw/loongarch/acpi-build.c
> +++ b/hw/loongarch/acpi-build.c
> @@ -47,6 +47,22 @@
>  #define ACPI_BUILD_DPRINTF(fmt, ...)
>  #endif
>  
> +static void virt_madt_cpu_entry(int uid,
> +                                const CPUArchIdList *apic_ids,
> +                                GArray *entry, bool force_enabled)
> +{
> +    uint32_t flags, apic_id = apic_ids->cpus[uid].arch_id;
> +
> +    flags = apic_ids->cpus[uid].cpu || force_enabled ? 1 /* Enabled */ : 0;
> +
> +    /* Rev 1.0b, Table 5-13 Processor Local APIC Structure */
> +    build_append_int_noprefix(entry, 0, 1);       /* Type */
> +    build_append_int_noprefix(entry, 8, 1);       /* Length */
> +    build_append_int_noprefix(entry, uid, 1);     /* ACPI Processor ID */
> +    build_append_int_noprefix(entry, apic_id, 1); /* APIC ID */

is it really APIC ID or just copy paste mistake from x86?


> +    build_append_int_noprefix(entry, flags, 4); /* Flags */
> +}
> +
>  /* build FADT */
>  static void init_common_fadt_data(AcpiFadtData *data)
>  {
> @@ -123,15 +139,17 @@ build_madt(GArray *table_data, BIOSLinker *linker,
>      build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags */
>  
>      for (i = 0; i < arch_ids->len; i++) {
> +        uint32_t flags;
> +
>          /* Processor Core Interrupt Controller Structure */
>          arch_id = arch_ids->cpus[i].arch_id;
> -
> +        flags   = arch_ids->cpus[i].cpu ? 1 : 0;
>          build_append_int_noprefix(table_data, 17, 1);    /* Type */
>          build_append_int_noprefix(table_data, 15, 1);    /* Length */
>          build_append_int_noprefix(table_data, 1, 1);     /* Version */
>          build_append_int_noprefix(table_data, i, 4);     /* ACPI Processor ID */
>          build_append_int_noprefix(table_data, arch_id, 4); /* Core ID */
> -        build_append_int_noprefix(table_data, 1, 4);     /* Flags */
> +        build_append_int_noprefix(table_data, flags, 4); /* Flags */
>      }
>  
>      /* Extend I/O Interrupt Controller Structure */
> @@ -334,6 +352,7 @@ build_la_ged_aml(Aml *dsdt, MachineState *machine)
>  {
>      uint32_t event;
>      LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
> +    CPUHotplugFeatures opts;
>  
>      build_ged_aml(dsdt, "\\_SB."GED_DEVICE,
>                    HOTPLUG_HANDLER(lvms->acpi_ged),
> @@ -346,6 +365,18 @@ build_la_ged_aml(Aml *dsdt, MachineState *machine)
>                                   AML_SYSTEM_MEMORY,
>                                   VIRT_GED_MEM_ADDR);
>      }
> +
> +    if (event & ACPI_GED_CPU_HOTPLUG_EVT) {
> +        opts.acpi_1_compatible = false;
> +        opts.has_legacy_cphp = false;
> +        opts.fw_unplugs_cpu = false;
> +        opts.smi_path = NULL;
> +
> +        build_cpus_aml(dsdt, machine, opts, virt_madt_cpu_entry,
> +                       VIRT_GED_CPUHP_ADDR, "\\_SB",
> +                       AML_GED_EVT_CPU_SCAN_METHOD, AML_SYSTEM_MEMORY);
> +    }
> +
>      acpi_dsdt_add_power_button(dsdt);
>  }
>  
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index 0e0c6c202b..b49b15c0f6 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -666,11 +666,17 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
>  {
>      DeviceState *dev;
>      MachineState *ms = MACHINE(lvms);
> +    MachineClass *mc = MACHINE_GET_CLASS(lvms);
>      uint32_t event = ACPI_GED_PWR_DOWN_EVT;
>  
>      if (ms->ram_slots) {
>          event |= ACPI_GED_MEM_HOTPLUG_EVT;
>      }
> +
> +    if (mc->has_hotpluggable_cpus) {
> +        event |= ACPI_GED_CPU_HOTPLUG_EVT;
> +    }
> +
>      dev = qdev_new(TYPE_ACPI_GED);
>      qdev_prop_set_uint32(dev, "ged-event", event);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> @@ -682,6 +688,10 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
>      /* ged regs used for reset and power down */
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, VIRT_GED_REG_ADDR);
>  
> +    if (mc->has_hotpluggable_cpus) {
> +        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 3, VIRT_GED_CPUHP_ADDR);
> +    }
> +
>      sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
>                         qdev_get_gpio_in(pch_pic, VIRT_SCI_IRQ - VIRT_GSI_BASE));
>      return dev;
> diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
> index 260e6bd7cf..79a85723c9 100644
> --- a/include/hw/loongarch/virt.h
> +++ b/include/hw/loongarch/virt.h
> @@ -30,6 +30,7 @@
>  #define VIRT_GED_EVT_ADDR       0x100e0000
>  #define VIRT_GED_MEM_ADDR       (VIRT_GED_EVT_ADDR + ACPI_GED_EVT_SEL_LEN)
>  #define VIRT_GED_REG_ADDR       (VIRT_GED_MEM_ADDR + MEMORY_HOTPLUG_IO_LEN)
> +#define VIRT_GED_CPUHP_ADDR     (VIRT_GED_REG_ADDR + ACPI_GED_REG_COUNT)
>  
>  #define COMMAND_LINE_SIZE       512
>  

Re: [PATCH v4 5/6] hw/loongarch/virt: Update the ACPI table for hotplug cpu

[bibo mao]

On 2024/11/19 上午12:51, Igor Mammedov wrote:
> On Tue, 12 Nov 2024 10:17:37 +0800
> Bibo Mao <maobibo@loongson.cn> wrote:
> 
>> On LoongArch virt machine, ACPI GED hardware is used for CPU hotplug
>> handler, here CPU hotplug support feature is added based on GED handler,
>> also CPU scan and reject method is added about CPU device in DSDT table.
>>
>> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/loongarch/Kconfig        |  1 +
>>   hw/loongarch/acpi-build.c   | 35 +++++++++++++++++++++++++++++++++--
>>   hw/loongarch/virt.c         | 10 ++++++++++
>>   include/hw/loongarch/virt.h |  1 +
>>   4 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
>> index fe1c6feac1..bb2838b7b5 100644
>> --- a/hw/loongarch/Kconfig
>> +++ b/hw/loongarch/Kconfig
>> @@ -17,6 +17,7 @@ config LOONGARCH_VIRT
>>       select LOONGARCH_EXTIOI
>>       select LS7A_RTC
>>       select SMBIOS
>> +    select ACPI_CPU_HOTPLUG
>>       select ACPI_PCI
>>       select ACPI_HW_REDUCED
>>       select FW_CFG_DMA
>> diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
>> index 50709bda0f..c220edec68 100644
>> --- a/hw/loongarch/acpi-build.c
>> +++ b/hw/loongarch/acpi-build.c
>> @@ -47,6 +47,22 @@
>>   #define ACPI_BUILD_DPRINTF(fmt, ...)
>>   #endif
>>   
>> +static void virt_madt_cpu_entry(int uid,
>> +                                const CPUArchIdList *apic_ids,
>> +                                GArray *entry, bool force_enabled)
>> +{
>> +    uint32_t flags, apic_id = apic_ids->cpus[uid].arch_id;
>> +
>> +    flags = apic_ids->cpus[uid].cpu || force_enabled ? 1 /* Enabled */ : 0;
>> +
>> +    /* Rev 1.0b, Table 5-13 Processor Local APIC Structure */
>> +    build_append_int_noprefix(entry, 0, 1);       /* Type */
>> +    build_append_int_noprefix(entry, 8, 1);       /* Length */
>> +    build_append_int_noprefix(entry, uid, 1);     /* ACPI Processor ID */
>> +    build_append_int_noprefix(entry, apic_id, 1); /* APIC ID */
> 
> is it really APIC ID or just copy paste mistake from x86?
That is copied from x86, from ACPI spec it is "Physical Processor ID", 
will modify it in next patch.

Regards
Bibo Mao
> 
> 
>> +    build_append_int_noprefix(entry, flags, 4); /* Flags */
>> +}
>> +
>>   /* build FADT */
>>   static void init_common_fadt_data(AcpiFadtData *data)
>>   {
>> @@ -123,15 +139,17 @@ build_madt(GArray *table_data, BIOSLinker *linker,
>>       build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags */
>>   
>>       for (i = 0; i < arch_ids->len; i++) {
>> +        uint32_t flags;
>> +
>>           /* Processor Core Interrupt Controller Structure */
>>           arch_id = arch_ids->cpus[i].arch_id;
>> -
>> +        flags   = arch_ids->cpus[i].cpu ? 1 : 0;
>>           build_append_int_noprefix(table_data, 17, 1);    /* Type */
>>           build_append_int_noprefix(table_data, 15, 1);    /* Length */
>>           build_append_int_noprefix(table_data, 1, 1);     /* Version */
>>           build_append_int_noprefix(table_data, i, 4);     /* ACPI Processor ID */
>>           build_append_int_noprefix(table_data, arch_id, 4); /* Core ID */
>> -        build_append_int_noprefix(table_data, 1, 4);     /* Flags */
>> +        build_append_int_noprefix(table_data, flags, 4); /* Flags */
>>       }
>>   
>>       /* Extend I/O Interrupt Controller Structure */
>> @@ -334,6 +352,7 @@ build_la_ged_aml(Aml *dsdt, MachineState *machine)
>>   {
>>       uint32_t event;
>>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
>> +    CPUHotplugFeatures opts;
>>   
>>       build_ged_aml(dsdt, "\\_SB."GED_DEVICE,
>>                     HOTPLUG_HANDLER(lvms->acpi_ged),
>> @@ -346,6 +365,18 @@ build_la_ged_aml(Aml *dsdt, MachineState *machine)
>>                                    AML_SYSTEM_MEMORY,
>>                                    VIRT_GED_MEM_ADDR);
>>       }
>> +
>> +    if (event & ACPI_GED_CPU_HOTPLUG_EVT) {
>> +        opts.acpi_1_compatible = false;
>> +        opts.has_legacy_cphp = false;
>> +        opts.fw_unplugs_cpu = false;
>> +        opts.smi_path = NULL;
>> +
>> +        build_cpus_aml(dsdt, machine, opts, virt_madt_cpu_entry,
>> +                       VIRT_GED_CPUHP_ADDR, "\\_SB",
>> +                       AML_GED_EVT_CPU_SCAN_METHOD, AML_SYSTEM_MEMORY);
>> +    }
>> +
>>       acpi_dsdt_add_power_button(dsdt);
>>   }
>>   
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index 0e0c6c202b..b49b15c0f6 100644
>> --- a/hw/loongarch/virt.c
>> +++ b/hw/loongarch/virt.c
>> @@ -666,11 +666,17 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
>>   {
>>       DeviceState *dev;
>>       MachineState *ms = MACHINE(lvms);
>> +    MachineClass *mc = MACHINE_GET_CLASS(lvms);
>>       uint32_t event = ACPI_GED_PWR_DOWN_EVT;
>>   
>>       if (ms->ram_slots) {
>>           event |= ACPI_GED_MEM_HOTPLUG_EVT;
>>       }
>> +
>> +    if (mc->has_hotpluggable_cpus) {
>> +        event |= ACPI_GED_CPU_HOTPLUG_EVT;
>> +    }
>> +
>>       dev = qdev_new(TYPE_ACPI_GED);
>>       qdev_prop_set_uint32(dev, "ged-event", event);
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> @@ -682,6 +688,10 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
>>       /* ged regs used for reset and power down */
>>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, VIRT_GED_REG_ADDR);
>>   
>> +    if (mc->has_hotpluggable_cpus) {
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 3, VIRT_GED_CPUHP_ADDR);
>> +    }
>> +
>>       sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
>>                          qdev_get_gpio_in(pch_pic, VIRT_SCI_IRQ - VIRT_GSI_BASE));
>>       return dev;
>> diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
>> index 260e6bd7cf..79a85723c9 100644
>> --- a/include/hw/loongarch/virt.h
>> +++ b/include/hw/loongarch/virt.h
>> @@ -30,6 +30,7 @@
>>   #define VIRT_GED_EVT_ADDR       0x100e0000
>>   #define VIRT_GED_MEM_ADDR       (VIRT_GED_EVT_ADDR + ACPI_GED_EVT_SEL_LEN)
>>   #define VIRT_GED_REG_ADDR       (VIRT_GED_MEM_ADDR + MEMORY_HOTPLUG_IO_LEN)
>> +#define VIRT_GED_CPUHP_ADDR     (VIRT_GED_REG_ADDR + ACPI_GED_REG_COUNT)
>>   
>>   #define COMMAND_LINE_SIZE       512
>>   
> 

[PATCH v4 6/6] hw/loongarch/virt: Enable cpu hotplug feature on virt machine

[Bibo Mao]

On virt machine, enable CPU hotplug feature has_hotpluggable_cpus. For
hot-added CPUs, there is socket-id/core-id/thread-id property set,
arch_id can be caculated from these properties. So that cpu slot can be
searched from its arch_id.

Also change num-cpu property of extioi and ipi from smp.cpus to
smp.max_cpus

Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/virt.c | 68 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 9 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index b49b15c0f6..5f81673368 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -890,7 +890,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
 
     /* Create IPI device */
     ipi = qdev_new(TYPE_LOONGARCH_IPI);
-    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.cpus);
+    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.max_cpus);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), &error_fatal);
     lvms->ipi = ipi;
 
@@ -905,7 +905,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
 
     /* Create EXTIOI device */
     extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
-    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.cpus);
+    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.max_cpus);
     if (virt_is_veiointc_enabled(lvms)) {
         qdev_prop_set_bit(extioi, "has-virtualization-extension", true);
     }
@@ -1369,11 +1369,15 @@ static void virt_get_topo_from_index(MachineState *ms,
 }
 
 /* Find cpu slot in machine->possible_cpus by arch_id */
-static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id)
+static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id, int *index)
 {
     int n;
     for (n = 0; n < ms->possible_cpus->len; n++) {
         if (ms->possible_cpus->cpus[n].arch_id == arch_id) {
+            if (index) {
+                *index = n;
+            }
+
             return &ms->possible_cpus->cpus[n];
         }
     }
@@ -1386,10 +1390,12 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
 {
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
     MachineState *ms = MACHINE(OBJECT(hotplug_dev));
+    CPUState *cs = CPU(dev);
     LoongArchCPU *cpu = LOONGARCH_CPU(dev);
     CPUArchId *cpu_slot;
     Error *local_err = NULL;
-    int arch_id;
+    LoongArchCPUTopo topo;
+    int arch_id, index;
 
     /* sanity check the cpu */
     if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
@@ -1408,12 +1414,45 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
      }
 
     if (cpu->phy_id == UNSET_PHY_ID) {
-        error_setg(&local_err, "CPU hotplug not supported");
-        goto out;
+        if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
+            error_setg(&local_err,
+                       "Invalid thread-id %u specified, must be in range 1:%u",
+                       cpu->thread_id, ms->smp.threads - 1);
+            goto out;
+        }
+
+        if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
+            error_setg(&local_err,
+                       "Invalid core-id %u specified, must be in range 1:%u",
+                       cpu->core_id, ms->smp.cores - 1);
+            goto out;
+        }
+
+        if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
+            error_setg(&local_err,
+                       "Invalid socket-id %u specified, must be in range 1:%u",
+                       cpu->socket_id, ms->smp.sockets - 1);
+            goto out;
+        }
+
+        topo.socket_id = cpu->socket_id;
+        topo.core_id = cpu->core_id;
+        topo.thread_id = cpu->thread_id;
+        arch_id =  virt_get_arch_id_from_topo(ms, &topo);
+        cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
+        if (CPU(cpu_slot->cpu)) {
+            error_setg(&local_err,
+                       "cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
+                       cs->cpu_index, cpu->socket_id, cpu->core_id,
+                       cpu->thread_id, cpu_slot->arch_id);
+            goto out;
+        }
+        cpu->phy_id = arch_id;
+        cs->cpu_index = index;
     } else {
         /* For cold-add cpu, find cpu slot from arch_id */
         arch_id = cpu->phy_id;
-        cpu_slot = virt_find_cpu_slot(ms, arch_id);
+        cpu_slot = virt_find_cpu_slot(ms, arch_id, NULL);
     }
 
     numa_cpu_pre_plug(cpu_slot, dev, &local_err);
@@ -1468,7 +1507,7 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
         return;
     }
 
-    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
+    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
     cpu_slot->cpu = NULL;
     return;
 }
@@ -1477,14 +1516,24 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
                                 DeviceState *dev, Error **errp)
 {
     CPUArchId *cpu_slot;
+    Error *local_err = NULL;
     LoongArchCPU *cpu = LOONGARCH_CPU(dev);
     MachineState *ms = MACHINE(hotplug_dev);
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
 
     /* Connect irq to cpu, including ipi and extioi irqchip */
     virt_init_cpu_irq(ms, CPU(cpu));
-    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
+    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
     cpu_slot->cpu = CPU(dev);
+
+    if (lvms->acpi_ged) {
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
     return;
 }
 
@@ -1667,6 +1716,7 @@ static void virt_class_init(ObjectClass *oc, void *data)
     mc->numa_mem_supported = true;
     mc->auto_enable_numa_with_memhp = true;
     mc->auto_enable_numa_with_memdev = true;
+    mc->has_hotpluggable_cpus = true;
     mc->get_hotplug_handler = virt_get_hotplug_handler;
     mc->default_nic = "virtio-net-pci";
     hc->plug = virt_device_plug_cb;
-- 
2.39.3

Re: [PATCH v4 6/6] hw/loongarch/virt: Enable cpu hotplug feature on virt machine

[Igor Mammedov]

On Tue, 12 Nov 2024 10:17:38 +0800
Bibo Mao <maobibo@loongson.cn> wrote:

> On virt machine, enable CPU hotplug feature has_hotpluggable_cpus. For
> hot-added CPUs, there is socket-id/core-id/thread-id property set,
> arch_id can be caculated from these properties. So that cpu slot can be
> searched from its arch_id.
> 
> Also change num-cpu property of extioi and ipi from smp.cpus to
> smp.max_cpus
> 
> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  hw/loongarch/virt.c | 68 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index b49b15c0f6..5f81673368 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -890,7 +890,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
>  
>      /* Create IPI device */
>      ipi = qdev_new(TYPE_LOONGARCH_IPI);
> -    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.cpus);
> +    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.max_cpus);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), &error_fatal);
>      lvms->ipi = ipi;
>  
> @@ -905,7 +905,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
>  
>      /* Create EXTIOI device */
>      extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
> -    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.cpus);
> +    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.max_cpus);
>      if (virt_is_veiointc_enabled(lvms)) {
>          qdev_prop_set_bit(extioi, "has-virtualization-extension", true);
>      }
> @@ -1369,11 +1369,15 @@ static void virt_get_topo_from_index(MachineState *ms,
>  }
>  
>  /* Find cpu slot in machine->possible_cpus by arch_id */
> -static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id)
> +static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id, int *index)
>  {
>      int n;
>      for (n = 0; n < ms->possible_cpus->len; n++) {
>          if (ms->possible_cpus->cpus[n].arch_id == arch_id) {
> +            if (index) {
> +                *index = n;
> +            }
> +
>              return &ms->possible_cpus->cpus[n];
>          }
>      }
> @@ -1386,10 +1390,12 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>  {
>      LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>      MachineState *ms = MACHINE(OBJECT(hotplug_dev));
> +    CPUState *cs = CPU(dev);
>      LoongArchCPU *cpu = LOONGARCH_CPU(dev);
>      CPUArchId *cpu_slot;
>      Error *local_err = NULL;
> -    int arch_id;
> +    LoongArchCPUTopo topo;
> +    int arch_id, index;
>  
>      /* sanity check the cpu */
>      if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
> @@ -1408,12 +1414,45 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>       }
>  
>      if (cpu->phy_id == UNSET_PHY_ID) {

> -        error_setg(&local_err, "CPU hotplug not supported");
> -        goto out;
> +        if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
> +            error_setg(&local_err,
> +                       "Invalid thread-id %u specified, must be in range 1:%u",
> +                       cpu->thread_id, ms->smp.threads - 1);
> +            goto out;
> +        }
> +
> +        if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
> +            error_setg(&local_err,
> +                       "Invalid core-id %u specified, must be in range 1:%u",
> +                       cpu->core_id, ms->smp.cores - 1);
> +            goto out;
> +        }
> +
> +        if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
> +            error_setg(&local_err,
> +                       "Invalid socket-id %u specified, must be in range 1:%u",
> +                       cpu->socket_id, ms->smp.sockets - 1);
> +            goto out;
> +        }
> +
> +        topo.socket_id = cpu->socket_id;
> +        topo.core_id = cpu->core_id;
> +        topo.thread_id = cpu->thread_id;
> +        arch_id =  virt_get_arch_id_from_topo(ms, &topo);
> +        cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
> +        if (CPU(cpu_slot->cpu)) {
> +            error_setg(&local_err,
> +                       "cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
> +                       cs->cpu_index, cpu->socket_id, cpu->core_id,
> +                       cpu->thread_id, cpu_slot->arch_id);
> +            goto out;
> +        }
> +        cpu->phy_id = arch_id;
> +        cs->cpu_index = index;
this whole branch applies to cold-plugged CPUs as well, especially
if both (hot/cold plugged CPUs are getting wired with help of pre_plug)
So this hunk should be introduced somewhere earlier in series,
and than I'd likely won't need (cpu->phy_id == UNSET_PHY_ID) check to begin with.

the only difference vs cold-plug would be need to call acpi_ged plug handler,
like you are dong below in virt_cpu_plug

>      } else {
>          /* For cold-add cpu, find cpu slot from arch_id */
>          arch_id = cpu->phy_id;
> -        cpu_slot = virt_find_cpu_slot(ms, arch_id);
> +        cpu_slot = virt_find_cpu_slot(ms, arch_id, NULL);
>      }
>  
>      numa_cpu_pre_plug(cpu_slot, dev, &local_err);
> @@ -1468,7 +1507,7 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
>      cpu_slot->cpu = NULL;
>      return;
>  }
> @@ -1477,14 +1516,24 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>                                  DeviceState *dev, Error **errp)
>  {
>      CPUArchId *cpu_slot;
> +    Error *local_err = NULL;
>      LoongArchCPU *cpu = LOONGARCH_CPU(dev);
>      MachineState *ms = MACHINE(hotplug_dev);
>      LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>  
>      /* Connect irq to cpu, including ipi and extioi irqchip */
>      virt_init_cpu_irq(ms, CPU(cpu));
> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
>      cpu_slot->cpu = CPU(dev);
> +
> +    if (lvms->acpi_ged) {
Why do you need check, can machine be created without acpi_ged?

> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
>      return;
>  }
>  
> @@ -1667,6 +1716,7 @@ static void virt_class_init(ObjectClass *oc, void *data)
>      mc->numa_mem_supported = true;
>      mc->auto_enable_numa_with_memhp = true;
>      mc->auto_enable_numa_with_memdev = true;
> +    mc->has_hotpluggable_cpus = true;
>      mc->get_hotplug_handler = virt_get_hotplug_handler;
>      mc->default_nic = "virtio-net-pci";
>      hc->plug = virt_device_plug_cb;

Re: [PATCH v4 6/6] hw/loongarch/virt: Enable cpu hotplug feature on virt machine

[bibo mao]

On 2024/11/19 上午1:03, Igor Mammedov wrote:
> On Tue, 12 Nov 2024 10:17:38 +0800
> Bibo Mao <maobibo@loongson.cn> wrote:
> 
>> On virt machine, enable CPU hotplug feature has_hotpluggable_cpus. For
>> hot-added CPUs, there is socket-id/core-id/thread-id property set,
>> arch_id can be caculated from these properties. So that cpu slot can be
>> searched from its arch_id.
>>
>> Also change num-cpu property of extioi and ipi from smp.cpus to
>> smp.max_cpus
>>
>> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/loongarch/virt.c | 68 +++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 59 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index b49b15c0f6..5f81673368 100644
>> --- a/hw/loongarch/virt.c
>> +++ b/hw/loongarch/virt.c
>> @@ -890,7 +890,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
>>   
>>       /* Create IPI device */
>>       ipi = qdev_new(TYPE_LOONGARCH_IPI);
>> -    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.cpus);
>> +    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.max_cpus);
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), &error_fatal);
>>       lvms->ipi = ipi;
>>   
>> @@ -905,7 +905,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
>>   
>>       /* Create EXTIOI device */
>>       extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
>> -    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.cpus);
>> +    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.max_cpus);
>>       if (virt_is_veiointc_enabled(lvms)) {
>>           qdev_prop_set_bit(extioi, "has-virtualization-extension", true);
>>       }
>> @@ -1369,11 +1369,15 @@ static void virt_get_topo_from_index(MachineState *ms,
>>   }
>>   
>>   /* Find cpu slot in machine->possible_cpus by arch_id */
>> -static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id)
>> +static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id, int *index)
>>   {
>>       int n;
>>       for (n = 0; n < ms->possible_cpus->len; n++) {
>>           if (ms->possible_cpus->cpus[n].arch_id == arch_id) {
>> +            if (index) {
>> +                *index = n;
>> +            }
>> +
>>               return &ms->possible_cpus->cpus[n];
>>           }
>>       }
>> @@ -1386,10 +1390,12 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>   {
>>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>>       MachineState *ms = MACHINE(OBJECT(hotplug_dev));
>> +    CPUState *cs = CPU(dev);
>>       LoongArchCPU *cpu = LOONGARCH_CPU(dev);
>>       CPUArchId *cpu_slot;
>>       Error *local_err = NULL;
>> -    int arch_id;
>> +    LoongArchCPUTopo topo;
>> +    int arch_id, index;
>>   
>>       /* sanity check the cpu */
>>       if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
>> @@ -1408,12 +1414,45 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>        }
>>   
>>       if (cpu->phy_id == UNSET_PHY_ID) {
> 
>> -        error_setg(&local_err, "CPU hotplug not supported");
>> -        goto out;
>> +        if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
>> +            error_setg(&local_err,
>> +                       "Invalid thread-id %u specified, must be in range 1:%u",
>> +                       cpu->thread_id, ms->smp.threads - 1);
>> +            goto out;
>> +        }
>> +
>> +        if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
>> +            error_setg(&local_err,
>> +                       "Invalid core-id %u specified, must be in range 1:%u",
>> +                       cpu->core_id, ms->smp.cores - 1);
>> +            goto out;
>> +        }
>> +
>> +        if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
>> +            error_setg(&local_err,
>> +                       "Invalid socket-id %u specified, must be in range 1:%u",
>> +                       cpu->socket_id, ms->smp.sockets - 1);
>> +            goto out;
>> +        }
>> +
>> +        topo.socket_id = cpu->socket_id;
>> +        topo.core_id = cpu->core_id;
>> +        topo.thread_id = cpu->thread_id;
>> +        arch_id =  virt_get_arch_id_from_topo(ms, &topo);
>> +        cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
>> +        if (CPU(cpu_slot->cpu)) {
>> +            error_setg(&local_err,
>> +                       "cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
>> +                       cs->cpu_index, cpu->socket_id, cpu->core_id,
>> +                       cpu->thread_id, cpu_slot->arch_id);
>> +            goto out;
>> +        }
>> +        cpu->phy_id = arch_id;
>> +        cs->cpu_index = index;
> this whole branch applies to cold-plugged CPUs as well, especially
> if both (hot/cold plugged CPUs are getting wired with help of pre_plug)
> So this hunk should be introduced somewhere earlier in series,
> and than I'd likely won't need (cpu->phy_id == UNSET_PHY_ID) check to begin with.
> 
> the only difference vs cold-plug would be need to call acpi_ged plug handler,
> like you are dong below in virt_cpu_plug
Sure, will check acpi_ged plug handler for cold-plug/hot-plug CPU.

> 
>>       } else {
>>           /* For cold-add cpu, find cpu slot from arch_id */
>>           arch_id = cpu->phy_id;
>> -        cpu_slot = virt_find_cpu_slot(ms, arch_id);
>> +        cpu_slot = virt_find_cpu_slot(ms, arch_id, NULL);
>>       }
>>   
>>       numa_cpu_pre_plug(cpu_slot, dev, &local_err);
>> @@ -1468,7 +1507,7 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
>>           return;
>>       }
>>   
>> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
>>       cpu_slot->cpu = NULL;
>>       return;
>>   }
>> @@ -1477,14 +1516,24 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>                                   DeviceState *dev, Error **errp)
>>   {
>>       CPUArchId *cpu_slot;
>> +    Error *local_err = NULL;
>>       LoongArchCPU *cpu = LOONGARCH_CPU(dev);
>>       MachineState *ms = MACHINE(hotplug_dev);
>>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>>   
>>       /* Connect irq to cpu, including ipi and extioi irqchip */
>>       virt_init_cpu_irq(ms, CPU(cpu));
>> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
>>       cpu_slot->cpu = CPU(dev);
>> +
>> +    if (lvms->acpi_ged) {
> Why do you need check, can machine be created without acpi_ged?
There is no NULL check with macro HOTPLUG_HANDLER() for cold-plug cpu.
Now machine is created with acpi_ged always, in later will add noapic 
option support.

Regards
Bibo Mao

> 
>> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +    }
>> +
>>       return;
>>   }
>>   
>> @@ -1667,6 +1716,7 @@ static void virt_class_init(ObjectClass *oc, void *data)
>>       mc->numa_mem_supported = true;
>>       mc->auto_enable_numa_with_memhp = true;
>>       mc->auto_enable_numa_with_memdev = true;
>> +    mc->has_hotpluggable_cpus = true;
>>       mc->get_hotplug_handler = virt_get_hotplug_handler;
>>       mc->default_nic = "virtio-net-pci";
>>       hc->plug = virt_device_plug_cb;

Re: [PATCH v4 6/6] hw/loongarch/virt: Enable cpu hotplug feature on virt machine

[Igor Mammedov]

On Tue, 19 Nov 2024 18:18:27 +0800
bibo mao <maobibo@loongson.cn> wrote:

> On 2024/11/19 上午1:03, Igor Mammedov wrote:
> > On Tue, 12 Nov 2024 10:17:38 +0800
> > Bibo Mao <maobibo@loongson.cn> wrote:
> >   
> >> On virt machine, enable CPU hotplug feature has_hotpluggable_cpus. For
> >> hot-added CPUs, there is socket-id/core-id/thread-id property set,
> >> arch_id can be caculated from these properties. So that cpu slot can be
> >> searched from its arch_id.
> >>
> >> Also change num-cpu property of extioi and ipi from smp.cpus to
> >> smp.max_cpus
> >>
> >> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
> >> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >> ---
> >>   hw/loongarch/virt.c | 68 +++++++++++++++++++++++++++++++++++++++------
> >>   1 file changed, 59 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> >> index b49b15c0f6..5f81673368 100644
> >> --- a/hw/loongarch/virt.c
> >> +++ b/hw/loongarch/virt.c
> >> @@ -890,7 +890,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
> >>   
> >>       /* Create IPI device */
> >>       ipi = qdev_new(TYPE_LOONGARCH_IPI);
> >> -    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.cpus);
> >> +    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.max_cpus);
> >>       sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), &error_fatal);
> >>       lvms->ipi = ipi;
> >>   
> >> @@ -905,7 +905,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
> >>   
> >>       /* Create EXTIOI device */
> >>       extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
> >> -    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.cpus);
> >> +    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.max_cpus);
> >>       if (virt_is_veiointc_enabled(lvms)) {
> >>           qdev_prop_set_bit(extioi, "has-virtualization-extension", true);
> >>       }
> >> @@ -1369,11 +1369,15 @@ static void virt_get_topo_from_index(MachineState *ms,
> >>   }
> >>   
> >>   /* Find cpu slot in machine->possible_cpus by arch_id */
> >> -static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id)
> >> +static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id, int *index)
> >>   {
> >>       int n;
> >>       for (n = 0; n < ms->possible_cpus->len; n++) {
> >>           if (ms->possible_cpus->cpus[n].arch_id == arch_id) {
> >> +            if (index) {
> >> +                *index = n;
> >> +            }
> >> +
> >>               return &ms->possible_cpus->cpus[n];
> >>           }
> >>       }
> >> @@ -1386,10 +1390,12 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >>   {
> >>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
> >>       MachineState *ms = MACHINE(OBJECT(hotplug_dev));
> >> +    CPUState *cs = CPU(dev);
> >>       LoongArchCPU *cpu = LOONGARCH_CPU(dev);
> >>       CPUArchId *cpu_slot;
> >>       Error *local_err = NULL;
> >> -    int arch_id;
> >> +    LoongArchCPUTopo topo;
> >> +    int arch_id, index;
> >>   
> >>       /* sanity check the cpu */
> >>       if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
> >> @@ -1408,12 +1414,45 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >>        }
> >>   
> >>       if (cpu->phy_id == UNSET_PHY_ID) {  
> >   
> >> -        error_setg(&local_err, "CPU hotplug not supported");
> >> -        goto out;
> >> +        if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
> >> +            error_setg(&local_err,
> >> +                       "Invalid thread-id %u specified, must be in range 1:%u",
> >> +                       cpu->thread_id, ms->smp.threads - 1);
> >> +            goto out;
> >> +        }
> >> +
> >> +        if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
> >> +            error_setg(&local_err,
> >> +                       "Invalid core-id %u specified, must be in range 1:%u",
> >> +                       cpu->core_id, ms->smp.cores - 1);
> >> +            goto out;
> >> +        }
> >> +
> >> +        if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
> >> +            error_setg(&local_err,
> >> +                       "Invalid socket-id %u specified, must be in range 1:%u",
> >> +                       cpu->socket_id, ms->smp.sockets - 1);
> >> +            goto out;
> >> +        }
> >> +
> >> +        topo.socket_id = cpu->socket_id;
> >> +        topo.core_id = cpu->core_id;
> >> +        topo.thread_id = cpu->thread_id;
> >> +        arch_id =  virt_get_arch_id_from_topo(ms, &topo);
> >> +        cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
> >> +        if (CPU(cpu_slot->cpu)) {
> >> +            error_setg(&local_err,
> >> +                       "cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
> >> +                       cs->cpu_index, cpu->socket_id, cpu->core_id,
> >> +                       cpu->thread_id, cpu_slot->arch_id);
> >> +            goto out;
> >> +        }
> >> +        cpu->phy_id = arch_id;
> >> +        cs->cpu_index = index;  
> > this whole branch applies to cold-plugged CPUs as well, especially
> > if both (hot/cold plugged CPUs are getting wired with help of pre_plug)
> > So this hunk should be introduced somewhere earlier in series,
> > and than I'd likely won't need (cpu->phy_id == UNSET_PHY_ID) check to begin with.
> > 
> > the only difference vs cold-plug would be need to call acpi_ged plug handler,
> > like you are dong below in virt_cpu_plug  
> Sure, will check acpi_ged plug handler for cold-plug/hot-plug CPU.
> 
> >   
> >>       } else {
> >>           /* For cold-add cpu, find cpu slot from arch_id */
> >>           arch_id = cpu->phy_id;
> >> -        cpu_slot = virt_find_cpu_slot(ms, arch_id);
> >> +        cpu_slot = virt_find_cpu_slot(ms, arch_id, NULL);
> >>       }
> >>   
> >>       numa_cpu_pre_plug(cpu_slot, dev, &local_err);
> >> @@ -1468,7 +1507,7 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
> >>           return;
> >>       }
> >>   
> >> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
> >> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
> >>       cpu_slot->cpu = NULL;
> >>       return;
> >>   }
> >> @@ -1477,14 +1516,24 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
> >>                                   DeviceState *dev, Error **errp)
> >>   {
> >>       CPUArchId *cpu_slot;
> >> +    Error *local_err = NULL;
> >>       LoongArchCPU *cpu = LOONGARCH_CPU(dev);
> >>       MachineState *ms = MACHINE(hotplug_dev);
> >>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
> >>   
> >>       /* Connect irq to cpu, including ipi and extioi irqchip */
> >>       virt_init_cpu_irq(ms, CPU(cpu));
> >> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
> >> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
> >>       cpu_slot->cpu = CPU(dev);
> >> +
> >> +    if (lvms->acpi_ged) {  
> > Why do you need check, can machine be created without acpi_ged?  
> There is no NULL check with macro HOTPLUG_HANDLER() for cold-plug cpu.
> Now machine is created with acpi_ged always, in later will add noapic 
> option support.

you've probably meant '-noacpi',
anyways right now acpi_ged is always present, so make this patch unconditional.

If later on you find a use-case for '-noacpi' and add it,
then introduce condition at that time.

> 
> Regards
> Bibo Mao
> 
> >   
> >> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &local_err);
> >> +        if (local_err) {
> >> +            error_propagate(errp, local_err);
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >>       return;
> >>   }
> >>   
> >> @@ -1667,6 +1716,7 @@ static void virt_class_init(ObjectClass *oc, void *data)
> >>       mc->numa_mem_supported = true;
> >>       mc->auto_enable_numa_with_memhp = true;
> >>       mc->auto_enable_numa_with_memdev = true;
> >> +    mc->has_hotpluggable_cpus = true;
> >>       mc->get_hotplug_handler = virt_get_hotplug_handler;
> >>       mc->default_nic = "virtio-net-pci";
> >>       hc->plug = virt_device_plug_cb;  
> 

Re: [PATCH v4 6/6] hw/loongarch/virt: Enable cpu hotplug feature on virt machine

[bibo mao]

On 2024/11/22 下午9:50, Igor Mammedov wrote:
> On Tue, 19 Nov 2024 18:18:27 +0800
> bibo mao <maobibo@loongson.cn> wrote:
> 
>> On 2024/11/19 上午1:03, Igor Mammedov wrote:
>>> On Tue, 12 Nov 2024 10:17:38 +0800
>>> Bibo Mao <maobibo@loongson.cn> wrote:
>>>    
>>>> On virt machine, enable CPU hotplug feature has_hotpluggable_cpus. For
>>>> hot-added CPUs, there is socket-id/core-id/thread-id property set,
>>>> arch_id can be caculated from these properties. So that cpu slot can be
>>>> searched from its arch_id.
>>>>
>>>> Also change num-cpu property of extioi and ipi from smp.cpus to
>>>> smp.max_cpus
>>>>
>>>> Co-developed-by: Xianglai Li <lixianglai@loongson.cn>
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>>> ---
>>>>    hw/loongarch/virt.c | 68 +++++++++++++++++++++++++++++++++++++++------
>>>>    1 file changed, 59 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>>>> index b49b15c0f6..5f81673368 100644
>>>> --- a/hw/loongarch/virt.c
>>>> +++ b/hw/loongarch/virt.c
>>>> @@ -890,7 +890,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
>>>>    
>>>>        /* Create IPI device */
>>>>        ipi = qdev_new(TYPE_LOONGARCH_IPI);
>>>> -    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.cpus);
>>>> +    qdev_prop_set_uint32(ipi, "num-cpu", ms->smp.max_cpus);
>>>>        sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), &error_fatal);
>>>>        lvms->ipi = ipi;
>>>>    
>>>> @@ -905,7 +905,7 @@ static void virt_irq_init(LoongArchVirtMachineState *lvms)
>>>>    
>>>>        /* Create EXTIOI device */
>>>>        extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
>>>> -    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.cpus);
>>>> +    qdev_prop_set_uint32(extioi, "num-cpu", ms->smp.max_cpus);
>>>>        if (virt_is_veiointc_enabled(lvms)) {
>>>>            qdev_prop_set_bit(extioi, "has-virtualization-extension", true);
>>>>        }
>>>> @@ -1369,11 +1369,15 @@ static void virt_get_topo_from_index(MachineState *ms,
>>>>    }
>>>>    
>>>>    /* Find cpu slot in machine->possible_cpus by arch_id */
>>>> -static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id)
>>>> +static CPUArchId *virt_find_cpu_slot(MachineState *ms, int arch_id, int *index)
>>>>    {
>>>>        int n;
>>>>        for (n = 0; n < ms->possible_cpus->len; n++) {
>>>>            if (ms->possible_cpus->cpus[n].arch_id == arch_id) {
>>>> +            if (index) {
>>>> +                *index = n;
>>>> +            }
>>>> +
>>>>                return &ms->possible_cpus->cpus[n];
>>>>            }
>>>>        }
>>>> @@ -1386,10 +1390,12 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>>>    {
>>>>        LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>>>>        MachineState *ms = MACHINE(OBJECT(hotplug_dev));
>>>> +    CPUState *cs = CPU(dev);
>>>>        LoongArchCPU *cpu = LOONGARCH_CPU(dev);
>>>>        CPUArchId *cpu_slot;
>>>>        Error *local_err = NULL;
>>>> -    int arch_id;
>>>> +    LoongArchCPUTopo topo;
>>>> +    int arch_id, index;
>>>>    
>>>>        /* sanity check the cpu */
>>>>        if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
>>>> @@ -1408,12 +1414,45 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>>>         }
>>>>    
>>>>        if (cpu->phy_id == UNSET_PHY_ID) {
>>>    
>>>> -        error_setg(&local_err, "CPU hotplug not supported");
>>>> -        goto out;
>>>> +        if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
>>>> +            error_setg(&local_err,
>>>> +                       "Invalid thread-id %u specified, must be in range 1:%u",
>>>> +                       cpu->thread_id, ms->smp.threads - 1);
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
>>>> +            error_setg(&local_err,
>>>> +                       "Invalid core-id %u specified, must be in range 1:%u",
>>>> +                       cpu->core_id, ms->smp.cores - 1);
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
>>>> +            error_setg(&local_err,
>>>> +                       "Invalid socket-id %u specified, must be in range 1:%u",
>>>> +                       cpu->socket_id, ms->smp.sockets - 1);
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        topo.socket_id = cpu->socket_id;
>>>> +        topo.core_id = cpu->core_id;
>>>> +        topo.thread_id = cpu->thread_id;
>>>> +        arch_id =  virt_get_arch_id_from_topo(ms, &topo);
>>>> +        cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
>>>> +        if (CPU(cpu_slot->cpu)) {
>>>> +            error_setg(&local_err,
>>>> +                       "cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
>>>> +                       cs->cpu_index, cpu->socket_id, cpu->core_id,
>>>> +                       cpu->thread_id, cpu_slot->arch_id);
>>>> +            goto out;
>>>> +        }
>>>> +        cpu->phy_id = arch_id;
>>>> +        cs->cpu_index = index;
>>> this whole branch applies to cold-plugged CPUs as well, especially
>>> if both (hot/cold plugged CPUs are getting wired with help of pre_plug)
>>> So this hunk should be introduced somewhere earlier in series,
>>> and than I'd likely won't need (cpu->phy_id == UNSET_PHY_ID) check to begin with.
>>>
>>> the only difference vs cold-plug would be need to call acpi_ged plug handler,
>>> like you are dong below in virt_cpu_plug
>> Sure, will check acpi_ged plug handler for cold-plug/hot-plug CPU.
>>
>>>    
>>>>        } else {
>>>>            /* For cold-add cpu, find cpu slot from arch_id */
>>>>            arch_id = cpu->phy_id;
>>>> -        cpu_slot = virt_find_cpu_slot(ms, arch_id);
>>>> +        cpu_slot = virt_find_cpu_slot(ms, arch_id, NULL);
>>>>        }
>>>>    
>>>>        numa_cpu_pre_plug(cpu_slot, dev, &local_err);
>>>> @@ -1468,7 +1507,7 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
>>>>            return;
>>>>        }
>>>>    
>>>> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>>>> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
>>>>        cpu_slot->cpu = NULL;
>>>>        return;
>>>>    }
>>>> @@ -1477,14 +1516,24 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>>>                                    DeviceState *dev, Error **errp)
>>>>    {
>>>>        CPUArchId *cpu_slot;
>>>> +    Error *local_err = NULL;
>>>>        LoongArchCPU *cpu = LOONGARCH_CPU(dev);
>>>>        MachineState *ms = MACHINE(hotplug_dev);
>>>>        LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>>>>    
>>>>        /* Connect irq to cpu, including ipi and extioi irqchip */
>>>>        virt_init_cpu_irq(ms, CPU(cpu));
>>>> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>>>> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id, NULL);
>>>>        cpu_slot->cpu = CPU(dev);
>>>> +
>>>> +    if (lvms->acpi_ged) {
>>> Why do you need check, can machine be created without acpi_ged?
>> There is no NULL check with macro HOTPLUG_HANDLER() for cold-plug cpu.
>> Now machine is created with acpi_ged always, in later will add noapic
>> option support.
> 
> you've probably meant '-noacpi',
> anyways right now acpi_ged is always present, so make this patch unconditional.
Sure, will do in this way.

Regards
Bibo Mao
> 
> If later on you find a use-case for '-noacpi' and add it,
> then introduce condition at that time.
> 
>>
>> Regards
>> Bibo Mao
>>
>>>    
>>>> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &local_err);
>>>> +        if (local_err) {
>>>> +            error_propagate(errp, local_err);
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>>        return;
>>>>    }
>>>>    
>>>> @@ -1667,6 +1716,7 @@ static void virt_class_init(ObjectClass *oc, void *data)
>>>>        mc->numa_mem_supported = true;
>>>>        mc->auto_enable_numa_with_memhp = true;
>>>>        mc->auto_enable_numa_with_memdev = true;
>>>> +    mc->has_hotpluggable_cpus = true;
>>>>        mc->get_hotplug_handler = virt_get_hotplug_handler;
>>>>        mc->default_nic = "virtio-net-pci";
>>>>        hc->plug = virt_device_plug_cb;
>>

Re: [PATCH v4 0/6] hw/loongarch/virt: Add cpu hotplug support

[lixianglai]

[-- Attachment #1: Type: text/plain, Size: 3998 bytes --]

Hello everyone, I have a question about cpu hotplug to consult you.

When I start qemu with the following parameters:
/usr/bin/qemu-system-loongarch64 \
-machine virt  \
-accel tcg \
-bios /usr/share/edk2/loongarch64/QEMU_EFI.fd \
-m size=1048576k \
-smp 1,maxcpus=4,cores=1,threads=1,sockets=4  \
-nographic \
-monitor telnet:localhost:4444,server,nowait \
-incoming tcp:0:6666 \
-serial stdio

The virtual machine is not running directly and is in the migration state,
At this point I insert a cpu using the following command:
telnet 127.0.0.1 4444
(qemu)  device_add 
la464-loongarch-cpu,socket-id=1,core-id=0,thread-id=0,id=cpu-1

I found that the ged device sends an interrupt signal to the interrupt 
controller,
My understanding is that the current machine is not in the running state,
whether the ged device should send interrupt signal in this state?

The "current_run_state" is  RUN_STATE_INMIGRATE,
And The "machine_phase" is PHASE_MACHINE_READY in qemu.

So do we need to add a conditional on current_run_state to the 
acpi_cpu_plug_cb function?
For example:

@@ -258,7 +258,8 @@ void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
      }

      cdev->cpu = CPU(dev);
-    if (dev->hotplugged) {
+    if (dev->hotplugged &&
+        runstate_check(RUN_STATE_RUNNING)) {
          cdev->is_inserting = true;
          acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
      }


> LoongArch cpu hotplug is based on ACPI GED device, there is a little
> change about ipi and extioi device, the value of num-cpu property is
> maximum cpu number rather than present cpu number.
>
> It can be verified with qemu command:
>    qemu-system-loongarch64 -smp 2,maxcpus=16,sockets=4,cores=4,threads=1
> and vcpu can be added or remove with hmp command:
>    device_add la464-loongarch-cpu,socket-id=0,core-id=2,thread-id=0,id=cpu-2
>    device_del cpu-2
>
> ---
> v3 ... v4:
>    1. For cold-plug CPUs, move socket-id/core-id/thread-id property
>       setting from preplug function to CPU object creating loop, since
>       there is topo information calculation already in CPU object creating
>       loop.
>    2. Init interrupt pin of CPU object in cpu plug interface for both
>       cold-plug CPUs and hot-plug CPUs.
>    3. Apply the patch based on latest qemu version.
>
> v2 ... v3:
>    1. Use qdev_realize_and_unref() with qdev_realize() and object_unref().
>    2. Set vcpus_count with 1 since vcpu object is created for every thread.
>    3. Remove property hw-id, use internal variable hw_id to differentiate
>       cold-plug cpus and hot-plug cpus.
>    4. Add generic function virt_init_cpu_irq() to init interrupt pin
>       of CPU object, used by both cold-plug and hot-plug CPUs
>
> v1 ... v2:
>    1. Add new property hw-id, property hw-id is set for cold-added CPUs,
>       and property socket-id/core-id/thread-id is set for hot-added CPUs.
>       The two properties can be generated from each other.
>    2. Use general hotplug api such as hotplug_handler_pre_plug etc
>    3. Reorganize the patch order, split the patch set into 4 small
>       patches.
> ---
> Bibo Mao (6):
>    hw/loongarch/virt: Add CPU topology support
>    hw/loongarch/virt: Implement cpu plug interface
>    hw/loongarch/virt: Add generic function to init interrupt pin of CPU
>    hw/loongarch/virt: Init interrupt pin of CPU during plug interface
>    hw/loongarch/virt: Update the ACPI table for hotplug cpu
>    hw/loongarch/virt: Enable cpu hotplug feature on virt machine
>
>   docs/system/loongarch/virt.rst |  31 +++
>   hw/loongarch/Kconfig           |   1 +
>   hw/loongarch/acpi-build.c      |  35 ++-
>   hw/loongarch/virt.c            | 374 ++++++++++++++++++++++++++++-----
>   include/hw/loongarch/virt.h    |   3 +
>   target/loongarch/cpu.c         |  25 +++
>   target/loongarch/cpu.h         |  17 ++
>   7 files changed, 428 insertions(+), 58 deletions(-)
>
>
> base-commit: 134b443512825bed401b6e141447b8cdc22d2efe


[-- Attachment #2: Type: text/html, Size: 5199 bytes --]