Hi Gustavo,
Sorry for the delay in reply...got pulled into something else.
> From: Gustavo Romero <gustavo.rom...@linaro.org>
> Sent: Wednesday, August 28, 2024 9:24 PM
> To: Salil Mehta <salil.me...@huawei.com>; Alex Bennée
> <alex.ben...@linaro.org>
>
> Hi Salil,
>
> On 8/19/24 9:35 AM, Salil Mehta via wrote:
> > Hi Alex,
> >
> >> From: Alex Bennée <alex.ben...@linaro.org>
> >> Sent: Friday, August 16, 2024 4:37 PM
> >> To: Salil Mehta <salil.me...@huawei.com>
> >>
> >> 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
> >
> >
> > The current master already has arch-agnostic patch-set. I've applied
> > the RFC V3 to the latest and complied. I did not see this issue?
> >
> > I've create a new branch for your reference.
> >
> > https://github.com/salil-mehta/qemu/tree/virt-cpuhp-armv8/rfc-v4-rc4
> >
> > Please let me know if this works for you?
>
> It still happens on the new branch. You need to configure Linux user mode
> to reproduce it, e.g.:
>
> $ ../configure --target-list=aarch64-linux-user,aarch64-softmmu [...]
>
> If you just configure the 'aarch64-softmmu' target it doesn't happen.
Aah, I see. I'll check it today. As vCPU Hotplug does not makes sense in
Qemu user-mode emulation. I think we might need to conditionally
compile certain code using !CONFIG_USER_ONLY switch.
Thanks for the clarification.
Cheers
>
>
> Cheers,
> Gustavo
