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;
Date:   Mon, 22 Aug 2022 22:27:11 -0700
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     "Huang, Kai" <kai.huang@intel.com>
Cc:     "Christopherson,, Sean" <seanjc@google.com>,
        "Shahar, Sagi" <sagis@google.com>,
        "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
        "Aktas, Erdem" <erdemaktas@google.com>,
        "will@kernel.org" <will@kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 003/103] KVM: Refactor CPU compatibility check on
 module initialization
Message-ID: <20220823052711.GB2147148@ls.amr.corp.intel.com>
References: <cover.1659854790.git.isaku.yamahata@intel.com>
 <4092a37d18f377003c6aebd9ced1280b0536c529.1659854790.git.isaku.yamahata@intel.com>
 <d36802ee3d96d0fdac00d2c11be341f94a362ef9.camel@intel.com>
 <YvU+6fdkHaqQiKxp@google.com>
 <283c3155f6f27229d507e6e0efc5179594a36855.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <283c3155f6f27229d507e6e0efc5179594a36855.camel@intel.com>
Precedence: bulk

On Fri, Aug 12, 2022 at 11:35:29AM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> >   3.  Provide arch hooks that are invoked for "power management" operations (including
> >       CPU hotplug and host reboot, hence the quotes).  Note, there's both a platform-
> >       wide PM notifier and a per-CPU notifier...
> > 
> >   4.  Rename kvm_arch_post_init_vm() to e.g. kvm_arch_add_vm(), call it under
> >       kvm_lock, and pass in kvm_usage_count.
> > 
> >   5a. Drop cpus_hardware_enabled and drop the common hardware enable/disable code.
> > 
> >  or 
> > 
> >   5b. Expose kvm_hardware_enable_all() and/or kvm_hardware_enable() so that archs
> >       don't need to implement their own error handling and per-CPU flags.
> > 
> > I.e. give each architecture hooks to handle possible transition points, but otherwise
> > let arch code decide when and how to do hardware enabling/disabling. 
> > 
> > I'm very tempted to vote for (5a); x86 is the only architecture has an error path
> > in kvm_arch_hardware_enable(), and trying to get common code to play nice with arm's
> > kvm_arm_hardware_enabled logic is probably going to be weird.
> > 

I ended up with (5a) with the following RFC patches.
https://lore.kernel.org/kvm/cover.1660974106.git.isaku.yamahata@intel.com/T/#m0239e7800b66174b49c5b1049462aad50293a994


> > E.g. if we can get the back half kvm_create_vm() to look like the below, then arch
> > code can enable hardware during kvm_arch_add_vm() if the existing count is zero
> > without generic KVM needing to worry about when hardware needs to be enabled and
> > disabled.
> > 
> > 	r = kvm_arch_init_vm(kvm, type);
> > 	if (r)
> > 		goto out_err_no_arch_destroy_vm;
> > 
> > 	r = kvm_init_mmu_notifier(kvm);
> > 	if (r)
> > 		goto out_err_no_mmu_notifier;
> > 
> > 	/*
> > 	 * When the fd passed to this ioctl() is opened it pins the module,
> > 	 * but try_module_get() also prevents getting a reference if the module
> > 	 * is in MODULE_STATE_GOING (e.g. if someone ran "rmmod --wait").
> > 	 */
> > 	if (!try_module_get(kvm_chardev_ops.owner)) {
> > 		r = -ENODEV;
> > 		goto out_err;
> > 	}
> > 
> > 	mutex_lock(&kvm_lock);
> > 	cpus_read_lock();
> > 	r = kvm_arch_add_vm(kvm, kvm_usage_count);
> 
> Holding cpus_read_lock() here implies CPU hotplug cannot happen during
> kvm_arch_add_vm().  This needs a justification/comment to explain why.  
> 
> Also, assuming we have a justification, since (based on your description above)
> arch _may_ choose to enable hardware within it, but it is not a _must_.  So
> maybe remove cpus_read_lock() here and let kvm_arch_add_vm() to decide whether
> to use it?

For now, I put locking outside of kvm_arch_{add, del}_vm().  But I haven't looked
at non-x86 arch.  Probably arm code has its preference for its implementation.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>