From: Sean Christopherson <seanjc@google.com>
To: Chao Gao <chao.gao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] KVM: Register cpuhp and syscore callbacks when enabling hardware
Date: Fri, 26 Apr 2024 10:07:47 -0700 [thread overview]
Message-ID: <ZivfY8F_MF1U6QS0@google.com> (raw)
In-Reply-To: <ZitmpS0lt+wVjRwl@chao-email>
On Fri, Apr 26, 2024, Chao Gao wrote:
> >+static int hardware_enable_all(void)
> >+{
> >+ int r;
> >+
> >+ guard(mutex)(&kvm_lock);
> >+
> >+ if (kvm_usage_count++)
> >+ return 0;
> >+
> >+ r = cpuhp_setup_state(CPUHP_AP_KVM_ONLINE, "kvm/cpu:online",
> >+ kvm_online_cpu, kvm_offline_cpu);
>
> A subtle change is: cpuhp_setup_state() calls kvm_online_cpu() serially
> on all CPUs. Previously, hardware enabling is done with on_each_cpu().
Ooh, I hadn't considered that side effect.
> I assume performance isn't a concern here. Right?
Hmm, performance isn't critical, but I wouldn't be at all surprised if it's
noticeable and problematic for large systems. Assuming we do end up going this
route, maybe we can "solve" this by documenting KVM's behavior, e.g. so that if
userspace cares about the latency of VM creation, it can that fudge around the
potential latency problem by creating a dummy VM. Hrm, that's pretty gross though.
Oh! Hah! What if we add a module param to let userspace force virtualization to
be enabled when KVM is loaded? And then kvm_enable_virtualization() doesn't even
need to be exported/public, because KVM can simply require enable_virt_at_load
(or whatever is a good name) to be %true in order to enable TDX.
I don't think there's any real danger for abuse, e.g. *unprivileged* userspace
can effectively do this already by creating a dummy VM.
Am I missing something? This seems too easy.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7579bda0e310..1bfbb612a98f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -96,6 +96,9 @@ unsigned int halt_poll_ns_shrink;
module_param(halt_poll_ns_shrink, uint, 0644);
EXPORT_SYMBOL_GPL(halt_poll_ns_shrink);
+static bool enable_virt_at_load;
+module_param(enable_virt_at_load, uint, 0444);
+
/*
* Ordering of locks:
*
@@ -6377,6 +6380,12 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
kvm_gmem_init(module);
+ if (enable_virt_at_load) {
+ r = kvm_enable_virtualization();
+ if (r)
+ goto err_virt;
+ }
+
/*
* Registration _must_ be the very last thing done, as this exposes
* /dev/kvm to userspace, i.e. all infrastructure must be setup!
@@ -6390,6 +6399,8 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
return 0;
err_register:
+ kvm_disable_virtualization();
+err_virt:
kvm_vfio_ops_exit();
err_vfio:
kvm_async_pf_deinit();
@@ -6415,6 +6426,11 @@ void kvm_exit(void)
*/
misc_deregister(&kvm_dev);
+ if (enable_virt_at_load) {
+ kvm_disable_virtualization();
+ BUG_ON(kvm_usage_count);
+ }
+
debugfs_remove_recursive(kvm_debugfs_dir);
for_each_possible_cpu(cpu)
free_cpumask_var(per_cpu(cpu_kick_mask, cpu));
> >+ if (r)
> >+ return r;
>
> decrease kvm_usage_count on error?
/facepalm, yes.
next prev parent reply other threads:[~2024-04-26 17:07 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 23:39 [PATCH 0/4] KVM: Register cpuhp/syscore callbacks when enabling virt Sean Christopherson
2024-04-25 23:39 ` [PATCH 1/4] x86/reboot: Unconditionally define cpu_emergency_virt_cb typedef Sean Christopherson
2024-05-13 12:50 ` Huang, Kai
2024-05-13 16:01 ` Sean Christopherson
2024-05-13 22:44 ` Huang, Kai
2024-05-14 22:41 ` Huang, Kai
2024-05-21 20:02 ` Sean Christopherson
2024-05-21 21:43 ` Huang, Kai
2024-05-21 23:16 ` Sean Christopherson
2024-04-25 23:39 ` [PATCH 2/4] KVM: x86: Register emergency virt callback in common code, via kvm_x86_ops Sean Christopherson
2024-04-26 8:52 ` Chao Gao
2024-04-26 17:08 ` Sean Christopherson
2024-05-13 12:55 ` Huang, Kai
2024-05-13 16:17 ` Sean Christopherson
2024-04-25 23:39 ` [PATCH 3/4] KVM: Register cpuhp and syscore callbacks when enabling hardware Sean Christopherson
2024-04-26 8:32 ` Chao Gao
2024-04-26 17:07 ` Sean Christopherson [this message]
2024-05-07 16:31 ` Sean Christopherson
2024-05-09 12:10 ` Huang, Kai
2024-05-13 12:56 ` Huang, Kai
2024-04-25 23:39 ` [PATCH 4/4] KVM: Rename functions related to enabling virtualization hardware Sean Christopherson
2024-05-13 12:59 ` Huang, Kai
2024-05-13 16:20 ` Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZivfY8F_MF1U6QS0@google.com \
--to=seanjc@google.com \
--cc=chao.gao@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).