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

[Gavin Shan <gshan@redhat.com>]

Hi Salil,

On 9/26/23 20:04, 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.
> 

Note that we don't have 'vcpu-id' property. It's actually the index to the array
ms->possible_cpus->cpus[] and cpu->cpu_index. Please improve the commit log if
it makes sense.

> Co-developed-by: Salil Mehta <salil.mehta@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> 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    | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>   target/arm/cpu.c |  4 +++
>   target/arm/cpu.h |  4 +++
>   3 files changed, 71 insertions(+)
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7d9dbc2663..57fe97c242 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -221,6 +221,11 @@ static const char *valid_cpus[] = {
>       ARM_CPU_TYPE_NAME("max"),
>   };
>   
> +static int virt_get_socket_id(const MachineState *ms, int cpu_index);
> +static int virt_get_cluster_id(const MachineState *ms, int cpu_index);
> +static int virt_get_core_id(const MachineState *ms, int cpu_index);
> +static int virt_get_thread_id(const MachineState *ms, int cpu_index);
> +
>   static bool cpu_type_valid(const char *cpu)
>   {
>       int i;
> @@ -2168,6 +2173,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);
> @@ -2652,10 +2665,59 @@ static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx)
>       return socket_id % ms->numa_state->num_nodes;
>   }
>   

It seems it's not unnecessary to keep virt_get_{socket, cluster, core, thread}_id()
because they're called for once. I would suggest to figure out the socket, cluster,
core and thread ID through @possible_cpus in machvirt_init(), like below.

Besides, we can't always expose property "cluster-id" since cluster in the CPU
topology isn't always supported, seeing MachineClass::smp_props. Some users may
want to hide cluster for unknown reasons. 'cluster-id' shouldn't be exposed in
this case. Otherwise, users may be confused by 'cluster-id' property while it
has been disabled. For example, a VM is started with the following command lines
and 'cluster-id' shouldn't be supported in vCPU hot-add.

     -cpu host -smp=maxcpus=2,cpus=1,sockets=2,cores=1,threads=1
     (qemu) device_add host,id=cpu1,socket-id=1,cluster-id=0,core-id=0,thread-id=0

     object_property_set_int(cpuobj, "socket-id",
                             possible_cpus->cpus[i].props.socket_id, NULL);
     if (mc->smp_props.cluster_supported && mc->smp_props.has_clusters) {
         object_property_set_int(cpuobj, "cluster-id",
                                 possible_cpus->cpus[i].props.cluster_id, NULL);
     }
     object_property_set_int(cpuobj, "core-id",
                             possible_cpus->cpus[i].props.core_id, NULL);
     object_property_set_int(cpuobj, "thread-id",
                             possible_cpus->cpus[i].props.thread_id, NULL);

> +static 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 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 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 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;
> +}
> +
> +static int
> +virt_get_cpu_id_from_cpu_topo(const MachineState *ms, DeviceState *dev)
> +{
> +    int cpu_id, sock_vcpu_num, clus_vcpu_num, core_vcpu_num;
> +    ARMCPU *cpu = ARM_CPU(dev);
> +
> +    /* calculate total logical cpus across socket/cluster/core */
> +    sock_vcpu_num = cpu->socket_id * (ms->smp.threads * ms->smp.cores *
> +                    ms->smp.clusters);
> +    clus_vcpu_num = cpu->cluster_id * (ms->smp.threads * ms->smp.cores);
> +    core_vcpu_num = cpu->core_id * ms->smp.threads;
> +
> +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
> +    cpu_id = (sock_vcpu_num + clus_vcpu_num + core_vcpu_num) + cpu->thread_id;
> +
> +    assert(cpu_id >= 0 && cpu_id < ms->possible_cpus->len);
> +
> +    return cpu_id;
> +}
> +

This function is called for once in PATCH[04/37]. I think it needs to be moved
around to PATCH[04/37].

[PATCH RFC V2 04/37] arm/virt,target/arm: Machine init time change common to vCPU {cold|hot}-plug

The function name can be shortened because I don't see the suffix "_from_cpu_topo"
is too much helpful. I think virt_get_cpu_index() would be good enough since it's
called for once to return the index in array MachineState::possible_cpus::cpus[]
and the return value is stored to CPUState::cpu_index.

static int virt_get_cpu_index(const MachineState *ms, ARMCPU *cpu)
{
     int index, cpus_in_socket, cpus_in_cluster, cpus_in_core;

     /*
      * It's fine to take cluster into account even it's not supported. In this
      * case, ms->smp.clusters is always one.
      */
}

>   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);
>   
> @@ -2669,6 +2731,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);
>   

This initialization seems to accomodate HMP command "info hotpluggable-cpus".
It would be nice if it can be mentioned in the commit log.

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 93c28d50e5..1376350416 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2277,6 +2277,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),
>       DEFINE_PROP_END_OF_LIST()
>   };

All those 4 properties are used for vCPU hot-add, meaning they're not needed
when vCPU hotplug isn't supported on the specific board. Even for hw/virt board,
cluster isn't always supported and 'cluster-id' shouldn't always be exposed,
as explained above. How about to register the properties dynamically only when
they're needed by vCPU hotplug?

> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 88e5accda6..d51d39f621 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1094,6 +1094,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;

It would be fine to keep those fields even the corresponding properties are
dynamically registered, but a little bit memory overhead incurred :)

>   
>       /* Used to synchronize KVM and QEMU in-kernel device levels */
>       uint8_t device_irq_level;

Thanks,
Gavin