Received-SPF: pass (google.com: domain of linux-kernel+bounces-159460-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 26 Apr 2024 11:21:25 +0800
From: Chao Gao <chao.gao@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
CC: "seanjc@google.com" <seanjc@google.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>, "Yamahata, Isaku"
	<isaku.yamahata@intel.com>, "isaku.yamahata@linux.intel.com"
	<isaku.yamahata@linux.intel.com>
Subject: Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when
 loading the KVM intel kernel module
Message-ID: <ZisdtTJoqe7yxnvI@chao-email>
References: <3771fee103b2d279c415e950be10757726a7bd3b.camel@intel.com>
 <Zib76LqLfWg3QkwB@google.com>
 <6e83e89f145aee496c6421fc5a7248aae2d6f933.camel@intel.com>
 <d0563f077a7f86f90e72183cf3406337423f41fe.camel@intel.com>
 <ZifQiCBPVeld-p8Y@google.com>
 <61ec08765f0cd79f2d5ea1e2acf285ea9470b239.camel@intel.com>
 <ZiqGRErxDJ1FE8iA@google.com>
 <22821630a2616990e5899252da3f29691b9c9ea8.camel@intel.com>
 <ZirUN9G-Y1VUSlDB@google.com>
 <2caa30250d3f6e04f4e23af96caed0f92bf5f8c3.camel@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2caa30250d3f6e04f4e23af96caed0f92bf5f8c3.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?5VGs4MaRZEoaN5f2YDTz6BBUJSfbRxh2REhPGdMhjCLb5YGnhQ0AEIkFmL?=
 =?iso-8859-1?Q?TKpYsKAKfz8oqCVN5g0ISp4hyCjoTK+oV8jtYXKs7yJEn8QfeBTWvdx0vC?=
 =?iso-8859-1?Q?Iu8li662M7/xhc4Bue72tMZAZGoRJakK0YA+RNpNB9hNf7AAXyO+q2TQw5?=
 =?iso-8859-1?Q?KJCKWaRRhtiydwFFVpNtfF0zOcL9xggPSlwWK8+2//NuLgx/OlNcdtPcGr?=
 =?iso-8859-1?Q?12Jj5tJk9Ol2Y33UfSTUQpOV4ZYXPLauTswHtNY7XeFpRSJM7mtl+Slif3?=
 =?iso-8859-1?Q?2nLWEaNPeoYbiqvvldzumQNipVa3Th+BlqNUW43x/N+Ji5FxtTqpPNbTqh?=
 =?iso-8859-1?Q?Sp0/UkMbuDjTNJuhPolnLyhvlKKp+MmA0ycZkbU1HWkEe9AXley/qIgHln?=
 =?iso-8859-1?Q?oJxPLOhemBXk5PBc8UyWpofP18YXSrN6J/TBEXt0yJOAjd5Py1UDHzhy7+?=
 =?iso-8859-1?Q?IfuVB33eWTi/VW4Rzu5P38eYEwLWi66Afu3vAMpQ2dVeyjyqHC8G30YkEz?=
 =?iso-8859-1?Q?LZ5ogeUP0EhcthMEIgmig67jWvAjJmb8A+tIUTGFh8mIBst07QnP3h+xSE?=
 =?iso-8859-1?Q?3RD5lQ1Wrm6+LU7/Yl+VWSI2HksLit9/N1+ggScIkOh3Sfq94Um25Ijrhq?=
 =?iso-8859-1?Q?jx7noHe/9XflGd2zGgRb1APLEO819/3i0QeLkbSbFU0YS6iaCsBzjPtqod?=
 =?iso-8859-1?Q?vQwvSQH/ZcgucVCnFktoE6MIhyfp+og76oDS3UL2THZTQECMoe+GHzDcHJ?=
 =?iso-8859-1?Q?txhwp2eUlVCuBAETHe11uEv/hfe2+B0iCI3Gt3TkSi11YCMcK4ddO6Rh4H?=
 =?iso-8859-1?Q?punO7b8dqtpFyuAeZ4lbslEQRrIP88KUDopZqfkAAxj5Mnnqcgto6G9v3+?=
 =?iso-8859-1?Q?itENKYJschCyGSGPWeKYmtdiheERYWUjnPWA3e55mnisbEhRhGpQnXHhRx?=
 =?iso-8859-1?Q?5k18cC8EbNTzoMS9tfQGD0w+hcaUUmV/uvCSIh5e7/fq47gmw9jTwFKO1O?=
 =?iso-8859-1?Q?304V9S5Vf7m2RXrL5NEAFmsz5uJnP03BAr8bQK1WqmOYyHUi10TXKutnjf?=
 =?iso-8859-1?Q?Sg+8GSQTJtEAKehT7iWzArytRfxvKY0zFRymkXLPOUl7BE4xppWnhsP29t?=
 =?iso-8859-1?Q?SxqrrWy/PV9aUdV400smKK46euMPGTXiSdfBZXpgPegD9gwHd8U2R+Zi1x?=
 =?iso-8859-1?Q?eHfMhVB2G/qPAPykOp5EC0iUbGukD84vXjP97CWXTLSYSCtvkaroP3sHnp?=
 =?iso-8859-1?Q?TbHcuU5td6wiToMRYNGEPwztAm3QYmuQjY6T7jllgARe7V5/Kt7WKb1HOR?=
 =?iso-8859-1?Q?BviYn1C4l3TyZ82HufcfceQAhRdGrfe5Kv2cbVCOmkuL0rbPuddMfQ8yZ+?=
 =?iso-8859-1?Q?grH246nmTddJzvSuJwK24CvXOYIXUjvgJe7Zvsv8VWfaj1SnXVyNmfimjn?=
 =?iso-8859-1?Q?P9IddGZmJSDv15JOHTlwCZoE8SMNYCv5y0iaTeWAAM6ppZWCDtFZ3kWWhd?=
 =?iso-8859-1?Q?BNQk4+eTO0OfETo9zy2dbPzy8k9E/V8Rru57bM3I7xFA1JtUahq35rVvlF?=
 =?iso-8859-1?Q?T0vpzGxbvaUcRYRBcrrWsAd9cuui2MmZ9typmnYC+Q81838aB/xyvq6fTw?=
 =?iso-8859-1?Q?LdLSO2tOgLnSgOzr0ENHaZPwqhVhv5zNh9?=
X-MS-Exchange-CrossTenant-Network-Message-Id: cccbe619-b384-44cc-2f57-08dc659ffa21
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2024 03:21:35.3929
 (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: OZEXYdRBQj2VHTRvhoYtISBD1W7PxCimb4v4/3SM//N8i7NSyx5OtNKds3mTi16rV8rIUfMw8BTUkR+s+9conQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR11MB8458
X-OriginatorOrg: intel.com

On Fri, Apr 26, 2024 at 12:21:46AM +0000, Huang, Kai wrote:
>
>> 
>> > > The important thing is that they're handled by _one_ entity.  What we have today
>> > > is probably the worst setup; VMXON is handled by KVM, but TDX.SYS.LP.INIT is
>> > > handled by core kernel (sort of).
>> > 
>> > I cannot argue against this :-)
>> > 
>> > But from this point of view, I cannot see difference between tdx_enable()
>> > and tdx_cpu_enable(), because they both in core-kernel while depend on KVM
>> > to handle VMXON.
>> 
>> My comments were made under the assumption that the code was NOT buggy, i.e. if
>> KVM did NOT need to call tdx_cpu_enable() independent of tdx_enable().
>> 
>> That said, I do think it makes to have tdx_enable() call an private/inner version,
>> e.g. __tdx_cpu_enable(), and then have KVM call a public version.  Alternatively,
>> the kernel could register yet another cpuhp hook that runs after KVM's, i.e. does
>> TDX.SYS.LP.INIT after KVM has done VMXON (if TDX has been enabled).
>
>We will need to handle tdx_cpu_online() in "some cpuhp callback" anyway,
>no matter whether tdx_enable() calls __tdx_cpu_enable() internally or not,
>because now tdx_enable() can be done on a subset of cpus that the platform
>has.

Can you confirm this is allowed again? it seems like this code indicates the
opposite:

https://github.com/intel/tdx-module/blob/tdx_1.5/src/vmm_dispatcher/api_calls/tdh_sys_config.c#L768C1-L775C6

>
>For the latter (after the "Alternatively" above), by "the kernel" do you
>mean the core-kernel but not KVM?
>
>E.g., you mean to register a cpuhp book _inside_ tdx_enable() after TDX is
>initialized successfully?
>
>That would have problem like when KVM is not present (e.g., KVM is
>unloaded after it enables TDX), the cpuhp book won't work at all.

Is "the cpuhp hook doesn't work if KVM is not loaded" a real problem?

The CPU about to online won't run any TDX code. So, it should be ok to
skip tdx_cpu_enable().

Don't get me wrong. I don't object to registering the cpuhp hook in KVM.
I just want you to make decisions based on good information.

>
>If we ever want a new TDX-specific cpuhp hook "at this stage", IMHO it's
>better to have it done by KVM, i.e., it goes away when KVM is unloaded.
>
>Logically, we have two approaches in terms of how to treat
>tdx_cpu_enable():
>
>1) We treat the two cases separately: calling tdx_cpu_enable() for all
>online cpus, and calling it when a new CPU tries to go online in some
>cpuhp hook. ?And we only want to call tdx_cpu_enable() in cpuhp book when
>tdx_enable() has done successfully.
>
>That is:?
>
>a) we always call tdx_cpu_enable() (or __tdx_cpu_enable()) inside
>tdx_enable() as the first step, or,
>
>b) let the caller (KVM) to make sure of tdx_cpu_enable() has been done for
>all online cpus before calling tdx_enable().
>
>Something like this:
>
>	if (enable_tdx) {
>		cpuhp_setup_state(CPUHP_AP_KVM_ONLINE, kvm_online_cpu,?
>			...);
>
>		cpus_read_lock();
>		on_each_cpu(tdx_cpu_enable, ...); /* or do it inside?
>						   * in tdx_enable() */
>		enable_tdx = tdx_enable();
>		if (enable_tdx)
>			cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>				tdx_online_cpu, ...);
>		cpus_read_unlock();
>	}
>
>	static int tdx_online_cpu(unsigned int cpu)
>	{
>		unsigned long flags;
>		int ret;
>
>		if (!enable_tdx)
>			return 0;
>
>		local_irq_save(flags);
>		ret = tdx_cpu_enable();
>		local_irq_restore(flags);
>
>		return ret;
>	}
>
>2) We treat tdx_cpu_enable() as a whole by viewing it as the first step to
>run any TDX code (SEAMCALL) on any cpu, including the SEAMCALLs involved
>in tdx_enable().
>
>That is, we *unconditionally* call tdx_cpu_enable() for all online cpus,
>and when a new CPU tries to go online.
>
>This can be handled at once if we do tdx_cpu_enable() inside KVM's cpuhp
>hook:
>
>	static int vt_hardware_enable(unsigned int cpu)
>	{
>		vmx_hardware_enable();
>
>		local_irq_save(flags);
>		ret = tdx_cpu_enable();
>		local_irq_restore(flags);
>
>		/*
>		 * -ENODEV means TDX is not supported by the platform
>		 * (TDX not enabled by the hardware or module is
>		 * not loaded) or the kernel isn't built with TDX.
>		 *
>		 * Allow CPU to go online as there's no way kernel
>		 * could use TDX in this case.
>		 *
>		 * Other error codes means TDX is available but something
>		 * went wrong.  Prevent this CPU to go online so that
>		 * TDX may still work on other online CPUs.
>		 */
>		if (ret && ret != -ENODEV)
>			return ret;
>
>		return ret;
>	}
>
>So with your change to always enable virtualization when TDX is enabled
>during module load, we can simply have:
>
>	if (enable_tdx)
>		cpuhp_setup_state(CPUHP_AP_KVM_ONLINE, kvm_online_cpu,?
>			...);
>
>		cpus_read_lock();
>		enable_tdx = tdx_enable();
>		cpus_read_unlock();
>	}
>
>So despite the cpus_read_lock() around tdx_enable() is a little bit silly,
>the logic is actually simpler IMHO.
>
>(local_irq_save()/restore() around tdx_cpu_enable() is also silly but that
>is a common problem to both above solution and can be changed
>independently).
>
>Also, as I mentioned that the final goal is to have a TDX-specific CPUHP
>hook in the core-kernel _BEFORE_ any in-kernel TDX user (KVM) to make sure
>all online CPUs are TDX-capable. ?
>
>When that happens, I can just move the code in vt_hardware_enable() to
>tdx_online_cpu() and do additional VMXOFF inside it, with the assumption
>that the in-kernel TDX users should manage VMXON/VMXOFF on their own. 
>Then all TDX users can remove the handling of tdx_cpu_enable().