[PATCH v2 00/10] i386: Track X86CPUTopoInfo in CPUX86State and track features in env->features[]

[PATCH v2 00/10] i386: Track X86CPUTopoInfo in CPUX86State and track features in env->features[]

[Xiaoyao Li]

This v2 series is follow up work of [1],[2].

The initial purpose is to track CPUID_HT in env->features[] for x86.
It's supposed to be implemented in x86_cpu_realizefn() and before
qemu_init_vcpu(). However, CPUID_HT bit needs to be evaluated based on
topology information, either 1) nr_cores and nr_threads of CPUState or
2) MachineState::smp.

1) are initialized in qemu_init_vcpu() which is too late. So [1] was
sent to initialize nr_threads and nr_cores of CPUState earlier. It
touches all the ARCHes and people disliked it. Followup attempt to
initialize them in cpu_common_initfn() was also rejected. [3]

2) needs to grab MachineState in cpu context and was disagreed[3]

As suggested by Igor[3], aduit nr_threads and nr_cores and initialize
them in pre_plug() callback for the ARCHes where they are actually needed.
I found nr_cores is only used by x86 and nr_threads is used by
X86/MIPS/PPC[4]. I didn't play with MIPS and PPC before and I'm
incapable to write patch for them. So I send an RFC work [2], which
drops nr_cores from CPUState and it instead maintains a substitute for
x86 only. The RFC work left how to handle nr_threads as open however I
didn't get any feedback yet.

Eventually, I decided to fix x86 only. The sulution is to maintain a
X86CPUTopoInfo in CPUX86State, so that x86 doesn't need to use
nr_threads and nr_cores anymore and the dependency on qemu_init_vcpu()
goes away.

x86 is the only user of CPUState::nr_core currently. When it switches to
use CPUX86state::X86CPUTopoInfo, CPUState::nr_core loses its only user
and can be dropped in patch 8.

In the future, if MIPS and PPC starts to maintain nr_threads in its own
CPU state, common CPUState::nr_threads can be dropped as well.

Besides the patches to fulfill the main goal to track CPUID_HT in
env->features[] for x86. This series also contains the cleanup patches
during the work.


[1] https://lore.kernel.org/qemu-devel/20241108070609.3653085-1-xiaoyao.li@intel.com/
[2] https://lore.kernel.org/qemu-devel/20241205145716.472456-1-xiaoyao.li@intel.com/
[3] https://lore.kernel.org/qemu-devel/20241125103857.78a23715@imammedo.users.ipa.redhat.com/
[4] https://lore.kernel.org/qemu-devel/045f9cb1-2b17-4b2c-985f-3c34e3626b36@intel.com/

Xiaoyao Li (10):
  i386/cpu: Extract a common fucntion to setup value of
    MSR_CORE_THREAD_COUNT
  i386/cpu: Drop the variable smp_cores and smp_threads in
    x86_cpu_pre_plug()
  i386/cpu: Drop cores_per_pkg in cpu_x86_cpuid()
  i386/topology: Update the comment of x86_apicid_from_topo_ids()
  i386/topology: Introduce helpers for various topology info of
    different level
  i386/cpu: Track a X86CPUTopoInfo directly in CPUX86State
  i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core
  cpu: Remove nr_cores from struct CPUState
  i386/cpu: Set up CPUID_HT in x86_cpu_expand_features() instead of
    cpu_x86_cpuid()
  i386/cpu: Set and track CPUID_EXT3_CMP_LEG in
    env->features[FEAT_8000_0001_ECX]

 hw/core/cpu-common.c                 |   1 -
 hw/i386/x86-common.c                 |  22 +++---
 include/hw/core/cpu.h                |   2 -
 include/hw/i386/topology.h           |  30 ++++++-
 system/cpus.c                        |   1 -
 target/i386/cpu-sysemu.c             |  11 +++
 target/i386/cpu.c                    | 113 ++++++++++++---------------
 target/i386/cpu.h                    |   8 +-
 target/i386/hvf/x86_emu.c            |   3 +-
 target/i386/kvm/kvm.c                |   5 +-
 target/i386/tcg/sysemu/misc_helper.c |   3 +-
 11 files changed, 103 insertions(+), 96 deletions(-)

-- 
2.34.1

[PATCH v2 01/10] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Xiaoyao Li]

There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
Extract a common function for it.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes in v2:
- move the implementation of cpu_x86_get_msr_core_thread_count() to
  target/i386/cpu-sysemu.c;
---
 target/i386/cpu-sysemu.c             | 11 +++++++++++
 target/i386/cpu.h                    |  2 ++
 target/i386/hvf/x86_emu.c            |  3 +--
 target/i386/kvm/kvm.c                |  5 +----
 target/i386/tcg/sysemu/misc_helper.c |  3 +--
 5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 227ac021f62f..4e9df0bc0156 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -309,3 +309,14 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
                                      errp);
     qapi_free_GuestPanicInformation(panic_info);
 }
+
+uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+    uint64_t val;
+
+    val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 15..0 */
+    val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
+
+    return val;
+}
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 4c239a6970fd..5e1beca94a62 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2390,6 +2390,8 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu,
     cs->halted = 0;
 }
 
+uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu);
+
 int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
                             target_ulong *base, unsigned int *limit,
                             unsigned int *flags);
diff --git a/target/i386/hvf/x86_emu.c b/target/i386/hvf/x86_emu.c
index 015f760acb39..69c61c9c0737 100644
--- a/target/i386/hvf/x86_emu.c
+++ b/target/i386/hvf/x86_emu.c
@@ -765,8 +765,7 @@ void simulate_rdmsr(CPUX86State *env)
         val = env->mtrr_deftype;
         break;
     case MSR_CORE_THREAD_COUNT:
-        val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 15..0 */
-        val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
+        val = cpu_x86_get_msr_core_thread_count(cpu);
         break;
     default:
         /* fprintf(stderr, "%s: unknown msr 0x%x\n", __func__, msr); */
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 8e17942c3ba1..18a1bd1297a4 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2602,10 +2602,7 @@ static bool kvm_rdmsr_core_thread_count(X86CPU *cpu,
                                         uint32_t msr,
                                         uint64_t *val)
 {
-    CPUState *cs = CPU(cpu);
-
-    *val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */
-    *val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
+    *val = cpu_x86_get_msr_core_thread_count(cpu);
 
     return true;
 }
diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index 094aa56a20d1..ff7b201b44d8 100644
--- a/target/i386/tcg/sysemu/misc_helper.c
+++ b/target/i386/tcg/sysemu/misc_helper.c
@@ -468,8 +468,7 @@ void helper_rdmsr(CPUX86State *env)
         val = x86_cpu->ucode_rev;
         break;
     case MSR_CORE_THREAD_COUNT: {
-        CPUState *cs = CPU(x86_cpu);
-        val = (cs->nr_threads * cs->nr_cores) | (cs->nr_cores << 16);
+        val = cpu_x86_get_msr_core_thread_count(x86_cpu);
         break;
     }
     case MSR_APIC_START ... MSR_APIC_END: {
-- 
2.34.1

[PATCH v2 02/10] i386/cpu: Drop the variable smp_cores and smp_threads in x86_cpu_pre_plug()

[Xiaoyao Li]

No need to define smp_cores and smp_threads, just using ms->smp.cores
and ms->smp.threads is straightforward. It's also consistent with other
checks of socket/die/module.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 hw/i386/x86-common.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index 3f7818269234..32a8d7a9db87 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -248,8 +248,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
     CPUX86State *env = &cpu->env;
     MachineState *ms = MACHINE(hotplug_dev);
     X86MachineState *x86ms = X86_MACHINE(hotplug_dev);
-    unsigned int smp_cores = ms->smp.cores;
-    unsigned int smp_threads = ms->smp.threads;
     X86CPUTopoInfo topo_info;
 
     if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
@@ -329,17 +327,17 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
         if (cpu->core_id < 0) {
             error_setg(errp, "CPU core-id is not set");
             return;
-        } else if (cpu->core_id > (smp_cores - 1)) {
+        } else if (cpu->core_id > (ms->smp.cores - 1)) {
             error_setg(errp, "Invalid CPU core-id: %u must be in range 0:%u",
-                       cpu->core_id, smp_cores - 1);
+                       cpu->core_id, ms->smp.cores - 1);
             return;
         }
         if (cpu->thread_id < 0) {
             error_setg(errp, "CPU thread-id is not set");
             return;
-        } else if (cpu->thread_id > (smp_threads - 1)) {
+        } else if (cpu->thread_id > (ms->smp.threads - 1)) {
             error_setg(errp, "Invalid CPU thread-id: %u must be in range 0:%u",
-                       cpu->thread_id, smp_threads - 1);
+                       cpu->thread_id, ms->smp.threads - 1);
             return;
         }
 
-- 
2.34.1

[PATCH v2 03/10] i386/cpu: Drop cores_per_pkg in cpu_x86_cpuid()

[Xiaoyao Li]

Local variable cores_per_pkg is only used to calculate threads_per_pkg.
No need for it. Drop it and open-code it instead.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 525339945920..ad6889abdf5e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6498,7 +6498,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     uint32_t limit;
     uint32_t signature[3];
     X86CPUTopoInfo topo_info;
-    uint32_t cores_per_pkg;
     uint32_t threads_per_pkg;
 
     topo_info.dies_per_pkg = env->nr_dies;
@@ -6506,9 +6505,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules;
     topo_info.threads_per_core = cs->nr_threads;
 
-    cores_per_pkg = topo_info.cores_per_module * topo_info.modules_per_die *
-                    topo_info.dies_per_pkg;
-    threads_per_pkg = cores_per_pkg * topo_info.threads_per_core;
+    threads_per_pkg = topo_info.threads_per_core * topo_info.cores_per_module *
+                      topo_info.modules_per_die * topo_info.dies_per_pkg;
 
     /* Calculate & apply limits for different index ranges */
     if (index >= 0xC0000000) {
-- 
2.34.1

[PATCH v2 04/10] i386/topology: Update the comment of x86_apicid_from_topo_ids()

[Xiaoyao Li]

Update the comment of x86_apicid_from_topo_ids() to match the current
implementation,

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
---
 include/hw/i386/topology.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index b2c8bf2de158..21b65219a5ca 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -121,9 +121,10 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
 }
 
 /*
- * Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
+ * Make APIC ID for the CPU based on topology and IDs of each topology level.
  *
- * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
+ * The caller must make sure the ID of each level doesn't exceed the width of
+ * the level.
  */
 static inline apic_id_t x86_apicid_from_topo_ids(X86CPUTopoInfo *topo_info,
                                                  const X86CPUTopoIDs *topo_ids)
-- 
2.34.1

[PATCH v2 05/10] i386/topology: Introduce helpers for various topology info of different level

[Xiaoyao Li]

Introduce various helpers for getting the topology info of different
semantics. Using the helper is more self-explanatory.

Besides, the semantic of the helper will stay unchanged even when new
topology is added in the future. At that time, updating the
implementation of the helper without affecting the callers.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 include/hw/i386/topology.h | 25 +++++++++++++++++++++++++
 target/i386/cpu.c          | 11 ++++-------
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index 21b65219a5ca..f6380f1ed756 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -203,4 +203,29 @@ static inline bool x86_has_extended_topo(unsigned long *topo_bitmap)
            test_bit(CPU_TOPOLOGY_LEVEL_DIE, topo_bitmap);
 }
 
+static inline unsigned x86_module_per_pkg(X86CPUTopoInfo *topo_info)
+{
+    return topo_info->modules_per_die * topo_info->dies_per_pkg;
+}
+
+static inline unsigned x86_cores_per_pkg(X86CPUTopoInfo *topo_info)
+{
+    return topo_info->cores_per_module * x86_module_per_pkg(topo_info);
+}
+
+static inline unsigned x86_threads_per_pkg(X86CPUTopoInfo *topo_info)
+{
+    return topo_info->threads_per_core * x86_cores_per_pkg(topo_info);
+}
+
+static inline unsigned x86_threads_per_module(X86CPUTopoInfo *topo_info)
+{
+    return topo_info->threads_per_core * topo_info->cores_per_module;
+}
+
+static inline unsigned x86_threads_per_die(X86CPUTopoInfo *topo_info)
+{
+    return x86_threads_per_module(topo_info) * topo_info->modules_per_die;
+}
+
 #endif /* HW_I386_TOPOLOGY_H */
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ad6889abdf5e..f11b5d401a77 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -311,13 +311,11 @@ static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info,
     case CPU_TOPOLOGY_LEVEL_CORE:
         return topo_info->threads_per_core;
     case CPU_TOPOLOGY_LEVEL_MODULE:
-        return topo_info->threads_per_core * topo_info->cores_per_module;
+        return x86_threads_per_module(topo_info);
     case CPU_TOPOLOGY_LEVEL_DIE:
-        return topo_info->threads_per_core * topo_info->cores_per_module *
-               topo_info->modules_per_die;
+        return x86_threads_per_die(topo_info);
     case CPU_TOPOLOGY_LEVEL_SOCKET:
-        return topo_info->threads_per_core * topo_info->cores_per_module *
-               topo_info->modules_per_die * topo_info->dies_per_pkg;
+        return x86_threads_per_pkg(topo_info);
     default:
         g_assert_not_reached();
     }
@@ -6505,8 +6503,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules;
     topo_info.threads_per_core = cs->nr_threads;
 
-    threads_per_pkg = topo_info.threads_per_core * topo_info.cores_per_module *
-                      topo_info.modules_per_die * topo_info.dies_per_pkg;
+    threads_per_pkg = x86_threads_per_pkg(&topo_info);
 
     /* Calculate & apply limits for different index ranges */
     if (index >= 0xC0000000) {
-- 
2.34.1

[PATCH v2 06/10] i386/cpu: Track a X86CPUTopoInfo directly in CPUX86State

[Xiaoyao Li]

The name of nr_modules/nr_dies are ambiguous and they mislead people.

The purpose of them is to record and form the topology information. So
just maintain a X86CPUTopoInfo member in CPUX86State instead. Then
nr_modules and nr_dies can be dropped.

As the benefit, x86 can switch to use information in
CPUX86State::topo_info and get rid of the nr_cores and nr_threads in
CPUState. This helps remove the dependency on qemu_init_vcpu() so that
x86 can get and use topology info earlier in x86_cpu_realizefn().

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 hw/i386/x86-common.c     | 12 ++++------
 target/i386/cpu-sysemu.c |  6 ++---
 target/i386/cpu.c        | 51 ++++++++++++++++------------------------
 target/i386/cpu.h        |  6 +----
 4 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index 32a8d7a9db87..26c3e3169cd4 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -248,7 +248,7 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
     CPUX86State *env = &cpu->env;
     MachineState *ms = MACHINE(hotplug_dev);
     X86MachineState *x86ms = X86_MACHINE(hotplug_dev);
-    X86CPUTopoInfo topo_info;
+    X86CPUTopoInfo *topo_info = &env->topo_info;
 
     if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
         error_setg(errp, "Invalid CPU type, expected cpu type: '%s'",
@@ -267,15 +267,13 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
         }
     }
 
-    init_topo_info(&topo_info, x86ms);
+    init_topo_info(topo_info, x86ms);
 
     if (ms->smp.modules > 1) {
-        env->nr_modules = ms->smp.modules;
         set_bit(CPU_TOPOLOGY_LEVEL_MODULE, env->avail_cpu_topo);
     }
 
     if (ms->smp.dies > 1) {
-        env->nr_dies = ms->smp.dies;
         set_bit(CPU_TOPOLOGY_LEVEL_DIE, env->avail_cpu_topo);
     }
 
@@ -346,12 +344,12 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
         topo_ids.module_id = cpu->module_id;
         topo_ids.core_id = cpu->core_id;
         topo_ids.smt_id = cpu->thread_id;
-        cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids);
+        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);
+        x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);
 
         error_setg(errp,
             "Invalid CPU [socket: %u, die: %u, module: %u, core: %u, thread: %u]"
@@ -374,7 +372,7 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
     /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
      * once -smp refactoring is complete and there will be CPU private
      * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
-    x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
+    x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);
     if (cpu->socket_id != -1 && cpu->socket_id != topo_ids.pkg_id) {
         error_setg(errp, "property socket-id: %u doesn't match set apic-id:"
             " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id,
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 4e9df0bc0156..31b37c63252c 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -312,11 +312,11 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
 
 uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
 {
-    CPUState *cs = CPU(cpu);
+    CPUX86State *env = &cpu->env;
     uint64_t val;
 
-    val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 15..0 */
-    val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
+    val = x86_threads_per_pkg(&env->topo_info);  /* thread count, bits 15..0 */
+    val |= x86_cores_per_pkg(&env->topo_info) << 16; /* core count, bits 31..16 */
 
     return val;
 }
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f11b5d401a77..d41768648ab9 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6495,15 +6495,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     CPUState *cs = env_cpu(env);
     uint32_t limit;
     uint32_t signature[3];
-    X86CPUTopoInfo topo_info;
+    X86CPUTopoInfo *topo_info = &env->topo_info;
     uint32_t threads_per_pkg;
 
-    topo_info.dies_per_pkg = env->nr_dies;
-    topo_info.modules_per_die = env->nr_modules;
-    topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules;
-    topo_info.threads_per_core = cs->nr_threads;
-
-    threads_per_pkg = x86_threads_per_pkg(&topo_info);
+    threads_per_pkg = x86_threads_per_pkg(topo_info);
 
     /* Calculate & apply limits for different index ranges */
     if (index >= 0xC0000000) {
@@ -6580,12 +6575,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                 int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
 
                 *eax &= ~0xFC000000;
-                *eax |= max_core_ids_in_package(&topo_info) << 26;
+                *eax |= max_core_ids_in_package(topo_info) << 26;
                 if (host_vcpus_per_cache > threads_per_pkg) {
                     *eax &= ~0x3FFC000;
 
                     /* Share the cache at package level. */
-                    *eax |= max_thread_ids_for_cache(&topo_info,
+                    *eax |= max_thread_ids_for_cache(topo_info,
                                 CPU_TOPOLOGY_LEVEL_SOCKET) << 14;
                 }
             }
@@ -6597,7 +6592,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             switch (count) {
             case 0: /* L1 dcache info */
                 encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache,
-                                    &topo_info,
+                                    topo_info,
                                     eax, ebx, ecx, edx);
                 if (!cpu->l1_cache_per_core) {
                     *eax &= ~MAKE_64BIT_MASK(14, 12);
@@ -6605,7 +6600,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                 break;
             case 1: /* L1 icache info */
                 encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache,
-                                    &topo_info,
+                                    topo_info,
                                     eax, ebx, ecx, edx);
                 if (!cpu->l1_cache_per_core) {
                     *eax &= ~MAKE_64BIT_MASK(14, 12);
@@ -6613,13 +6608,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                 break;
             case 2: /* L2 cache info */
                 encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache,
-                                    &topo_info,
+                                    topo_info,
                                     eax, ebx, ecx, edx);
                 break;
             case 3: /* L3 cache info */
                 if (cpu->enable_l3_cache) {
                     encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache,
-                                        &topo_info,
+                                        topo_info,
                                         eax, ebx, ecx, edx);
                     break;
                 }
@@ -6702,12 +6697,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
 
         switch (count) {
         case 0:
-            *eax = apicid_core_offset(&topo_info);
-            *ebx = topo_info.threads_per_core;
+            *eax = apicid_core_offset(topo_info);
+            *ebx = topo_info->threads_per_core;
             *ecx |= CPUID_B_ECX_TOPO_LEVEL_SMT << 8;
             break;
         case 1:
-            *eax = apicid_pkg_offset(&topo_info);
+            *eax = apicid_pkg_offset(topo_info);
             *ebx = threads_per_pkg;
             *ecx |= CPUID_B_ECX_TOPO_LEVEL_CORE << 8;
             break;
@@ -6733,7 +6728,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             break;
         }
 
-        encode_topo_cpuid1f(env, count, &topo_info, eax, ebx, ecx, edx);
+        encode_topo_cpuid1f(env, count, topo_info, eax, ebx, ecx, edx);
         break;
     case 0xD: {
         /* Processor Extended State */
@@ -7036,7 +7031,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
              * thread ID within a package".
              * Bits 7:0 is "The number of threads in the package is NC+1"
              */
-            *ecx = (apicid_pkg_offset(&topo_info) << 12) |
+            *ecx = (apicid_pkg_offset(topo_info) << 12) |
                    (threads_per_pkg - 1);
         } else {
             *ecx = 0;
@@ -7065,19 +7060,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         switch (count) {
         case 0: /* L1 dcache info */
             encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
-                                       &topo_info, eax, ebx, ecx, edx);
+                                       topo_info, eax, ebx, ecx, edx);
             break;
         case 1: /* L1 icache info */
             encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
-                                       &topo_info, eax, ebx, ecx, edx);
+                                       topo_info, eax, ebx, ecx, edx);
             break;
         case 2: /* L2 cache info */
             encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
-                                       &topo_info, eax, ebx, ecx, edx);
+                                       topo_info, eax, ebx, ecx, edx);
             break;
         case 3: /* L3 cache info */
             encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
-                                       &topo_info, eax, ebx, ecx, edx);
+                                       topo_info, eax, ebx, ecx, edx);
             break;
         default: /* end of info */
             *eax = *ebx = *ecx = *edx = 0;
@@ -7089,7 +7084,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 0x8000001E:
         if (cpu->core_id <= 255) {
-            encode_topo_cpuid8000001e(cpu, &topo_info, eax, ebx, ecx, edx);
+            encode_topo_cpuid8000001e(cpu, topo_info, eax, ebx, ecx, edx);
         } else {
             *eax = 0;
             *ebx = 0;
@@ -7994,17 +7989,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
      * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
      * based on inputs (sockets,cores,threads), it is still better to give
      * users a warning.
-     *
-     * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
-     * cs->nr_threads hasn't be populated yet and the checking is incorrect.
      */
     if (IS_AMD_CPU(env) &&
         !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
-        cs->nr_threads > 1) {
+        env->topo_info.threads_per_core > 1) {
             warn_report_once("This family of AMD CPU doesn't support "
                              "hyperthreading(%d). Please configure -smp "
                              "options properly or try enabling topoext "
-                             "feature.", cs->nr_threads);
+                             "feature.", env->topo_info.threads_per_core);
     }
 
 #ifndef CONFIG_USER_ONLY
@@ -8165,9 +8157,6 @@ static void x86_cpu_init_default_topo(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
 
-    env->nr_modules = 1;
-    env->nr_dies = 1;
-
     /* thread, core and socket levels are set by default. */
     set_bit(CPU_TOPOLOGY_LEVEL_THREAD, env->avail_cpu_topo);
     set_bit(CPU_TOPOLOGY_LEVEL_CORE, env->avail_cpu_topo);
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 5e1beca94a62..bcdebdf48f7c 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2045,11 +2045,7 @@ typedef struct CPUArchState {
 
     TPRAccess tpr_access_type;
 
-    /* Number of dies within this CPU package. */
-    unsigned nr_dies;
-
-    /* Number of modules within one die. */
-    unsigned nr_modules;
+    X86CPUTopoInfo topo_info;
 
     /* Bitmap of available CPU topology levels for this CPU. */
     DECLARE_BITMAP(avail_cpu_topo, CPU_TOPOLOGY_LEVEL__MAX);
-- 
2.34.1

[PATCH v2 07/10] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core

[Xiaoyao Li]

Now it changes to use env->topo_info.threads_per_core and doesn't depend
on qemu_init_vcpu() anymore. Drop the comment of the order dependency on
qemu_init_vcpu() and hoist code to put it together with other feature
checking.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d41768648ab9..fd59da5d445d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7880,6 +7880,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
      */
     cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
 
+    /*
+     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
+     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+     * based on inputs (sockets,cores,threads), it is still better to give
+     * users a warning.
+     */
+    if (IS_AMD_CPU(env) &&
+        !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+        env->nr_threads > 1) {
+            warn_report_once("This family of AMD CPU doesn't support "
+                             "hyperthreading(%d). Please configure -smp "
+                             "options properly or try enabling topoext "
+                             "feature.", env->nr_threads);
+    }
+
     /* For 64bit systems think about the number of physical bits to present.
      * ideally this should be the same as the host; anything other than matching
      * the host can cause incorrect guest behaviour.
@@ -7984,21 +7999,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     x86_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 
-    /*
-     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
-     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
-     * based on inputs (sockets,cores,threads), it is still better to give
-     * users a warning.
-     */
-    if (IS_AMD_CPU(env) &&
-        !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
-        env->topo_info.threads_per_core > 1) {
-            warn_report_once("This family of AMD CPU doesn't support "
-                             "hyperthreading(%d). Please configure -smp "
-                             "options properly or try enabling topoext "
-                             "feature.", env->topo_info.threads_per_core);
-    }
-
 #ifndef CONFIG_USER_ONLY
     x86_cpu_apic_realize(cpu, &local_err);
     if (local_err != NULL) {
-- 
2.34.1

[PATCH v2 08/10] cpu: Remove nr_cores from struct CPUState

[Xiaoyao Li]

There is no user of it now, remove it.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/core/cpu-common.c  | 1 -
 include/hw/core/cpu.h | 2 --
 system/cpus.c         | 1 -
 3 files changed, 4 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 09c79035949b..77089d4ed304 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -243,7 +243,6 @@ static void cpu_common_initfn(Object *obj)
     cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX;
     /* user-mode doesn't have configurable SMP topology */
     /* the default value is changed by qemu_init_vcpu() for system-mode */
-    cpu->nr_cores = 1;
     cpu->nr_threads = 1;
     cpu->cflags_next_tb = -1;
 
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index c3ca0babcb3f..fb397cdfc53d 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -407,7 +407,6 @@ struct qemu_work_item;
  *   Under TCG this value is propagated to @tcg_cflags.
  *   See TranslationBlock::TCG CF_CLUSTER_MASK.
  * @tcg_cflags: Pre-computed cflags for this cpu.
- * @nr_cores: Number of cores within this CPU package.
  * @nr_threads: Number of threads within this CPU core.
  * @thread: Host thread details, only live once @created is #true
  * @sem: WIN32 only semaphore used only for qtest
@@ -466,7 +465,6 @@ struct CPUState {
     CPUClass *cc;
     /*< public >*/
 
-    int nr_cores;
     int nr_threads;
 
     struct QemuThread *thread;
diff --git a/system/cpus.c b/system/cpus.c
index ba633c7688b2..3db4a7d0ab4a 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -681,7 +681,6 @@ void qemu_init_vcpu(CPUState *cpu)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
 
-    cpu->nr_cores = machine_topo_get_cores_per_socket(ms);
     cpu->nr_threads =  ms->smp.threads;
     cpu->stopped = true;
     cpu->random_seed = qemu_guest_random_seed_thread_part1();
-- 
2.34.1

[PATCH v2 09/10] i386/cpu: Set up CPUID_HT in x86_cpu_expand_features() instead of cpu_x86_cpuid()

[Xiaoyao Li]

Currently CPUID_HT is evaluated in cpu_x86_cpuid() each time. It's not a
correct usage of how feature bit is maintained and evaluated. The
expected practice is that features are tracked in env->features[] and
cpu_x86_cpuid() should be the consumer of env->features[].

Track CPUID_HT in env->features[FEAT_1_EDX] instead and evaluate it in
cpu's realizefn().

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
There is one issue[1] of CPUID_HT being user settable that when
"-cpu xxx,-ht" with "-smp 2", HT flag is still exposed to guest.
However, the issue is not irrelevant to this patch. If anyone has
interest to reslove it please go ahead.

[1] https://lore.kernel.org/qemu-devel/Z1FUDGnenETEFV6Z@intel.com/
---
 target/i386/cpu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fd59da5d445d..bee494bdd029 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6537,7 +6537,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *edx = env->features[FEAT_1_EDX];
         if (threads_per_pkg > 1) {
             *ebx |= threads_per_pkg << 16;
-            *edx |= CPUID_HT;
         }
         if (!cpu->enable_pmu) {
             *ecx &= ~CPUID_EXT_PDCM;
@@ -7528,6 +7527,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         }
     }
 
+    if (x86_threads_per_pkg(&env->topo_info) > 1) {
+        env->features[FEAT_1_EDX] |= CPUID_HT;
+    }
+
     for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
         FeatureDep *d = &feature_dependencies[i];
         if (!(env->features[d->from.index] & d->from.mask)) {
-- 
2.34.1

[PATCH v2 10/10] i386/cpu: Set and track CPUID_EXT3_CMP_LEG in env->features[FEAT_8000_0001_ECX]

[Xiaoyao Li]

The correct usage is tracking and maintaining features in env->features[]
instead of manually set it in cpu_x86_cpuid().

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index bee494bdd029..8d3744aa6d26 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6952,17 +6952,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *ecx = env->features[FEAT_8000_0001_ECX];
         *edx = env->features[FEAT_8000_0001_EDX];
 
-        /* The Linux kernel checks for the CMPLegacy bit and
-         * discards multiple thread information if it is set.
-         * So don't set it here for Intel to make Linux guests happy.
-         */
-        if (threads_per_pkg > 1) {
-            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
-                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
-                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
-                *ecx |= 1 << 1;    /* CmpLegacy bit */
-            }
-        }
         if (tcg_enabled() && env->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 &&
             !(env->hflags & HF_LMA_MASK)) {
             *edx &= ~CPUID_EXT2_SYSCALL;
@@ -7529,6 +7518,15 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
 
     if (x86_threads_per_pkg(&env->topo_info) > 1) {
         env->features[FEAT_1_EDX] |= CPUID_HT;
+
+        /*
+         * The Linux kernel checks for the CMPLegacy bit and
+         * discards multiple thread information if it is set.
+         * So don't set it here for Intel to make Linux guests happy.
+         */
+        if (!IS_INTEL_CPU(env)) {
+            env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_CMP_LEG;
+        }
     }
 
     for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
-- 
2.34.1

Re: [PATCH v2 01/10] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Zhao Liu]

On Thu, Dec 19, 2024 at 06:01:16AM -0500, Xiaoyao Li wrote:
> Date: Thu, 19 Dec 2024 06:01:16 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH v2 01/10] i386/cpu: Extract a common fucntion to setup
>  value of MSR_CORE_THREAD_COUNT
> X-Mailer: git-send-email 2.34.1
> 
> There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
> Extract a common function for it.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> Changes in v2:
> - move the implementation of cpu_x86_get_msr_core_thread_count() to
>   target/i386/cpu-sysemu.c;
> ---
>  target/i386/cpu-sysemu.c             | 11 +++++++++++
>  target/i386/cpu.h                    |  2 ++
>  target/i386/hvf/x86_emu.c            |  3 +--
>  target/i386/kvm/kvm.c                |  5 +----
>  target/i386/tcg/sysemu/misc_helper.c |  3 +--
>  5 files changed, 16 insertions(+), 8 deletions(-)
> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH v2 03/10] i386/cpu: Drop cores_per_pkg in cpu_x86_cpuid()

[Zhao Liu]

On Thu, Dec 19, 2024 at 06:01:18AM -0500, Xiaoyao Li wrote:
> Date: Thu, 19 Dec 2024 06:01:18 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH v2 03/10] i386/cpu: Drop cores_per_pkg in cpu_x86_cpuid()
> X-Mailer: git-send-email 2.34.1
> 
> Local variable cores_per_pkg is only used to calculate threads_per_pkg.
> No need for it. Drop it and open-code it instead.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/cpu.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH v2 02/10] i386/cpu: Drop the variable smp_cores and smp_threads in x86_cpu_pre_plug()

[Zhao Liu]

On Thu, Dec 19, 2024 at 06:01:17AM -0500, Xiaoyao Li wrote:
> Date: Thu, 19 Dec 2024 06:01:17 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH v2 02/10] i386/cpu: Drop the variable smp_cores and
>  smp_threads in x86_cpu_pre_plug()
> X-Mailer: git-send-email 2.34.1
> 
> No need to define smp_cores and smp_threads, just using ms->smp.cores
> and ms->smp.threads is straightforward. It's also consistent with other
> checks of socket/die/module.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  hw/i386/x86-common.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH v2 05/10] i386/topology: Introduce helpers for various topology info of different level

[Zhao Liu]

> +static inline unsigned x86_module_per_pkg(X86CPUTopoInfo *topo_info)
> +{
> +    return topo_info->modules_per_die * topo_info->dies_per_pkg;
> +}
> +
> +static inline unsigned x86_cores_per_pkg(X86CPUTopoInfo *topo_info)
> +{
> +    return topo_info->cores_per_module * x86_module_per_pkg(topo_info);
> +}

The above helpers can be ignored this time until someone wants them...

> +static inline unsigned x86_threads_per_pkg(X86CPUTopoInfo *topo_info)
> +{
> +    return topo_info->threads_per_core * x86_cores_per_pkg(topo_info);
> +}

...then this can be x86_threads_per_die(topo_info) * topo_info->dies_per_package

> +static inline unsigned x86_threads_per_module(X86CPUTopoInfo *topo_info)
> +{
> +    return topo_info->threads_per_core * topo_info->cores_per_module;
> +}
> +
> +static inline unsigned x86_threads_per_die(X86CPUTopoInfo *topo_info)
> +{
> +    return x86_threads_per_module(topo_info) * topo_info->modules_per_die;
> +}
> +
> 

Re: [PATCH v2 06/10] i386/cpu: Track a X86CPUTopoInfo directly in CPUX86State

[Zhao Liu]

> @@ -8165,9 +8157,6 @@ static void x86_cpu_init_default_topo(X86CPU *cpu)
>  {
>      CPUX86State *env = &cpu->env;
>  
> -    env->nr_modules = 1;
> -    env->nr_dies = 1;
> -

Instead, initialize topo_info?

>      /* thread, core and socket levels are set by default. */
>      set_bit(CPU_TOPOLOGY_LEVEL_THREAD, env->avail_cpu_topo);

Re: [PATCH v2 07/10] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core

[Zhao Liu]

On Thu, Dec 19, 2024 at 06:01:22AM -0500, Xiaoyao Li wrote:
> Date: Thu, 19 Dec 2024 06:01:22 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH v2 07/10] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT
>  against threads_per_core
> X-Mailer: git-send-email 2.34.1
> 
> Now it changes to use env->topo_info.threads_per_core and doesn't depend
> on qemu_init_vcpu() anymore. Drop the comment of the order dependency on
> qemu_init_vcpu() and hoist code to put it together with other feature
> checking.

The comment was dropped in patch 6. You could move such description to
there.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/cpu.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d41768648ab9..fd59da5d445d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7880,6 +7880,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>       */
>      cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
>  
> +    /*
> +     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
> +     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
> +     * based on inputs (sockets,cores,threads), it is still better to give
> +     * users a warning.
> +     */
> +    if (IS_AMD_CPU(env) &&
> +        !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
> +        env->nr_threads > 1) {

Typo, env->topo_info.threads_per_core.

> +            warn_report_once("This family of AMD CPU doesn't support "
> +                             "hyperthreading(%d). Please configure -smp "
> +                             "options properly or try enabling topoext "
> +                             "feature.", env->nr_threads);
> +    }
> +
>      /* For 64bit systems think about the number of physical bits to present.
>       * ideally this should be the same as the host; anything other than matching
>       * the host can cause incorrect guest behaviour.

Re: [PATCH v2 01/10] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Philippe Mathieu-Daudé]

On 19/12/24 12:01, Xiaoyao Li wrote:
> There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
> Extract a common function for it.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> Changes in v2:
> - move the implementation of cpu_x86_get_msr_core_thread_count() to
>    target/i386/cpu-sysemu.c;
> ---
>   target/i386/cpu-sysemu.c             | 11 +++++++++++
>   target/i386/cpu.h                    |  2 ++
>   target/i386/hvf/x86_emu.c            |  3 +--
>   target/i386/kvm/kvm.c                |  5 +----
>   target/i386/tcg/sysemu/misc_helper.c |  3 +--
>   5 files changed, 16 insertions(+), 8 deletions(-)


> +uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
> +{
> +    CPUState *cs = CPU(cpu);
> +    uint64_t val;
> +
> +    val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 15..0 */
> +    val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
> +
> +    return val;

Alternatively:

        return deposit64(cs->nr_threads * cs->nr_cores, 16, 16,
                         cs->nr_cores);

> +}

Re: [PATCH v2 01/10] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Philippe Mathieu-Daudé]

On 28/12/24 18:37, Philippe Mathieu-Daudé wrote:
> On 19/12/24 12:01, Xiaoyao Li wrote:
>> There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
>> Extract a common function for it.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> Changes in v2:
>> - move the implementation of cpu_x86_get_msr_core_thread_count() to
>>    target/i386/cpu-sysemu.c;
>> ---
>>   target/i386/cpu-sysemu.c             | 11 +++++++++++
>>   target/i386/cpu.h                    |  2 ++
>>   target/i386/hvf/x86_emu.c            |  3 +--
>>   target/i386/kvm/kvm.c                |  5 +----
>>   target/i386/tcg/sysemu/misc_helper.c |  3 +--
>>   5 files changed, 16 insertions(+), 8 deletions(-)
> 
> 
>> +uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
>> +{
>> +    CPUState *cs = CPU(cpu);
>> +    uint64_t val;
>> +
>> +    val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 15..0 */
>> +    val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
>> +
>> +    return val;
> 
> Alternatively:
> 
>         return deposit64(cs->nr_threads * cs->nr_cores, 16, 16,
>                          cs->nr_cores);
> 
>> +}
> 

Typo "function" in patch subject.

Re: [PATCH v2 01/10] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Xiaoyao Li]

On 1/3/2025 4:52 PM, Philippe Mathieu-Daudé wrote:
> On 28/12/24 18:37, Philippe Mathieu-Daudé wrote:
>> On 19/12/24 12:01, Xiaoyao Li wrote:
>>> There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
>>> Extract a common function for it.
>>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> ---
>>> Changes in v2:
>>> - move the implementation of cpu_x86_get_msr_core_thread_count() to
>>>    target/i386/cpu-sysemu.c;
>>> ---
>>>   target/i386/cpu-sysemu.c             | 11 +++++++++++
>>>   target/i386/cpu.h                    |  2 ++
>>>   target/i386/hvf/x86_emu.c            |  3 +--
>>>   target/i386/kvm/kvm.c                |  5 +----
>>>   target/i386/tcg/sysemu/misc_helper.c |  3 +--
>>>   5 files changed, 16 insertions(+), 8 deletions(-)
>>
>>
>>> +uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
>>> +{
>>> +    CPUState *cs = CPU(cpu);
>>> +    uint64_t val;
>>> +
>>> +    val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 
>>> 15..0 */
>>> +    val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 
>>> 31..16 */
>>> +
>>> +    return val;
>>
>> Alternatively:
>>
>>         return deposit64(cs->nr_threads * cs->nr_cores, 16, 16,
>>                          cs->nr_cores);

I would rather keep it as-is as suguested by Philippe[1].

(deposit64() is really a new thing for i386)

[1] 
https://lore.kernel.org/qemu-devel/20241210173524.48e203a3@imammedo.users.ipa.redhat.com/

>>> +}
>>
> 
> Typo "function" in patch subject.

thanks for catching it!

Re: [PATCH v2 01/10] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Xiaoyao Li]

On 1/7/2025 12:31 PM, Xiaoyao Li wrote:
> On 1/3/2025 4:52 PM, Philippe Mathieu-Daudé wrote:
>> On 28/12/24 18:37, Philippe Mathieu-Daudé wrote:
>>> On 19/12/24 12:01, Xiaoyao Li wrote:
>>>> There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
>>>> Extract a common function for it.
>>>>
>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>> ---
>>>> Changes in v2:
>>>> - move the implementation of cpu_x86_get_msr_core_thread_count() to
>>>>    target/i386/cpu-sysemu.c;
>>>> ---
>>>>   target/i386/cpu-sysemu.c             | 11 +++++++++++
>>>>   target/i386/cpu.h                    |  2 ++
>>>>   target/i386/hvf/x86_emu.c            |  3 +--
>>>>   target/i386/kvm/kvm.c                |  5 +----
>>>>   target/i386/tcg/sysemu/misc_helper.c |  3 +--
>>>>   5 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>>
>>>> +uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
>>>> +{
>>>> +    CPUState *cs = CPU(cpu);
>>>> +    uint64_t val;
>>>> +
>>>> +    val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 
>>>> 15..0 */
>>>> +    val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 
>>>> 31..16 */
>>>> +
>>>> +    return val;
>>>
>>> Alternatively:
>>>
>>>         return deposit64(cs->nr_threads * cs->nr_cores, 16, 16,
>>>                          cs->nr_cores);
> 
> I would rather keep it as-is as suguested by Philippe[1].

Sorry for being wrong with the name. (awkward)

s/Philippe/Igor

> (deposit64() is really a new thing for i386)
> 
> [1] https://lore.kernel.org/qemu- 
> devel/20241210173524.48e203a3@imammedo.users.ipa.redhat.com/
> 
>>>> +}
>>>
>>
>> Typo "function" in patch subject.
> 
> thanks for catching it!
> 
> 

Re: [PATCH v2 07/10] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core

[Xiaoyao Li]

On 12/27/2024 4:07 PM, Zhao Liu wrote:
> On Thu, Dec 19, 2024 at 06:01:22AM -0500, Xiaoyao Li wrote:
>> Date: Thu, 19 Dec 2024 06:01:22 -0500
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>> Subject: [PATCH v2 07/10] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT
>>   against threads_per_core
>> X-Mailer: git-send-email 2.34.1
>>
>> Now it changes to use env->topo_info.threads_per_core and doesn't depend
>> on qemu_init_vcpu() anymore. Drop the comment of the order dependency on
>> qemu_init_vcpu() and hoist code to put it together with other feature
>> checking.
> 
> The comment was dropped in patch 6. You could move such description to
> there.

Obviously, I didn't take care of the patch split and reorder.

I tent to move the dropping of the comment from patch 6 to this one.

>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   target/i386/cpu.c | 30 +++++++++++++++---------------
>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index d41768648ab9..fd59da5d445d 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -7880,6 +7880,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>        */
>>       cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
>>   
>> +    /*
>> +     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
>> +     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
>> +     * based on inputs (sockets,cores,threads), it is still better to give
>> +     * users a warning.
>> +     */
>> +    if (IS_AMD_CPU(env) &&
>> +        !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
>> +        env->nr_threads > 1) {
> 
> Typo, env->topo_info.threads_per_core.

Thanks for catching it.

>> +            warn_report_once("This family of AMD CPU doesn't support "
>> +                             "hyperthreading(%d). Please configure -smp "
>> +                             "options properly or try enabling topoext "
>> +                             "feature.", env->nr_threads);
>> +    }
>> +
>>       /* For 64bit systems think about the number of physical bits to present.
>>        * ideally this should be the same as the host; anything other than matching
>>        * the host can cause incorrect guest behaviour.

Re: [PATCH v2 05/10] i386/topology: Introduce helpers for various topology info of different level

[Xiaoyao Li]

On 12/27/2024 3:51 PM, Zhao Liu wrote:
>> +static inline unsigned x86_module_per_pkg(X86CPUTopoInfo *topo_info)
>> +{
>> +    return topo_info->modules_per_die * topo_info->dies_per_pkg;
>> +}
>> +
>> +static inline unsigned x86_cores_per_pkg(X86CPUTopoInfo *topo_info)
>> +{
>> +    return topo_info->cores_per_module * x86_module_per_pkg(topo_info);
>> +}
> 
> The above helpers can be ignored this time until someone wants them...

x86_cores_per_pkg() will be used in next patch (06).

So the only one without a real user is x86_module_per_pkg(). We can drop 
it, and implement x86_cores_per_pkg() as

	return  topo_info->cores_per_module * topo_info->modules_per_die
		* topo_info->dies_per_pkg;

However, I don't see no real user as a big concern and I think current 
implementation looks more consistent and complete.

>> +static inline unsigned x86_threads_per_pkg(X86CPUTopoInfo *topo_info)
>> +{
>> +    return topo_info->threads_per_core * x86_cores_per_pkg(topo_info);
>> +}
> 
> ...then this can be x86_threads_per_die(topo_info) * topo_info->dies_per_package
> 
>> +static inline unsigned x86_threads_per_module(X86CPUTopoInfo *topo_info)
>> +{
>> +    return topo_info->threads_per_core * topo_info->cores_per_module;
>> +}
>> +
>> +static inline unsigned x86_threads_per_die(X86CPUTopoInfo *topo_info)
>> +{
>> +    return x86_threads_per_module(topo_info) * topo_info->modules_per_die;
>> +}
>> +
>>

Re: [PATCH v2 01/10] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Philippe Mathieu-Daudé]

On 7/1/25 05:34, Xiaoyao Li wrote:
> On 1/7/2025 12:31 PM, Xiaoyao Li wrote:
>> On 1/3/2025 4:52 PM, Philippe Mathieu-Daudé wrote:
>>> On 28/12/24 18:37, Philippe Mathieu-Daudé wrote:
>>>> On 19/12/24 12:01, Xiaoyao Li wrote:
>>>>> There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
>>>>> Extract a common function for it.
>>>>>
>>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - move the implementation of cpu_x86_get_msr_core_thread_count() to
>>>>>    target/i386/cpu-sysemu.c;
>>>>> ---
>>>>>   target/i386/cpu-sysemu.c             | 11 +++++++++++
>>>>>   target/i386/cpu.h                    |  2 ++
>>>>>   target/i386/hvf/x86_emu.c            |  3 +--
>>>>>   target/i386/kvm/kvm.c                |  5 +----
>>>>>   target/i386/tcg/sysemu/misc_helper.c |  3 +--
>>>>>   5 files changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>>
>>>>> +uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
>>>>> +{
>>>>> +    CPUState *cs = CPU(cpu);
>>>>> +    uint64_t val;
>>>>> +
>>>>> +    val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 
>>>>> 15..0 */
>>>>> +    val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 
>>>>> 31..16 */
>>>>> +
>>>>> +    return val;
>>>>
>>>> Alternatively:
>>>>
>>>>         return deposit64(cs->nr_threads * cs->nr_cores, 16, 16,
>>>>                          cs->nr_cores);
>>
>> I would rather keep it as-is as suguested by Philippe[1].
> 
> Sorry for being wrong with the name. (awkward)
> 
> s/Philippe/Igor
> 
>> (deposit64() is really a new thing for i386)
>>
>> [1] https://lore.kernel.org/qemu- 
>> devel/20241210173524.48e203a3@imammedo.users.ipa.redhat.com/

Then use deposit64() in another patch on top?

>>>>> +}
>>>>
>>>
>>> Typo "function" in patch subject.
>>
>> thanks for catching it!
>>
>>
>