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.
-- PMM
