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

[Salil Mehta]

HI Gavin,

Sorry, I was away for almost entire last week. Joined back today.
Thanks for taking out time to review. 

>  From: Gavin Shan <gshan@redhat.com>
>  Sent: Monday, August 12, 2024 5:36 AM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>  qemu-arm@nongnu.org; mst@redhat.com
>  
>  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[]. 

Correct.

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' 

Are you suggesting to use CPU index while specifying vCPUs through
command line and I'm not even sure how will it simply CPU naming?

CPU index is internal index to QOM. The closest thing which you can
have is the 'slot-id'  and later can have mapping to the CPU index
internally but I'm not sure how much useful it is to introduce this 
slot abstraction. I did raise this in the original RFC I posted in 2020.


for a couple of facts: (1) 'cpu-index'
>  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.


I'm not sure if anybody wants to use it this way. People want to specify 
topology
i.e. where the vCPU fits. Internally it's up to QOM to translate that topology 
to
some index.


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


Because every thread internally amounts to a vCPU in QOM and which
is in 1:1 relationship with KVM vCPU. AFAIK, QOM does not strictly follows
any architecture. Once you start to get into details of threads there
are many aspects of shared resources one will have to consider and
these can vary across different implementations of architecture.

It is a bigger problem than you think, which I've touched at very nascent
stages while doing POC of vCPU hotplug but tried to avoid till now. 


But I would like to hear other community members views on this.

Hi Igor/Peter,

What is your take on this?

Thanks
Salil.



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