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 5E9B6ECAAA1 for ; Tue, 6 Sep 2022 06:32:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238167AbiIFGc2 (ORCPT ); Tue, 6 Sep 2022 02:32:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53018 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231569AbiIFGc1 (ORCPT ); Tue, 6 Sep 2022 02:32:27 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 684DF6B16B; Mon, 5 Sep 2022 23:32:26 -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 dfw.source.kernel.org (Postfix) with ESMTPS id DA45F6131C; Tue, 6 Sep 2022 06:32:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 37967C433C1; Tue, 6 Sep 2022 06:32:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662445945; bh=8LMCsYlvl0CUwYsQ2G7wmr8HmSnIgXOgFBXtW7glAw0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=r8JMlanDPN5JhKFA/O5mbNlEqqfeumtL6rk3hWkNYXtbSzkZu3hYbaFVDU7q/EK1L abt8/KMjuQWTpEajHFPIJl96puonsddBsiL1LASz6I0C6s8mWIFCLFJJP2Lk5TkaA/ 6a9bmoQKC0yFGl0in68Egcx8yfQE4e1ObLTa8MCB/TdtkPUARAlb7yZYngZWCHAUn4 9bzkkxOvWz1v1rlIZhdD8Q1RU5UGd5y5tIsGDqGG7Ke4GhePC03/t9mwoXvHeU9+Vg nTO4ZHUNTMoC2hucxl0iRhNUj7fCBOSKhCe2QslnkDGCE/VEIYft4gVTzYF2AjjZxR wcQuGmN/9VBIg== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.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 1oVS8F-008EWN-13; Tue, 06 Sep 2022 07:32:23 +0100 Date: Tue, 06 Sep 2022 07:32:22 +0100 Message-ID: <87pmg9ui6h.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 Subject: Re: [PATCH v3 10/22] KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock In-Reply-To: <20220906024643.ti66dw2y6m6jgch2@yy-desk-7060> References: <20212af31729ba27e29c3856b78975c199b5365c.1662084396.git.isaku.yamahata@intel.com> <20220906024643.ti66dw2y6m6jgch2@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: 185.219.108.64 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 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: kvm@vger.kernel.org On Tue, 06 Sep 2022 03:46:43 +0100, Yuan Yao wrote: > > On Thu, Sep 01, 2022 at 07:17:45PM -0700, isaku.yamahata@intel.com wrote: > > From: Isaku Yamahata > > > > Because kvm_count_lock unnecessarily complicates the KVM locking convention > > Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock for > > simplicity. > > > > Opportunistically add some comments on locking. > > > > Suggested-by: Sean Christopherson > > Signed-off-by: Isaku Yamahata > > --- > > Documentation/virt/kvm/locking.rst | 14 +++++------- > > virt/kvm/kvm_main.c | 34 ++++++++++++++++++++---------- > > 2 files changed, 28 insertions(+), 20 deletions(-) > > > > diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst > > index 845a561629f1..8957e32aa724 100644 > > --- a/Documentation/virt/kvm/locking.rst > > +++ b/Documentation/virt/kvm/locking.rst > > @@ -216,15 +216,11 @@ time it will be set using the Dirty tracking mechanism described above. > > :Type: mutex > > :Arch: any > > :Protects: - vm_list > > - > > -``kvm_count_lock`` > > -^^^^^^^^^^^^^^^^^^ > > - > > -:Type: raw_spinlock_t > > -:Arch: any > > -:Protects: - hardware virtualization enable/disable > > -:Comment: 'raw' because hardware enabling/disabling must be atomic /wrt > > - migration. > > + - kvm_usage_count > > + - hardware virtualization enable/disable > > +:Comment: Use cpus_read_lock() for hardware virtualization enable/disable > > + because hardware enabling/disabling must be atomic /wrt > > + migration. The lock order is cpus lock => kvm_lock. > > > > ``kvm->mn_invalidate_lock`` > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index fc55447c4dba..082d5dbc8d7f 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -100,7 +100,6 @@ EXPORT_SYMBOL_GPL(halt_poll_ns_shrink); > > */ > > > > DEFINE_MUTEX(kvm_lock); > > -static DEFINE_RAW_SPINLOCK(kvm_count_lock); > > LIST_HEAD(vm_list); > > > > static cpumask_var_t cpus_hardware_enabled; > > @@ -4996,6 +4995,8 @@ static void hardware_enable_nolock(void *caller_name) > > int cpu = raw_smp_processor_id(); > > int r; > > > > + WARN_ON_ONCE(preemptible()); > > This looks incorrect, it may triggers everytime when online CPU. > Because patch 7 moved CPUHP_AP_KVM_STARTING *AFTER* > CPUHP_AP_ONLINE_IDLE as CPUHP_AP_KVM_ONLINE, then cpuhp_thread_fun() > runs the new CPUHP_AP_KVM_ONLINE in *non-atomic* context: > > cpuhp_thread_fun(unsigned int cpu) { > ... > if (cpuhp_is_atomic_state(state)) { > local_irq_disable(); > st->result = cpuhp_invoke_callback(cpu, state, bringup, st->node, &st->last); > local_irq_enable(); > > WARN_ON_ONCE(st->result); > } else { > st->result = cpuhp_invoke_callback(cpu, state, bringup, st->node, &st->last); > } > ... > } > > static bool cpuhp_is_atomic_state(enum cpuhp_state state) > { > return CPUHP_AP_IDLE_DEAD <= state && state < CPUHP_AP_ONLINE; > } > > The hardware_enable_nolock() now is called in 2 cases: > 1. in atomic context by on_each_cpu(). > 2. From non-atomic context by CPU hotplug thread. > > so how about "WARN_ONCE(preemptible() && cpu_active(cpu))" ? I suspect similar changes must be applied to the arm64 side (though I'm still looking for a good definition of cpu_active()). M. -- Without deviation from the norm, progress is not possible.