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

[Philippe Mathieu-Daudé]

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.

> > -
> >       destroy_ppc_opcodes(cpu);
> >   }
> >