Re: [PATCH RESEND 10/18] i386: Update APIC ID parsing rule to support module level

[Zhao Liu]

On Wed, Feb 15, 2023 at 07:06:32PM +0800, wangyanan (Y) wrote:
> Date: Wed, 15 Feb 2023 19:06:32 +0800
> From: "wangyanan (Y)" <wangyanan55@huawei.com>
> Subject: Re: [PATCH RESEND 10/18] i386: Update APIC ID parsing rule to
>  support module level
> 
> Hi Zhao,
> 
> 在 2023/2/13 17:36, Zhao Liu 写道:
> > From: Zhuocheng Ding <zhuocheng.ding@intel.com>
> > 
> > Add the module level parsing support for APIC ID.
> > 
> > With this support, now the conversion between X86CPUTopoIDs,
> > X86CPUTopoInfo and APIC ID is completed.
> IIUC, contents in patch 6-8 and 10 are all about "Introduce the module-level
> CPU topology support for x86", why do we need gradually do this with kinds
> of temporary things instead of warp them into one patch?

Patch 6 is about CPUX86State.nr_dies, which is independent from
patch 7, 8, 10.

Patch 7 (X86CPUTopoInfo.modules_per_die), patch 8 (X86CPUTopoIDs.module_id),
and patch 10 (APIC ID parsing rule) are related but have their own
relatively clear little themes, and are gradually completing full
support for module level in apic id.

Patch 7, 8, 10 can be combined into one big patch. This current
splitting way is actually designed to make it easier to review...
But if you think this is not convenient for review, sorry for that,
and I'm willing to merge them together. ;-)

Thanks,
Zhao

> Before support
> for smp.clusters in the CLI for x86, we can ensure that modules_per_dies is
> always 1 so that the code is save in one diff. Or do I miss something?
> 
> Thanks,
> Yanan
> > 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>
> > ---
> >   hw/i386/x86.c              | 19 ++++++++-----------
> >   include/hw/i386/topology.h | 36 ++++++++++++++++++------------------
> >   2 files changed, 26 insertions(+), 29 deletions(-)
> > 
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > index b90c6584930a..2a9d080a8e7a 100644
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -311,11 +311,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
> > @@ -379,15 +379,12 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >           x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
> > -        /*
> > -         * TODO: Before APIC ID supports module level parsing, there's no 
> > need
> > -         * to expose module_id info.
> > -         */
> >           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;
> >       }
> > diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> > index 5de905dc00d3..3cec97b377f2 100644
> > --- a/include/hw/i386/topology.h
> > +++ b/include/hw/i386/topology.h
> > @@ -79,12 +79,13 @@ static inline unsigned apicid_smt_width(X86CPUTopoInfo 
> > *topo_info)
> >   /* Bit width of the Core_ID field */
> >   static inline unsigned apicid_core_width(X86CPUTopoInfo *topo_info)
> >   {
> > -    /*
> > -     * TODO: Will separate module info from core_width when update
> > -     * APIC ID with module level.
> > -     */
> > -    return apicid_bitwidth_for_count(topo_info->cores_per_module *
> > -                                     topo_info->modules_per_die);
> > +    return apicid_bitwidth_for_count(topo_info->cores_per_module);
> > +}
> > +
> > +/* Bit width of the Module_ID (cluster ID) field */
> > +static inline unsigned apicid_module_width(X86CPUTopoInfo *topo_info)
> > +{
> > +    return apicid_bitwidth_for_count(topo_info->modules_per_die);
> >   }
> >   /* Bit width of the Die_ID field */
> > @@ -99,10 +100,16 @@ static inline unsigned 
> > apicid_core_offset(X86CPUTopoInfo *topo_info)
> >       return apicid_smt_width(topo_info);
> >   }
> > +/* Bit offset of the Module_ID (cluster ID) field */
> > +static inline unsigned apicid_module_offset(X86CPUTopoInfo *topo_info)
> > +{
> > +    return apicid_core_offset(topo_info) + apicid_core_width(topo_info);
> > +}
> > +
> >   /* Bit offset of the Die_ID field */
> >   static inline unsigned apicid_die_offset(X86CPUTopoInfo *topo_info)
> >   {
> > -    return apicid_core_offset(topo_info) + apicid_core_width(topo_info);
> > +    return apicid_module_offset(topo_info) + 
> > apicid_module_width(topo_info);
> >   }
> >   /* Bit offset of the Pkg_ID (socket ID) field */
> > @@ -121,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;
> >   }
> > @@ -138,11 +146,6 @@ static inline void 
> > x86_topo_ids_from_idx(X86CPUTopoInfo *topo_info,
> >       unsigned nr_cores = topo_info->cores_per_module;
> >       unsigned nr_threads = topo_info->threads_per_core;
> > -    /*
> > -     * Currently smp for i386 doesn't support "clusters", modules_per_die 
> > is
> > -     * only 1. Therefore, the module_id generated from the module topology 
> > will
> > -     * not conflict with the module_id generated according to the apicid.
> > -     */
> >       topo_ids->pkg_id = cpu_index / (nr_dies * nr_modules *
> >                          nr_cores * nr_threads);
> >       topo_ids->die_id = cpu_index / (nr_modules * nr_cores *
> > @@ -166,12 +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));
> > -    /*
> > -     * TODO: This is the temporary initialization for topo_ids.module_id to
> > -     * avoid "maybe-uninitialized" compilation errors. Will remove when 
> > APIC
> > -     * ID supports module level parsing.
> > -     */
> > -    topo_ids->module_id = 0;
> > +    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));
>