| Message ID | 20241029095335.2219343-3-maobibo@loongson.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/loongarch/virt: Add cpu hotplug support | expand |
(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
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 >
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 > > >
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 >>> >> >
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);
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(+)