Received: by 2002:a05:6358:489b:b0:bb:da1:e618 with SMTP id x27csp1055075rwn; Thu, 8 Sep 2022 12:39:10 -0700 (PDT) X-Google-Smtp-Source: AA6agR6+3JV2GX45nJGLc7pgT7yyMvXFCXth0JGFD8pFitIQO/lVQ+31UwaLB370NBdUOm31g0sK X-Received: by 2002:aa7:8b52:0:b0:537:c6c7:3ef4 with SMTP id i18-20020aa78b52000000b00537c6c73ef4mr10474883pfd.48.1662665949773; Thu, 08 Sep 2022 12:39:09 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1662665949; cv=none; d=google.com; s=arc-20160816; b=PPsLcwY+2g7P3eY3cprm6cT71EwoxJQ8l3olFoqxyn7u4/X30vNuer3hcqQVaRdL8q jphHjN83jkIS2AGW9XOFn6HuJiFk2tvgYcwnEIggZBKTv5E7EkfWCRXV2DVpXHUrJ/7R dQQ3fSUO0SQuA/URuRgGG9ZixuIrU3TrTNZPGjNGF5kECrlkfMtTsfhXjAIBwfWeauI1 KvvKtNpve8ERvxLPzQnwpO67xY+zCge3qVWhxZtIbE/0IHAVhkuJbe7mCyvlpHjzmm6W wRRC4Y0aj/lr4X6OMES+LNwiU3+Pnw0njPIwNFD3ePIUZytZd/aWlCgVfP3rQbI3y6Yl hiSw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=WWPpV4ixsV0K1cIJ9BDV3bxoua94lxlJko1XjEnfsYc=; b=yU2FnkL8zaTPupOpe1GOiAE7pHzXRjqITmJiVF3XDFUORBI8k0h6QvQaU4YqsfvuC3 YEoVBiKqUu6E1gJI8u6kMHVKjBx8uM9X2CrRShL33uJaRkOlJyj/BDI+SoxRhJrFk/MN eGSmSWVg9EgUrHgkg2CGfFZkSFIbZoWHq/YGsvE3wtiwDSaSvIABg41baz/n5OHpzNWy WK/z38KkWSyONT1jFjj/3T6kHZondhGs+uE9B2N1GX5PjHxx83PjaDJPCaVWn9msiER4 XP2+54wxI4Mp4+ardcV5Bhvv3SE5cbrAQLZ7zhPbDjX64XcC4Ip5YkRgVE3qRihg3A6Q QbSQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=pM9SToCv; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y2-20020a056a00190200b0053e67522538si7098917pfi.331.2022.09.08.12.38.57; Thu, 08 Sep 2022 12:39:09 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=pM9SToCv; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231818AbiIHSZF (ORCPT + 99 others); Thu, 8 Sep 2022 14:25:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37288 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231685AbiIHSZC (ORCPT ); Thu, 8 Sep 2022 14:25:02 -0400 Received: from mail-pf1-x429.google.com (mail-pf1-x429.google.com [IPv6:2607:f8b0:4864:20::429]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A1EF22B25E; Thu, 8 Sep 2022 11:24:58 -0700 (PDT) Received: by mail-pf1-x429.google.com with SMTP id u132so458642pfc.6; Thu, 08 Sep 2022 11:24:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=WWPpV4ixsV0K1cIJ9BDV3bxoua94lxlJko1XjEnfsYc=; b=pM9SToCvqdevFaHVOaqDTNZrhKS0g/Ftkr23Id9BY8c+KV0OVyacoAwhh1drTjgjxm 6khMYPG/6BUAcnGRg8KVZSDwUIST6gk66eB2ydkmpfX3M0Lbb/tXONzs6qMwgx+/pTP0 /ufcUUZr1cSUvHO05KIM7zG4qmdaBheE0NDLUdqb6WT/jE5ovuSAfu8gSU2dmbTWetlz bj2sX06VYfCOump+E01E0qbBH5Vx0r6rqYz67yXEHliJ1J7A2CI5a92TRe9jBhi5Yf8C 98fhjcxnYq0e/jht9sWQv9nWFThsSM6YIlHS1SYUf+pxSPmjQTrrNNeogwvxPRxvQ3Eu HXzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=WWPpV4ixsV0K1cIJ9BDV3bxoua94lxlJko1XjEnfsYc=; b=DZPscz+eQCNCUYN0wzIJlH9ACc57l/CrRHGMPQMUVAeF6dw8xrC6Jl6Jcstb0cTtPd GOS6H2n9nVrHCXUxqg5XhKtQkvGmKfOcEEr1xzoaU0FYXdPFF4QLpG4kP6lA95wjZdgP 02+NTu6xPgcQiam6lJD30Ms7qEXa5Fefi27aBziJp1EZ/YzzmxviZ42jyot95kVoQZ7t 5+1JOf7LAZT4biTJKQm5LaolN6xxOTt4TNbaKU6AjVV9JbS2yBHpNwzyMCLCIalPrw7B MgYhmP805dfa9BjG9WAdracbni3dqNuDNrQfx4Y8hSKsqHK+AD9wlQbYZmWT4edP/MqO uHrw== X-Gm-Message-State: ACgBeo1TR/DSHGwo935UJC4RbYc+a0UN6lsL5Y065EI398uH6nTbtxUz 1OKO+mMDHU8iFYQLKDusqec= X-Received: by 2002:a05:6a00:801:b0:53e:5e35:336c with SMTP id m1-20020a056a00080100b0053e5e35336cmr10412755pfk.62.1662661496949; Thu, 08 Sep 2022 11:24:56 -0700 (PDT) Received: from localhost ([192.55.55.51]) by smtp.gmail.com with ESMTPSA id m3-20020a170902f64300b0016c9e5f291bsm14841888plg.111.2022.09.08.11.24.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Sep 2022 11:24:56 -0700 (PDT) Date: Thu, 8 Sep 2022 11:24:55 -0700 From: Isaku Yamahata To: Marc Zyngier Cc: Yuan Yao , 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 Message-ID: <20220908182455.GB470011@ls.amr.corp.intel.com> References: <20212af31729ba27e29c3856b78975c199b5365c.1662084396.git.isaku.yamahata@intel.com> <20220906024643.ti66dw2y6m6jgch2@yy-desk-7060> <87pmg9ui6h.wl-maz@kernel.org> <20220906214434.GA443010@ls.amr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220906214434.GA443010@ls.amr.corp.intel.com> X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 06, 2022 at 02:44:34PM -0700, Isaku Yamahata wrote: > On Tue, Sep 06, 2022 at 07:32:22AM +0100, > Marc Zyngier wrote: > > > 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()). > > It seems plausible. I tested cpu online/offline on x86. Let me update arm64 code > too. On second thought, I decided to add preempt_disable/enable() instead of fixing up possible arch callback and let each arch handle it. -- Isaku Yamahata