* [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-10-29 9:53 [PATCH v2 0/4] hw/loongarch/virt: Add cpu hotplug support Bibo Mao
@ 2024-10-29 9:53 ` Bibo Mao
2024-10-29 13:19 ` Zhao Liu
2024-10-29 9:53 ` [PATCH v2 2/4] hw/loongarch/virt: Implement cpu plug interface Bibo Mao
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Bibo Mao @ 2024-10-29 9:53 UTC (permalink / raw)
To: Song Gao, Paolo Bonzini; +Cc: Jiaxun Yang, qemu-devel, Xianglai Li
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-10-29 9:53 ` [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support Bibo Mao
@ 2024-10-29 13:19 ` Zhao Liu
2024-10-30 1:42 ` maobibo
2024-11-06 10:41 ` Igor Mammedov
0 siblings, 2 replies; 20+ messages in thread
From: Zhao Liu @ 2024-10-29 13:19 UTC (permalink / raw)
To: Bibo Mao; +Cc: Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel, Xianglai Li
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-10-29 13:19 ` Zhao Liu
@ 2024-10-30 1:42 ` maobibo
2024-11-06 10:42 ` Igor Mammedov
2024-11-06 10:41 ` Igor Mammedov
1 sibling, 1 reply; 20+ messages in thread
From: maobibo @ 2024-10-30 1:42 UTC (permalink / raw)
To: Zhao Liu; +Cc: Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel, Xianglai Li
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-10-30 1:42 ` maobibo
@ 2024-11-06 10:42 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2024-11-06 10:42 UTC (permalink / raw)
To: maobibo
Cc: Zhao Liu, Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel,
Xianglai Li
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
> >
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-10-29 13:19 ` Zhao Liu
2024-10-30 1:42 ` maobibo
@ 2024-11-06 10:41 ` Igor Mammedov
2024-11-07 2:13 ` maobibo
2024-11-07 14:00 ` Zhao Liu
1 sibling, 2 replies; 20+ messages in thread
From: Igor Mammedov @ 2024-11-06 10:41 UTC (permalink / raw)
To: Zhao Liu
Cc: Bibo Mao, Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel,
Xianglai Li
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-11-06 10:41 ` Igor Mammedov
@ 2024-11-07 2:13 ` maobibo
2024-11-18 15:21 ` Igor Mammedov
2024-11-07 14:00 ` Zhao Liu
1 sibling, 1 reply; 20+ messages in thread
From: maobibo @ 2024-11-07 2:13 UTC (permalink / raw)
To: Igor Mammedov, Zhao Liu
Cc: Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel, Xianglai Li
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
>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-11-07 2:13 ` maobibo
@ 2024-11-18 15:21 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2024-11-18 15:21 UTC (permalink / raw)
To: maobibo
Cc: Zhao Liu, Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel,
Xianglai Li
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
> >>
> >>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-11-06 10:41 ` Igor Mammedov
2024-11-07 2:13 ` maobibo
@ 2024-11-07 14:00 ` Zhao Liu
2024-11-22 14:09 ` Igor Mammedov
1 sibling, 1 reply; 20+ messages in thread
From: Zhao Liu @ 2024-11-07 14:00 UTC (permalink / raw)
To: Igor Mammedov
Cc: Bibo Mao, Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel,
Xianglai Li
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support
2024-11-07 14:00 ` Zhao Liu
@ 2024-11-22 14:09 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2024-11-22 14:09 UTC (permalink / raw)
To: Zhao Liu
Cc: Bibo Mao, Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel,
Xianglai Li
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/4] hw/loongarch/virt: Implement cpu plug interface
2024-10-29 9:53 [PATCH v2 0/4] hw/loongarch/virt: Add cpu hotplug support Bibo Mao
2024-10-29 9:53 ` [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support Bibo Mao
@ 2024-10-29 9:53 ` Bibo Mao
2024-10-29 13:37 ` Zhao Liu
2024-10-29 9:53 ` [PATCH v2 3/4] hw/loongarch/virt: Update the ACPI table for hotplug cpu Bibo Mao
2024-10-29 9:53 ` [PATCH v2 4/4] hw/loongarch/virt: Enable cpu hotplug feature on virt machine Bibo Mao
3 siblings, 1 reply; 20+ messages in thread
From: Bibo Mao @ 2024-10-29 9:53 UTC (permalink / raw)
To: Song Gao, Paolo Bonzini; +Cc: Jiaxun Yang, qemu-devel, Xianglai Li
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 | 145 +++++++++++++++++++++++++++++++++++++++++
target/loongarch/cpu.c | 13 ++++
2 files changed, 158 insertions(+)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index e138dac510..28a46bf195 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1327,6 +1327,142 @@ 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 *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];
+ }
+ }
+
+ 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);
+ CPUState *cs = CPU(dev);
+ CPUArchId *cpu_slot;
+ Error *local_err = NULL;
+ LoongArchCPUTopo topo;
+ int arch_id, index = 0;
+
+ /* 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 non hot-add cpu, topo property is not set. And only physical id
+ * property is set, setup topo information from physical id.
+ *
+ * Supposing arch_id is equal to cpu slot index
+ */
+ arch_id = cpu->phy_id;
+ virt_get_topo_from_index(ms, &topo, arch_id);
+ cpu->socket_id = topo.socket_id;
+ cpu->core_id = topo.core_id;
+ cpu->thread_id = topo.thread_id;
+ cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
+ }
+
+ /*
+ * update cpu_index calculation method since it is easily used as index
+ * with possible_cpus array by function virt_cpu_index_to_props
+ */
+ cs->cpu_index = index;
+ 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, NULL);
+ 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, NULL);
+ cpu_slot->cpu = CPU(dev);
+ return;
+}
+
static bool memhp_type_supported(DeviceState *dev)
{
/* we only support pc dimm now */
@@ -1345,6 +1481,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);
}
}
@@ -1363,6 +1501,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);
}
}
@@ -1381,6 +1521,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);
}
}
@@ -1408,6 +1550,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);
}
}
@@ -1417,6 +1561,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 5dfc0d5c43..c31607b89c 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);
@@ -800,6 +811,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);
--
2.39.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/4] hw/loongarch/virt: Implement cpu plug interface
2024-10-29 9:53 ` [PATCH v2 2/4] hw/loongarch/virt: Implement cpu plug interface Bibo Mao
@ 2024-10-29 13:37 ` Zhao Liu
2024-10-30 1:50 ` maobibo
0 siblings, 1 reply; 20+ messages in thread
From: Zhao Liu @ 2024-10-29 13:37 UTC (permalink / raw)
To: Bibo Mao
Cc: Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel, Xianglai Li,
Igor Mammedov
(CC Igor since I want to refer his comment on hotplug design.)
Hi Bibo,
I have some comments about your hotplug design.
[snip]
> +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);
> + CPUState *cs = CPU(dev);
> + CPUArchId *cpu_slot;
> + Error *local_err = NULL;
> + LoongArchCPUTopo topo;
> + int arch_id, index = 0;
[snip]
> + if (cpu->phy_id == UNSET_PHY_ID) {
> + error_setg(&local_err, "CPU hotplug not supported");
> + goto out;
> + } else {
> + /*
> + * For non hot-add cpu, topo property is not set. And only physical id
> + * property is set, setup topo information from physical id.
> + *
> + * Supposing arch_id is equal to cpu slot index
> + */
> + arch_id = cpu->phy_id;
> + virt_get_topo_from_index(ms, &topo, arch_id);
> + cpu->socket_id = topo.socket_id;
> + cpu->core_id = topo.core_id;
> + cpu->thread_id = topo.thread_id;
> + cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
> + }
It seems you want to use "phy_id" (instead of topology IDs as for now)
as the parameter to plug CPU. And IIUC in previous patch, "phy_id" is
essentially the CPU index.
Igor has previously commented [1] on ARM's hotplug design that the
current QEMU should use the topology-id (socket-id/core-id/thread-id) as
the parameters, not the CPU index or the x86-like apic id.
So I think his comment can apply on loongarch, too.
From my own understanding, the CPU index lacks topological intuition,
and the APIC ID for x86 is even worse, as its sometimes even
discontinuous. Requiring the user to specify topology ids would allow
for clearer localization to CPU slots.
[1]: https://lore.kernel.org/qemu-devel/20240812101556.1a395712@imammedo.users.ipa.redhat.com/
> + /*
> + * update cpu_index calculation method since it is easily used as index
> + * with possible_cpus array by function virt_cpu_index_to_props
> + */
> + cs->cpu_index = index;
> + numa_cpu_pre_plug(cpu_slot, dev, &local_err);
> + return ;
> +
> +out:
> + error_propagate(errp, local_err);
> +}
> +
Thanks,
Zhao
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/4] hw/loongarch/virt: Implement cpu plug interface
2024-10-29 13:37 ` Zhao Liu
@ 2024-10-30 1:50 ` maobibo
2024-11-06 10:56 ` Igor Mammedov
0 siblings, 1 reply; 20+ messages in thread
From: maobibo @ 2024-10-30 1:50 UTC (permalink / raw)
To: Zhao Liu
Cc: Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel, Xianglai Li,
Igor Mammedov
Hi Zhao,
On 2024/10/29 下午9:37, Zhao Liu wrote:
> (CC Igor since I want to refer his comment on hotplug design.)
>
> Hi Bibo,
>
> I have some comments about your hotplug design.
>
> [snip]
>
>> +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);
>> + CPUState *cs = CPU(dev);
>> + CPUArchId *cpu_slot;
>> + Error *local_err = NULL;
>> + LoongArchCPUTopo topo;
>> + int arch_id, index = 0;
>
> [snip]
>
>> + if (cpu->phy_id == UNSET_PHY_ID) {
>> + error_setg(&local_err, "CPU hotplug not supported");
>> + goto out;
>> + } else {
>> + /*
>> + * For non hot-add cpu, topo property is not set. And only physical id
>> + * property is set, setup topo information from physical id.
>> + *
>> + * Supposing arch_id is equal to cpu slot index
>> + */
>> + arch_id = cpu->phy_id;
>> + virt_get_topo_from_index(ms, &topo, arch_id);
>> + cpu->socket_id = topo.socket_id;
>> + cpu->core_id = topo.core_id;
>> + cpu->thread_id = topo.thread_id;
>> + cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
>> + }
>
> It seems you want to use "phy_id" (instead of topology IDs as for now)
> as the parameter to plug CPU. And IIUC in previous patch, "phy_id" is
> essentially the CPU index.
>
> Igor has previously commented [1] on ARM's hotplug design that the
> current QEMU should use the topology-id (socket-id/core-id/thread-id) as
> the parameters, not the CPU index or the x86-like apic id.
>
> So I think his comment can apply on loongarch, too.
Yes, I agree. This piece of code is for cold-plug CPUs which is added
during VM power-on stage, not hot-plugged cpu. For hot-plugged cpu,
value of cpu->phy_id is UNSET_PHY_ID.
Topology-id (socket-id/core-id/thread-id) is not set for cold-plug CPUs.
For cold-plug CPUs topology-id is calculated from archid.
Regards
Bibo
>
> From my own understanding, the CPU index lacks topological intuition,
> and the APIC ID for x86 is even worse, as its sometimes even
> discontinuous. Requiring the user to specify topology ids would allow
> for clearer localization to CPU slots.
>
> [1]: https://lore.kernel.org/qemu-devel/20240812101556.1a395712@imammedo.users.ipa.redhat.com/
>
>> + /*
>> + * update cpu_index calculation method since it is easily used as index
>> + * with possible_cpus array by function virt_cpu_index_to_props
>> + */
>> + cs->cpu_index = index;
>> + numa_cpu_pre_plug(cpu_slot, dev, &local_err);
>> + return ;
>> +
>> +out:
>> + error_propagate(errp, local_err);
>> +}
>> +
>
> Thanks,
> Zhao
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/4] hw/loongarch/virt: Implement cpu plug interface
2024-10-30 1:50 ` maobibo
@ 2024-11-06 10:56 ` Igor Mammedov
2024-11-07 2:18 ` maobibo
0 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2024-11-06 10:56 UTC (permalink / raw)
To: maobibo
Cc: Zhao Liu, Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel,
Xianglai Li
On Wed, 30 Oct 2024 09:50:56 +0800
maobibo <maobibo@loongson.cn> wrote:
> Hi Zhao,
>
> On 2024/10/29 下午9:37, Zhao Liu wrote:
> > (CC Igor since I want to refer his comment on hotplug design.)
> >
> > Hi Bibo,
> >
> > I have some comments about your hotplug design.
> >
> > [snip]
> >
> >> +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);
> >> + CPUState *cs = CPU(dev);
> >> + CPUArchId *cpu_slot;
> >> + Error *local_err = NULL;
> >> + LoongArchCPUTopo topo;
> >> + int arch_id, index = 0;
> >
> > [snip]
> >
> >> + if (cpu->phy_id == UNSET_PHY_ID) {
> >> + error_setg(&local_err, "CPU hotplug not supported");
> >> + goto out;
> >> + } else {
> >> + /*
> >> + * For non hot-add cpu, topo property is not set. And only physical id
> >> + * property is set, setup topo information from physical id.
> >> + *
> >> + * Supposing arch_id is equal to cpu slot index
> >> + */
> >> + arch_id = cpu->phy_id;
> >> + virt_get_topo_from_index(ms, &topo, arch_id);
> >> + cpu->socket_id = topo.socket_id;
> >> + cpu->core_id = topo.core_id;
> >> + cpu->thread_id = topo.thread_id;
> >> + cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
> >> + }
> >
> > It seems you want to use "phy_id" (instead of topology IDs as for now)
> > as the parameter to plug CPU. And IIUC in previous patch, "phy_id" is
> > essentially the CPU index.
> >
> > Igor has previously commented [1] on ARM's hotplug design that the
> > current QEMU should use the topology-id (socket-id/core-id/thread-id) as
> > the parameters, not the CPU index or the x86-like apic id.
> >
> > So I think his comment can apply on loongarch, too.
> Yes, I agree. This piece of code is for cold-plug CPUs which is added
> during VM power-on stage, not hot-plugged cpu. For hot-plugged cpu,
> value of cpu->phy_id is UNSET_PHY_ID.
>
> Topology-id (socket-id/core-id/thread-id) is not set for cold-plug CPUs.
> For cold-plug CPUs topology-id is calculated from archid.
that's basically copying what x86 does.
When possible_cpus are initialized, it has all topo info.
So instead of copying bad example of acpid_id, I'd suggest
in a loop that creates cold-plugged cpus, fetch topo ids
from possible_cpus[] and set them instead of phy_id.
>
> Regards
> Bibo
>
> >
> > From my own understanding, the CPU index lacks topological intuition,
> > and the APIC ID for x86 is even worse, as its sometimes even
> > discontinuous. Requiring the user to specify topology ids would allow
> > for clearer localization to CPU slots.
> >
> > [1]: https://lore.kernel.org/qemu-devel/20240812101556.1a395712@imammedo.users.ipa.redhat.com/
> >
> >> + /*
> >> + * update cpu_index calculation method since it is easily used as index
> >> + * with possible_cpus array by function virt_cpu_index_to_props
> >> + */
> >> + cs->cpu_index = index;
> >> + numa_cpu_pre_plug(cpu_slot, dev, &local_err);
> >> + return ;
> >> +
> >> +out:
> >> + error_propagate(errp, local_err);
> >> +}
> >> +
> >
> > Thanks,
> > Zhao
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/4] hw/loongarch/virt: Implement cpu plug interface
2024-11-06 10:56 ` Igor Mammedov
@ 2024-11-07 2:18 ` maobibo
0 siblings, 0 replies; 20+ messages in thread
From: maobibo @ 2024-11-07 2:18 UTC (permalink / raw)
To: Igor Mammedov
Cc: Zhao Liu, Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel,
Xianglai Li
On 2024/11/6 下午6:56, Igor Mammedov wrote:
> On Wed, 30 Oct 2024 09:50:56 +0800
> maobibo <maobibo@loongson.cn> wrote:
>
>> Hi Zhao,
>>
>> On 2024/10/29 下午9:37, Zhao Liu wrote:
>>> (CC Igor since I want to refer his comment on hotplug design.)
>>>
>>> Hi Bibo,
>>>
>>> I have some comments about your hotplug design.
>>>
>>> [snip]
>>>
>>>> +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);
>>>> + CPUState *cs = CPU(dev);
>>>> + CPUArchId *cpu_slot;
>>>> + Error *local_err = NULL;
>>>> + LoongArchCPUTopo topo;
>>>> + int arch_id, index = 0;
>>>
>>> [snip]
>>>
>>>> + if (cpu->phy_id == UNSET_PHY_ID) {
>>>> + error_setg(&local_err, "CPU hotplug not supported");
>>>> + goto out;
>>>> + } else {
>>>> + /*
>>>> + * For non hot-add cpu, topo property is not set. And only physical id
>>>> + * property is set, setup topo information from physical id.
>>>> + *
>>>> + * Supposing arch_id is equal to cpu slot index
>>>> + */
>>>> + arch_id = cpu->phy_id;
>>>> + virt_get_topo_from_index(ms, &topo, arch_id);
>>>> + cpu->socket_id = topo.socket_id;
>>>> + cpu->core_id = topo.core_id;
>>>> + cpu->thread_id = topo.thread_id;
>>>> + cpu_slot = virt_find_cpu_slot(ms, arch_id, &index);
>>>> + }
>>>
>>> It seems you want to use "phy_id" (instead of topology IDs as for now)
>>> as the parameter to plug CPU. And IIUC in previous patch, "phy_id" is
>>> essentially the CPU index.
>>>
>>> Igor has previously commented [1] on ARM's hotplug design that the
>>> current QEMU should use the topology-id (socket-id/core-id/thread-id) as
>>> the parameters, not the CPU index or the x86-like apic id.
>>>
>>> So I think his comment can apply on loongarch, too.
>> Yes, I agree. This piece of code is for cold-plug CPUs which is added
>> during VM power-on stage, not hot-plugged cpu. For hot-plugged cpu,
>> value of cpu->phy_id is UNSET_PHY_ID.
>>
>> Topology-id (socket-id/core-id/thread-id) is not set for cold-plug CPUs.
>> For cold-plug CPUs topology-id is calculated from archid.
>
> that's basically copying what x86 does.
yes, the idea comes from x86.
>
> When possible_cpus are initialized, it has all topo info.
> So instead of copying bad example of acpid_id, I'd suggest
> in a loop that creates cold-plugged cpus, fetch topo ids
> from possible_cpus[] and set them instead of phy_id.
Sure, will set topo property in the loop that creates cold-plugged cpus.
Regards
Bibo Mao
>
>>
>> Regards
>> Bibo
>>
>>>
>>> From my own understanding, the CPU index lacks topological intuition,
>>> and the APIC ID for x86 is even worse, as its sometimes even
>>> discontinuous. Requiring the user to specify topology ids would allow
>>> for clearer localization to CPU slots.
>>>
>>> [1]: https://lore.kernel.org/qemu-devel/20240812101556.1a395712@imammedo.users.ipa.redhat.com/
>>>
>>>> + /*
>>>> + * update cpu_index calculation method since it is easily used as index
>>>> + * with possible_cpus array by function virt_cpu_index_to_props
>>>> + */
>>>> + cs->cpu_index = index;
>>>> + numa_cpu_pre_plug(cpu_slot, dev, &local_err);
>>>> + return ;
>>>> +
>>>> +out:
>>>> + error_propagate(errp, local_err);
>>>> +}
>>>> +
>>>
>>> Thanks,
>>> Zhao
>>>
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 3/4] hw/loongarch/virt: Update the ACPI table for hotplug cpu
2024-10-29 9:53 [PATCH v2 0/4] hw/loongarch/virt: Add cpu hotplug support Bibo Mao
2024-10-29 9:53 ` [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support Bibo Mao
2024-10-29 9:53 ` [PATCH v2 2/4] hw/loongarch/virt: Implement cpu plug interface Bibo Mao
@ 2024-10-29 9:53 ` Bibo Mao
2024-10-29 9:53 ` [PATCH v2 4/4] hw/loongarch/virt: Enable cpu hotplug feature on virt machine Bibo Mao
3 siblings, 0 replies; 20+ messages in thread
From: Bibo Mao @ 2024-10-29 9:53 UTC (permalink / raw)
To: Song Gao, Paolo Bonzini; +Cc: Jiaxun Yang, qemu-devel, Xianglai Li
On LoongArch virt machine, ACPI GED hardware is used for cpu
hotplug, here cpu hotplug support feature is added on GED device,
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 28a46bf195..655312b0fe 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -652,11 +652,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);
@@ -668,6 +674,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 9ba47793ef..861034d614 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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v2 4/4] hw/loongarch/virt: Enable cpu hotplug feature on virt machine
2024-10-29 9:53 [PATCH v2 0/4] hw/loongarch/virt: Add cpu hotplug support Bibo Mao
` (2 preceding siblings ...)
2024-10-29 9:53 ` [PATCH v2 3/4] hw/loongarch/virt: Update the ACPI table for hotplug cpu Bibo Mao
@ 2024-10-29 9:53 ` Bibo Mao
2024-10-29 13:48 ` Zhao Liu
3 siblings, 1 reply; 20+ messages in thread
From: Bibo Mao @ 2024-10-29 9:53 UTC (permalink / raw)
To: Song Gao, Paolo Bonzini; +Cc: Jiaxun Yang, qemu-devel, Xianglai Li
On virt machine, enable cpu hotplug feature has_hotpluggable_cpus.
For hot-added cpu after power on, interrupt pin of extioi and ipi
interrupt controller need connect to pins of new cpu.
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 ++++++++++++++++++++++++++++++++++---
include/hw/loongarch/virt.h | 2 ++
2 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 655312b0fe..cecb12786b 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -851,8 +851,9 @@ 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;
/* IPI iocsr memory region */
memory_region_add_subregion(&lvms->system_iocsr, SMP_IPI_MAILBOX,
@@ -877,11 +878,12 @@ 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);
}
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)) {
@@ -1382,8 +1384,40 @@ 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;
} else {
/*
* For non hot-add cpu, topo property is not set. And only physical id
@@ -1465,8 +1499,33 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
CPUArchId *cpu_slot;
+ Error *local_err = NULL;
LoongArchCPU *cpu = LOONGARCH_CPU(dev);
+ CPUState *cs = CPU(cpu);
+ CPULoongArchState *env;
LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
+ int pin;
+
+ if (lvms->acpi_ged) {
+ env = &(cpu->env);
+ env->address_space_iocsr = &lvms->as_iocsr;
+
+ /* connect ipi irq to cpu irq, logic cpu index used here */
+ qdev_connect_gpio_out(lvms->ipi, cs->cpu_index,
+ qdev_get_gpio_in(dev, IRQ_IPI));
+ env->ipistate = lvms->ipi;
+
+ for (pin = 0; pin < LS3A_INTC_IP; pin++) {
+ qdev_connect_gpio_out(lvms->extioi, (cs->cpu_index * 8 + pin),
+ qdev_get_gpio_in(dev, pin + 2));
+ }
+
+ hotplug_handler_plug(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, NULL);
cpu_slot->cpu = CPU(dev);
@@ -1652,6 +1711,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;
diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
index 861034d614..79a85723c9 100644
--- a/include/hw/loongarch/virt.h
+++ b/include/hw/loongarch/virt.h
@@ -61,6 +61,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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] hw/loongarch/virt: Enable cpu hotplug feature on virt machine
2024-10-29 9:53 ` [PATCH v2 4/4] hw/loongarch/virt: Enable cpu hotplug feature on virt machine Bibo Mao
@ 2024-10-29 13:48 ` Zhao Liu
2024-10-30 1:55 ` maobibo
0 siblings, 1 reply; 20+ messages in thread
From: Zhao Liu @ 2024-10-29 13:48 UTC (permalink / raw)
To: Bibo Mao
Cc: Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel, Xianglai Li,
Igor Mammedov
[snip]
> @@ -1382,8 +1384,40 @@ 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;
> } else {
Here you allow user to specify topology IDs, but "else" still indicates
user could use "phy_id" instead of topology IDs, right?
Is it necessary to expose "phy_id" to user?
Thanks,
Zhao
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] hw/loongarch/virt: Enable cpu hotplug feature on virt machine
2024-10-29 13:48 ` Zhao Liu
@ 2024-10-30 1:55 ` maobibo
2024-10-30 2:18 ` Zhao Liu
0 siblings, 1 reply; 20+ messages in thread
From: maobibo @ 2024-10-30 1:55 UTC (permalink / raw)
To: Zhao Liu
Cc: Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel, Xianglai Li,
Igor Mammedov
On 2024/10/29 下午9:48, Zhao Liu wrote:
> [snip]
>
>> @@ -1382,8 +1384,40 @@ 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;
>> } else {
>
> Here you allow user to specify topology IDs, but "else" still indicates
> user could use "phy_id" instead of topology IDs, right?
"else" for cold-plug CPUs which is created with index from [0 -- smp.cpus)
>
> Is it necessary to expose "phy_id" to user?
We will remove phy_id property and unexpose it to user.
Regards
Bibo Mao
>
> Thanks,
> Zhao
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] hw/loongarch/virt: Enable cpu hotplug feature on virt machine
2024-10-30 1:55 ` maobibo
@ 2024-10-30 2:18 ` Zhao Liu
0 siblings, 0 replies; 20+ messages in thread
From: Zhao Liu @ 2024-10-30 2:18 UTC (permalink / raw)
To: maobibo
Cc: Song Gao, Paolo Bonzini, Jiaxun Yang, qemu-devel, Xianglai Li,
Igor Mammedov
> > > @@ -1382,8 +1384,40 @@ 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;
> > > } else {
> >
> > Here you allow user to specify topology IDs, but "else" still indicates
> > user could use "phy_id" instead of topology IDs, right?
> "else" for cold-plug CPUs which is created with index from [0 -- smp.cpus)
>
> >
> > Is it necessary to expose "phy_id" to user?
> We will remove phy_id property and unexpose it to user.
>
Thanks! The issue lies with the property itself. Even if you try to
support a hotplug scheme based on topology IDs, users can actually use
phy_id for hotplugging. Removing it seems fine.
Regards,
Zhao
^ permalink raw reply [flat|nested] 20+ messages in thread