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

[Zhao Liu <zhao1.liu@intel.com>]

(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