Re: [PATCH 06/22] exec/cpu: Call cpu_remove_sync() once in cpu_common_unrealize()

[Philippe Mathieu-Daudé]

On 16/1/25 19:05, Philippe Mathieu-Daudé wrote:
> On 28/11/23 17:42, Igor Mammedov wrote:
> > On Mon, 18 Sep 2023 18:02:39 +0200
> > Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > > While create_vcpu_thread() creates a vCPU thread, its counterpart
> > > is cpu_remove_sync(), which join and destroy the thread.
> > >
> > > create_vcpu_thread() is called in qemu_init_vcpu(), itself called
> > > in cpu_common_realizefn(). Since we don't have qemu_deinit_vcpu()
> > > helper (we probably don't need any), simply destroy the thread in
> > > cpu_common_unrealizefn().
> > >
> > > Note: only the PPC and X86 targets were calling cpu_remove_sync(),
> > > meaning all other targets were leaking the thread when the vCPU
> > > was unrealized (mostly when vCPU are hot-unplugged).
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/core/cpu-common.c  | 3 +++
> > >   target/i386/cpu.c     | 1 -
> > >   target/ppc/cpu_init.c | 2 --
> > >   3 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> > > index a3b8de7054..e5841c59df 100644
> > > --- a/hw/core/cpu-common.c
> > > +++ b/hw/core/cpu-common.c
> > > @@ -221,6 +221,9 @@ static void cpu_common_unrealizefn(DeviceState *dev)
> > >       /* NOTE: latest generic point before the cpu is fully 
> > > unrealized */
> > >       cpu_exec_unrealizefn(cpu);
> > > +
> > > +    /* Destroy vCPU thread */
> > > +    cpu_remove_sync(cpu);
> > >   }
> > >   static void cpu_common_initfn(Object *obj)
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index cb41d30aab..d79797d963 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -7470,7 +7470,6 @@ static void x86_cpu_unrealizefn(DeviceState *dev)
> > >       X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> > >   #ifndef CONFIG_USER_ONLY
> > > -    cpu_remove_sync(CPU(dev));
> > >       qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
> > >   #endif
> >
> > missing  followup context:
> >      ...
> >      xcc->parent_unrealize(dev);
> >
> > Before the patch, vcpu thread is stopped and onnly then
> > clean up happens.
> >
> > After the patch we have cleanup while vcpu thread is still running.
> >
> > Even if it doesn't explode, such ordering still seems to be wrong.
>
> OK.
>
> > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > > index e2c06c1f32..24d4e8fa7e 100644
> > > --- a/target/ppc/cpu_init.c
> > > +++ b/target/ppc/cpu_init.c
> > > @@ -6853,8 +6853,6 @@ static void ppc_cpu_unrealize(DeviceState *dev)
> > >       pcc->parent_unrealize(dev);
> > > -    cpu_remove_sync(CPU(cpu));
> >
> > bug in current code?
>
> Plausibly. See:
>
> commit f1023d21e81b7bf523ddf2ac91a48117f20ef9d7
> Author: Greg Kurz <groug@kaod.org>
> Date:   Thu Oct 15 23:18:32 2020 +0200
>
>      spapr: Unrealize vCPUs with qdev_unrealize()
>
>      Since we introduced CPU hot-unplug in sPAPR, we don't unrealize the
>      vCPU objects explicitly. Instead, we let QOM handle that for us
>      under object_property_del_all() when the CPU core object is
>      finalized. The only thing we do is calling cpu_remove_sync() to
>      tear the vCPU thread down.
>
>      This happens to work but it is ugly because:
>      - we call qdev_realize() but the corresponding qdev_unrealize() is
>        buried deep in the QOM code
>      - we call cpu_remove_sync() to undo qemu_init_vcpu() called by
>        ppc_cpu_realize() in target/ppc/translate_init.c.inc
>      - the CPU init and teardown paths aren't really symmetrical
>
>      The latter didn't bite us so far but a future patch that greatly
>      simplifies the CPU core realize path needs it to avoid a crash
>      in QOM.
>
>      For all these reasons, have ppc_cpu_unrealize() to undo the changes
>      of ppc_cpu_realize() by calling cpu_remove_sync() at the right
>      place, and have the sPAPR CPU core code to call qdev_unrealize().
>
>      This requires to add a missing stub because translate_init.c.inc is
>      also compiled for user mode.

See also:

commit 5e22e29201d80124bca0124f2034e72b698cbb6f
Author: David Gibson <david@gibson.dropbear.id.au>
Date:   Wed Jun 13 12:08:42 2018 +1000

    pnv: Add cpu unrealize path

    Currently we don't have any unrealize path for pnv cpu cores.
    We get away with this because we don't yet support cpu hotplug
    for pnv.

    However, we're going to want it eventually, and in the meantime,
    it makes it non-obvious why there are a bunch of allocations on
    the realize() path that don't have matching frees.

    So, implement the missing unrealize path.

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index f4c41d89d6d..f7cf33f547a 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -186,6 +186,26 @@ err:
     error_propagate(errp, local_err);
 }

+static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
+{
+    qemu_unregister_reset(pnv_cpu_reset, cpu);
+    object_unparent(cpu->intc);
+    cpu_remove_sync(CPU(cpu));
+    object_unparent(OBJECT(cpu));
+}