On 12/11/2024 12:43 AM, Igor Mammedov wrote:
On Thu, 5 Dec 2024 09:57:15 -0500
Xiaoyao Li <xiaoyao...@intel.com> wrote:
x86 is the only user of CPUState::nr_cores.
Define cores_per_module in CPUX86State, which can serve as the
substitute of CPUState::nr_cores. After x86 switches to use
CPUX86State::cores_per_module, CPUState::nr_cores will lose the only
user and QEMU can drop it.
Signed-off-by: Xiaoyao Li <xiaoyao...@intel.com>
---
hw/i386/x86-common.c | 2 ++
target/i386/cpu.c | 2 +-
target/i386/cpu.h | 9 +++++++--
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index dc031af66217..f7a20c1da30c 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -271,6 +271,8 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
init_topo_info(&topo_info, x86ms);
+ env->nr_cores = ms->smp.cores;
this doesn't look like the same as in qemu_init_vcpu(),
which uses machine_topo_get_cores_per_socket()
Can you clarify the change?
because env->nr_cores has a different meaning vs. cs->nr_cores.
As the below comments of nr_cores in the diff
+ /* Number of cores within one module. */
+ unsigned nr_cores;
it means the number of cores within one module. It uses the similar
convention as nr_dies and nr_modules in struct CPUArchState.
However, CPUState::nr_cores means the number of cores in the package.
yes, we can keep the same meaning of nr_cores as "number of cores in the
package" when define it struct CPUArchState. However, it leads to
inconsistency with nr_dies and nr_modules already there and confuses people.
+
if (ms->smp.modules > 1) {
env->nr_modules = ms->smp.modules;
set_bit(CPU_TOPOLOGY_LEVEL_MODULE, env->avail_cpu_topo);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3725dbbc4b3f..15b50c44ae79 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6503,7 +6503,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
uint32_t count,
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.cores_per_module = env->nr_cores;
topo_info.threads_per_core = cs->nr_threads;
cores_per_pkg = topo_info.cores_per_module * topo_info.modules_per_die *
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 5795a497e567..c37a49a1a02b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2051,6 +2051,9 @@ typedef struct CPUArchState {
/* Number of modules within one die. */
unsigned nr_modules;
+ /* Number of cores within one module. */
+ unsigned nr_cores;
+
/* Bitmap of available CPU topology levels for this CPU. */
DECLARE_BITMAP(avail_cpu_topo, CPU_TOPOLOGY_LEVEL__MAX);
} CPUX86State;
@@ -2393,10 +2396,12 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU
*cpu,
static inline uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
{
CPUState *cs = CPU(cpu);
+ CPUX86State *env = &cpu->env;
uint64_t val;
+ uint64_t cores_per_package = env->nr_cores * env->nr_modules *
env->nr_dies;
- 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 = cs->nr_threads * cores_per_package; /* thread count, bits 15..0 */
+ val |= (cores_per_package << 16); /* core count, bits 31..16 */
return val;
}
