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.
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.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
> >
>
