cpu_list_add() was doing 2 distinct things:
- assign some index to vCPU
- add unrealized (thus in inconsistent state) vCPU to &cpus_queue
Code using CPU_FOREACH() macro would iterate over possibly
unrealized vCPUs, often dealt with special casing.
Instead of working around of vCPU existence in cpus_queue,
split out cpu_index assignment from cpu_list_add(),
and move the later to the end of realize stage,
right before vCPU is let run.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Yanan Wang <wangyanan55@huawei.com>
CC: Zhao Liu <zhao1.liu@intel.com>
---
include/hw/core/cpu.h | 6 ++++++
cpu-common.c | 23 ++++++++++++++---------
cpu-target.c | 2 +-
hw/core/cpu-common.c | 2 ++
4 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index fb397cdfc5..c338fd31bd 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -750,6 +750,12 @@ bool cpu_virtio_is_big_endian(CPUState *cpu);
#endif /* CONFIG_USER_ONLY */
+/**
+ * cpu_auto_assign_cpu_index:
+ * @cpu: The CPU to be assigned a cpu_index
+ */
+void cpu_auto_assign_cpu_index(CPUState *cpu);
+
/**
* cpu_list_add:
* @cpu: The CPU to be added to the list of CPUs.
diff --git a/cpu-common.c b/cpu-common.c
index 4248b2d727..92f3d00e56 100644
--- a/cpu-common.c
+++ b/cpu-common.c
@@ -71,15 +71,7 @@ int cpu_get_free_index(void)
return max_cpu_index;
}
-CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
-static unsigned int cpu_list_generation_id;
-
-unsigned int cpu_list_generation_id_get(void)
-{
- return cpu_list_generation_id;
-}
-
-void cpu_list_add(CPUState *cpu)
+void cpu_auto_assign_cpu_index(CPUState *cpu)
{
static bool cpu_index_auto_assigned;
@@ -91,6 +83,19 @@ void cpu_list_add(CPUState *cpu)
} else {
assert(!cpu_index_auto_assigned);
}
+}
+
+CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
+static unsigned int cpu_list_generation_id;
+
+unsigned int cpu_list_generation_id_get(void)
+{
+ return cpu_list_generation_id;
+}
+
+void cpu_list_add(CPUState *cpu)
+{
+ QEMU_LOCK_GUARD(&qemu_cpu_list_lock);
QTAILQ_INSERT_TAIL_RCU(&cpus_queue, cpu, node);
cpu_list_generation_id++;
}
diff --git a/cpu-target.c b/cpu-target.c
index 667688332c..0c86c18a50 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -142,7 +142,7 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
}
/* Wait until cpu initialization complete before exposing cpu. */
- cpu_list_add(cpu);
+ cpu_auto_assign_cpu_index(cpu);
#ifdef CONFIG_USER_ONLY
assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index cb79566cc5..c29737e5e3 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -211,6 +211,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
}
}
+ cpu_list_add(cpu);
+
if (dev->hotplugged) {
cpu_synchronize_post_init(cpu);
cpu_resume(cpu);
--
2.43.0On 7/2/25 17:20, Igor Mammedov wrote:
> cpu_list_add() was doing 2 distinct things:
> - assign some index to vCPU
> - add unrealized (thus in inconsistent state) vCPU to &cpus_queue
>
> Code using CPU_FOREACH() macro would iterate over possibly
> unrealized vCPUs, often dealt with special casing.
>
> Instead of working around of vCPU existence in cpus_queue,
> split out cpu_index assignment from cpu_list_add(),
Better split 2 distinct changes in 2 patches for clarity.
> and move the later to the end of realize stage,
> right before vCPU is let run.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: Yanan Wang <wangyanan55@huawei.com>
> CC: Zhao Liu <zhao1.liu@intel.com>
> ---
> include/hw/core/cpu.h | 6 ++++++
> cpu-common.c | 23 ++++++++++++++---------
> cpu-target.c | 2 +-
> hw/core/cpu-common.c | 2 ++
> 4 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index fb397cdfc5..c338fd31bd 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -750,6 +750,12 @@ bool cpu_virtio_is_big_endian(CPUState *cpu);
>
> #endif /* CONFIG_USER_ONLY */
>
> +/**
> + * cpu_auto_assign_cpu_index:
> + * @cpu: The CPU to be assigned a cpu_index
> + */
> +void cpu_auto_assign_cpu_index(CPUState *cpu);
> +
> /**
> * cpu_list_add:
> * @cpu: The CPU to be added to the list of CPUs.
> diff --git a/cpu-common.c b/cpu-common.c
> index 4248b2d727..92f3d00e56 100644
> --- a/cpu-common.c
> +++ b/cpu-common.c
> @@ -71,15 +71,7 @@ int cpu_get_free_index(void)
> return max_cpu_index;
> }
>
> -CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> -static unsigned int cpu_list_generation_id;
> -
> -unsigned int cpu_list_generation_id_get(void)
> -{
> - return cpu_list_generation_id;
> -}
> -
> -void cpu_list_add(CPUState *cpu)
> +void cpu_auto_assign_cpu_index(CPUState *cpu)
> {
> static bool cpu_index_auto_assigned;
>
> @@ -91,6 +83,19 @@ void cpu_list_add(CPUState *cpu)
> } else {
> assert(!cpu_index_auto_assigned);
> }
> +}
> +
> +CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> +static unsigned int cpu_list_generation_id;
> +
> +unsigned int cpu_list_generation_id_get(void)
> +{
> + return cpu_list_generation_id;
> +}
> +
> +void cpu_list_add(CPUState *cpu)
> +{
> + QEMU_LOCK_GUARD(&qemu_cpu_list_lock);
> QTAILQ_INSERT_TAIL_RCU(&cpus_queue, cpu, node);
> cpu_list_generation_id++;
> }
> diff --git a/cpu-target.c b/cpu-target.c
> index 667688332c..0c86c18a50 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -142,7 +142,7 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
> }
>
> /* Wait until cpu initialization complete before exposing cpu. */
> - cpu_list_add(cpu);
> + cpu_auto_assign_cpu_index(cpu);
>
> #ifdef CONFIG_USER_ONLY
> assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index cb79566cc5..c29737e5e3 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -211,6 +211,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
> }
> }
>
> + cpu_list_add(cpu);
> +
> if (dev->hotplugged) {
> cpu_synchronize_post_init(cpu);
> cpu_resume(cpu);
On Wed, 26 Feb 2025 08:16:52 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 7/2/25 17:20, Igor Mammedov wrote:
> > cpu_list_add() was doing 2 distinct things:
> > - assign some index to vCPU
> > - add unrealized (thus in inconsistent state) vCPU to &cpus_queue
> >
> > Code using CPU_FOREACH() macro would iterate over possibly
> > unrealized vCPUs, often dealt with special casing.
> >
> > Instead of working around of vCPU existence in cpus_queue,
> > split out cpu_index assignment from cpu_list_add(),
>
> Better split 2 distinct changes in 2 patches for clarity.
Will do it later, once folks decide how to fix broken TCG reset path.
do you mean:
#1 - introduce cpu_auto_assign_cpu_index()
#2 - move cpu_list_add() to later stage but keep cpu_auto_assign_cpu_index()
where it's now?
>
> > and move the later to the end of realize stage,
> > right before vCPU is let run.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: Yanan Wang <wangyanan55@huawei.com>
> > CC: Zhao Liu <zhao1.liu@intel.com>
> > ---
> > include/hw/core/cpu.h | 6 ++++++
> > cpu-common.c | 23 ++++++++++++++---------
> > cpu-target.c | 2 +-
> > hw/core/cpu-common.c | 2 ++
> > 4 files changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index fb397cdfc5..c338fd31bd 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -750,6 +750,12 @@ bool cpu_virtio_is_big_endian(CPUState *cpu);
> >
> > #endif /* CONFIG_USER_ONLY */
> >
> > +/**
> > + * cpu_auto_assign_cpu_index:
> > + * @cpu: The CPU to be assigned a cpu_index
> > + */
> > +void cpu_auto_assign_cpu_index(CPUState *cpu);
> > +
> > /**
> > * cpu_list_add:
> > * @cpu: The CPU to be added to the list of CPUs.
> > diff --git a/cpu-common.c b/cpu-common.c
> > index 4248b2d727..92f3d00e56 100644
> > --- a/cpu-common.c
> > +++ b/cpu-common.c
> > @@ -71,15 +71,7 @@ int cpu_get_free_index(void)
> > return max_cpu_index;
> > }
> >
> > -CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> > -static unsigned int cpu_list_generation_id;
> > -
> > -unsigned int cpu_list_generation_id_get(void)
> > -{
> > - return cpu_list_generation_id;
> > -}
> > -
> > -void cpu_list_add(CPUState *cpu)
> > +void cpu_auto_assign_cpu_index(CPUState *cpu)
> > {
> > static bool cpu_index_auto_assigned;
> >
> > @@ -91,6 +83,19 @@ void cpu_list_add(CPUState *cpu)
> > } else {
> > assert(!cpu_index_auto_assigned);
> > }
> > +}
> > +
> > +CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> > +static unsigned int cpu_list_generation_id;
> > +
> > +unsigned int cpu_list_generation_id_get(void)
> > +{
> > + return cpu_list_generation_id;
> > +}
> > +
> > +void cpu_list_add(CPUState *cpu)
> > +{
> > + QEMU_LOCK_GUARD(&qemu_cpu_list_lock);
> > QTAILQ_INSERT_TAIL_RCU(&cpus_queue, cpu, node);
> > cpu_list_generation_id++;
> > }
> > diff --git a/cpu-target.c b/cpu-target.c
> > index 667688332c..0c86c18a50 100644
> > --- a/cpu-target.c
> > +++ b/cpu-target.c
> > @@ -142,7 +142,7 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
> > }
> >
> > /* Wait until cpu initialization complete before exposing cpu. */
> > - cpu_list_add(cpu);
> > + cpu_auto_assign_cpu_index(cpu);
> >
> > #ifdef CONFIG_USER_ONLY
> > assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
> > diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> > index cb79566cc5..c29737e5e3 100644
> > --- a/hw/core/cpu-common.c
> > +++ b/hw/core/cpu-common.c
> > @@ -211,6 +211,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
> > }
> > }
> >
> > + cpu_list_add(cpu);
> > +
> > if (dev->hotplugged) {
> > cpu_synchronize_post_init(cpu);
> > cpu_resume(cpu);
>
On 3/3/25 14:09, Igor Mammedov wrote: > On Wed, 26 Feb 2025 08:16:52 +0100 > Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >> On 7/2/25 17:20, Igor Mammedov wrote: >>> cpu_list_add() was doing 2 distinct things: >>> - assign some index to vCPU >>> - add unrealized (thus in inconsistent state) vCPU to &cpus_queue >>> >>> Code using CPU_FOREACH() macro would iterate over possibly >>> unrealized vCPUs, often dealt with special casing. >>> >>> Instead of working around of vCPU existence in cpus_queue, >>> split out cpu_index assignment from cpu_list_add(), >> >> Better split 2 distinct changes in 2 patches for clarity. > > > Will do it later, once folks decide how to fix broken TCG reset path. > > do you mean: > #1 - introduce cpu_auto_assign_cpu_index() > #2 - move cpu_list_add() to later stage but keep cpu_auto_assign_cpu_index() > where it's now? Exactly, thanks!
© 2016 - 2025 Red Hat, Inc.