Hi Peter,
> From: Peter Maydell <peter.mayd...@linaro.org>
> Sent: Friday, August 16, 2024 4:51 PM
> To: Alex Bennée <alex.ben...@linaro.org>
>
> On Fri, 16 Aug 2024 at 16:37, Alex Bennée <alex.ben...@linaro.org> wrote:
> >
> > Salil Mehta <salil.me...@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 <zhukeqi...@huawei.com>
> > > Signed-off-by: Keqian Zhu <zhukeqi...@huawei.com>
> > > Signed-off-by: Salil Mehta <salil.me...@huawei.com>
> > > Reported-by: Vishnu Pajjuri <vis...@os.amperecomputing.com>
> > > [VP: Identified CPU stall issue & suggested probable fix]
> > > Signed-off-by: Salil Mehta <salil.me...@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/a308e1f4f06f4e3ab6ab51f353601...@huawei.com/
Thanks
Salil.
>
> -- PMM
