From patchwork Wed Oct 9 03:18:09 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Salil Mehta X-Patchwork-Id: 13827493 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8A5B5CF043B for ; Wed, 9 Oct 2024 03:29:01 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1syNNb-0003UE-Ca; Tue, 08 Oct 2024 23:28:51 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1syNNG-0003Bu-UL; Tue, 08 Oct 2024 23:28:31 -0400 Received: from frasgout.his.huawei.com ([185.176.79.56]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1syNNE-00056U-QW; Tue, 08 Oct 2024 23:28:30 -0400 Received: from mail.maildlp.com (unknown [172.18.186.231]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4XNdbx1lPNz687Yd; Wed, 9 Oct 2024 11:27:09 +0800 (CST) Received: from frapeml500007.china.huawei.com (unknown [7.182.85.172]) by mail.maildlp.com (Postfix) with ESMTPS id A8351140A34; Wed, 9 Oct 2024 11:28:25 +0800 (CST) Received: from 00293818-MRGF.huawei.com (10.126.173.89) by frapeml500007.china.huawei.com (7.182.85.172) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Wed, 9 Oct 2024 05:28:06 +0200 To: , , CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: [PATCH RFC V4 27/33] target/arm: Add support to *unrealize* ARMCPU during vCPU Hot-unplug Date: Wed, 9 Oct 2024 04:18:09 +0100 Message-ID: <20241009031815.250096-28-salil.mehta@huawei.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20241009031815.250096-1-salil.mehta@huawei.com> References: <20241009031815.250096-1-salil.mehta@huawei.com> MIME-Version: 1.0 X-Originating-IP: [10.126.173.89] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To frapeml500007.china.huawei.com (7.182.85.172) Received-SPF: pass client-ip=185.176.79.56; envelope-from=salil.mehta@huawei.com; helo=frasgout.his.huawei.com X-Spam_score_int: -41 X-Spam_score: -4.2 X-Spam_bar: ---- X-Spam_report: (-4.2 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, RCVD_IN_VALIDITY_SAFE_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-to: Salil Mehta X-Patchwork-Original-From: Salil Mehta via From: Salil Mehta Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org 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 rather their Qemu context is parked at the QEMU KVM layer. Co-developed-by: Keqian Zhu Signed-off-by: Keqian Zhu Signed-off-by: Salil Mehta Reported-by: Vishnu Pajjuri [VP: Identified CPU stall issue & suggested probable fix] Signed-off-by: Salil Mehta --- system/physmem.c | 8 ++- target/arm/cpu.c | 117 ++++++++++++++++++++++++++++++++++++++++- 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 ++ 7 files changed, 176 insertions(+), 2 deletions(-) diff --git a/system/physmem.c b/system/physmem.c index d71a2b1bbd..1bec3049fe 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -2613,8 +2613,14 @@ static void tcg_commit(MemoryListener *listener) * * That said, the listener is also called during realize, before * all of the tcg machinery for run-on is initialized: thus halt_cond. + * Similarly, the listener can also be triggered during the *unrealize* + * operation. In such a case, we should avoid using `run_on_cpu` because the + * TCG vCPU thread might already be terminated. As a result, the CPU work + * will never get processed, and `tcg_commit_cpu` will not be called. This + * means that operations like `tlb_flush()` might not be executed, + * potentially leading to inconsistencies. */ - if (cpu->halt_cond) { + if (cpu->halt_cond && !cpu->unplug) { async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas)); } else { tcg_commit_cpu(cpu, RUN_ON_CPU_HOST_PTR(cpuas)); diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 14fcabc2c9..19d2f89f5f 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 */ @@ -2642,6 +2662,98 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) acc->parent_realize(dev, errp); } +#ifndef CONFIG_USER_ONLY +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; + + /* rock 'n' un-roll, whatever happened in the arm_cpu_realizefn cleanly */ + destroy_cpreg_list(cpu); + arm_cpu_unregister_gdb_regs(cpu); + unregister_cp_regs_for_features(cpu); + + if (cpu->sau_sregion && arm_feature(env, ARM_FEATURE_M_SECURITY)) { + g_free(env->sau.rbar); + g_free(env->sau.rlar); + } + + if (arm_feature(env, ARM_FEATURE_PMSA) && + arm_feature(env, ARM_FEATURE_V7) && + cpu->pmsav7_dregion) { + if (arm_feature(env, ARM_FEATURE_V8)) { + g_free(env->pmsav8.rbar[M_REG_NS]); + g_free(env->pmsav8.rlar[M_REG_NS]); + if (arm_feature(env, ARM_FEATURE_M_SECURITY)) { + g_free(env->pmsav8.rbar[M_REG_S]); + g_free(env->pmsav8.rlar[M_REG_S]); + } + } else { + g_free(env->pmsav7.drbar); + g_free(env->pmsav7.drsr); + g_free(env->pmsav7.dracr); + } + if (cpu->pmsav8r_hdregion) { + g_free(env->pmsav8.hprbar); + g_free(env->pmsav8.hprlar); + } + } + + if (arm_feature(env, ARM_FEATURE_PMU)) { + if (!kvm_enabled()) { + arm_unregister_pre_el_change_hooks(cpu); + arm_unregister_el_change_hooks(cpu); + } + + if (cpu->pmu_timer) { + timer_del(cpu->pmu_timer); + } + } + + cpu_remove_sync(CPU(dev)); + + /* + * We are intentionally destroying the CPU address space after the vCPU + * threads have been joined. This ensures that for TCG, any pending TLB + * flushes associated with the CPU are completed. The destruction of the + * address space also removes associated listeners, and joining threads + * after the address space no longer exists can lead to race conditions with + * already queued work for this CPU, which may result in a segmentation + * fault (SEGV) in `tcg_commit_cpu()`. + * + * Alternatively, Peter Maydell has suggested moving the CPU address space + * destruction to `cpu_common_unrealize()`, which would be called in the + * context of `parent_unrealize()`. This would also address the race + * condition in TCG. + * + * RFC: Question: Any additional thoughts or feedback on this approach would + * be appreciated? + */ + has_secure = cpu->has_el3 || arm_feature(env, ARM_FEATURE_M_SECURITY); + cpu_address_space_destroy(cs, ARMASIdx_NS); + if (cpu->tag_memory != NULL) { + cpu_address_space_destroy(cs, ARMASIdx_TagNS); + if (has_secure) { + cpu_address_space_destroy(cs, ARMASIdx_TagS); + } + } + if (has_secure) { + cpu_address_space_destroy(cs, ARMASIdx_S); + } + + acc->parent_unrealize(dev); + + timer_del(cpu->gt_timer[GTIMER_PHYS]); + timer_del(cpu->gt_timer[GTIMER_VIRT]); + timer_del(cpu->gt_timer[GTIMER_HYP]); + timer_del(cpu->gt_timer[GTIMER_SEC]); + timer_del(cpu->gt_timer[GTIMER_HYPVIRT]); +} +#endif + static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; @@ -2745,7 +2857,10 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) device_class_set_parent_realize(dc, arm_cpu_realizefn, &acc->parent_realize); - +#ifndef CONFIG_USER_ONLY + device_class_set_parent_unrealize(dc, arm_cpu_unrealizefn, + &acc->parent_unrealize); +#endif device_class_set_props(dc, arm_cpu_properties); resettable_class_set_parent_phases(rc, NULL, arm_cpu_reset_hold, NULL, diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 1277a0ddfc..07bd7d6542 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1128,6 +1128,7 @@ struct ARMCPUClass { const ARMCPUInfo *info; DeviceRealize parent_realize; + DeviceUnrealize parent_unrealize; ResettablePhases parent_phases; }; @@ -3293,6 +3294,13 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs) */ void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook, void *opaque); +/** + * arm_unregister_pre_el_change_hook: + * unregister all pre EL change hook functions. Generally called during + * unrealize'ing leg + */ +void arm_unregister_pre_el_change_hooks(ARMCPU *cpu); + /** * arm_register_el_change_hook: * Register a hook function which will be called immediately after this @@ -3305,6 +3313,12 @@ void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook, */ void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook, void *opaque); +/** + * arm_unregister_el_change_hook: + * unregister all EL change hook functions. Generally called during + * unrealize'ing leg + */ +void arm_unregister_el_change_hooks(ARMCPU *cpu); /** * arm_rebuild_hflags: diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c index 554b8736bb..58067e30a5 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -595,3 +595,9 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) } #endif /* CONFIG_TCG */ } + +void arm_cpu_unregister_gdb_regs(ARMCPU *cpu) +{ + CPUState *cs = CPU(cpu); + gdb_unregister_coprocessor_all(cs); +} diff --git a/target/arm/helper.c b/target/arm/helper.c index 0a582c1cd3..a890f98445 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -264,6 +264,19 @@ void init_cpreg_list(ARMCPU *cpu) g_list_free(keys); } +void destroy_cpreg_list(ARMCPU *cpu) +{ + assert(cpu->cpreg_indexes); + assert(cpu->cpreg_values); + assert(cpu->cpreg_vmstate_indexes); + assert(cpu->cpreg_vmstate_values); + + g_free(cpu->cpreg_indexes); + g_free(cpu->cpreg_values); + g_free(cpu->cpreg_vmstate_indexes); + g_free(cpu->cpreg_vmstate_values); +} + static bool arm_pan_enabled(CPUARMState *env) { if (is_a64(env)) { @@ -9985,6 +9998,18 @@ void register_cp_regs_for_features(ARMCPU *cpu) #endif } +void unregister_cp_regs_for_features(ARMCPU *cpu) +{ + CPUARMState *env = &cpu->env; + if (arm_feature(env, ARM_FEATURE_M)) { + /* M profile has no coprocessor registers */ + return; + } + + /* empty it all. unregister all the coprocessor registers */ + g_hash_table_remove_all(cpu->cp_regs); +} + /* * Private utility function for define_one_arm_cp_reg_with_opaque(): * add a single reginfo struct to the hash table. diff --git a/target/arm/internals.h b/target/arm/internals.h index 203a2dae14..722c4dd00b 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -367,9 +367,12 @@ void arm_cpu_register(const ARMCPUInfo *info); void aarch64_cpu_register(const ARMCPUInfo *info); void register_cp_regs_for_features(ARMCPU *cpu); +void unregister_cp_regs_for_features(ARMCPU *cpu); void init_cpreg_list(ARMCPU *cpu); +void destroy_cpreg_list(ARMCPU *cpu); void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu); +void arm_cpu_unregister_gdb_regs(ARMCPU *cpu); void arm_translate_init(void); void arm_cpu_register_gdb_commands(ARMCPU *cpu); diff --git a/target/arm/kvm.c b/target/arm/kvm.c index 8ed160cbca..369d7ad135 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -1983,6 +1983,11 @@ int kvm_arch_init_vcpu(CPUState *cs) int kvm_arch_destroy_vcpu(CPUState *cs) { + /* vCPUs which are yet to be realized will not have handler */ + if (cs->thread_id) { + qemu_del_vm_change_state_handler(cs->vmcse); + } + return 0; }