Add topological relationship for Loongarch VCPU and initialize
topology member variables, the topo information includes socket-id,
core-id and thread-id. And its physical cpu id is calculated from
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            | 74 ++++++++++++++++++++++++++--------
 target/loongarch/cpu.c         | 12 ++++++
 target/loongarch/cpu.h         | 17 ++++++++
 4 files changed, 118 insertions(+), 16 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..e138dac510 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1137,7 +1137,6 @@ 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);
@@ -1145,7 +1144,7 @@ static void virt_init(MachineState *machine)
     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");
@@ -1164,12 +1163,25 @@ static void virt_init(MachineState *machine)
 
     /* Init CPUs */
     possible_cpus = mc->possible_cpu_arch_ids(machine);
-    for (i = 0; i < possible_cpus->len; i++) {
-        cpu = cpu_create(machine->cpu_type);
-        cpu->cpu_index = i;
-        machine->possible_cpus->cpus[i].cpu = cpu;
-        lacpu = LOONGARCH_CPU(cpu);
-        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
+    for (i = 0; i < machine->smp.cpus; i++) {
+        cpuobj = object_new(machine->cpu_type);
+        object_property_set_uint(cpuobj, "phy-id",
+                                possible_cpus->cpus[i].arch_id, NULL);
+        /*
+         * The CPU in place at the time of machine startup will also enter
+         * the CPU hot-plug process when it is created, but at this time,
+         * the GED device has not been created, resulting in exit in the CPU
+         * hot-plug process, which can avoid the incumbent CPU repeatedly
+         * applying for resources.
+         *
+         * The interrupt resource of the in-place CPU will be requested at
+         * the current function call virt_irq_init().
+         *
+         * The interrupt resource of the subsequently inserted CPU will be
+         * requested in the CPU hot-plug process.
+         */
+        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
+        object_unref(cpuobj);
     }
     fdt_add_cpu_nodes(lvms);
     fdt_add_memory_nodes(machine);
@@ -1286,6 +1298,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 +1426,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 +1439,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 = ms->smp.threads;
         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 7212fb5f8f..5dfc0d5c43 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"
@@ -780,6 +781,15 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
 }
 #endif
 
+static Property loongarch_cpu_properties[] = {
+    DEFINE_PROP_UINT32("phy-id", LoongArchCPU, phy_id, UNSET_PHY_ID),
+    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);
@@ -787,6 +797,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,
@@ -811,6 +822,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 6c41fafb70..fb50cbbeee 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
@@ -369,6 +375,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
@@ -381,6 +393,10 @@ struct ArchCPU {
     CPULoongArchState env;
     QEMUTimer timer;
     uint32_t  phy_id;
+    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;
@@ -399,6 +415,7 @@ struct LoongArchCPUClass {
     CPUClass parent_class;
 
     DeviceRealize parent_realize;
+    DeviceUnrealize parent_unrealize;
     ResettablePhases parent_phases;
 };
 
-- 
2.39.3

Hi Bibo,

[snip]

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

What's the difference between arch_id and CPU index (CPUState.cpu_index)?
  
>  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);
> @@ -1145,7 +1144,7 @@ static void virt_init(MachineState *machine)
>      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");
> @@ -1164,12 +1163,25 @@ static void virt_init(MachineState *machine)
>  
>      /* Init CPUs */
>      possible_cpus = mc->possible_cpu_arch_ids(machine);
> -    for (i = 0; i < possible_cpus->len; i++) {
> -        cpu = cpu_create(machine->cpu_type);
> -        cpu->cpu_index = i;
> -        machine->possible_cpus->cpus[i].cpu = cpu;
> -        lacpu = LOONGARCH_CPU(cpu);
> -        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
> +    for (i = 0; i < machine->smp.cpus; i++) {
> +        cpuobj = object_new(machine->cpu_type);
> +        object_property_set_uint(cpuobj, "phy-id",
> +                                possible_cpus->cpus[i].arch_id, NULL);
> +        /*
> +         * The CPU in place at the time of machine startup will also enter
> +         * the CPU hot-plug process when it is created, but at this time,
> +         * the GED device has not been created, resulting in exit in the CPU
> +         * hot-plug process, which can avoid the incumbent CPU repeatedly
> +         * applying for resources.
> +         *
> +         * The interrupt resource of the in-place CPU will be requested at
> +         * the current function call virt_irq_init().
> +         *
> +         * The interrupt resource of the subsequently inserted CPU will be
> +         * requested in the CPU hot-plug process.
> +         */
> +        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> +        object_unref(cpuobj);

You can use qdev_realize_and_unref().

>      }
>      fdt_add_cpu_nodes(lvms);
>      fdt_add_memory_nodes(machine);
> @@ -1286,6 +1298,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;

Looking at the calculations, arch_id looks the same as cpu_index, right?

> +    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 +1426,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 +1439,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 = ms->smp.threads;

In include/hw/boards.h, the doc of CPUArchId said:

vcpus_count - number of threads provided by @cpu object

And I undersatnd each element in possible_cpus->cpus[] is mapped to a
CPU object, so that here vcpus_count should be 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 7212fb5f8f..5dfc0d5c43 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"
> @@ -780,6 +781,15 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
>  }
>  #endif
>  
> +static Property loongarch_cpu_properties[] = {
> +    DEFINE_PROP_UINT32("phy-id", LoongArchCPU, phy_id, UNSET_PHY_ID),

IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
derived from topology, so why do you need to define it as a property and then
expose it to the user?

Thanks,
Zhao

On Tue, 29 Oct 2024 21:19:15 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> Hi Bibo,
> 
> [snip]
> 
> > +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)``  
> 
> What's the difference between arch_id and CPU index (CPUState.cpu_index)?

They might be the same but not necessarily.
arch_id is unique cpu identifier from architecture point of view
(which easily could be sparse and without specific order).
(ex: for x86 it's apic_id, for spapr it's core_id)

while cpu_index is internal QEMU, that existed before possible_cpus[]
and non-sparse range and depends on order of cpus are created.
For machines that support possible_cpus[], we override default
cpu_index assignment to fit possible_cpus[].

It might be nice to get rid of cpu_index in favor of possible_cpus[],
but that would be a lot work probably with no huge benefit
when it comes majority of machines that don't need variable
cpu count. 

>   
> >  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);
> > @@ -1145,7 +1144,7 @@ static void virt_init(MachineState *machine)
> >      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");
> > @@ -1164,12 +1163,25 @@ static void virt_init(MachineState *machine)
> >  
> >      /* Init CPUs */
> >      possible_cpus = mc->possible_cpu_arch_ids(machine);
> > -    for (i = 0; i < possible_cpus->len; i++) {
> > -        cpu = cpu_create(machine->cpu_type);
> > -        cpu->cpu_index = i;
> > -        machine->possible_cpus->cpus[i].cpu = cpu;
> > -        lacpu = LOONGARCH_CPU(cpu);
> > -        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
> > +    for (i = 0; i < machine->smp.cpus; i++) {
> > +        cpuobj = object_new(machine->cpu_type);
> > +        object_property_set_uint(cpuobj, "phy-id",
> > +                                possible_cpus->cpus[i].arch_id, NULL);
> > +        /*
> > +         * The CPU in place at the time of machine startup will also enter
> > +         * the CPU hot-plug process when it is created, but at this time,
> > +         * the GED device has not been created, resulting in exit in the CPU
> > +         * hot-plug process, which can avoid the incumbent CPU repeatedly
> > +         * applying for resources.
> > +         *
> > +         * The interrupt resource of the in-place CPU will be requested at
> > +         * the current function call virt_irq_init().
> > +         *
> > +         * The interrupt resource of the subsequently inserted CPU will be
> > +         * requested in the CPU hot-plug process.
> > +         */
> > +        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > +        object_unref(cpuobj);  
> 
> You can use qdev_realize_and_unref().
> 
> >      }
> >      fdt_add_cpu_nodes(lvms);
> >      fdt_add_memory_nodes(machine);
> > @@ -1286,6 +1298,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;  
> 
> Looking at the calculations, arch_id looks the same as cpu_index, right?
> 
> > +    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 +1426,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 +1439,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 = ms->smp.threads;  
> 
> In include/hw/boards.h, the doc of CPUArchId said:
> 
> vcpus_count - number of threads provided by @cpu object
> 
> And I undersatnd each element in possible_cpus->cpus[] is mapped to a
> CPU object, so that here vcpus_count should be 1.

it's arch specific, CPU object in possible_cpus was meant to point to
thread/core/..higher layers in future../

For example spapr put's there CPUCore, where vcpus_count can be > 1

That is a bit broken though, since CPUCore is not CPUState by any means,
spapr_core_plug() gets away with it only because
  core_slot->cpu = CPU(dev)
CPU() is dumb pointer cast.

Ideally CPUArchId::cpu should be (Object*) to accommodate various
levels of granularity correctly (with dumb cast above it's just
cosmetics though as long as we treat it as Object in non arch
specific code).

> >          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 7212fb5f8f..5dfc0d5c43 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"
> > @@ -780,6 +781,15 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
> >  }
> >  #endif
> >  
> > +static Property loongarch_cpu_properties[] = {
> > +    DEFINE_PROP_UINT32("phy-id", LoongArchCPU, phy_id, UNSET_PHY_ID),  
> 
> IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
> derived from topology, so why do you need to define it as a property and then
> expose it to the user?

for x86 we do expose apic_id as a property as well, partly from historical pov
but also it's better to access cpu fields via properties from outside of
CPU object than directly poke inside of CPU structure from outside of CPU
(especially if it comes to generic code)

> 
> Thanks,
> Zhao
> 
>

Hi Igor,

> > What's the difference between arch_id and CPU index (CPUState.cpu_index)?
> 
> They might be the same but not necessarily.
> arch_id is unique cpu identifier from architecture point of view
> (which easily could be sparse and without specific order).
> (ex: for x86 it's apic_id, for spapr it's core_id)

Yes, I was previously puzzled as to why the core_id of spapr is global,
which is completely different from the meaning of core_id in x86. Now,
your analogy has made it very clear to me. Thanks!

> while cpu_index is internal QEMU, that existed before possible_cpus[]
> and non-sparse range and depends on order of cpus are created.
> For machines that support possible_cpus[], we override default
> cpu_index assignment to fit possible_cpus[].
> 
> It might be nice to get rid of cpu_index in favor of possible_cpus[],
> but that would be a lot work probably with no huge benefit
> when it comes majority of machines that don't need variable
> cpu count. 

Thank you! Now I see.

> > In include/hw/boards.h, the doc of CPUArchId said:
> > 
> > vcpus_count - number of threads provided by @cpu object
> > 
> > And I undersatnd each element in possible_cpus->cpus[] is mapped to a
> > CPU object, so that here vcpus_count should be 1.
> 
> it's arch specific, CPU object in possible_cpus was meant to point to
> thread/core/..higher layers in future../
> 
> For example spapr put's there CPUCore, where vcpus_count can be > 1
>
> That is a bit broken though, since CPUCore is not CPUState by any means,
> spapr_core_plug() gets away with it only because
>   core_slot->cpu = CPU(dev)
> CPU() is dumb pointer cast.

Is it also because of architectural reasons that the smallest granularity
supported by spapr can only be the core?

> Ideally CPUArchId::cpu should be (Object*) to accommodate various
> levels of granularity correctly (with dumb cast above it's just
> cosmetics though as long as we treat it as Object in non arch
> specific code).

Thank you. So, I would like to ask, should the elements in possible_cpus
be understood as the smallest granularity supported by hotplug?

I want to understand that this reason is unrelated to the loongarch patch,
instead I mainly want to continue thinking about my previous qom-topo[*]
proposal.

I remember your hotplug slides also mentioned larger granularity hotplug,
which I understand, for example, allows x86 to support core/socket, etc.
(this of course requires core/socket object abstraction).

If possible_cpus[] only needs to correspond to the smallest granularity
topo object, then it matches my qom-topo proposal quite well, essentially
mapping one layer of a complete topology tree (which is built from socket
to thread, layer by layer) to possible_cpus[] (actually, this is my design:
mapping the thread layer of the x86 topology tree to possible_cpus[]. :) )

Although many years have passed, I still believe that larger granularity
hotplug is valuable, especially as hardware includes more and more CPUs.

[*]: https://lore.kernel.org/qemu-devel/20240919015533.766754-1-zhao1.liu@intel.com/

[snip]

> > IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
> > derived from topology, so why do you need to define it as a property and then
> > expose it to the user?
> 
> for x86 we do expose apic_id as a property as well, partly from historical pov
> but also it's better to access cpu fields via properties from outside of
> CPU object than directly poke inside of CPU structure from outside of CPU
> (especially if it comes to generic code)

Thank you for your guidance. Similar to Bibo’s question, I also wonder
if there is the need for a property that won't be exposed to users.

Regards,
Zhao

On Thu, 7 Nov 2024 22:00:17 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> Hi Igor,
> 
> > > What's the difference between arch_id and CPU index (CPUState.cpu_index)?  
> > 
> > They might be the same but not necessarily.
> > arch_id is unique cpu identifier from architecture point of view
> > (which easily could be sparse and without specific order).
> > (ex: for x86 it's apic_id, for spapr it's core_id)  
> 
> Yes, I was previously puzzled as to why the core_id of spapr is global,
> which is completely different from the meaning of core_id in x86. Now,
> your analogy has made it very clear to me. Thanks!
> 
> > while cpu_index is internal QEMU, that existed before possible_cpus[]
> > and non-sparse range and depends on order of cpus are created.
> > For machines that support possible_cpus[], we override default
> > cpu_index assignment to fit possible_cpus[].
> > 
> > It might be nice to get rid of cpu_index in favor of possible_cpus[],
> > but that would be a lot work probably with no huge benefit
> > when it comes majority of machines that don't need variable
> > cpu count.   
> 
> Thank you! Now I see.
> 
> > > In include/hw/boards.h, the doc of CPUArchId said:
> > > 
> > > vcpus_count - number of threads provided by @cpu object
> > > 
> > > And I undersatnd each element in possible_cpus->cpus[] is mapped to a
> > > CPU object, so that here vcpus_count should be 1.  
> > 
> > it's arch specific, CPU object in possible_cpus was meant to point to
> > thread/core/..higher layers in future../
> > 
> > For example spapr put's there CPUCore, where vcpus_count can be > 1
> >
> > That is a bit broken though, since CPUCore is not CPUState by any means,
> > spapr_core_plug() gets away with it only because
> >   core_slot->cpu = CPU(dev)
> > CPU() is dumb pointer cast.  
> 
> Is it also because of architectural reasons that the smallest granularity
> supported by spapr can only be the core?

Yes, I think so if I recall it correctly.

> > Ideally CPUArchId::cpu should be (Object*) to accommodate various
> > levels of granularity correctly (with dumb cast above it's just
> > cosmetics though as long as we treat it as Object in non arch
> > specific code).  
> 
> Thank you. So, I would like to ask, should the elements in possible_cpus
> be understood as the smallest granularity supported by hotplug?

not necessarily, eventually we might want to plug sockets someday,
machine would expose only have_socket in hotpluggable CPUs interface
and hotplugging a CPU would look like (device_add xeon_E5_2630,socket-id=X),
which resembles real hw use-case. And that xeon device would create internally
all necessary vCPUs, and configure internal parameters topo/caches/whatnot.
 
> I want to understand that this reason is unrelated to the loongarch patch,
> instead I mainly want to continue thinking about my previous qom-topo[*]
> proposal.
> 
> I remember your hotplug slides also mentioned larger granularity hotplug,
> which I understand, for example, allows x86 to support core/socket, etc.
> (this of course requires core/socket object abstraction).
> 
> If possible_cpus[] only needs to correspond to the smallest granularity
> topo object, then it matches my qom-topo proposal quite well, essentially
> mapping one layer of a complete topology tree (which is built from socket
> to thread, layer by layer) to possible_cpus[] (actually, this is my design:
> mapping the thread layer of the x86 topology tree to possible_cpus[]. :) )
> 
> Although many years have passed, I still believe that larger granularity
> hotplug is valuable, especially as hardware includes more and more CPUs.

that wasn't the case in the past for virt world, as main goal of it there
is dynamic scalability, while in real hw (in x86 world) it's mainly RAS
feature.

> [*]: https://lore.kernel.org/qemu-devel/20240919015533.766754-1-zhao1.liu@intel.com/
It looks to me as too complicated approach, I'll comment there.

> 
> [snip]
> 
> > > IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
> > > derived from topology, so why do you need to define it as a property and then
> > > expose it to the user?  
> > 
> > for x86 we do expose apic_id as a property as well, partly from historical pov
> > but also it's better to access cpu fields via properties from outside of
> > CPU object than directly poke inside of CPU structure from outside of CPU
> > (especially if it comes to generic code)  
> 
> Thank you for your guidance. Similar to Bibo’s question, I also wonder
> if there is the need for a property that won't be exposed to users.
> 
> Regards,
> Zhao
> 

On 2024/11/6 下午6:41, Igor Mammedov wrote:
> On Tue, 29 Oct 2024 21:19:15 +0800
> Zhao Liu <zhao1.liu@intel.com> wrote:
> 
>> Hi Bibo,
>>
>> [snip]
>>
>>> +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)``
>>
>> What's the difference between arch_id and CPU index (CPUState.cpu_index)?
> 
> They might be the same but not necessarily.
> arch_id is unique cpu identifier from architecture point of view
> (which easily could be sparse and without specific order).
> (ex: for x86 it's apic_id, for spapr it's core_id)
> 
> while cpu_index is internal QEMU, that existed before possible_cpus[]
> and non-sparse range and depends on order of cpus are created.
> For machines that support possible_cpus[], we override default
> cpu_index assignment to fit possible_cpus[].
> 
> It might be nice to get rid of cpu_index in favor of possible_cpus[],
> but that would be a lot work probably with no huge benefit
> when it comes majority of machines that don't need variable
> cpu count.
> 
>>    
>>>   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);
>>> @@ -1145,7 +1144,7 @@ static void virt_init(MachineState *machine)
>>>       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");
>>> @@ -1164,12 +1163,25 @@ static void virt_init(MachineState *machine)
>>>   
>>>       /* Init CPUs */
>>>       possible_cpus = mc->possible_cpu_arch_ids(machine);
>>> -    for (i = 0; i < possible_cpus->len; i++) {
>>> -        cpu = cpu_create(machine->cpu_type);
>>> -        cpu->cpu_index = i;
>>> -        machine->possible_cpus->cpus[i].cpu = cpu;
>>> -        lacpu = LOONGARCH_CPU(cpu);
>>> -        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
>>> +    for (i = 0; i < machine->smp.cpus; i++) {
>>> +        cpuobj = object_new(machine->cpu_type);
>>> +        object_property_set_uint(cpuobj, "phy-id",
>>> +                                possible_cpus->cpus[i].arch_id, NULL);
>>> +        /*
>>> +         * The CPU in place at the time of machine startup will also enter
>>> +         * the CPU hot-plug process when it is created, but at this time,
>>> +         * the GED device has not been created, resulting in exit in the CPU
>>> +         * hot-plug process, which can avoid the incumbent CPU repeatedly
>>> +         * applying for resources.
>>> +         *
>>> +         * The interrupt resource of the in-place CPU will be requested at
>>> +         * the current function call virt_irq_init().
>>> +         *
>>> +         * The interrupt resource of the subsequently inserted CPU will be
>>> +         * requested in the CPU hot-plug process.
>>> +         */
>>> +        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
>>> +        object_unref(cpuobj);
>>
>> You can use qdev_realize_and_unref().
>>
>>>       }
>>>       fdt_add_cpu_nodes(lvms);
>>>       fdt_add_memory_nodes(machine);
>>> @@ -1286,6 +1298,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;
>>
>> Looking at the calculations, arch_id looks the same as cpu_index, right?
>>
>>> +    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 +1426,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 +1439,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 = ms->smp.threads;
>>
>> In include/hw/boards.h, the doc of CPUArchId said:
>>
>> vcpus_count - number of threads provided by @cpu object
>>
>> And I undersatnd each element in possible_cpus->cpus[] is mapped to a
>> CPU object, so that here vcpus_count should be 1.
> 
> it's arch specific, CPU object in possible_cpus was meant to point to
> thread/core/..higher layers in future../
> 
> For example spapr put's there CPUCore, where vcpus_count can be > 1
> 
> That is a bit broken though, since CPUCore is not CPUState by any means,
> spapr_core_plug() gets away with it only because
>    core_slot->cpu = CPU(dev)
> CPU() is dumb pointer cast.
> 
> Ideally CPUArchId::cpu should be (Object*) to accommodate various
> levels of granularity correctly (with dumb cast above it's just
> cosmetics though as long as we treat it as Object in non arch
> specific code).
> 
>>>           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 7212fb5f8f..5dfc0d5c43 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"
>>> @@ -780,6 +781,15 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
>>>   }
>>>   #endif
>>>   
>>> +static Property loongarch_cpu_properties[] = {
>>> +    DEFINE_PROP_UINT32("phy-id", LoongArchCPU, phy_id, UNSET_PHY_ID),
>>
>> IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
>> derived from topology, so why do you need to define it as a property and then
>> expose it to the user?
> 
> for x86 we do expose apic_id as a property as well, partly from historical pov
> but also it's better to access cpu fields via properties from outside of
> CPU object than directly poke inside of CPU structure from outside of CPU
> (especially if it comes to generic code)
yes, accessing fields via properties is better than directly poking 
internal structure elements. Is there internal property for cpu object? 
so that internal property is invisible for users.

There will be problem if user adds CPU object with apic-id property, for 
example:

qemu-system-x86_64 -display none -no-user-config -m 2048 -nodefaults 
-monitor stdio -machine pc,accel=kvm,usb=off -smp 1,maxcpus=2 -cpu 
IvyBridge-IBRS -qmp unix:/tmp/qmp-sock,server=on,wait=off

(qemu) device_add 
id=cpu-2,driver=IvyBridge-IBRS-x86_64-cpu,socket-id=0,core-id=1,thread-id=0,apic-id=100
Error: Invalid CPU [socket: 50, die: 0, module: 0, core: 0, thread: 0] 
with APIC ID 100, valid index range 0:1

(qemu) device_add 
id=cpu-2,driver=IvyBridge-IBRS-x86_64-cpu,socket-id=0,core-id=1,thread-id=0,apic-id=0
Error: CPU[0] with APIC ID 0 exists


Regards
Bibo Mao
> 
>>
>> Thanks,
>> Zhao
>>
>>

On Thu, 7 Nov 2024 10:13:38 +0800
maobibo <maobibo@loongson.cn> wrote:

> On 2024/11/6 下午6:41, Igor Mammedov wrote:
> > On Tue, 29 Oct 2024 21:19:15 +0800
> > Zhao Liu <zhao1.liu@intel.com> wrote:
> >   
> >> Hi Bibo,
> >>
> >> [snip]
> >>  
> >>> +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)``  
> >>
> >> What's the difference between arch_id and CPU index (CPUState.cpu_index)?  
> > 
> > They might be the same but not necessarily.
> > arch_id is unique cpu identifier from architecture point of view
> > (which easily could be sparse and without specific order).
> > (ex: for x86 it's apic_id, for spapr it's core_id)
> > 
> > while cpu_index is internal QEMU, that existed before possible_cpus[]
> > and non-sparse range and depends on order of cpus are created.
> > For machines that support possible_cpus[], we override default
> > cpu_index assignment to fit possible_cpus[].
> > 
> > It might be nice to get rid of cpu_index in favor of possible_cpus[],
> > but that would be a lot work probably with no huge benefit
> > when it comes majority of machines that don't need variable
> > cpu count.
> >   
> >>      
> >>>   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);
> >>> @@ -1145,7 +1144,7 @@ static void virt_init(MachineState *machine)
> >>>       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");
> >>> @@ -1164,12 +1163,25 @@ static void virt_init(MachineState *machine)
> >>>   
> >>>       /* Init CPUs */
> >>>       possible_cpus = mc->possible_cpu_arch_ids(machine);
> >>> -    for (i = 0; i < possible_cpus->len; i++) {
> >>> -        cpu = cpu_create(machine->cpu_type);
> >>> -        cpu->cpu_index = i;
> >>> -        machine->possible_cpus->cpus[i].cpu = cpu;
> >>> -        lacpu = LOONGARCH_CPU(cpu);
> >>> -        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
> >>> +    for (i = 0; i < machine->smp.cpus; i++) {
> >>> +        cpuobj = object_new(machine->cpu_type);
> >>> +        object_property_set_uint(cpuobj, "phy-id",
> >>> +                                possible_cpus->cpus[i].arch_id, NULL);
> >>> +        /*
> >>> +         * The CPU in place at the time of machine startup will also enter
> >>> +         * the CPU hot-plug process when it is created, but at this time,
> >>> +         * the GED device has not been created, resulting in exit in the CPU
> >>> +         * hot-plug process, which can avoid the incumbent CPU repeatedly
> >>> +         * applying for resources.
> >>> +         *
> >>> +         * The interrupt resource of the in-place CPU will be requested at
> >>> +         * the current function call virt_irq_init().
> >>> +         *
> >>> +         * The interrupt resource of the subsequently inserted CPU will be
> >>> +         * requested in the CPU hot-plug process.
> >>> +         */
> >>> +        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> >>> +        object_unref(cpuobj);  
> >>
> >> You can use qdev_realize_and_unref().
> >>  
> >>>       }
> >>>       fdt_add_cpu_nodes(lvms);
> >>>       fdt_add_memory_nodes(machine);
> >>> @@ -1286,6 +1298,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;  
> >>
> >> Looking at the calculations, arch_id looks the same as cpu_index, right?
> >>  
> >>> +    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 +1426,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 +1439,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 = ms->smp.threads;  
> >>
> >> In include/hw/boards.h, the doc of CPUArchId said:
> >>
> >> vcpus_count - number of threads provided by @cpu object
> >>
> >> And I undersatnd each element in possible_cpus->cpus[] is mapped to a
> >> CPU object, so that here vcpus_count should be 1.  
> > 
> > it's arch specific, CPU object in possible_cpus was meant to point to
> > thread/core/..higher layers in future../
> > 
> > For example spapr put's there CPUCore, where vcpus_count can be > 1
> > 
> > That is a bit broken though, since CPUCore is not CPUState by any means,
> > spapr_core_plug() gets away with it only because
> >    core_slot->cpu = CPU(dev)
> > CPU() is dumb pointer cast.
> > 
> > Ideally CPUArchId::cpu should be (Object*) to accommodate various
> > levels of granularity correctly (with dumb cast above it's just
> > cosmetics though as long as we treat it as Object in non arch
> > specific code).
> >   
> >>>           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 7212fb5f8f..5dfc0d5c43 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"
> >>> @@ -780,6 +781,15 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
> >>>   }
> >>>   #endif
> >>>   
> >>> +static Property loongarch_cpu_properties[] = {
> >>> +    DEFINE_PROP_UINT32("phy-id", LoongArchCPU, phy_id, UNSET_PHY_ID),  
> >>
> >> IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
> >> derived from topology, so why do you need to define it as a property and then
> >> expose it to the user?  
> > 
> > for x86 we do expose apic_id as a property as well, partly from historical pov
> > but also it's better to access cpu fields via properties from outside of
> > CPU object than directly poke inside of CPU structure from outside of CPU
> > (especially if it comes to generic code)  
> yes, accessing fields via properties is better than directly poking 
> internal structure elements. Is there internal property for cpu object? 
> so that internal property is invisible for users.

not that I'm aware of. usually we add prefix 'x-' to a property that is
'experimental/not stable/shouldn't be used by endusers' 

extending properties API to mark/create internal properties,
would be nice to have but I won't ask you to do that as it's very
much off topic.

> There will be problem if user adds CPU object with apic-id property, for 
> example:

apic-id is there for historical reasons. that's why x86 has a bunch of checks
to make sure input is correct.

> qemu-system-x86_64 -display none -no-user-config -m 2048 -nodefaults 
> -monitor stdio -machine pc,accel=kvm,usb=off -smp 1,maxcpus=2 -cpu 
> IvyBridge-IBRS -qmp unix:/tmp/qmp-sock,server=on,wait=off
> 
> (qemu) device_add 
> id=cpu-2,driver=IvyBridge-IBRS-x86_64-cpu,socket-id=0,core-id=1,thread-id=0,apic-id=100
> Error: Invalid CPU [socket: 50, die: 0, module: 0, core: 0, thread: 0] 
> with APIC ID 100, valid index range 0:1
> 
> (qemu) device_add 
> id=cpu-2,driver=IvyBridge-IBRS-x86_64-cpu,socket-id=0,core-id=1,thread-id=0,apic-id=0
> Error: CPU[0] with APIC ID 0 exists
> 
> 
> Regards
> Bibo Mao
> >   
> >>
> >> Thanks,
> >> Zhao
> >>
> >>  
> 

Hi Zhao,

Thanks for reviewing the patch.

On 2024/10/29 下午9:19, Zhao Liu wrote:
> Hi Bibo,
> 
> [snip]
> 
>> +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)``
> 
> What's the difference between arch_id and CPU index (CPUState.cpu_index)?
>    
>>   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);
>> @@ -1145,7 +1144,7 @@ static void virt_init(MachineState *machine)
>>       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");
>> @@ -1164,12 +1163,25 @@ static void virt_init(MachineState *machine)
>>   
>>       /* Init CPUs */
>>       possible_cpus = mc->possible_cpu_arch_ids(machine);
>> -    for (i = 0; i < possible_cpus->len; i++) {
>> -        cpu = cpu_create(machine->cpu_type);
>> -        cpu->cpu_index = i;
>> -        machine->possible_cpus->cpus[i].cpu = cpu;
>> -        lacpu = LOONGARCH_CPU(cpu);
>> -        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
>> +    for (i = 0; i < machine->smp.cpus; i++) {
>> +        cpuobj = object_new(machine->cpu_type);
>> +        object_property_set_uint(cpuobj, "phy-id",
>> +                                possible_cpus->cpus[i].arch_id, NULL);
>> +        /*
>> +         * The CPU in place at the time of machine startup will also enter
>> +         * the CPU hot-plug process when it is created, but at this time,
>> +         * the GED device has not been created, resulting in exit in the CPU
>> +         * hot-plug process, which can avoid the incumbent CPU repeatedly
>> +         * applying for resources.
>> +         *
>> +         * The interrupt resource of the in-place CPU will be requested at
>> +         * the current function call virt_irq_init().
>> +         *
>> +         * The interrupt resource of the subsequently inserted CPU will be
>> +         * requested in the CPU hot-plug process.
>> +         */
>> +        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
>> +        object_unref(cpuobj);
> 
> You can use qdev_realize_and_unref().
sure, will do.

> 
>>       }
>>       fdt_add_cpu_nodes(lvms);
>>       fdt_add_memory_nodes(machine);
>> @@ -1286,6 +1298,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;
> 
> Looking at the calculations, arch_id looks the same as cpu_index, right?
The value of arch_id and cpu_index is the same now, however meaning is 
different. cpu_index is cpuslot index of possible_cpus array, arch_id is 
physical id.

Now there is no special HW calculation for physical id, value of them is 
the same. In future if physical id width exceeds 8-bit because extioi 
only support max 256 cpu routing, its calculation method will change.
> 
>> +    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 +1426,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 +1439,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 = ms->smp.threads;
> 
> In include/hw/boards.h, the doc of CPUArchId said:
> 
> vcpus_count - number of threads provided by @cpu object
> 
> And I undersatnd each element in possible_cpus->cpus[] is mapped to a
> CPU object, so that here vcpus_count should be 1.
yes, it is should be 1, thank for pointing it out
> 
> 
>>           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 7212fb5f8f..5dfc0d5c43 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"
>> @@ -780,6 +781,15 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
>>   }
>>   #endif
>>   
>> +static Property loongarch_cpu_properties[] = {
>> +    DEFINE_PROP_UINT32("phy-id", LoongArchCPU, phy_id, UNSET_PHY_ID),
> 
> IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
> derived from topology, so why do you need to define it as a property and then
> expose it to the user?
The property is wrongly used here. we want to differentiate cold-added 
cpus and hot-added cpus.  phy_id of cold-added cpus can be set during 
power on, however that of hot-added cpus is default -1.

Internal variable phy_id can be set with default value -1 in function 
loongarch_cpu_init(), property can be removed.

Regards
Bibo Mao
> 
> Thanks,
> Zhao
> 

On Wed, 30 Oct 2024 09:42:10 +0800
maobibo <maobibo@loongson.cn> wrote:

> Hi Zhao,
> 
> Thanks for reviewing the patch.
> 
> On 2024/10/29 下午9:19, Zhao Liu wrote:
> > Hi Bibo,
> > 
> > [snip]
> >   
> >> +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)``  
> > 
> > What's the difference between arch_id and CPU index (CPUState.cpu_index)?
> >      
> >>   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);
> >> @@ -1145,7 +1144,7 @@ static void virt_init(MachineState *machine)
> >>       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");
> >> @@ -1164,12 +1163,25 @@ static void virt_init(MachineState *machine)
> >>   
> >>       /* Init CPUs */
> >>       possible_cpus = mc->possible_cpu_arch_ids(machine);
> >> -    for (i = 0; i < possible_cpus->len; i++) {
> >> -        cpu = cpu_create(machine->cpu_type);
> >> -        cpu->cpu_index = i;
> >> -        machine->possible_cpus->cpus[i].cpu = cpu;
> >> -        lacpu = LOONGARCH_CPU(cpu);
> >> -        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
> >> +    for (i = 0; i < machine->smp.cpus; i++) {
> >> +        cpuobj = object_new(machine->cpu_type);
> >> +        object_property_set_uint(cpuobj, "phy-id",
> >> +                                possible_cpus->cpus[i].arch_id, NULL);
> >> +        /*
> >> +         * The CPU in place at the time of machine startup will also enter
> >> +         * the CPU hot-plug process when it is created, but at this time,
> >> +         * the GED device has not been created, resulting in exit in the CPU
> >> +         * hot-plug process, which can avoid the incumbent CPU repeatedly
> >> +         * applying for resources.
> >> +         *
> >> +         * The interrupt resource of the in-place CPU will be requested at
> >> +         * the current function call virt_irq_init().
> >> +         *
> >> +         * The interrupt resource of the subsequently inserted CPU will be
> >> +         * requested in the CPU hot-plug process.
> >> +         */
> >> +        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> >> +        object_unref(cpuobj);  
> > 
> > You can use qdev_realize_and_unref().  
> sure, will do.
> 
> >   
> >>       }
> >>       fdt_add_cpu_nodes(lvms);
> >>       fdt_add_memory_nodes(machine);
> >> @@ -1286,6 +1298,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;  
> > 
> > Looking at the calculations, arch_id looks the same as cpu_index, right?  
> The value of arch_id and cpu_index is the same now, however meaning is 
> different. cpu_index is cpuslot index of possible_cpus array, arch_id is 
> physical id.
> 
> Now there is no special HW calculation for physical id, value of them is 
> the same. In future if physical id width exceeds 8-bit because extioi 
> only support max 256 cpu routing, its calculation method will change.
> >   
> >> +    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 +1426,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 +1439,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 = ms->smp.threads;  
> > 
> > In include/hw/boards.h, the doc of CPUArchId said:
> > 
> > vcpus_count - number of threads provided by @cpu object
> > 
> > And I undersatnd each element in possible_cpus->cpus[] is mapped to a
> > CPU object, so that here vcpus_count should be 1.  
> yes, it is should be 1, thank for pointing it out
> > 
> >   
> >>           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 7212fb5f8f..5dfc0d5c43 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"
> >> @@ -780,6 +781,15 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
> >>   }
> >>   #endif
> >>   
> >> +static Property loongarch_cpu_properties[] = {
> >> +    DEFINE_PROP_UINT32("phy-id", LoongArchCPU, phy_id, UNSET_PHY_ID),  
> > 
> > IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
> > derived from topology, so why do you need to define it as a property and then
> > expose it to the user?  

> The property is wrongly used here. we want to differentiate cold-added 
> cpus and hot-added cpus.  phy_id of cold-added cpus can be set during 
> power on, however that of hot-added cpus is default -1.
why would you need to differentiate them?

> 
> Internal variable phy_id can be set with default value -1 in function 
> loongarch_cpu_init(), property can be removed.
> 
> Regards
> Bibo Mao
> > 
> > Thanks,
> > Zhao
> >   
> 
>