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

[Zhao Liu <zhao1.liu@linux.intel.com>]

Hi Xiaoyao,

On Sun, Jan 14, 2024 at 08:42:00PM +0800, Xiaoyao Li wrote:
> Date: Sun, 14 Jan 2024 20:42:00 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH v7 09/16] i386: Support module_id in X86CPUTopoIDs
> 
> 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.

Then, we need another trick to resolve "cpu->cluster_id = topo_ids.module_id;" 
in patch 10. ;-)

Thanks,
Zhao

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