Re: [PATCH v7 09/16] i386: Support module_id in X86CPUTopoIDs

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 1/8/2024 4:27 PM, Zhao Liu wrote:
> From: Zhuocheng Ding <zhuocheng.ding@intel.com>
> 
> Add module_id member in X86CPUTopoIDs.
> 
> module_id can be parsed from APIC ID, so also update APIC ID parsing
> rule to support module level. With this support, the conversions with
> module level between X86CPUTopoIDs, X86CPUTopoInfo and APIC ID are
> completed.
> 
> module_id can be also generated from cpu topology, and before i386
> supports "clusters" in smp, the default "clusters per die" is only 1,
> thus the module_id generated in this way is 0, so that it will not
> conflict with the module_id generated by APIC ID.
> 
> Signed-off-by: Zhuocheng Ding <zhuocheng.ding@intel.com>
> Co-developed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Babu Moger <babu.moger@amd.com>
> Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> Changes since v1:
>   * Merge the patch "i386: Update APIC ID parsing rule to support module
>     level" into this one. (Yanan)
>   * Move the apicid_module_width() and apicid_module_offset() support
>     into the previous modules_per_die related patch. (Yanan)
> ---
>   hw/i386/x86.c              | 28 +++++++++++++++++++++-------
>   include/hw/i386/topology.h | 17 +++++++++++++----
>   2 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 85b847ac7914..5269aae3a5c2 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -315,11 +315,11 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>   
>       /*
>        * If APIC ID is not set,
> -     * set it based on socket/die/core/thread properties.
> +     * set it based on socket/die/cluster/core/thread properties.
>        */
>       if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> -        int max_socket = (ms->smp.max_cpus - 1) /
> -                                smp_threads / smp_cores / ms->smp.dies;
> +        int max_socket = (ms->smp.max_cpus - 1) / smp_threads / smp_cores /
> +                                ms->smp.clusters / ms->smp.dies;
>   
>           /*
>            * die-id was optional in QEMU 4.0 and older, so keep it optional
> @@ -366,17 +366,27 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>           topo_ids.die_id = cpu->die_id;
>           topo_ids.core_id = cpu->core_id;
>           topo_ids.smt_id = cpu->thread_id;
> +
> +        /*
> +         * TODO: This is the temporary initialization for topo_ids.module_id to
> +         * avoid "maybe-uninitialized" compilation errors. Will remove when
> +         * X86CPU supports cluster_id.
> +         */
> +        topo_ids.module_id = 0;
> 

if you put patch 10 before this patch, then we don't need this trick.

>           cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids);
>       }
>   
>       cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, &idx);
>       if (!cpu_slot) {
>           x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
> +
>           error_setg(errp,
> -            "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
> -            " APIC ID %" PRIu32 ", valid index range 0:%d",
> -            topo_ids.pkg_id, topo_ids.die_id, topo_ids.core_id, topo_ids.smt_id,
> -            cpu->apic_id, ms->possible_cpus->len - 1);
> +            "Invalid CPU [socket: %u, die: %u, module: %u, core: %u, thread: %u]"
> +            " with APIC ID %" PRIu32 ", valid index range 0:%d",
> +            topo_ids.pkg_id, topo_ids.die_id, topo_ids.module_id,
> +            topo_ids.core_id, topo_ids.smt_id, cpu->apic_id,
> +            ms->possible_cpus->len - 1);
>           return;
>       }
>   
> @@ -502,6 +512,10 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
>               ms->possible_cpus->cpus[i].props.has_die_id = true;
>               ms->possible_cpus->cpus[i].props.die_id = topo_ids.die_id;
>           }
> +        if (ms->smp.clusters > 1) {
> +            ms->possible_cpus->cpus[i].props.has_cluster_id = true;
> +            ms->possible_cpus->cpus[i].props.cluster_id = topo_ids.module_id;
> +        }
>           ms->possible_cpus->cpus[i].props.has_core_id = true;
>           ms->possible_cpus->cpus[i].props.core_id = topo_ids.core_id;
>           ms->possible_cpus->cpus[i].props.has_thread_id = true;
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index 517e51768c13..ed1f3d6c1d5e 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -50,6 +50,7 @@ typedef uint32_t apic_id_t;
>   typedef struct X86CPUTopoIDs {
>       unsigned pkg_id;
>       unsigned die_id;
> +    unsigned module_id;
>       unsigned core_id;
>       unsigned smt_id;
>   } X86CPUTopoIDs;
> @@ -127,6 +128,7 @@ static inline apic_id_t x86_apicid_from_topo_ids(X86CPUTopoInfo *topo_info,
>   {
>       return (topo_ids->pkg_id  << apicid_pkg_offset(topo_info)) |
>              (topo_ids->die_id  << apicid_die_offset(topo_info)) |
> +           (topo_ids->module_id << apicid_module_offset(topo_info)) |
>              (topo_ids->core_id << apicid_core_offset(topo_info)) |
>              topo_ids->smt_id;
>   }
> @@ -140,12 +142,16 @@ static inline void x86_topo_ids_from_idx(X86CPUTopoInfo *topo_info,
>                                            X86CPUTopoIDs *topo_ids)
>   {
>       unsigned nr_dies = topo_info->dies_per_pkg;
> -    unsigned nr_cores = topo_info->cores_per_module *
> -                        topo_info->modules_per_die;
> +    unsigned nr_modules = topo_info->modules_per_die;
> +    unsigned nr_cores = topo_info->cores_per_module;
>       unsigned nr_threads = topo_info->threads_per_core;
>   
> -    topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
> -    topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
> +    topo_ids->pkg_id = cpu_index / (nr_dies * nr_modules *
> +                       nr_cores * nr_threads);
> +    topo_ids->die_id = cpu_index / (nr_modules * nr_cores *
> +                       nr_threads) % nr_dies;
> +    topo_ids->module_id = cpu_index / (nr_cores * nr_threads) %
> +                          nr_modules;
>       topo_ids->core_id = cpu_index / nr_threads % nr_cores;
>       topo_ids->smt_id = cpu_index % nr_threads;
>   }
> @@ -163,6 +169,9 @@ static inline void x86_topo_ids_from_apicid(apic_id_t apicid,
>       topo_ids->core_id =
>               (apicid >> apicid_core_offset(topo_info)) &
>               ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
> +    topo_ids->module_id =
> +            (apicid >> apicid_module_offset(topo_info)) &
> +            ~(0xFFFFFFFFUL << apicid_module_width(topo_info));
>       topo_ids->die_id =
>               (apicid >> apicid_die_offset(topo_info)) &
>               ~(0xFFFFFFFFUL << apicid_die_width(topo_info));