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

[wangyanan (Y)]

Hi Zhao,

在 2023/2/13 17:36, Zhao Liu 写道:

From: Zhuocheng Ding <zhuocheng.d...@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? 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.d...@intel.com>
Co-developed-by: Zhao Liu <zhao1....@intel.com>
Signed-off-by: Zhao Liu <zhao1....@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));