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().
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();