Received-SPF: pass (google.com: domain of linux-kernel+bounces-149313-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 17 Apr 2024 16:35:51 -0700
In-Reply-To: <2383a1e9-ba2b-470f-8807-5f5f2528c7ad@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240322212321.GA1994522@ls.amr.corp.intel.com>
 <461b78c38ffb3e59229caa806b6ed22e2c847b77.camel@intel.com>
 <ZhawUG0BduPVvVhN@google.com> <8afbb648-b105-4e04-bf90-0572f589f58c@intel.com>
 <Zhftbqo-0lW-uGGg@google.com> <6cd2a9ce-f46a-44d0-9f76-8e493b940dc4@intel.com>
 <Zh7KrSwJXu-odQpN@google.com> <900fc6f75b3704780ac16c90ace23b2f465bb689.camel@intel.com>
 <Zh_exbWc90khzmYm@google.com> <2383a1e9-ba2b-470f-8807-5f5f2528c7ad@intel.com>
Message-ID: <ZiBc13qU6P3OBn7w@google.com>
Subject: Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when
 loading the KVM intel kernel module
From: Sean Christopherson <seanjc@google.com>
To: Kai Huang <kai.huang@intel.com>
Cc: Tina Zhang <tina.zhang@intel.com>, Hang Yuan <hang.yuan@intel.com>, 
	Bo2 Chen <chen.bo@intel.com>, "sagis@google.com" <sagis@google.com>, 
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Erdem Aktas <erdemaktas@google.com>, 
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "pbonzini@redhat.com" <pbonzini@redhat.com>, 
	Isaku Yamahata <isaku.yamahata@intel.com>, 
	"isaku.yamahata@linux.intel.com" <isaku.yamahata@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"

On Thu, Apr 18, 2024, Kai Huang wrote:
> On 18/04/2024 2:40 am, Sean Christopherson wrote:
> > This way, architectures that aren't saddled with out-of-tree hypervisors can do
> > the dead simple thing of enabling hardware during their initialization sequence,
> > and the TDX code is much more sane, e.g. invoke kvm_x86_enable_virtualization()
> > during late_hardware_setup(), and kvm_x86_disable_virtualization() during module
> > exit (presumably).
> 
> Fine to me, given I am not familiar with other ARCHs, assuming always enable
> virtualization when KVM present is fine to them. :-)
> 
> Two questions below:
> 
> > +int kvm_x86_enable_virtualization(void)
> > +{
> > +	int r;
> > +
> > +	guard(mutex)(&vendor_module_lock);
> 
> It's a little bit odd to take the vendor_module_lock mutex.
> 
> It is called by kvm_arch_init_vm(), so more reasonablly we should still use
> kvm_lock?

I think this should take an x86-specific lock, since it's guarding x86-specific
data.  And vendor_module_lock fits the bill perfectly.  Well, except for the
name, and I definitely have no objection to renaming it.

> Also, if we invoke kvm_x86_enable_virtualization() from
> kvm_x86_ops->late_hardware_setup(), then IIUC we will deadlock here because
> kvm_x86_vendor_init() already takes the vendor_module_lock?

Ah, yeah.  Oh, duh.  I think the reason I didn't initially suggest late_hardware_setup()
is that I was assuming/hoping TDX setup could be done after kvm_x86_vendor_exit().
E.g. in vt_init() or whatever it gets called:

	r = kvm_x86_vendor_exit(...);
	if (r)
		return r;

	if (enable_tdx) {
		r = tdx_blah_blah_blah();
		if (r)
			goto vendor_exit;
	}

> > +	if (kvm_usage_count++)
> > +		return 0;
> > +
> > +	r = kvm_enable_virtualization();
> > +	if (r)
> > +		--kvm_usage_count;
> > +
> > +	return r;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_x86_enable_virtualization);
> > +
> 
> [...]
> 
> > +int kvm_enable_virtualization(void)
> >   {
> > +	int r;
> > +
> > +	r = cpuhp_setup_state(CPUHP_AP_KVM_ONLINE, "kvm/cpu:online",
> > +			      kvm_online_cpu, kvm_offline_cpu);
> > +	if (r)
> > +		return r;
> > +
> > +	register_syscore_ops(&kvm_syscore_ops);
> > +
> > +	/*
> > +	 * Manually undo virtualization enabling if the system is going down.
> > +	 * If userspace initiated a forced reboot, e.g. reboot -f, then it's
> > +	 * possible for an in-flight module load to enable virtualization
> > +	 * after syscore_shutdown() is called, i.e. without kvm_shutdown()
> > +	 * being invoked.  Note, this relies on system_state being set _before_
> > +	 * kvm_shutdown(), e.g. to ensure either kvm_shutdown() is invoked
> > +	 * or this CPU observes the impedning shutdown.  Which is why KVM uses
> > +	 * a syscore ops hook instead of registering a dedicated reboot
> > +	 * notifier (the latter runs before system_state is updated).
> > +	 */
> > +	if (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF ||
> > +	    system_state == SYSTEM_RESTART) {
> > +		unregister_syscore_ops(&kvm_syscore_ops);
> > +		cpuhp_remove_state(CPUHP_AP_KVM_ONLINE);
> > +		return -EBUSY;
> > +	}
> > +
> 
> Aren't we also supposed to do:
> 
> 	on_each_cpu(__kvm_enable_virtualization, NULL, 1);
> 
> here?

No, cpuhp_setup_state() invokes the callback, kvm_online_cpu(), on each CPU.
I.e. KVM has been doing things the hard way by using cpuhp_setup_state_nocalls().
That's part of the complexity I would like to get rid of.