Received-SPF: pass (google.com: domain of linux-kernel+bounces-160448-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 26 Apr 2024 10:07:47 -0700
In-Reply-To: <ZitmpS0lt+wVjRwl@chao-email>
Precedence: bulk
Mime-Version: 1.0
References: <20240425233951.3344485-1-seanjc@google.com> <20240425233951.3344485-4-seanjc@google.com>
 <ZitmpS0lt+wVjRwl@chao-email>
Message-ID: <ZivfY8F_MF1U6QS0@google.com>
Subject: Re: [PATCH 3/4] KVM: Register cpuhp and syscore callbacks when
 enabling hardware
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
Content-Type: text/plain; charset="us-ascii"

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.