RE: [PATCH RFC V3 24/29] target/arm: Add support of *unrealize* ARMCPU during vCPU Hot-unplug

[Salil Mehta]

Hi Peter,

>  From: Peter Maydell <peter.maydell@linaro.org>
>  Sent: Friday, August 16, 2024 4:51 PM
>  To: Alex Bennée <alex.bennee@linaro.org>
>  
>  On Fri, 16 Aug 2024 at 16:37, Alex Bennée <alex.bennee@linaro.org> wrote:
>  >
>  > Salil Mehta <salil.mehta@huawei.com> writes:
>  >
>  > > vCPU Hot-unplug will result in QOM CPU object unrealization which
>  > > will do away with all the vCPU thread creations, allocations,
>  > > registrations that happened as part of the realization process. This
>  > > change introduces the ARM CPU unrealize function taking care of exactly
>  that.
>  > >
>  > > Note, initialized KVM vCPUs are not destroyed in host KVM but their
>  > > Qemu context is parked at the QEMU KVM layer.
>  > >
>  > > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
>  > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>  > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > > Reported-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > > [VP: Identified CPU stall issue & suggested probable fix]
>  > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > > ---
>  > >  target/arm/cpu.c       | 101
>  +++++++++++++++++++++++++++++++++++++++++
>  > >  target/arm/cpu.h       |  14 ++++++
>  > >  target/arm/gdbstub.c   |   6 +++
>  > >  target/arm/helper.c    |  25 ++++++++++
>  > >  target/arm/internals.h |   3 ++
>  > >  target/arm/kvm.c       |   5 ++
>  > >  6 files changed, 154 insertions(+)
>  > >
>  > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c index
>  > > c92162fa97..a3dc669309 100644
>  > > --- a/target/arm/cpu.c
>  > > +++ b/target/arm/cpu.c
>  > > @@ -157,6 +157,16 @@ void
>  arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn
>  *hook,
>  > >      QLIST_INSERT_HEAD(&cpu->pre_el_change_hooks, entry, node);  }
>  > >
>  > > +void arm_unregister_pre_el_change_hooks(ARMCPU *cpu) {
>  > > +    ARMELChangeHook *entry, *next;
>  > > +
>  > > +    QLIST_FOREACH_SAFE(entry, &cpu->pre_el_change_hooks, node,
>  next) {
>  > > +        QLIST_REMOVE(entry, node);
>  > > +        g_free(entry);
>  > > +    }
>  > > +}
>  > > +
>  > >  void arm_register_el_change_hook(ARMCPU *cpu,
>  ARMELChangeHookFn *hook,
>  > >                                   void *opaque)  { @@ -168,6 +178,16
>  > > @@ void arm_register_el_change_hook(ARMCPU *cpu,
>  ARMELChangeHookFn *hook,
>  > >      QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);  }
>  > >
>  > > +void arm_unregister_el_change_hooks(ARMCPU *cpu) {
>  > > +    ARMELChangeHook *entry, *next;
>  > > +
>  > > +    QLIST_FOREACH_SAFE(entry, &cpu->el_change_hooks, node, next)
>  {
>  > > +        QLIST_REMOVE(entry, node);
>  > > +        g_free(entry);
>  > > +    }
>  > > +}
>  > > +
>  > >  static void cp_reg_reset(gpointer key, gpointer value, gpointer
>  > > opaque)  {
>  > >      /* Reset a single ARMCPRegInfo register */ @@ -2552,6 +2572,85
>  > > @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>  > >      acc->parent_realize(dev, errp);  }
>  > >
>  > > +static void arm_cpu_unrealizefn(DeviceState *dev) {
>  > > +    ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>  > > +    ARMCPU *cpu = ARM_CPU(dev);
>  > > +    CPUARMState *env = &cpu->env;
>  > > +    CPUState *cs = CPU(dev);
>  > > +    bool has_secure;
>  > > +
>  > > +    has_secure = cpu->has_el3 || arm_feature(env,
>  > > + ARM_FEATURE_M_SECURITY);
>  > > +
>  > > +    /* rock 'n' un-roll, whatever happened in the arm_cpu_realizefn
>  cleanly */
>  > > +    cpu_address_space_destroy(cs, ARMASIdx_NS);
>  >
>  > On current master this will fail:
>  >
>  > ../../target/arm/cpu.c: In function ‘arm_cpu_unrealizefn’:
>  > ../../target/arm/cpu.c:2626:5: error: implicit declaration of function
>  ‘cpu_address_space_destroy’ [-Werror=implicit-function-declaration]
>  >  2626 |     cpu_address_space_destroy(cs, ARMASIdx_NS);
>  >       |     ^~~~~~~~~~~~~~~~~~~~~~~~~
>  > ../../target/arm/cpu.c:2626:5: error: nested extern declaration of
>  > ‘cpu_address_space_destroy’ [-Werror=nested-externs]
>  > cc1: all warnings being treated as errors
>  
>  We shouldn't need to explicitly call cpu_address_space_destroy() from a
>  target-specific unrealize anyway: we can do it all from the base class (and I
>  think this would fix some leaks in current code for targets that hot-unplug,
>  though I should check that). Otherwise you need to duplicate all the logic 
> for
>  figuring out which address spaces we created in realize, which is fragile and
>  not necessary when all we want to do is "delete every address space the
>  CPU object has"
>  and we want to do that for every target architecture always.


Agreed but I would suggest to make it optional i.e. in case architecture want
to release to from its code. It should be allowed.  This also ensures clarity 
of the
flows,

https://lore.kernel.org/qemu-devel/a308e1f4f06f4e3ab6ab51f353601f43@huawei.com/


Thanks
Salil.



>  
>  -- PMM