Received-SPF: pass (google.com: domain of linux-kernel+bounces-170590-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <0ec64962-393c-4b2d-9689-c0375d7346aa@intel.com>
Date: Tue, 7 May 2024 10:37:47 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v10 24/27] KVM: x86: Enable CET virtualization for VMX and
 advertise to userspace
To: Sean Christopherson <seanjc@google.com>
CC: <pbonzini@redhat.com>, <dave.hansen@intel.com>, <x86@kernel.org>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<peterz@infradead.org>, <chao.gao@intel.com>, <rick.p.edgecombe@intel.com>,
	<mlevitsk@redhat.com>, <john.allen@amd.com>
References: <20240219074733.122080-1-weijiang.yang@intel.com>
 <20240219074733.122080-25-weijiang.yang@intel.com>
 <ZjLNEPwXwPFJ5HJ3@google.com>
 <e39f609f-314b-43c7-b297-5c01e90c023a@intel.com>
 <ZjkLVj01V4bM8z5c@google.com>
Content-Language: en-US
From: "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZjkLVj01V4bM8z5c@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?RnFvM3llM0ZhVndjUUVpS20zbDEvM3NDV0xUamp0OFNKT3QweFc2WHMwSTNK?=
 =?utf-8?B?MnZBVElRQmJSRzdLb055ZHBtWTlZb3pxY0hmK05wblUzbUJWNkF4YVg2SmRB?=
 =?utf-8?B?MW9hbVI0Y2FvYXFGV0NPeXBoZEFCTWhlUzlQZEhXNDl2V0hkNmFXc09VWW1D?=
 =?utf-8?B?cEdjNTl2NVpiZWt3WHQ1V1BlOHVKWXgzMG4yUVhVU3hDbVRZb1VEcU14SWgz?=
 =?utf-8?B?Mk50T3ZzV2FGNGQxTGdDMVVzNE11bEhWNHNDRmNZVnNnb0xlMnNKdDB2VitO?=
 =?utf-8?B?NEl4WFh2MnZOVlRDdkIvZFJiSXg5SDRndmhvVk1VejVETFQxUUxEZ2U5c1NI?=
 =?utf-8?B?WGo3aWd1bG9qY0lkalpFakxCeTFkZmJKTzZ1bHlrS3hIZE9KczltSTlFWmM0?=
 =?utf-8?B?VHYxekl3SllrTUZRZTJKdjBUeDVQMjhhbTVXTStGbmNyL3ZZZ0tLaWxPTGdy?=
 =?utf-8?B?LzYxZ0haR2JwY0ExS2p0MWV2Nnp4M28xQjY4YUNyaCtxM0N6OG1qSFYxUGpm?=
 =?utf-8?B?MW5MUHdkbEp2aWNzMGpacVhZbk1xbDFPR09NVWlyRzFITXdTcTNieFZ2Y3Uv?=
 =?utf-8?B?ZVVJWElvZ01QTmRQQitmU2s4SWJLRkZtUTJybElCMlJkV0U4UVA5S2xJbE02?=
 =?utf-8?B?S1hmVnNYakdEZlRlMVVJOWlsUmdVeDVwaVJxMVFUVUxnaXprYlVOVHhQYnNn?=
 =?utf-8?B?eGxBZXFhK0N0Y2J4Q1U3dkRFd0ZIL3N3MGFidVpZeTA5RTZjVUdYb1IwTmRE?=
 =?utf-8?B?V3c2c2NPeEFuS2VWU2MwVjFDbFBXaDJrcUJyb1ZUMEpodnYvOVkvVWo5MG5H?=
 =?utf-8?B?SUhXbjJQTEF6TmpvcWRJSFhnQk1hN0x0bnZUT1NubHI3TSsrZlVrN0ZHV0ll?=
 =?utf-8?B?MkRNeStxa2lpK2pETUNYdk13dFRKdnI5WnRkTG9rbWdaTFZQaWNObUJuSUhz?=
 =?utf-8?B?azZMN3JaN0JMSXFDbm15T2JTU0RGUnJXSDBKK20yeVlBaDZjQTJKUUdEOHUr?=
 =?utf-8?B?cTZoM1pkVzJFd1M2dnNNZDNSNnVlVEs4MVpzZnpvSzdyb3JjUHllWE1KYXFz?=
 =?utf-8?B?dVY2andtbGtPUmFHOUgyTG5vdlNFbzN2Qk1HUGE5b1dFTXUwWnZnekNqS1pa?=
 =?utf-8?B?Ynl5MUhzKzJCT21rRmJaMmZUUzZXbE5QaFVZMmU5d3VrYmszZUpoWXB4Z2ND?=
 =?utf-8?B?eVBhOTF0bUdvbVRzYkdhK1c2UFUxMFFadTV4KytORkFpakY1VGY0akVwamd3?=
 =?utf-8?B?c2M3R1FNSkFyOVFCOEl2QnFpT0luVXk4L3dQekt5RS9vWlQraXBGOUhwd0th?=
 =?utf-8?B?OE1MMnI3ZnhPNHBwbFBpTUtwS0M3Sm1ZRXNmcGZTRENtekpYbHRPYVFEV2wx?=
 =?utf-8?B?NXlaaE54U3UxSXVNVWFvS01rUHp6alhwcWoya0pNQ0oveXZZMnIxY2Q2SUZF?=
 =?utf-8?B?QVRyV3QyM1l2cVF4TnI2T0FuelphMHdKTjBMN2h3QW5RUzhFRHBiVVB6bWxr?=
 =?utf-8?B?MjhRa0N4N29ldzdndGkxajNBSmplSkl2TGIwbXdTOU44NURqODc2UG9GaVAw?=
 =?utf-8?B?SGh4UGJRaldBejZqbFJNbXdENTJ6bjMyWDJ4cVg0ck44ZHYrWW4vRXdrbzly?=
 =?utf-8?B?azQxalV6NjhBWmVicHRlRWtHZ1hFa0p0bVJoT20vYWNLVUR0dDFzWlVSQjFx?=
 =?utf-8?B?MnI0clBsdDNOZDd0aGdMYlhoMTdsZHVXQkdYZ0kxWmR5NCt2MitRb1ppOUY5?=
 =?utf-8?B?VG5lS2ZOa1hkVHZzaFBxb25NQjkyV2loMkJqZU5ZK2ovRWlwclNidTU4NWdo?=
 =?utf-8?B?azBTMHhBUXRQQjZxWjVoVjI2Z1NLR1NlOXpabFBGakoyanBBUjY4WmdjcWw4?=
 =?utf-8?B?dG1zcXZROHFkMEZOK0svRXZtNVF0ZVppNFV2R2V0eFBrVVZjSXVSYk9vQTFu?=
 =?utf-8?B?a3RVSUF2TVRPaUc5QlArck9kelpJcHo5cWRNalJ1UEhJRWk1MDd5Vk5KeHdQ?=
 =?utf-8?B?TzQ1MVpuZmFKQzZ3UjFNR3VWbG43bVRqdW5oYmc5Q2dmcjY1TWpVK1RMMG05?=
 =?utf-8?B?OFg1SmtCT1FDZGkzbHlMd3RhdktDOTBndWQvUWp3U1drK0hTM2VqTGtSL2FV?=
 =?utf-8?B?ei9qWnZpeXVxQnR6cFNjSDJEbUlpU2lWMWZxSzN1djRINU9YT2NPakxtVHlP?=
 =?utf-8?B?aXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d80f7212-478c-4d6e-f8b5-08dc6e3eb40a
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 02:37:57.2156
 (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: 1xeo9QczOdjgepbu7K1sCBKOvlEuXzgVEvOT+YagNhpjz+JCruo1nEjouY/YTK+FQwzVj9gCUDpk4K+qEh8WMw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6504
X-OriginatorOrg: intel.com

On 5/7/2024 12:54 AM, Sean Christopherson wrote:
> On Mon, May 06, 2024, Weijiang Yang wrote:
>> On 5/2/2024 7:15 AM, Sean Christopherson wrote:
>>> On Sun, Feb 18, 2024, Yang Weijiang wrote:
>>>> @@ -696,6 +697,20 @@ void kvm_set_cpu_caps(void)
>>>>    		kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
>>>>    	if (boot_cpu_has(X86_FEATURE_AMD_SSBD))
>>>>    		kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL_SSBD);
>>>> +	/*
>>>> +	 * Don't use boot_cpu_has() to check availability of IBT because the
>>>> +	 * feature bit is cleared in boot_cpu_data when ibt=off is applied
>>>> +	 * in host cmdline.
>>> I'm not convinced this is a good reason to diverge from the host kernel.  E.g.
>>> PCID and many other features honor the host setup, I don't see what makes IBT
>>> special.
>> This is mostly based on our user experience and the hypothesis for cloud
>> computing: When we evolve host kernels, we constantly encounter issues when
>> kernel IBT is on, so we have to disable kernel IBT by adding ibt=off. But we
>> need to test the CET features in VM, if we just simply refer to host boot
>> cpuid data, then IBT cannot be enabled in VM which makes CET features
>> incomplete in guest.
>>
>> I guess in cloud computing, it could run into similar dilemma. In this case,
>> the tenant cannot benefit the feature just because of host SW problem.
> Hmm, but such issues should be found before deploying a kernel to production.
>
> The one scenario that comes to mind where I can see someone wanting to disable
> IBT would be running a out-of-tree and/or third party module.

Yes, the developers may neglect IBT violations in modules/kernel components and deploy
them, in this case, host side has to either fix the issues or disable IBT.

>
>> I know currently KVM except LA57 always honors host feature configurations,
>> but in CET case, there could be divergence wrt honoring host configuration as
>> long as there's no quirk for the feature.
>>
>> But I think the issue is still open for discussion...
> I'm not totally opposed to the idea.
>
> Somewhat off-topic, the existing LA57 code upon which the IBT check is based is
> flawed, as it doesn't account for the max supported CPUID leaf.  On Intel CPUs,
> that could result in a false positive due CPUID (stupidly) returning the value
> of the last implemented CPUID leaf, no zeros.  In practice, it doesn't cause
> problems because CPUID.0x7 has been supported since forever, but it's still a
> bug.
>
> Hmm, actually, __kvm_cpu_cap_mask() has the exact same bug.  And that's much less
> theoretical, e.g. kvm_cpu_cap_init_kvm_defined() in particular is likely to cause
> problems at some point.
>
> And I really don't like that KVM open codes calls to cpuid_<reg>() for these
> "raw" features.  One option would be to and helpers to change this:
>
> 	if (cpuid_edx(7) & F(IBT))
> 		kvm_cpu_cap_set(X86_FEATURE_IBT);
>
> to this:
>
> 	if (raw_cpuid_has(X86_FEATURE_IBT))
> 		kvm_cpu_cap_set(X86_FEATURE_IBT);
>
> but I think we can do better, and harden the CPUID code in the process.  If we
> do kvm_cpu_cap_set() _before_ kvm_cpu_cap_mask(), then incorporating the raw host
> CPUID will happen automagically, as __kvm_cpu_cap_mask() will clear bits that
> aren't in host CPUID.
>
> The most obvious approach would be to simply call kvm_cpu_cap_set() before
> kvm_cpu_cap_mask(), but that's more than a bit confusing, and would open the door
> for potential bugs due to calling kvm_cpu_cap_set() after kvm_cpu_cap_mask().
> And detecting such bugs would be difficult, because there are features that KVM
> fully emulates, i.e. _must_ be stuffed after kvm_cpu_cap_mask().
>
> Instead of calling kvm_cpu_cap_set() directly, we can take advantage of the fact
> that the F() maskes are fed into kvm_cpu_cap_mask(), i.e. are naturally processed
> before the corresponding kvm_cpu_cap_mask().
>
> If we add an array to track which capabilities have been initialized, then F()
> can WARN on improper usage.  That would allow detecting bad "raw" usage, *and*
> would detect (some) scenarios where a F() is fed into the wrong leaf, e.g. if
> we added F(LA57) to CPUID_7_EDX instead of CPUID_7_ECX.
>
> #define F(name)								\
> ({									\
> 	u32 __leaf = __feature_leaf(X86_FEATURE_##name);		\
> 									\
> 	BUILD_BUG_ON(__leaf >= ARRAY_SIZE(kvm_cpu_cap_initialized));	\
> 	WARN_ON_ONCE(kvm_cpu_cap_initialized[__leaf]);			\
> 									\
> 	feature_bit(name);						\
> })
>
> /*
>   * Raw Feature - For features that KVM supports based purely on raw host CPUID,
>   * i.e. that KVM virtualizes even if the host kernel doesn't use the feature.
>   * Simply force set the feature in KVM's capabilities, raw CPUID support will
>   * be factored in by kvm_cpu_cap_mask().
>   */
> #define RAW_F(name)						\
> ({								\
> 	kvm_cpu_cap_set(X86_FEATURE_##name);			\
> 	F(name);						\
> })
>
> Assuming testing doesn't poke a hole in my idea, I'll post a small series.

Fancy enough! But I like the idea :-)

>