[RFC PATCH 0/4] cpu: Drop CPUState::nr_cores

[RFC PATCH 0/4] cpu: Drop CPUState::nr_cores

[Xiaoyao Li]

The series is motivated by auditing the usage of CPUState::nr_cores and
CPUState::nr_threads, which is motivated by [1].

The initial goal is to initialize nr_threads and nr_cores earlier for
x86, which leads to patches [2] and [3]. Patch [2] touches all the ARCHes
and patch [3] looks hacky. At last Igor suggested to audit nr_threads and
nr_cores, and only set them in the pre_plug() callback for the ARCHes that
really need them[1].

By audting nr_threads and nr_cores, I found nr_cores is only used by
x86. So we can introduce a x86 specific one and initialize in
x86_cpu_pre_plug(), then drop nr_cores totally.

However for nr_threads, it's used by MIPS and PPC as well[4]. There are
two options:
1. maintain separate substitute in X86/MIPS/PPS, so we can drop
CPUState::nr_threads like for CPUState::nr_cores.

2. keep CPUState::nr_threads and find place (or introduce pre_plug()) to
initialize them earlier for MIPS/PPC.

I would like to seek for opinions for which one is prefered. 

This series implments the drop for CPUState::nr_cores. Though it doesn't
help on the initial goal without the solution for nr_threads, I think it
is still a good cleanup?

BTW, by initializing nr_threads and nr_cores earlier than
qemu_init_vcpu(), it also unblocks [5].


[1] https://lore.kernel.org/qemu-devel/20241125103857.78a23715@imammedo.users.ipa.redhat.com/
[2] https://lore.kernel.org/qemu-devel/5f8db586-cdda-4d00-be02-f9880a20e1a3@redhat.com/
[3] https://lore.kernel.org/qemu-devel/20241122160317.4070177-1-xiaoyao.li@intel.com/
[4] https://lore.kernel.org/qemu-devel/045f9cb1-2b17-4b2c-985f-3c34e3626b36@intel.com/
[5] https://lore.kernel.org/qemu-devel/20231128171239.69b6d7b1@imammedo.users.ipa.redhat.com/

Xiaoyao Li (4):
  i386/topology: Update the comment of x86_apicid_from_topo_ids()
  i386: Extract a common fucntion to setup value of
    MSR_CORE_THREAD_COUNT
  i386: Track cores_per_module in CPUX86State
  cpu: Remove nr_cores from struct CPUState

 hw/core/cpu-common.c                 |  1 -
 hw/i386/x86-common.c                 |  4 +++-
 include/hw/core/cpu.h                |  2 --
 include/hw/i386/topology.h           |  5 +++--
 system/cpus.c                        |  1 -
 target/i386/cpu.c                    |  2 +-
 target/i386/cpu.h                    | 16 ++++++++++++++++
 target/i386/hvf/x86_emu.c            |  3 +--
 target/i386/kvm/kvm.c                |  5 +----
 target/i386/tcg/sysemu/misc_helper.c |  3 +--
 10 files changed, 26 insertions(+), 16 deletions(-)

-- 
2.34.1

[RFC PATCH 1/4] 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>
---
 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

[RFC PATCH 2/4] i386: 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>
---
 target/i386/cpu.h                    | 11 +++++++++++
 target/i386/hvf/x86_emu.c            |  3 +--
 target/i386/kvm/kvm.c                |  5 +----
 target/i386/tcg/sysemu/misc_helper.c |  3 +--
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 4c239a6970fd..5795a497e567 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2390,6 +2390,17 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu,
     cs->halted = 0;
 }
 
+static inline 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;
+}
+
 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

[RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State

[Xiaoyao Li]

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.li@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;
+
     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;
 }
-- 
2.34.1

[RFC PATCH 4/4] 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>
---
 hw/core/cpu-common.c  | 1 -
 hw/i386/x86-common.c  | 2 +-
 include/hw/core/cpu.h | 2 --
 system/cpus.c         | 1 -
 4 files changed, 1 insertion(+), 5 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/hw/i386/x86-common.c b/hw/i386/x86-common.c
index f7a20c1da30c..2dd7d8e34b76 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -377,7 +377,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 */
+     * CPUState::nr_threads fields instead of globals */
     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:"
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 1c818ff6828c..909d8128e81b 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -666,7 +666,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

Re: [RFC PATCH 2/4] i386: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Philippe Mathieu-Daudé]

Hi Xiaoyao,

On 5/12/24 15:57, 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>
> ---
>   target/i386/cpu.h                    | 11 +++++++++++
>   target/i386/hvf/x86_emu.c            |  3 +--
>   target/i386/kvm/kvm.c                |  5 +----
>   target/i386/tcg/sysemu/misc_helper.c |  3 +--
>   4 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 4c239a6970fd..5795a497e567 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2390,6 +2390,17 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu,
>       cs->halted = 0;
>   }
>   
> +static inline uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)

Please do not add more inlined functions in this huge file, the
inlining performance isn't justified at all.

target/i386/cpu-sysemu.c looks the correct place for this helper.

> +{
> +    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 */

Personally I'd change to:

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

> +
> +    return val;
> +}

Re: [RFC PATCH 2/4] i386: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Igor Mammedov]

On Thu, 5 Dec 2024 22:38:41 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Hi Xiaoyao,
> 
> On 5/12/24 15:57, 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>
> > ---
> >   target/i386/cpu.h                    | 11 +++++++++++
> >   target/i386/hvf/x86_emu.c            |  3 +--
> >   target/i386/kvm/kvm.c                |  5 +----
> >   target/i386/tcg/sysemu/misc_helper.c |  3 +--
> >   4 files changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 4c239a6970fd..5795a497e567 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -2390,6 +2390,17 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu,
> >       cs->halted = 0;
> >   }
> >   
> > +static inline uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)  
> 
> Please do not add more inlined functions in this huge file, the
> inlining performance isn't justified at all.
> 
> target/i386/cpu-sysemu.c looks the correct place for this helper.

ack

> 
> > +{
> > +    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 */  
> 
> Personally I'd change to:
> 
>         return deposit64(cs->nr_threads * cs->nr_cores, 16, 16,
>                          cs->nr_cores);
that I wouldn't do in this patch to make it easier to compare apples to apples
That however could be a separate patch on top

> > +
> > +    return val;
> > +}  
> 

Re: [RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State

[Igor Mammedov]

On Thu,  5 Dec 2024 09:57:15 -0500
Xiaoyao Li <xiaoyao.li@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.li@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?

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

Re: [RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State

[Zhao Liu]

On Tue, Dec 10, 2024 at 05:43:38PM +0100, Igor Mammedov wrote:
> Date: Tue, 10 Dec 2024 17:43:38 +0100
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State
> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-redhat-linux-gnu)
> 
> On Thu,  5 Dec 2024 09:57:15 -0500
> Xiaoyao Li <xiaoyao.li@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.li@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?

I think Xiaoyao is correct here. CPUState.nr_cores means number of cores
in socket, and current CPUX86State.nr_cores means number of cores per
module (or parent container) ...though they have same name. (It's better
to mention the such difference in commit message.)

However, I also think that names like nr_cores or nr_* are prone to
errors. Names like cores_per_module are clearer, similar to the naming
in X86CPUTopoInfo. This might be an opportunity to clean up the current
nr_* naming convention.

And further, we can directly cache two additional items in CPUX86State:
threads_per_pkg and cores_per_pkg, as these are the most common
calculations and can help avoid missing any topology levels.

I think both of these changes can be made on top of the current series.

@xiaoyao, do you agree?

Regards,
Zhao

Re: [RFC PATCH 1/4] i386/topology: Update the comment of x86_apicid_from_topo_ids()

[Zhao Liu]

On Thu, Dec 05, 2024 at 09:57:13AM -0500, Xiaoyao Li wrote:
> Date: Thu, 5 Dec 2024 09:57:13 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [RFC PATCH 1/4] i386/topology: Update the comment of
>  x86_apicid_from_topo_ids()
> X-Mailer: git-send-email 2.34.1
> 
> Update the comment of x86_apicid_from_topo_ids() to match the current
> implementation,
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@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.

Maybe "based on sub-topology ID"?

Otherwise,

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

Re: [RFC PATCH 4/4] cpu: Remove nr_cores from struct CPUState

[Zhao Liu]

On Thu, Dec 05, 2024 at 09:57:16AM -0500, Xiaoyao Li wrote:
> Date: Thu, 5 Dec 2024 09:57:16 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [RFC PATCH 4/4] cpu: Remove nr_cores from struct CPUState
> X-Mailer: git-send-email 2.34.1
> 
> There is no user of it now, remove it.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  hw/core/cpu-common.c  | 1 -
>  hw/i386/x86-common.c  | 2 +-
>  include/hw/core/cpu.h | 2 --
>  system/cpus.c         | 1 -
>  4 files changed, 1 insertion(+), 5 deletions(-)
> 

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

Re: [RFC PATCH 2/4] i386: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Xiaoyao Li]

On 12/11/2024 12:35 AM, Igor Mammedov wrote:
> On Thu, 5 Dec 2024 22:38:41 +0100
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
>> Hi Xiaoyao,
>>
>> On 5/12/24 15:57, 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>
>>> ---
>>>    target/i386/cpu.h                    | 11 +++++++++++
>>>    target/i386/hvf/x86_emu.c            |  3 +--
>>>    target/i386/kvm/kvm.c                |  5 +----
>>>    target/i386/tcg/sysemu/misc_helper.c |  3 +--
>>>    4 files changed, 14 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>>> index 4c239a6970fd..5795a497e567 100644
>>> --- a/target/i386/cpu.h
>>> +++ b/target/i386/cpu.h
>>> @@ -2390,6 +2390,17 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu,
>>>        cs->halted = 0;
>>>    }
>>>    
>>> +static inline uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
>>
>> Please do not add more inlined functions in this huge file, the
>> inlining performance isn't justified at all.
>>
>> target/i386/cpu-sysemu.c looks the correct place for this helper.
> 
> ack

Will move it to target/i386/cpu-sysemu.c in next version.

>>
>>> +{
>>> +    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 */
>>
>> Personally I'd change to:
>>
>>          return deposit64(cs->nr_threads * cs->nr_cores, 16, 16,
>>                           cs->nr_cores);
> that I wouldn't do in this patch to make it easier to compare apples to apples
> That however could be a separate patch on top

OK. I will keep it as-is in this patch.

>>> +
>>> +    return val;
>>> +}
>>
> 

Re: [RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State

[Xiaoyao Li]

On 12/11/2024 12:43 AM, Igor Mammedov wrote:
> On Thu,  5 Dec 2024 09:57:15 -0500
> Xiaoyao Li <xiaoyao.li@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.li@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;
>>   }
> 

Re: [RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State

[Xiaoyao Li]

On 12/11/2024 10:50 AM, Zhao Liu wrote:
> On Tue, Dec 10, 2024 at 05:43:38PM +0100, Igor Mammedov wrote:
>> Date: Tue, 10 Dec 2024 17:43:38 +0100
>> From: Igor Mammedov <imammedo@redhat.com>
>> Subject: Re: [RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State
>> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-redhat-linux-gnu)
>>
>> On Thu,  5 Dec 2024 09:57:15 -0500
>> Xiaoyao Li <xiaoyao.li@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.li@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?
> 
> I think Xiaoyao is correct here. CPUState.nr_cores means number of cores
> in socket, and current CPUX86State.nr_cores means number of cores per
> module (or parent container) ...though they have same name. (It's better
> to mention the such difference in commit message.)
> 
> However, I also think that names like nr_cores or nr_* are prone to
> errors. Names like cores_per_module are clearer, similar to the naming
> in X86CPUTopoInfo. This might be an opportunity to clean up the current
> nr_* naming convention.
> 
> And further, we can directly cache two additional items in CPUX86State:
> threads_per_pkg and cores_per_pkg, as these are the most common
> calculations and can help avoid missing any topology levels.
> 
> I think both of these changes can be made on top of the current series.

yes, my plan is to just maintain a "struct X86CPUTopoInfo" in 
CPUX86State. After we clean up the nr_threads and nr_cores in CPUState 
usage.

> @xiaoyao, do you agree?
> 
> Regards,
> Zhao
> 

Re: [RFC PATCH 1/4] i386/topology: Update the comment of x86_apicid_from_topo_ids()

[Xiaoyao Li]

On 12/11/2024 10:54 AM, Zhao Liu wrote:
> On Thu, Dec 05, 2024 at 09:57:13AM -0500, Xiaoyao Li wrote:
>> Date: Thu, 5 Dec 2024 09:57:13 -0500
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>> Subject: [RFC PATCH 1/4] i386/topology: Update the comment of
>>   x86_apicid_from_topo_ids()
>> X-Mailer: git-send-email 2.34.1
>>
>> Update the comment of x86_apicid_from_topo_ids() to match the current
>> implementation,
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@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.
> 
> Maybe "based on sub-topology ID"?

I interpret "sub-topology ID" the same as "IDs of each topology level". 
But only with the information of IDs cannot produce a APIC ID, we need 
the width of each level as well. I think the "topology" expresses the 
width information, so I used the statement as "topology and IDs of each 
topology level"

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

Re: [RFC PATCH 0/4] cpu: Drop CPUState::nr_cores

[Igor Mammedov]

On Thu,  5 Dec 2024 09:57:12 -0500
Xiaoyao Li <xiaoyao.li@intel.com> wrote:

> The series is motivated by auditing the usage of CPUState::nr_cores and
> CPUState::nr_threads, which is motivated by [1].
> 
> The initial goal is to initialize nr_threads and nr_cores earlier for
> x86, which leads to patches [2] and [3]. Patch [2] touches all the ARCHes
> and patch [3] looks hacky. At last Igor suggested to audit nr_threads and
> nr_cores, and only set them in the pre_plug() callback for the ARCHes that
> really need them[1].
> 
> By audting nr_threads and nr_cores, I found nr_cores is only used by
> x86. So we can introduce a x86 specific one and initialize in
> x86_cpu_pre_plug(), then drop nr_cores totally.
> 
> However for nr_threads, it's used by MIPS and PPC as well[4]. There are
> two options:
> 1. maintain separate substitute in X86/MIPS/PPS, so we can drop
> CPUState::nr_threads like for CPUState::nr_cores.
> 
> 2. keep CPUState::nr_threads and find place (or introduce pre_plug()) to
> initialize them earlier for MIPS/PPC.
> 
> I would like to seek for opinions for which one is prefered. 
> 
> This series implments the drop for CPUState::nr_cores. Though it doesn't
> help on the initial goal without the solution for nr_threads, I think it
> is still a good cleanup?
> 
> BTW, by initializing nr_threads and nr_cores earlier than
> qemu_init_vcpu(), it also unblocks [5].

With minor fixes included mentioned during review

Acked-by: Igor Mammedov <imammedo@redhat.com>

> 
> 
> [1] https://lore.kernel.org/qemu-devel/20241125103857.78a23715@imammedo.users.ipa.redhat.com/
> [2] https://lore.kernel.org/qemu-devel/5f8db586-cdda-4d00-be02-f9880a20e1a3@redhat.com/
> [3] https://lore.kernel.org/qemu-devel/20241122160317.4070177-1-xiaoyao.li@intel.com/
> [4] https://lore.kernel.org/qemu-devel/045f9cb1-2b17-4b2c-985f-3c34e3626b36@intel.com/
> [5] https://lore.kernel.org/qemu-devel/20231128171239.69b6d7b1@imammedo.users.ipa.redhat.com/
> 
> Xiaoyao Li (4):
>   i386/topology: Update the comment of x86_apicid_from_topo_ids()
>   i386: Extract a common fucntion to setup value of
>     MSR_CORE_THREAD_COUNT
>   i386: Track cores_per_module in CPUX86State
>   cpu: Remove nr_cores from struct CPUState
> 
>  hw/core/cpu-common.c                 |  1 -
>  hw/i386/x86-common.c                 |  4 +++-
>  include/hw/core/cpu.h                |  2 --
>  include/hw/i386/topology.h           |  5 +++--
>  system/cpus.c                        |  1 -
>  target/i386/cpu.c                    |  2 +-
>  target/i386/cpu.h                    | 16 ++++++++++++++++
>  target/i386/hvf/x86_emu.c            |  3 +--
>  target/i386/kvm/kvm.c                |  5 +----
>  target/i386/tcg/sysemu/misc_helper.c |  3 +--
>  10 files changed, 26 insertions(+), 16 deletions(-)
> 

Re: [RFC PATCH 0/4] cpu: Drop CPUState::nr_cores

[Xiaoyao Li]

On 12/31/2024 12:11 AM, Igor Mammedov wrote:
> On Thu,  5 Dec 2024 09:57:12 -0500
> Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> 
>> The series is motivated by auditing the usage of CPUState::nr_cores and
>> CPUState::nr_threads, which is motivated by [1].
>>
>> The initial goal is to initialize nr_threads and nr_cores earlier for
>> x86, which leads to patches [2] and [3]. Patch [2] touches all the ARCHes
>> and patch [3] looks hacky. At last Igor suggested to audit nr_threads and
>> nr_cores, and only set them in the pre_plug() callback for the ARCHes that
>> really need them[1].
>>
>> By audting nr_threads and nr_cores, I found nr_cores is only used by
>> x86. So we can introduce a x86 specific one and initialize in
>> x86_cpu_pre_plug(), then drop nr_cores totally.
>>
>> However for nr_threads, it's used by MIPS and PPC as well[4]. There are
>> two options:
>> 1. maintain separate substitute in X86/MIPS/PPS, so we can drop
>> CPUState::nr_threads like for CPUState::nr_cores.
>>
>> 2. keep CPUState::nr_threads and find place (or introduce pre_plug()) to
>> initialize them earlier for MIPS/PPC.
>>
>> I would like to seek for opinions for which one is prefered.
>>
>> This series implments the drop for CPUState::nr_cores. Though it doesn't
>> help on the initial goal without the solution for nr_threads, I think it
>> is still a good cleanup?
>>
>> BTW, by initializing nr_threads and nr_cores earlier than
>> qemu_init_vcpu(), it also unblocks [5].
> 
> With minor fixes included mentioned during review
> 
> Acked-by: Igor Mammedov <imammedo@redhat.com>

Appreciated for your Ack, Igor!

There is a v2[*], could you take a look at that?

[*] 
https://lore.kernel.org/qemu-devel/20241219110125.1266461-1-xiaoyao.li@intel.com/

>>
>>
>> [1] https://lore.kernel.org/qemu-devel/20241125103857.78a23715@imammedo.users.ipa.redhat.com/
>> [2] https://lore.kernel.org/qemu-devel/5f8db586-cdda-4d00-be02-f9880a20e1a3@redhat.com/
>> [3] https://lore.kernel.org/qemu-devel/20241122160317.4070177-1-xiaoyao.li@intel.com/
>> [4] https://lore.kernel.org/qemu-devel/045f9cb1-2b17-4b2c-985f-3c34e3626b36@intel.com/
>> [5] https://lore.kernel.org/qemu-devel/20231128171239.69b6d7b1@imammedo.users.ipa.redhat.com/
>>
>> Xiaoyao Li (4):
>>    i386/topology: Update the comment of x86_apicid_from_topo_ids()
>>    i386: Extract a common fucntion to setup value of
>>      MSR_CORE_THREAD_COUNT
>>    i386: Track cores_per_module in CPUX86State
>>    cpu: Remove nr_cores from struct CPUState
>>
>>   hw/core/cpu-common.c                 |  1 -
>>   hw/i386/x86-common.c                 |  4 +++-
>>   include/hw/core/cpu.h                |  2 --
>>   include/hw/i386/topology.h           |  5 +++--
>>   system/cpus.c                        |  1 -
>>   target/i386/cpu.c                    |  2 +-
>>   target/i386/cpu.h                    | 16 ++++++++++++++++
>>   target/i386/hvf/x86_emu.c            |  3 +--
>>   target/i386/kvm/kvm.c                |  5 +----
>>   target/i386/tcg/sysemu/misc_helper.c |  3 +--
>>   10 files changed, 26 insertions(+), 16 deletions(-)
>>
>