diff mbox series

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

Message ID 20240108082727.420817-10-zhao1.liu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Support smp.clusters for x86 in QEMU | expand

Commit Message

Zhao Liu Jan. 8, 2024, 8:27 a.m. UTC 
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(-)

Comments

Xiaoyao Li Jan. 14, 2024, 12:42 p.m. UTC | #1 
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));
Zhao Liu Jan. 15, 2024, 3:52 a.m. UTC | #2 
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));
>
diff mbox series

Patch

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