From mboxrd@z Thu Jan 1 00:00:00 1970 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DDF3EECAAD5 for ; Mon, 5 Sep 2022 09:29:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237359AbiIEJ3O (ORCPT ); Mon, 5 Sep 2022 05:29:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42024 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236123AbiIEJ3M (ORCPT ); Mon, 5 Sep 2022 05:29:12 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 05EA040BF5; Mon, 5 Sep 2022 02:29:11 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 874F7B81002; Mon, 5 Sep 2022 09:29:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2A16AC433C1; Mon, 5 Sep 2022 09:29:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662370148; bh=EzoYAjmNzeumo0W8ileFO5Rxtk1eB/zeGuwPitMqjZc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=merysyxw+G4r+G9miJa9zhxCwAchU85T+FaRY8yjKaju7dwPz6Al68DWJXcxViiBf e33sPMXWC5I1+ahbGFJxqxWdWq9gT4OH5ylJ029B2KmF6vE5Z6jiO682DQWw0DT1EN BbFudQ6nZl59074a1NAoiAdHAvw6xVRAdMjwPnWY5844TyhEd9rFYc2UNc6XnxJ2JE gqrOVyLylfrByGYfaM1O0Q2XUD9EHapaW4gT/7+jgAoIDzbqVKNT7nbXy1tFPyJhgM 7sdXXECLQZZfT+EmVRT7Z3UzHNwpbV5nPQNhLvm8tKCWUY36s5f2CGPhnP5h8HOtN7 8KVCM0ooAeOdg== Received: from [104.132.45.102] (helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oV8Ph-0082I3-UT; Mon, 05 Sep 2022 10:29:06 +0100 Date: Mon, 05 Sep 2022 10:29:05 +0100 Message-ID: <87pmgaqie6.wl-maz@kernel.org> From: Marc Zyngier To: Yuan Yao Cc: isaku.yamahata@intel.com, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Paolo Bonzini , Sean Christopherson , Thomas Gleixner , Will Deacon , isaku.yamahata@gmail.com, Kai Huang , Chao Gao , Atish Patra , Shaokun Zhang , Qi Liu , John Garry , Daniel Lezcano , Huang Ying , Huacai Chen , Dave Hansen , Borislav Petkov , Oliver Upton Subject: Re: [PATCH v3 06/22] KVM: arm64: Simplify the CPUHP logic In-Reply-To: <20220905070509.f5neutyqgvbklefi@yy-desk-7060> References: <72481a7bc0ff08093f4f0f04cece877ee82de0cf.1662084396.git.isaku.yamahata@intel.com> <20220905070509.f5neutyqgvbklefi@yy-desk-7060> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 104.132.45.102 X-SA-Exim-Rcpt-To: yuan.yao@linux.intel.com, isaku.yamahata@intel.com, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, pbonzini@redhat.com, seanjc@google.com, tglx@linutronix.de, will@kernel.org, isaku.yamahata@gmail.com, kai.huang@intel.com, chao.gao@intel.com, atishp@atishpatra.org, zhangshaokun@hisilicon.com, liuqi115@huawei.com, john.garry@huawei.com, daniel.lezcano@linaro.org, ying.huang@intel.com, chenhuacai@kernel.org, dave.hansen@linux.intel.com, bp@alien8.de, oupton@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 05 Sep 2022 08:05:09 +0100, Yuan Yao wrote: > > On Thu, Sep 01, 2022 at 07:17:41PM -0700, isaku.yamahata@intel.com wrote: > > From: Marc Zyngier > > > > For a number of historical reasons, the KVM/arm64 hotplug setup is pretty > > complicated, and we have two extra CPUHP notifiers for vGIC and timers. > > > > It looks pretty pointless, and gets in the way of further changes. > > So let's just expose some helpers that can be called from the core > > CPUHP callback, and get rid of everything else. > > > > This gives us the opportunity to drop a useless notifier entry, > > as well as tidy-up the timer enable/disable, which was a bit odd. > > > > Signed-off-by: Marc Zyngier > > Signed-off-by: Chao Gao > > Reviewed-by: Oliver Upton > > Link: https://lore.kernel.org/r/20220216031528.92558-5-chao.gao@intel.com > > Signed-off-by: Isaku Yamahata > > --- > > arch/arm64/kvm/arch_timer.c | 27 ++++++++++----------------- > > arch/arm64/kvm/arm.c | 4 ++++ > > arch/arm64/kvm/vgic/vgic-init.c | 19 ++----------------- > > include/kvm/arm_arch_timer.h | 4 ++++ > > include/kvm/arm_vgic.h | 4 ++++ > > include/linux/cpuhotplug.h | 3 --- > > 6 files changed, 24 insertions(+), 37 deletions(-) > > > > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > > index bb24a76b4224..33fca1a691a5 100644 > > --- a/arch/arm64/kvm/arch_timer.c > > +++ b/arch/arm64/kvm/arch_timer.c > > @@ -811,10 +811,18 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) > > ptimer->host_timer_irq_flags = host_ptimer_irq_flags; > > } > > > > -static void kvm_timer_init_interrupt(void *info) > > +void kvm_timer_cpu_up(void) > > { > > enable_percpu_irq(host_vtimer_irq, host_vtimer_irq_flags); > > - enable_percpu_irq(host_ptimer_irq, host_ptimer_irq_flags); > > + if (host_ptimer_irq) > > + enable_percpu_irq(host_ptimer_irq, host_ptimer_irq_flags); > > +} > > + > > +void kvm_timer_cpu_down(void) > > +{ > > + disable_percpu_irq(host_vtimer_irq); > > + if (host_ptimer_irq) > > + disable_percpu_irq(host_ptimer_irq); > > } > > Should "host_vtimer_irq" be checked yet as host_ptimer_irq ? No, because although the ptimer interrupt is optional (on older systems, we fully emulate that timer, including the interrupt), the vtimer interrupt is always present and can be used unconditionally. > Because > the host_{v,p}timer_irq is set in same function kvm_irq_init() which > called AFTER the on_each_cpu(_kvm_arch_hardware_enable, NULL, 1) from > init_subsystems(): > > kvm_init() > kvm_arch_init() > init_subsystems() > on_each_cpu(_kvm_arch_hardware_enable, NULL, 1); > kvm_timer_hyp_init() > kvm_irq_init() > host_vtimer_irq = info->virtual_irq; > host_ptimer_irq = info->physical_irq; > hardware_enable_all() This, however, is a very nice catch. I doubt this results in anything really bad (the interrupt enable will fail as the interrupt number is 0, and the disable will also fail due to no prior enable), but that's extremely ugly anyway. The best course of action AFAICS is to differentiate between the arm64-specific initialisation (which is a one-off) and the runtime stuff. Something like the hack below, that I haven't tested yet: diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 32c1022eb4b3..65d03c28f32a 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1671,23 +1671,27 @@ static void _kvm_arch_hardware_enable(void *discard) { if (!__this_cpu_read(kvm_arm_hardware_enabled)) { cpu_hyp_reinit(); - kvm_vgic_cpu_up(); - kvm_timer_cpu_up(); __this_cpu_write(kvm_arm_hardware_enabled, 1); } } int kvm_arch_hardware_enable(void) { + int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled); + _kvm_arch_hardware_enable(NULL); + + if (!was_enabled) { + kvm_vgic_cpu_up(); + kvm_timer_cpu_up(); + } + return 0; } static void _kvm_arch_hardware_disable(void *discard) { if (__this_cpu_read(kvm_arm_hardware_enabled)) { - kvm_timer_cpu_down(); - kvm_vgic_cpu_down(); cpu_hyp_reset(); __this_cpu_write(kvm_arm_hardware_enabled, 0); } @@ -1695,6 +1699,11 @@ static void _kvm_arch_hardware_disable(void *discard) void kvm_arch_hardware_disable(void) { + if (__this_cpu_read(kvm_arm_hardware_enabled)) { + kvm_timer_cpu_down(); + kvm_vgic_cpu_down(); + } + if (!is_protected_kvm_enabled()) _kvm_arch_hardware_disable(NULL); } This should ensure that the init still works as it used to, and that vgic and timers get switched as per the CPUHP notifiers. Thanks, M. -- Without deviation from the norm, progress is not possible.