On 2024/11/6 下午6:56, Igor Mammedov wrote:
On Wed, 30 Oct 2024 09:50:56 +0800
maobibo <maob...@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.1a395...@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
