Currently cpu->nr_cores and cpu->nr_threads are initialized in
qemu_init_vcpu(), which is called a bit late in *cpu_realizefn() for
each ARCHes.
x86 arch would like to use nr_cores and nr_threads earlier in its
realizefn(). Introduce qemu_early_init_vcpu() and move the
initialization of nr_cores and nr_threads from qemu_init_vcpu() to it.
Call qemu_early_init_vcpu() at the beginning of realizefn() for each
ARCH.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
This patch only is testes on x86 machine, please help test on other ARCHes.
---
accel/tcg/user-exec-stub.c | 4 ++++
hw/core/cpu-common.c | 2 +-
include/hw/core/cpu.h | 8 ++++++++
system/cpus.c | 6 +++++-
target/alpha/cpu.c | 2 ++
target/arm/cpu.c | 2 ++
target/avr/cpu.c | 2 ++
target/hexagon/cpu.c | 2 ++
target/hppa/cpu.c | 2 ++
target/i386/cpu.c | 2 ++
target/loongarch/cpu.c | 2 ++
target/m68k/cpu.c | 2 ++
target/microblaze/cpu.c | 2 ++
target/mips/cpu.c | 2 ++
target/openrisc/cpu.c | 2 ++
target/ppc/cpu_init.c | 2 ++
target/riscv/cpu.c | 2 ++
target/rx/cpu.c | 2 ++
target/s390x/cpu.c | 2 ++
target/sh4/cpu.c | 2 ++
target/sparc/cpu.c | 2 ++
target/tricore/cpu.c | 2 ++
target/xtensa/cpu.c | 2 ++
23 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
index 4fbe2dbdc883..64baf917b55c 100644
--- a/accel/tcg/user-exec-stub.c
+++ b/accel/tcg/user-exec-stub.c
@@ -10,6 +10,10 @@ void cpu_remove_sync(CPUState *cpu)
{
}
+void qemu_early_init_vcpu(CPUState *cpu)
+{
+}
+
void qemu_init_vcpu(CPUState *cpu)
{
}
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 09c79035949b..3b60e62228e4 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -242,7 +242,7 @@ static void cpu_common_initfn(Object *obj)
cpu->cpu_index = UNASSIGNED_CPU_INDEX;
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 */
+ /* the default value is changed by qemu_early_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..99ecf18eec02 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -1063,6 +1063,14 @@ void start_exclusive(void);
*/
void end_exclusive(void);
+/**
+ * qemu_early_init_vcpu:
+ * @cpu: The vCPU to initialize.
+ *
+ * Early initialize a vCPU.
+ */
+void qemu_early_init_vcpu(CPUState *cpu);
+
/**
* qemu_init_vcpu:
* @cpu: The vCPU to initialize.
diff --git a/system/cpus.c b/system/cpus.c
index 1c818ff6828c..a1b46f68476a 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -662,12 +662,16 @@ const AccelOpsClass *cpus_get_accel(void)
return cpus_accel;
}
-void qemu_init_vcpu(CPUState *cpu)
+void qemu_early_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;
+}
+
+void qemu_init_vcpu(CPUState *cpu)
+{
cpu->stopped = true;
cpu->random_seed = qemu_guest_random_seed_thread_part1();
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 9db1dffc03ec..56309a647763 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -93,6 +93,8 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
#ifndef CONFIG_USER_ONLY
/* Use pc-relative instructions in system-mode */
cs->tcg_cflags |= CF_PCREL;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5b751439bdc7..98dcc0855868 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1971,6 +1971,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
CPUARMState *env = &cpu->env;
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
/* Use pc-relative instructions in system-mode */
tcg_cflags_set(cs, CF_PCREL);
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 3132842d5654..b8beaf9682c0 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -111,6 +111,8 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
AVRCPUClass *mcc = AVR_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 020038fc4902..5931dce05bec 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -299,6 +299,8 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
HexagonCPUClass *mcc = HEXAGON_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index c38439c1800e..cf7b46e4ff42 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -169,6 +169,8 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
HPPACPUClass *acc = HPPA_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3baa95481fbc..1179b7a3ce62 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7757,6 +7757,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
Error *local_err = NULL;
unsigned requested_lbr_fmt;
+ qemu_early_init_vcpu(cs);
+
#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
/* Use pc-relative instructions in system-mode */
tcg_cflags_set(cs, CF_PCREL);
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 57cc4f314bf7..9572cbd2a4f5 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -598,6 +598,8 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
LoongArchCPUClass *lacc = LOONGARCH_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 1d49f4cb2382..77acbadeb65f 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -304,6 +304,8 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
register_m68k_insns(&cpu->env);
cpu_exec_realizefn(cs, &local_err);
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 135947ee8004..0850043ca1a2 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -226,6 +226,8 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
int i = 0;
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 9724e71a5e0f..8cd1b794e0af 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -460,6 +460,8 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
if (!clock_get(cpu->clock)) {
#ifndef CONFIG_USER_ONLY
if (!qtest_enabled()) {
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 6ec54ad7a6cf..071239dd3134 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -147,6 +147,8 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
OpenRISCCPUClass *occ = OPENRISC_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 23881d09e9f3..5c2ab87319e7 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6949,6 +6949,8 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f219f0c3b527..40cf24d2e759 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1166,6 +1166,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 36d2a6f18906..a24b3f28b455 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -117,6 +117,8 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
RXCPUClass *rcc = RX_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 4e41a3dff59b..20f083834309 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -247,6 +247,8 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
Error *err = NULL;
+ qemu_early_init_vcpu(cs);
+
/* the model has to be realized before qemu_init_vcpu() due to kvm */
s390_realize_cpu_model(cs, &err);
if (err) {
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 8f07261dcfd5..74488a1c0ba4 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -212,6 +212,8 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
SuperHCPUClass *scc = SUPERH_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 54cb269e0af1..57cd7911263e 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -788,6 +788,8 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
Error *local_err = NULL;
CPUSPARCState *env = cpu_env(cs);
+ qemu_early_init_vcpu(cs);
+
#if defined(CONFIG_USER_ONLY)
/* We are emulating the kernel, which will trap and emulate float128. */
env->def.features |= CPU_FEATURE_FLOAT128;
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 1a261715907d..d58271696631 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -88,6 +88,8 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
CPUTriCoreState *env = &cpu->env;
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index a08c7a0b1f20..92120141fee2 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -163,6 +163,8 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
XtensaCPUClass *xcc = XTENSA_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+ qemu_early_init_vcpu(cs);
+
#ifndef CONFIG_USER_ONLY
xtensa_irq_init(&XTENSA_CPU(dev)->env);
#endif
--
2.34.1Currently cpu->nr_cores and cpu->nr_threads are initialized in
qemu_init_vcpu(), which is called a bit late in *cpu_realizefn() for
each ARCHes.
x86 arch would like to use nr_cores and nr_threads earlier in its
realizefn(). To serve this purpose, initialize nr_cores and nr_threads
in cpu_common_initfn(), which is earlier than *cpu_realizefn().
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
hw/core/cpu-common.c | 10 +++++++++-
system/cpus.c | 4 ----
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 09c79035949b..6de92ed854bc 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -237,14 +237,22 @@ static void cpu_common_unrealizefn(DeviceState *dev)
static void cpu_common_initfn(Object *obj)
{
CPUState *cpu = CPU(obj);
+ Object *machine = qdev_get_machine();
+ MachineState *ms;
gdb_init_cpu(cpu);
cpu->cpu_index = UNASSIGNED_CPU_INDEX;
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;
+#ifndef CONFIG_USER_ONLY
+ if (object_dynamic_cast(machine, TYPE_MACHINE)) {
+ ms = MACHINE(machine);
+ cpu->nr_cores = machine_topo_get_cores_per_socket(ms);
+ cpu->nr_threads = ms->smp.threads;
+ }
+#endif
cpu->cflags_next_tb = -1;
/* allocate storage for thread info, initialise condition variables */
diff --git a/system/cpus.c b/system/cpus.c
index 1c818ff6828c..c1547fbfd39b 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -664,10 +664,6 @@ const AccelOpsClass *cpus_get_accel(void)
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.1On Fri, 22 Nov 2024 11:03:17 -0500
Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> Currently cpu->nr_cores and cpu->nr_threads are initialized in
> qemu_init_vcpu(), which is called a bit late in *cpu_realizefn() for
> each ARCHes.
>
> x86 arch would like to use nr_cores and nr_threads earlier in its
> realizefn(). To serve this purpose, initialize nr_cores and nr_threads
> in cpu_common_initfn(), which is earlier than *cpu_realizefn().
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> hw/core/cpu-common.c | 10 +++++++++-
> system/cpus.c | 4 ----
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 09c79035949b..6de92ed854bc 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -237,14 +237,22 @@ static void cpu_common_unrealizefn(DeviceState *dev)
> static void cpu_common_initfn(Object *obj)
> {
> CPUState *cpu = CPU(obj);
> + Object *machine = qdev_get_machine();
> + MachineState *ms;
>
> gdb_init_cpu(cpu);
> cpu->cpu_index = UNASSIGNED_CPU_INDEX;
> 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;
> +#ifndef CONFIG_USER_ONLY
> + if (object_dynamic_cast(machine, TYPE_MACHINE)) {
> + ms = MACHINE(machine);
> + cpu->nr_cores = machine_topo_get_cores_per_socket(ms);
> + cpu->nr_threads = ms->smp.threads;
> + }
> +#endif
Can't say, that I'm fond of adding/moving hack to access MachineState
from CPU context. Granted we did/still do it elsewhere, But I'd rather
prefer getting rid of those remnants that access globals.
It's basically technical debt we are carrying since 2009 (dc6b1c09849).
Moving that around doesn't help with getting rid of arbitrary access to globals.
As Paolo've noted there are other ways to set cores/threads,
albeit at expense of adding more code. And that could be fine
if it's done within expected cpu initialization flow.
Instead of accessing MachineState directly from CPU code (which is
essentially a layer violation), I'd suggest to set cores_nr/threads_nr
from pre_plug handler (which is machine code).
We do similar thing for nr_dies/nr_modules already, and we should do
same for cores/trheads.
Quick hack would be do the same for cores/threads in x86_cpu_pre_plug(),
and make qemu_init_vcpu() conditional to avoid touching other targets/machines.
I'd even ack that, however that's just leaves us with the same
old technical debt. So I'd like to coax a promise to fix it properly
(i.e. get rid of access to machine from CPU code).
(here goes typical ask to rewrite whole QEMU before doing thing you
actually need)
To do that we would need to:
1. audit usage of cpu->nr_cores/cpu->nr_threads across QEMU, to figure out
what targets/machines need them
2. then add pre_plug() handlers to those machines to set them.
3. In the end get rid of initializing them in cpu_common_initfn().
With that done we can then add a common helper to generalize topo config
based on -smp from pre_plug() handlers to reduce duplication caused by
per machine pre_plug handlers.
Or introduce a generic cpu_pre_plug() handler at Machine level and make
_pre_plug call chain to call it (sort of what we do with nested realize calls);
I'd prefer the 1st option (#2) as it explicitly documents what
targets/machines care about cores/threads at expense of some boiler plate code
duplication, instead of blanket generic fallback like we have now (regardless of
if it's actually needed).
> cpu->cflags_next_tb = -1;
>
> /* allocate storage for thread info, initialise condition variables */
> diff --git a/system/cpus.c b/system/cpus.c
> index 1c818ff6828c..c1547fbfd39b 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -664,10 +664,6 @@ const AccelOpsClass *cpus_get_accel(void)
>
> 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();
>
On 11/25/2024 5:38 PM, Igor Mammedov wrote:
> On Fri, 22 Nov 2024 11:03:17 -0500
> Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
>> Currently cpu->nr_cores and cpu->nr_threads are initialized in
>> qemu_init_vcpu(), which is called a bit late in *cpu_realizefn() for
>> each ARCHes.
>>
>> x86 arch would like to use nr_cores and nr_threads earlier in its
>> realizefn(). To serve this purpose, initialize nr_cores and nr_threads
>> in cpu_common_initfn(), which is earlier than *cpu_realizefn().
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> hw/core/cpu-common.c | 10 +++++++++-
>> system/cpus.c | 4 ----
>> 2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index 09c79035949b..6de92ed854bc 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -237,14 +237,22 @@ static void cpu_common_unrealizefn(DeviceState *dev)
>> static void cpu_common_initfn(Object *obj)
>> {
>> CPUState *cpu = CPU(obj);
>> + Object *machine = qdev_get_machine();
>> + MachineState *ms;
>>
>> gdb_init_cpu(cpu);
>> cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>> 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;
>> +#ifndef CONFIG_USER_ONLY
>> + if (object_dynamic_cast(machine, TYPE_MACHINE)) {
>> + ms = MACHINE(machine);
>> + cpu->nr_cores = machine_topo_get_cores_per_socket(ms);
>> + cpu->nr_threads = ms->smp.threads;
>> + }
>> +#endif
>
> Can't say, that I'm fond of adding/moving hack to access MachineState
> from CPU context. Granted we did/still do it elsewhere, But I'd rather
> prefer getting rid of those remnants that access globals.
> It's basically technical debt we are carrying since 2009 (dc6b1c09849).
> Moving that around doesn't help with getting rid of arbitrary access to globals.
>
> As Paolo've noted there are other ways to set cores/threads,
> albeit at expense of adding more code. And that could be fine
> if it's done within expected cpu initialization flow.
>
> Instead of accessing MachineState directly from CPU code (which is
> essentially a layer violation), I'd suggest to set cores_nr/threads_nr
> from pre_plug handler (which is machine code).
> We do similar thing for nr_dies/nr_modules already, and we should do
> same for cores/trheads.
>
> Quick hack would be do the same for cores/threads in x86_cpu_pre_plug(),
> and make qemu_init_vcpu() conditional to avoid touching other targets/machines.
>
> I'd even ack that, however that's just leaves us with the same
> old technical debt. So I'd like to coax a promise to fix it properly
> (i.e. get rid of access to machine from CPU code).
>
> (here goes typical ask to rewrite whole QEMU before doing thing you
> actually need)
>
> To do that we would need to:
> 1. audit usage of cpu->nr_cores/cpu->nr_threads across QEMU, to figure out
> what targets/machines need them
> 2. then add pre_plug() handlers to those machines to set them.
> 3. In the end get rid of initializing them in cpu_common_initfn().
here is the update:
For cpu->nr_cores, it's only used by x86 ARCH. We can remove it and
maintain one for x86 separately.
For cpu->nr_threads, besides x86, it's also used by
1) hw/mips/malta.c
env->mvp->CP0_MVPConf0 = deposit32(env->mvp->CP0_MVPConf0,
CP0MVPC0_PTC, 8,
smp_cpus * cs->nr_threads - 1);
2) target/mips/tcg/sysemu
vpe_idx = tc_idx / cs->nr_threads;
*tc = tc_idx % cs->nr_threads;
3) target/ppc/compat.c
int n_threads = CPU(cpu)->nr_threads;
There are no existing CPU pre_plug() function for above cases, and I
don't know how to add it because I know nothing about MIPS/PPC at all.
If desire is still to go with direction, I need someone else to help
MIPS/PPC. Or is it OK that only change the X86 implementation to
initialize cpu->nr_threads earlier in x86_cpu_pre_plug() and leave
other ARCHes as todo?
> With that done we can then add a common helper to generalize topo config
> based on -smp from pre_plug() handlers to reduce duplication caused by
> per machine pre_plug handlers.
>
> Or introduce a generic cpu_pre_plug() handler at Machine level and make
> _pre_plug call chain to call it (sort of what we do with nested realize calls);
>
> I'd prefer the 1st option (#2) as it explicitly documents what
> targets/machines care about cores/threads at expense of some boiler plate code
> duplication, instead of blanket generic fallback like we have now (regardless of
> if it's actually needed).
>
>> cpu->cflags_next_tb = -1;
>>
>> /* allocate storage for thread info, initialise condition variables */
>> diff --git a/system/cpus.c b/system/cpus.c
>> index 1c818ff6828c..c1547fbfd39b 100644
>> --- a/system/cpus.c
>> +++ b/system/cpus.c
>> @@ -664,10 +664,6 @@ const AccelOpsClass *cpus_get_accel(void)
>>
>> 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();
>>
>
On 11/25/2024 5:38 PM, Igor Mammedov wrote:
> On Fri, 22 Nov 2024 11:03:17 -0500
> Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
>> Currently cpu->nr_cores and cpu->nr_threads are initialized in
>> qemu_init_vcpu(), which is called a bit late in *cpu_realizefn() for
>> each ARCHes.
>>
>> x86 arch would like to use nr_cores and nr_threads earlier in its
>> realizefn(). To serve this purpose, initialize nr_cores and nr_threads
>> in cpu_common_initfn(), which is earlier than *cpu_realizefn().
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> hw/core/cpu-common.c | 10 +++++++++-
>> system/cpus.c | 4 ----
>> 2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index 09c79035949b..6de92ed854bc 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -237,14 +237,22 @@ static void cpu_common_unrealizefn(DeviceState *dev)
>> static void cpu_common_initfn(Object *obj)
>> {
>> CPUState *cpu = CPU(obj);
>> + Object *machine = qdev_get_machine();
>> + MachineState *ms;
>>
>> gdb_init_cpu(cpu);
>> cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>> 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;
>> +#ifndef CONFIG_USER_ONLY
>> + if (object_dynamic_cast(machine, TYPE_MACHINE)) {
>> + ms = MACHINE(machine);
>> + cpu->nr_cores = machine_topo_get_cores_per_socket(ms);
>> + cpu->nr_threads = ms->smp.threads;
>> + }
>> +#endif
>
> Can't say, that I'm fond of adding/moving hack to access MachineState
> from CPU context. Granted we did/still do it elsewhere, But I'd rather
> prefer getting rid of those remnants that access globals.
> It's basically technical debt we are carrying since 2009 (dc6b1c09849).
> Moving that around doesn't help with getting rid of arbitrary access to globals.
>
> As Paolo've noted there are other ways to set cores/threads,
> albeit at expense of adding more code. And that could be fine
> if it's done within expected cpu initialization flow.
>
> Instead of accessing MachineState directly from CPU code (which is
> essentially a layer violation), I'd suggest to set cores_nr/threads_nr
> from pre_plug handler (which is machine code).
> We do similar thing for nr_dies/nr_modules already, and we should do
> same for cores/trheads.
>
> Quick hack would be do the same for cores/threads in x86_cpu_pre_plug(),
> and make qemu_init_vcpu() conditional to avoid touching other targets/machines.
>
> I'd even ack that, however that's just leaves us with the same
> old technical debt. So I'd like to coax a promise to fix it properly
> (i.e. get rid of access to machine from CPU code).
>
> (here goes typical ask to rewrite whole QEMU before doing thing you
> actually need)
>
> To do that we would need to:
> 1. audit usage of cpu->nr_cores/cpu->nr_threads across QEMU, to figure out
> what targets/machines need them
> 2. then add pre_plug() handlers to those machines to set them.>
3. In the end get rid of initializing them in cpu_common_initfn().
It looks sane to me.
I'm wondering how to audit usage of cpu->nr_cores/cpu->nr_threads for
other target than x86. I haven't played with them.
> With that done we can then add a common helper to generalize topo config
> based on -smp from pre_plug() handlers to reduce duplication caused by
> per machine pre_plug handlers.
>
> Or introduce a generic cpu_pre_plug() handler at Machine level and make
> _pre_plug call chain to call it (sort of what we do with nested realize calls);
>
> I'd prefer the 1st option (#2) as it explicitly documents what
> targets/machines care about cores/threads at expense of some boiler plate code
> duplication, instead of blanket generic fallback like we have now (regardless of
> if it's actually needed).
>
>> cpu->cflags_next_tb = -1;
>>
>> /* allocate storage for thread info, initialise condition variables */
>> diff --git a/system/cpus.c b/system/cpus.c
>> index 1c818ff6828c..c1547fbfd39b 100644
>> --- a/system/cpus.c
>> +++ b/system/cpus.c
>> @@ -664,10 +664,6 @@ const AccelOpsClass *cpus_get_accel(void)
>>
>> 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();
>>
>
On 22/11/24 17:03, Xiaoyao Li wrote:
> Currently cpu->nr_cores and cpu->nr_threads are initialized in
> qemu_init_vcpu(), which is called a bit late in *cpu_realizefn() for
> each ARCHes.
>
> x86 arch would like to use nr_cores and nr_threads earlier in its
> realizefn(). To serve this purpose, initialize nr_cores and nr_threads
> in cpu_common_initfn(), which is earlier than *cpu_realizefn().
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> hw/core/cpu-common.c | 10 +++++++++-
> system/cpus.c | 4 ----
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 09c79035949b..6de92ed854bc 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -237,14 +237,22 @@ static void cpu_common_unrealizefn(DeviceState *dev)
> static void cpu_common_initfn(Object *obj)
> {
> CPUState *cpu = CPU(obj);
> + Object *machine = qdev_get_machine();
> + MachineState *ms;
>
> gdb_init_cpu(cpu);
> cpu->cpu_index = UNASSIGNED_CPU_INDEX;
> 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;
> +#ifndef CONFIG_USER_ONLY
Is CONFIG_USER_ONLY available in an common_ss[] object? I don't recall.
Anyway, can we not use CONFIG_USER_ONLY in cpu-common.c?
> + if (object_dynamic_cast(machine, TYPE_MACHINE)) {
> + ms = MACHINE(machine);
> + cpu->nr_cores = machine_topo_get_cores_per_socket(ms);
> + cpu->nr_threads = ms->smp.threads;
> + }
> +#endif
> cpu->cflags_next_tb = -1;
>
> /* allocate storage for thread info, initialise condition variables */
> diff --git a/system/cpus.c b/system/cpus.c
> index 1c818ff6828c..c1547fbfd39b 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -664,10 +664,6 @@ const AccelOpsClass *cpus_get_accel(void)
>
> 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();
>
On 11/22/24 11:26, Philippe Mathieu-Daudé wrote:
> On 22/11/24 17:03, Xiaoyao Li wrote:
>> Currently cpu->nr_cores and cpu->nr_threads are initialized in
>> qemu_init_vcpu(), which is called a bit late in *cpu_realizefn() for
>> each ARCHes.
>>
>> x86 arch would like to use nr_cores and nr_threads earlier in its
>> realizefn(). To serve this purpose, initialize nr_cores and nr_threads
>> in cpu_common_initfn(), which is earlier than *cpu_realizefn().
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> hw/core/cpu-common.c | 10 +++++++++-
>> system/cpus.c | 4 ----
>> 2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index 09c79035949b..6de92ed854bc 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -237,14 +237,22 @@ static void cpu_common_unrealizefn(DeviceState *dev)
>> static void cpu_common_initfn(Object *obj)
>> {
>> CPUState *cpu = CPU(obj);
>> + Object *machine = qdev_get_machine();
>> + MachineState *ms;
>> gdb_init_cpu(cpu);
>> cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>> 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;
>> +#ifndef CONFIG_USER_ONLY
>
> Is CONFIG_USER_ONLY available in an common_ss[] object? I don't recall.
No.
r~
© 2016 - 2026 Red Hat, Inc.