On Wed, 26 Feb 2025 08:16:52 +0100
Philippe Mathieu-Daudé <phi...@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 <imamm...@redhat.com>
> > ---
> > CC: Yanan Wang <wangyana...@huawei.com>
> > CC: Zhao Liu <zhao1....@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);
>
