Received-SPF: pass (google.com: domain of linux-kernel+bounces-139519-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: <8afbb648-b105-4e04-bf90-0572f589f58c@intel.com>
Date: Thu, 11 Apr 2024 11:15:43 +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: "Yamahata, Isaku" <isaku.yamahata@intel.com>, "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>, "isaku.yamahata@linux.intel.com"
	<isaku.yamahata@linux.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <f028d43abeadaa3134297d28fb99f283445c0333.1708933498.git.isaku.yamahata@intel.com>
 <d45bb93fb5fc18e7cda97d587dad4a1c987496a1.camel@intel.com>
 <20240322212321.GA1994522@ls.amr.corp.intel.com>
 <461b78c38ffb3e59229caa806b6ed22e2c847b77.camel@intel.com>
 <ZhawUG0BduPVvVhN@google.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZhawUG0BduPVvVhN@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?VFVEZEUxeTNvL3h1VGpkRmEzbzVKUDUreklrcjRqSDlOSjFzTjcrNkNCWHhj?=
 =?utf-8?B?NTgwUmkrZWM5Z1AxQXQzVkduY1pHNFl5eXNkMEhnMFBYZEVkK2lhSU50M0FJ?=
 =?utf-8?B?S1YwSTBudEVQaVZZYndyNzRzWmJSV0g3QzBUTThXTHpveTArWGNrQ1kvWUxl?=
 =?utf-8?B?L0NhNHZzTEVIa1Q1Q3RQa3lsM3FsS3pzSjF1Y0ZKMDNUOTdVbWt1dEpmYnRP?=
 =?utf-8?B?Wkk1VUNCSkVBQUoyYzJ4dTBVcW5vbm1kUU02aHcrZ0lDOEhudDNTb3hBSENN?=
 =?utf-8?B?bEI3WlIwOUZ3UTJ6aFlrQ0N1ZHVYSHA2c1J1dytZVDVxc1hMYkpZR3NsejlJ?=
 =?utf-8?B?WVg0WGtZbTkwejZJRXh1dzlENERkbHFqZzYxN0hRQVZxdytCdUdLZS9rYjNt?=
 =?utf-8?B?S2VYaHVqZW5yRWlqRGdxTXVPd1NUK2gwaDc5RDAvdk9OTytvMUpmOURyQWc1?=
 =?utf-8?B?dUtvYzFGTlVIY2c0SjVscEMzYXhBMDF1NTZKYzQzV1JLaHM3cEtISHloWkVO?=
 =?utf-8?B?UVUxNmdqVGd5SFM2Ty9SNnBCSFJPTlZ3STJmbFJheHZPMkJ0Lyt1TFc0SE5x?=
 =?utf-8?B?SFRFK3UyMERLejVVdmMzTjBvYXRyaU5aYk1Cb3Rhc2drNURBbWFacmozSUxh?=
 =?utf-8?B?ZHd0VUNGTm1YMm1iMHN6NjNHcVYwR1FEQVdwYW9PMVQ0RWlVdlV6cXhzMksz?=
 =?utf-8?B?c0lCM2V2YVIrLzBaeDJ1WnYwYVlLRG8yRDQ5b2d3c2FseWJ1RnZER21HcDYx?=
 =?utf-8?B?WVZ0QVkraWJjMVcwM3IwS2NFR3JESHl5YkJXd2Q5RUNvNnBTMVpjbWlhSi93?=
 =?utf-8?B?UFNxWDRtTzZxL3UxQUR5Z1NqSDNFQ1diTmg2T1QxVHladDZYUnlVekQ4NEFX?=
 =?utf-8?B?blRaZnJZdU0yVjFpZEVrK2V3UUgxL2ROZFZBT2I1WFkybDJvKzRxeGo4TjNn?=
 =?utf-8?B?OE52T0lTSXdGOWNhRmNLTzdsQ3BGb0Q2c1l6M3lhcmhzUUZjZDVBSk8vVkVC?=
 =?utf-8?B?WnhKK3E2RG5vSU1OMTRtKzZxQlRoQkQxd25WOW1ha2J5SkdmR1hNRUlkSEpn?=
 =?utf-8?B?emtBNG0wS0srclhPUzRhVnBJaUtzSGNvWU0rNEh3OEhpbDJ1UGN4ZC9kcGRZ?=
 =?utf-8?B?alJWUG8wSGs2ZjdmQVphQzR5c0ZCRnV4YUFJS3lLTGdEUnpTZk0rbHBQOFU2?=
 =?utf-8?B?Q1R3VWNkWXNrS2FmSExVRCtiK1p1ZDI2V0UraEgrcy9JQkhIVm5IVDVXSTJ1?=
 =?utf-8?B?ZUJteHcvOFNvamhkdCtzNkErVXhxQnJqL2Rmclh3TmRlaUdJa0luUWVNZ1Nm?=
 =?utf-8?B?N2xLbFRlMElTVDA4a2V5SU1FVUtWdW9GNE8zMnNNblBtTXI5MFB5RzRmMGMx?=
 =?utf-8?B?M0lKZTNnbjVUblRkenZxcS82TmgrNWVNNVRyUm83eVNFL3l0RlF6cmp5VVYw?=
 =?utf-8?B?MVhiTW1FeGxBb1R1NThsVFVmMjBDQnV4VUhwSmVGL0ZXQjllSEFCQXJqLzFE?=
 =?utf-8?B?VXVvelFDUlZ3YjZBa0RnUWxiSi96TXhBdHkrNVRuSzBoTzViL1VaY3I0VG93?=
 =?utf-8?B?cHR3SXJWMFdKdElRUXhyMFhtRHNSOERFZWN0N1JQeFlBc1pwakErSWxNbmx3?=
 =?utf-8?B?N0s4WkNUaWxxTHZSanpvVXovOU5wSjAwcTUrcUx3dUE0cU51REN1S3JBa0NP?=
 =?utf-8?B?elhTSWJjaXA3Y1pkUGVUYVgyM3ozSHR4ZCtSWXA3bHo1ZEQ0bnNHOXkvWk4v?=
 =?utf-8?B?bjIwKy8vclpSOHpVbEo3K3J0T0dKdVo0SlRQZHQ2WUdKR1Avei9tUGRRTkZL?=
 =?utf-8?B?RGlxblVtNGxpZGxIU0REaDVxaS9JUVNyWFIrUlIwbGxGV1dHRkE3Kyt3SlVC?=
 =?utf-8?B?TW8xMmhDMEVlT2JERjNZc0dTcVhTb1VxQ1BXQkdvK3dPaXFSOGNseHArVUov?=
 =?utf-8?B?ejBsSVZDZmMzOGJXRUVBbXh1Vit6bGtrRWVqZXpNOE1jdWxhWVBValh5WERT?=
 =?utf-8?B?bmNyREc4WStsbS95YWJ5S0Y5TDJvTVg1NnJwSGRLRm9GcWplWDZKczNOdWFo?=
 =?utf-8?B?WjBtaGloeElEVmRwekl2UFVJL0pld2kwc3ZHNzlHSEtUeTllS2ZKcllQZ0hV?=
 =?utf-8?Q?XOUa/dZHlRAqQeYge61bws8MX?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fdfe53ce-8752-474e-e23c-08dc59b42b85
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2024 23:15:54.1243
 (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: fkXOmBLO9Rl7SK8oqS/iWRraz4exxs0vECW5Iy4s4WIOq2ek8KJjodey6IR1RgySV0457jEfnvfuiEyYEtyfSg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6490
X-OriginatorOrg: intel.com


On 11/04/2024 3:29 am, Sean Christopherson wrote:
> On Wed, Apr 10, 2024, Kai Huang wrote:
>> On Fri, 2024-03-22 at 14:23 -0700, Isaku Yamahata wrote:
>>>>> +	r = atomic_read(&enable.err);
>>>>> +	if (!r)
>>>>> +		r = tdx_module_setup();
>>>>> +	else
>>>>> +		r = -EIO;
>>>>> +	on_each_cpu(vmx_off, &enable.enabled, true);
>>>>> +	cpus_read_unlock();
>>>>> +	free_cpumask_var(enable.enabled);
>>>>> +
>>>>> +out:
>>>>> +	return r;
>>>>> +}
>>>>
>>>> At last, I think there's one problem here:
>>>>
>>>> KVM actually only registers CPU hotplug callback in kvm_init(), which happens
>>>> way after tdx_hardware_setup().
>>>>
>>>> What happens if any CPU goes online *BETWEEN* tdx_hardware_setup() and
>>>> kvm_init()?
>>>>
>>>> Looks we have two options:
>>>>
>>>> 1) move registering CPU hotplug callback before tdx_hardware_setup(), or
>>>> 2) we need to disable CPU hotplug until callbacks have been registered.
> 
> This is all so dumb (not TDX, the current state of KVM).  All of the hardware
> enabling crud is pointless complex inherited from misguided, decade old paranoia
> that led to the decision to enable VMX if and only if VMs are running.  Enabling
> VMX doesn't make the system less secure, and the insane dances we are doing to
> do VMXON on-demand makes everything *more* fragile.
> 
> And all of this complexity really was driven by VMX, enabling virtualization for
> every other vendor, including AMD/SVM, is completely uninteresting.  Forcing other
> architectures/vendors to take on yet more complexity doesn't make any sense.

Ah, I actually preferred this solution, but I was trying to follow your 
suggestion here:

https://lore.kernel.org/lkml/ZW6FRBnOwYV-UCkY@google.com/

form which I interpreted you didn't like always having VMX enabled when 
KVM is present. :-)

> 
> Barely tested, and other architectures would need to be converted, but I don't
> see any obvious reasons why we can't simply enable virtualization when the module
> is loaded.
> 
> The diffstat pretty much says it all.

Thanks a lot for the code!

I can certainly follow up with this and generate a reviewable patchset 
if I can confirm with you that this is what you want?