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

[Salil Mehta]

Hi Gustavo,

Sorry for the delay in reply...got pulled into something else.

>  From: Gustavo Romero <gustavo.romero@linaro.org>
>  Sent: Wednesday, August 28, 2024 9:24 PM
>  To: Salil Mehta <salil.mehta@huawei.com>; Alex Bennée
>  <alex.bennee@linaro.org>
>  
>  Hi Salil,
>  
>  On 8/19/24 9:35 AM, Salil Mehta via wrote:
>  > Hi Alex,
>  >
>  >>   From: Alex Bennée <alex.bennee@linaro.org>
>  >>   Sent: Friday, August 16, 2024 4:37 PM
>  >>   To: Salil Mehta <salil.mehta@huawei.com>
>  >>
>  >>   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
>  >
>  >
>  > 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