Re: [PATCH RFC V3 01/29] arm/virt,target/arm: Add new ARMCPU {socket,cluster,core,thread}-id property

[Igor Mammedov]

On Mon, 12 Aug 2024 14:35:56 +1000
Gavin Shan <gshan@redhat.com> wrote:

> On 6/14/24 9:36 AM, Salil Mehta wrote:
> > This shall be used to store user specified 
> > topology{socket,cluster,core,thread}
> > and shall be converted to a unique 'vcpu-id' which is used as slot-index 
> > during
> > hot(un)plug of vCPU.
> > 
> > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > ---
> >   hw/arm/virt.c         | 10 ++++++++++
> >   include/hw/arm/virt.h | 28 ++++++++++++++++++++++++++++
> >   target/arm/cpu.c      |  4 ++++
> >   target/arm/cpu.h      |  4 ++++
> >   4 files changed, 46 insertions(+)
> >   
> 
> Those 4 properties are introduced to determine the vCPU's slot, which is the 
> index
> to MachineState::possible_cpus::cpus[]. From there, the CPU object or 
> instance is
> referenced and then the CPU's state can be further determined. It sounds 
> reasonable
> to use the CPU's topology to determine the index. However, I'm wandering if 
> this can
> be simplified to use 'cpu-index' or 'index' for a couple of facts: (1) 
> 'cpu-index'

Please, don't. We've spent a bunch of time to get rid of cpu-index in user
visible interface (well, old NUMA CLI is still there along with 'new' topology
based one, but that's the last one).

> or 'index' is simplified. Users have to provide 4 parameters in order to 
> determine
> its index in the extreme case, for example "device_add host-arm-cpu, 
> id=cpu7,socket-id=1,
> cluster-id=1,core-id=1,thread-id=1". With 'cpu-index' or 'index', it can be 
> simplified
> to 'index=7'. (2) The cold-booted and hotpluggable CPUs are determined by 
> their index
> instead of their topology. For example, CPU0/1/2/3 are cold-booted CPUs while 
> CPU4/5/6/7
> are hotpluggable CPUs with command lines '-smp maxcpus=8,cpus=4'. So 'index' 
> makes
> more sense to identify a vCPU's slot.
cpu-index have been used for hotplug with x86 machines as a starting point
to implement hotplug as it was easy to hack and it has already existed in QEMU.

But that didn't scale as was desired and had its own issues.
Hence the current interface that majority agreed upon.
I don't remember exact arguments anymore (they could be found qemu-devel if 
needed)
Here is a link to the talk that tried to explain why topo based was introduced.
  
http://events17.linuxfoundation.org/sites/events/files/slides/CPU%20Hot-plug%20support%20in%20QEMU.pdf


> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 3c93c0c0a6..11fc7fc318 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2215,6 +2215,14 @@ static void machvirt_init(MachineState *machine)
> >                             &error_fatal);
> >   
> >           aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL);
> > +        object_property_set_int(cpuobj, "socket-id",
> > +                                virt_get_socket_id(machine, n), NULL);
> > +        object_property_set_int(cpuobj, "cluster-id",
> > +                                virt_get_cluster_id(machine, n), NULL);
> > +        object_property_set_int(cpuobj, "core-id",
> > +                                virt_get_core_id(machine, n), NULL);
> > +        object_property_set_int(cpuobj, "thread-id",
> > +                                virt_get_thread_id(machine, n), NULL);
> >   
> >           if (!vms->secure) {
> >               object_property_set_bool(cpuobj, "has_el3", false, NULL);
> > @@ -2708,6 +2716,7 @@ static const CPUArchIdList 
> > *virt_possible_cpu_arch_ids(MachineState *ms)
> >   {
> >       int n;
> >       unsigned int max_cpus = ms->smp.max_cpus;
> > +    unsigned int smp_threads = ms->smp.threads;
> >       VirtMachineState *vms = VIRT_MACHINE(ms);
> >       MachineClass *mc = MACHINE_GET_CLASS(vms);
> >   
> > @@ -2721,6 +2730,7 @@ static const CPUArchIdList 
> > *virt_possible_cpu_arch_ids(MachineState *ms)
> >       ms->possible_cpus->len = max_cpus;
> >       for (n = 0; n < ms->possible_cpus->len; n++) {
> >           ms->possible_cpus->cpus[n].type = ms->cpu_type;
> > +        ms->possible_cpus->cpus[n].vcpus_count = smp_threads;
> >           ms->possible_cpus->cpus[n].arch_id =
> >               virt_cpu_mp_affinity(vms, n);
> >     
> 
> Why @vcpus_count is initialized to @smp_threads? it needs to be documented in
> the commit log.
> 
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index bb486d36b1..6f9a7bb60b 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -209,4 +209,32 @@ static inline int 
> > virt_gicv3_redist_region_count(VirtMachineState *vms)
> >               vms->highmem_redists) ? 2 : 1;
> >   }
> >   
> > +static inline int virt_get_socket_id(const MachineState *ms, int cpu_index)
> > +{
> > +    assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len);
> > +
> > +    return ms->possible_cpus->cpus[cpu_index].props.socket_id;
> > +}
> > +
> > +static inline int virt_get_cluster_id(const MachineState *ms, int 
> > cpu_index)
> > +{
> > +    assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len);
> > +
> > +    return ms->possible_cpus->cpus[cpu_index].props.cluster_id;
> > +}
> > +
> > +static inline int virt_get_core_id(const MachineState *ms, int cpu_index)
> > +{
> > +    assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len);
> > +
> > +    return ms->possible_cpus->cpus[cpu_index].props.core_id;
> > +}
> > +
> > +static inline int virt_get_thread_id(const MachineState *ms, int cpu_index)
> > +{
> > +    assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len);
> > +
> > +    return ms->possible_cpus->cpus[cpu_index].props.thread_id;
> > +}
> > +
> >   #endif /* QEMU_ARM_VIRT_H */
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 77f8c9c748..abc4ed0842 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -2582,6 +2582,10 @@ static Property arm_cpu_properties[] = {
> >       DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
> >                           mp_affinity, ARM64_AFFINITY_INVALID),
> >       DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
> > +    DEFINE_PROP_INT32("socket-id", ARMCPU, socket_id, 0),
> > +    DEFINE_PROP_INT32("cluster-id", ARMCPU, cluster_id, 0),
> > +    DEFINE_PROP_INT32("core-id", ARMCPU, core_id, 0),
> > +    DEFINE_PROP_INT32("thread-id", ARMCPU, thread_id, 0),
> >       DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1),
> >       /* True to default to the backward-compat old CNTFRQ rather than 1Ghz 
> > */
> >       DEFINE_PROP_BOOL("backcompat-cntfrq", ARMCPU, backcompat_cntfrq, 
> > false),
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index c17264c239..208c719db3 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -1076,6 +1076,10 @@ struct ArchCPU {
> >       QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
> >   
> >       int32_t node_id; /* NUMA node this CPU belongs to */
> > +    int32_t socket_id;
> > +    int32_t cluster_id;
> > +    int32_t core_id;
> > +    int32_t thread_id;
> >   
> >       /* Used to synchronize KVM and QEMU in-kernel device levels */
> >       uint8_t device_irq_level;  
> 
> Thanks,
> Gavin
>