Received-SPF: pass (google.com: domain of linux-kernel+bounces-164880-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d143b41e-e8f9-4f89-a571-6771ff538aa7@intel.com>
Date: Wed, 1 May 2024 14:56:30 +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: 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>
References: <d0563f077a7f86f90e72183cf3406337423f41fe.camel@intel.com>
 <ZifQiCBPVeld-p8Y@google.com>
 <61ec08765f0cd79f2d5ea1e2acf285ea9470b239.camel@intel.com>
 <9c6119dacac30750defb2b799f1a192c516ac79c.camel@intel.com>
 <ZiqFQ1OSFM4OER3g@google.com>
 <b605722ac1ffb0ffdc1d3a4702d4e987a5639399.camel@intel.com>
 <Zircphag9i1h-aAK@google.com>
 <b2bfc0d157929b098dde09b32c9a3af18835ec57.camel@intel.com>
 <Zi_93AF1qRapsUOq@google.com>
 <2eab6265-3478-45db-86a5-722de6f39e74@intel.com>
 <ZjEYqCgtt4ZgVKth@google.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZjEYqCgtt4ZgVKth@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?ZUZ0WkYvREV6TFVBajA2ZTNSd2ZWRFpOSmNxcUpXUTlrSjVabXoxMU94VERa?=
 =?utf-8?B?bDJiRU84WndhOFQ3TkMxNitoT2p0Zm5PaHBRQmFzNlptMlV4RENnZWhtQVhE?=
 =?utf-8?B?K05tc1J5RE9ZcTkwOG5JdGxTYkNhdzZld0t6anpXQ0YyMGViS3BNdy9IWHJp?=
 =?utf-8?B?YnUvSE1vQVR5ZE0yV0FpVkI0aVg0Y09ZNXV1dFZzREtDUmVQQTRUZUxEOTBk?=
 =?utf-8?B?TXhjdlFKZkEyNGtkd3RQQmtmM1NaaitiOHZKVVEvR3RzUW9ZRGJnWldTVG9O?=
 =?utf-8?B?cHhleW02cUx6MnhkWDFJT1hIWjlLbWJ4UlM1eUhrZURDTjlGRy9iQy95Q1NR?=
 =?utf-8?B?YlNiZHpMbUpMY2cxTFRHT1JKYnFiNERuaW9JZ0tHUWNhSUwxNXV6dkhRWTlO?=
 =?utf-8?B?SU1TcWRSYUJkVTY2WGtXTGVpbCthTGlOUXlMT294OFhlUUN1N29DcldpRE5z?=
 =?utf-8?B?RjVoaDNQR2FVNnpCV2swekxNUWdYWkRxNFpnalQzQ2dHTVN2ZnEwS0ErOTVL?=
 =?utf-8?B?RWh5VVNVZUh6V3ZtejJDVElFeVloTmpDbjFUc2JsVWxlbUtGeWV6RHE4L0k2?=
 =?utf-8?B?TXY0Q0o1emRhd0xYeElWaUpPUUwwT1hsNk81UWZCSkVoWXdEd3FwUjhjb2VH?=
 =?utf-8?B?K3dxTTJyanE5MjUvZ2QzR3FUYVc1WUQvVEpiSGd5YkRSUVRhOFZ1S1N0ZC9x?=
 =?utf-8?B?eXZibG8xandtWmJwc0g0VWpWMThXZFdaMXVrQjlKV1BMY1VMcU0rRG9INDNI?=
 =?utf-8?B?M2kxZDBOcFNQcm9SN2haTVNzZ242OHord0Q3UVcydlU0UE9PeGFSTlR3anUy?=
 =?utf-8?B?eUVtU01sOVhJbXBqcmRJWk94TlFFMU9FcnZ5d0txTTRqd2RRTkZSNmI0SXRN?=
 =?utf-8?B?Sk53RDdZdXFBTC8yTXpIL1o1MlFpRWtaNUVRc0U3b2NLSEtoQ05zcGdFQzdO?=
 =?utf-8?B?RXVPUk4wVFBRc0RKZUs2UmVwa292MlMrWGpZTjVxRjE3S2JuVjBjRFh6cHhD?=
 =?utf-8?B?ckhUbGVpNDRoajlQbzR2Qm1jYklIamtkWm91WmxrYmNsckhDYmlwZlBEMFpV?=
 =?utf-8?B?RkMxeGdlbm5YT1RId1ExSzI1amtHNDFIK0QrQjRuNUhtOFUzUTBBdWVRYmxD?=
 =?utf-8?B?aklmK0pTekZMMVczVWFCMGo4WXdaNU9BdnNuQ0oxZ291UzBuc1BwdGNoYWw2?=
 =?utf-8?B?UG9WWHV6QUJUbUFMZFJSYnZ2VzVITzczYjRNNEtscUk4aUJhbGNYNXNqUjhx?=
 =?utf-8?B?ekN4Uk15N2pka1VZdEw4OVU2WmdsMDdScW55SVNGejVORlI5d0JYWDQ4ZU9T?=
 =?utf-8?B?YWt5T0ovYnlQYWJ3VEpDaVN2Q2JYWkc1QXUyL1k2YWdrb2pYM3UrUUR0Z0tM?=
 =?utf-8?B?VE9IbHY3alhOekJOenlzNUZhQW9KWjV0REFMNkwrU0s0OWt2aEZxWU5xcmhS?=
 =?utf-8?B?dTJaZDY0ZkRldHo1NllsVUMrTnRmMmFBTW1aRk1ld2dOZlY1TzNWOW9vWkJ1?=
 =?utf-8?B?eFB1bXUxdjh0RUZwR0ZkdlZvYVloMmRsWUNucmRRNktoSVlIdHpPeVorZUpw?=
 =?utf-8?B?Tk12MERlaEJpTklhTUlsbjE5Ri9Pd0RScE85a3lCTEg4WHlLMXpJRFl4YTQ1?=
 =?utf-8?B?TXd6S0NLQlFRWktzZVRuUW90Z2ZoVlNzaU5DWFltM1hKQUZPRjZvcVFEaTlD?=
 =?utf-8?B?UXJpS2pMS3M4NTYrR05rVFpPWTlpTVg5S0F2K1JVVVdqaDAyQ1BtWC9Fclkr?=
 =?utf-8?B?dnN1ZzZmK2tVdW9uSmZTQ3BhTm9xNGJRdm1ydVZlV01ldkUvUmhjeDdSSXAw?=
 =?utf-8?B?VFB2RnhLM25Ha2xpMUF5RkFSTmFNQS9Ra2RWSzlaVDJGTUo5Y2o4aDRBWXhj?=
 =?utf-8?B?VXNka21QeWUrNUpwSGI5Q1NRTW9BSDQ5YlZTbWRKTURYeWs2R05iNjhOSTdu?=
 =?utf-8?B?U2pLY1N4MERPZk55c1NjOGZTb1R0N1NPSzFSWVkxNlE4MjhxbHpKVVJTVkdo?=
 =?utf-8?B?VXdIVVJFR0pRMC9mVkowZWt1N3hwTEJFdmx5Vys0QkVaOWNEaUN2MXRDOWFX?=
 =?utf-8?B?bXV4K2dRaDFhc01rMkNGZE1qa2M3MUxXeVNtVU5YUGt6RW1xQjV2TlBHT21Z?=
 =?utf-8?Q?TuasR+LddRtjnBbBsGFBPxegm?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9430df2c-52a0-4784-ff3c-08dc698a543a
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 May 2024 02:56:42.0379
 (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: Sm8yxq2GXN24i1tsM0cxXlrqY3gTUtALry3qY/fDyhTRG4nwpbMj/HLXq7LTonNbwh93Ae6SkY9+z6KYSbv3Sg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB8502
X-OriginatorOrg: intel.com


On 1/05/2024 4:13 am, Sean Christopherson wrote:
> On Tue, Apr 30, 2024, Kai Huang wrote:
>> On 30/04/2024 8:06 am, Sean Christopherson wrote:
>>> My suggestion is essentially "throw in a CR4.VMXE check before
>>> TDH.SYS.LP.INIT if it's easy".  If it's not easy for some reason, then don't do
>>> it.
>>
>> I see.  The disconnection between us is I am not super clear why we should
>> treat TDH.SYS.LP.INIT as a special one that deserves a CR4.VMXE check but
>> not other SEAMCALLs.
> 
> Because TDH.SYS.LP.INIT is done on all CPUs via an IPI function call, is a one-
> time thing, and is at the intersection of core TDX and KVM module code, e.g. the
> the core TDX code has an explicit assumption that:
> 
>   * This function assumes the caller has: 1) held read lock of CPU hotplug
>   * lock to prevent any new cpu from becoming online; 2) done both VMXON
>   * and tdx_cpu_enable() on all online cpus.

Yeah but from this perspective, both tdx_cpu_enable() and tdx_enable() 
are "a one time thing" and "at the intersection of core TDX and KVM"  :-)

But from the perspective that tdx_cpu_enable() must be called in IRQ 
disabled context, and there's no possibility that other thread/code 
could potentially mess up VMX enabling after the CR4.VMXE check, so it's 
fine to add such check.

And looking again, in fact the comment of tdx_cpu_enable() doesn't 
explicitly call out it requires the caller to do VMXON first (although 
kinda implied by the comment of tdx_enable() as you quoted above).

I can add a patch to make it more clear by calling out in the comment of 
tdx_cpu_enable() that it requires caller to do VMXON and adding a 
WARN_ON_ONCE(!CR4.VMXE) check inside.  I just don't know whether it is 
worth to do at this stage given it's not something mandatory because it 
requires review time from maintainers.  I can include such patch in next 
KVM TDX patchset if you prefer so we can see how it goes.

> 
> KVM can obviously screw up and attempt SEAMCALLs without being post-VMXON, but
> that's entirely a _KVM_ bug.  And the probability of getting all the way to
> something like TDH_MEM_SEPT_ADD without being post-VMXON is comically low, e.g.
> KVM and/or the kernel would likely crash long before that point.

Yeah fully agree SEAMCALLs managed by KVM shouldn't need to do CR4.VMXE 
check.  I was talking about those involved in tdx_enable(), i.e., 
TDH.SYS.xxx.