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

[Igor Mammedov]

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