diff mbox series

[v2,2/4] hw/loongarch/virt: Implement cpu plug interface

Message ID 20241029095335.2219343-3-maobibo@loongson.cn (mailing list archive)
State New
Headers show
Series hw/loongarch/virt: Add cpu hotplug support | expand

Commit Message

Bibo Mao Oct. 29, 2024, 9:53 a.m. UTC 
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(+)

Comments

Zhao Liu Oct. 29, 2024, 1:37 p.m. UTC | #1 
(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
Bibo Mao Oct. 30, 2024, 1:50 a.m. UTC | #2 
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
>
diff mbox series

Patch

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