Received-SPF: pass (google.com: domain of linux-kernel+bounces-149366-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <5ffd4052-4735-449a-9bee-f42563add778@intel.com>
Date: Thu, 18 Apr 2024 12:47:00 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when
 loading the KVM intel kernel module
To: Sean Christopherson <seanjc@google.com>
CC: "Zhang, Tina" <tina.zhang@intel.com>, "Yuan, Hang" <hang.yuan@intel.com>,
	"Chen, Bo2" <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>, "Aktas, Erdem"
	<erdemaktas@google.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>, "Yamahata, Isaku"
	<isaku.yamahata@intel.com>, "isaku.yamahata@linux.intel.com"
	<isaku.yamahata@linux.intel.com>
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>
 <ZiBc13qU6P3OBn7w@google.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZiBc13qU6P3OBn7w@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Q0NYYWdSMVNuTitwMHVkd1ZtWkE2aG45c1hsaXdZRkpWenBPUkk5T1p0bW5R?=
 =?utf-8?B?T3QyR3ZGdGRiQUJreVVJdE1HM0FqOEU4b2F3RUZISGJsWWZuQ3c5dmFwL3pt?=
 =?utf-8?B?cEwrNGdSUnFBRDFRKzBlODJSL3pYdVorMWV2R1ZpdktVRk5JTWNTYTlEeE4w?=
 =?utf-8?B?Mm9xUzJzTFlEdDI1blNOZ1cvRldDUEFhaTgrVTF6cjU0Mzl3WCt1WXJ2UUFC?=
 =?utf-8?B?aXVJclFzRWFJdDl2OWh1V1UyR2Q5NTlwVWhOaVkvbU1VMjkrblM5OVJWc2RY?=
 =?utf-8?B?bHNVWkJaMHhNd3k0MVk5K1grdElFWmFnNzFyd1BKdDkxV2I2QkR2ZkhGM0dB?=
 =?utf-8?B?ZDlGZzJFSHpVM3NhSWRIU2w4aEwwUkk3SWg1Ly9MdzFFOFZKZEV2bEcyMUM2?=
 =?utf-8?B?UXR5NW1ETjRkbysvZVgyc3FVbHQyd1dQc0ZFRXJ2MjVsSkZqeExmbzJwdjZv?=
 =?utf-8?B?RGl6YjIxOFlHYW5Yb0RHZlEzRnBGeWlvS3BoVUdLc05PSC9hL3JTOGp2eFk0?=
 =?utf-8?B?QktOZ3ByNk5OYWVNbHRQbG95QmsxOFVvb0RzOHVheVMzTjBCZ3lOakwwQmZG?=
 =?utf-8?B?blFCaklvMjFtdnFtTGtsaTI4WXI0QXJoUzVrTTVvbHVMa0phM1dkNmhKZm85?=
 =?utf-8?B?Nzh0VWV1K3VHbjJzL2hrZTBRUGd2UlNDZlNWVHMrcUtMT1RSbXgzUHFqN2pV?=
 =?utf-8?B?ZFVNM0t5eFVTWkJKZ0dEaE9iYTlRQjJCUTBQSkVtSUh5OW5yOS9xKzN6R043?=
 =?utf-8?B?dStvdkNYV3YwTUtDQ3FYSkFuemJGc082ZEg5VjRVM1JTVXJwSHVoUTZtczUw?=
 =?utf-8?B?a0F0MGJOVER2RFpXMENBMG1FRHgxSkgyMjYrYVFVRk9aQ3BSTjJiOU9zQUQ3?=
 =?utf-8?B?MlBIQmpydnZNNVVXNzZ5MmRYaENKL0JwNHg4VE03ODdpM3RBd24yWUovVHRQ?=
 =?utf-8?B?bDN6dmlTbDlmenIrTXppM1d3WElvRmJXanZDVU8xdXJFNmF3aC81Nk1Xb3ZG?=
 =?utf-8?B?S2I0TzZXQ0dVWklvbTI4MDZ3Q3FUQTBWekVJbUd4NHhBTDBMNnlCQWtXSG9q?=
 =?utf-8?B?ZGNOZFU2emVEQkYybjc4WER1YzhxUG5DRmUyNkMrMFkvUVRiMnU4SGZXcnNC?=
 =?utf-8?B?a3I0dG8rQ1dWT3BKQkdvSENSc1VxcVUrNFk5YzBNaUQ4U3pMTnhSeUtGVDRF?=
 =?utf-8?B?SlNZU0R1WkxJcHoxNjB6R1FGUnpWNW5rdGRCZVE4NTYzaVZiR1VCaVlVNXlz?=
 =?utf-8?B?aWJLSGV1VEZLZXJkTWoyL2szWnUxdmJBS1dkdEg1dTllQkxUVFo4VkJSMjVR?=
 =?utf-8?B?L1doWTRQazhsczJ2bGxPeC83QWRzWTFvbjNtM1Zya21Gd3VncXJ6RVQ5b1pN?=
 =?utf-8?B?Y3dJelplY014MmJCbkxkYXVoSW95YjhiN2EybFBGQnBqMW5nMlEzdmp1UjNT?=
 =?utf-8?B?cFYxQ1l6SUlJVThGWjhpL3l6T1MydkFrZHhhWlZBODE3dXgxNnEwbWFPSVdM?=
 =?utf-8?B?T3RQV0drb1IxUkJxeUIraE5WN2xlVW5wY0dNb0NUaHhkWkJYTmc3OU1Kendw?=
 =?utf-8?B?QW9MdmZMZWhQL3U5ZlV1OE9XVSsreE4yS0ZhYTVRK1RodWxuNDF3MHd3ZlFl?=
 =?utf-8?B?TVcxaDlhMHM3R1RxWjhhMkc0VG5PUnJDSDJyWXE0eDBpZXR5Skt0QU5uUjZp?=
 =?utf-8?B?dWl2enppazBNU1BjcW1aNmMwT1hUUklMcEM3OXVLV2pocjdCYTBjNDNDdXpi?=
 =?utf-8?B?OE9CZnRuWUliaU9nU2FDSVNGcld1R3ZMYXViVXk5akQyZnk4Yk93RGk3RlEz?=
 =?utf-8?B?ZzY0NmZOSForSHlVdGpCVjBVQm1MaFk1b091dGhBYXl6TVgvZjh2bi9teDZF?=
 =?utf-8?B?ZmhzTCtGKys3K2VzSlA2Y3ZXaTdPSVdGcmRRSkp0YlBYNzNQcjZjNjJNYzFO?=
 =?utf-8?B?a1kxajB5SnBhck5EUWZXQ0Z3VjQ0WHFwbGJTY3Y3dFpNMVRGL0hrSnVHT0dz?=
 =?utf-8?B?NlpGczNNQlplTk5sNzlkRm9mdkFJVVBvRk92cVhzcXZxWmhBRmJUaXFNK2pv?=
 =?utf-8?B?a1N5WWpQU2ZOK2NlVm40SGFYWVR2QmlWTWdXUjVrWEw0L2xyMWRsV0thVHd2?=
 =?utf-8?Q?RFhMU98suG6yceSf9jIF+XxtO?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 52650961-3834-4cc6-0d95-08dc5f41141c
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 00:47:09.6577
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: obC7jfxJzv92KrLnuabOuFSbLeTqsicj2ib3MAGJXp5k8WuyCey2RNeY5+UoElMfEqWAk2EPzbZy+gAp91lpiw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB8144
X-OriginatorOrg: intel.com


On 18/04/2024 11:35 am, Sean Christopherson wrote:
> 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.  

OK.  This makes sense.

And vendor_module_lock fits the bill perfectly.  Well, except for the
> name, and I definitely have no objection to renaming it.

No opinion on renaming.  Personally I wouldn't bother to rename.  We can 
add a comment in kvm_x86_enable_virtualization() to explain.  Perhaps in 
the future we just want to change to always enable virtualization for 
x86 too..

> 
>> 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;
> 	}


I assume the reason you introduced the late_hardware_setup() is purely 
because you want to do:

   cpu_emergency_register_virt_callback(kvm_x86_ops.emergency_enable);

after

   kvm_ops_update()?

Anyway, we can also do 'enable_tdx' outside of kvm_x86_vendor_init() as 
above, given it cannot be done in hardware_setup() anyway.

If we do 'enable_tdx' in late_hardware_setup(), we will need a 
kvm_x86_enable_virtualization_nolock(), but that's also not a problem to me.

So which way do you prefer?

Btw, with kvm_x86_virtualization_enable(), it seems the compatibility 
check is lost, which I assume is OK?

Btw2, currently tdx_enable() requires cpus_read_lock() must be called 
prior.  If we do unconditional tdx_cpu_enable() in vt_hardware_enable(), 
then with your proposal IIUC there's no such requirement anymore, 
because no task will be scheduled to the new CPU before it reaches 
CPUHP_AP_ACTIVE.  But now calling cpus_read_lock()/unlock() around 
tdx_enable() also acceptable to me.

[...]

>>
>>> +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.

Ah, right :-)

Btw, why couldn't we do the 'system_state' check at the very beginning 
of this function?