Received-SPF: pass (google.com: domain of linux-kernel+bounces-169529-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <3f483ce7-0fcd-4f01-8d99-232582e03136@intel.com>
Date: Mon, 6 May 2024 16:48:54 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v10 22/27] KVM: VMX: Set up interception for CET MSRs
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-23-weijiang.yang@intel.com>
 <ZjLLNyvbpfemyN5g@google.com>
Content-Language: en-US
From: "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZjLLNyvbpfemyN5g@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?Rk84Q3ZnUmRrNW91c2tTQlpJUjNNMzlMODhGbVE0WG1tYXFLK1cydW9OMThi?=
 =?utf-8?B?ZlliQWNpbEtQNGVzRmZYb2VTcjFVWEhzN3YrWTQ2OXpsc2liVzMraEkrQm1x?=
 =?utf-8?B?SHJ6Q05rb1paZThlRGRQWjM1LzlmeC9HSjB0SWdYNVNBT2ZDc3RaaDFrNmh3?=
 =?utf-8?B?VE5KU1FqK0FpR2IzdGJUdnluenRXZGJoQmZuMndSdm5kcmpxL0xLL2h6b1cw?=
 =?utf-8?B?S3hUR2tYd0w5VWFvSUEreXBNV2hNRXVmS0ZnakFUYmswZmVnMzhHcmdoN3dN?=
 =?utf-8?B?OEJNSDFXMHZZQmxjVzdGZm8rRktRSG5lNk5EbnkvWVFNVVY3SFlIcFh6U0NT?=
 =?utf-8?B?RmV2bEhQL1UyOTFtYVQxdXBHcHhPYUovZ0t6dXNqVzlTSm9TMEtMRnV2ejMw?=
 =?utf-8?B?anFFTUlYZlJoRC9XRnY0ZTNOcGt3Y0xVbDhGSGo2b0dnbVVHWHpsb2grYmNK?=
 =?utf-8?B?b1JhZ0psKzJORGhtS0UwMXZVWjZxTkVaQVFQOThMcDBhbUhVcmphREpxQ1Jm?=
 =?utf-8?B?TGhGbEFBZFUyU2VJSUtQN1o2UUV4WXl1eHJ1Z3lVM3VtTmg0T3E1bmNVSEIw?=
 =?utf-8?B?c3lNMWhwZ2szc0tYSmUyR1pkRmZpQ2g4OVhqNXB2MGs3TlcxVHpDdy9Ecitz?=
 =?utf-8?B?QU52Mng4M0NDZVgwM2ZNRko1SHFiV3ZsaHRnT2cvSU10Nm42ai9Nc3BsZGJG?=
 =?utf-8?B?d1NzNE1UUUhLTXNOTnE0MnJRdHdkY0J6b1ZQU0U5Tm1JZUtiRzBsMkJ0c1VY?=
 =?utf-8?B?QTRlK01yUEZ6YmxtUVlYNVVaUm5LY1lHUFVyeVNKelVINzk5WlB2Q3dOdlFr?=
 =?utf-8?B?VUJvMXRFcUk2YVEyZXNjVUE2Tnpmb1dCdXR3TVYwQTEreHNSZ09xZG1Saml2?=
 =?utf-8?B?REZsWXpBL1Y4ZzloTFpNZ2ZqYmpWZWRKaVhUY0U3dUlYMEN6REM1VllIQ2Ny?=
 =?utf-8?B?Nld1VVZub1NXOGZoYVJZUUlZUmxEbXZScytZYTVyeTJuMVlXc3FJeXJkSkRm?=
 =?utf-8?B?YmRjdTZ4M1Nxcm40Rnk5Y1pRcDlYTFJTZGJkRHNVZ3JldHRYVkJQaVNzVDh6?=
 =?utf-8?B?S1d6YzBQZVVXUGd1aitwam1Hb29STlh0eFJUQzl1TThzR2ZCRWRUckhKOUZL?=
 =?utf-8?B?dGN4SCszMHdtWkNRZTBzRWltU3h3dWpEV0FCR3FrUnJtV093cTJyNm5LVEoz?=
 =?utf-8?B?em8vbW1kR3VDcmI3N1VXT0plNStLMkJuOUNLN0pmelEzQVdkcXBIK2xITXZr?=
 =?utf-8?B?MHVLWmQ2SE9pcDloZ2hnWFR1Z0JwZWdJQllEQXhSREZ0dlVnbmY1c1U3aGhu?=
 =?utf-8?B?Sjd6NTY5UllwS2hnR01PR1Q4Ky93eWo2QU1jMTVta3lHQnNzRWZxSURLOVlM?=
 =?utf-8?B?b1c4UmV4WVlkVDd5NXNXUGh2Y2dyejBLcDBpWm5wUjZYdjhMN1lTenlxaCtV?=
 =?utf-8?B?ZG1CYWJ0T2pDbFBIWXBFRGVIa2toMVY2cnlpbnRrbzJRYlVYT0hRRit6RnR1?=
 =?utf-8?B?dkZtbDd2clRuZjB2ZWdRWk1uVWN5TEhyWXpHWTloQldUWDNVblNEZmZCbTRK?=
 =?utf-8?B?NTdESHRBQXpKWFI0Q2RVVHUxOE5MMHFFOUJPSFI5cFhFQ2RMbzZwa0Y5OGxI?=
 =?utf-8?B?cWxtWExJaE5hdXZrdG9ZZll2UjA4eTczTWhYam9ENmROUFljWU1PYW5SMGwv?=
 =?utf-8?B?SXdsWjV3OE9iekJSa0FiL2h1dUQydWVvbE54WnJmb1piQ3lpcmx4RlpJb2Q4?=
 =?utf-8?B?YUNPU042NWxtV05ncWdVblhJTTIraWxmbG5FdzZXZnVOZm53S05UaUcrSWRz?=
 =?utf-8?B?dVZuU0NsQzliRzN3QkQrN1l4akhkWVd2bVNXMitlYm5IUFRhOUV4UFNvdmhP?=
 =?utf-8?B?cU5xZFRDSTJ1ZUhsRzg5MUxROHZnQWJ5Y1JZZzZPSnJMUC9FSFpSL0lUNmxv?=
 =?utf-8?B?Unk5NWR6NVRUREpoZjFhaUN4NURoUnk3WkJXM2NhYm5PR0pNbkxFc1VDbGEv?=
 =?utf-8?B?NEtIbGlTTXFLaGRvMHZFVTJxM014dERNOVZoZHBTVElzZFRaVElKUmdCNGNP?=
 =?utf-8?B?Qk1Va0MzUkJjOXEyNDZTaEorTmFFRzdmZUZRQ1pFaEZzcjdTVUlFbEtBSGNI?=
 =?utf-8?B?K3RQMzJWVlMzOU5qRk5VRjJ6MFdlTXhVVkhJL1laaldNQm5CSzJTQUU2MHB4?=
 =?utf-8?B?a1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b6b0adbb-c2be-48d0-8a65-08dc6da9612b
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 08:49:02.9941
 (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: rwClc7+yZxnbH/gFyQ2omO0FppJUHjaJCsY31VOh8vUm2fnerC3tWfTaOY5F7z8Q51vn/s0drsNZSlqekPf47w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4907
X-OriginatorOrg: intel.com

On 5/2/2024 7:07 AM, Sean Christopherson wrote:
> On Sun, Feb 18, 2024, Yang Weijiang wrote:
>> @@ -7767,6 +7771,41 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>>   		vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4));
>>   }
>>   
>> +static void vmx_update_intercept_for_cet_msr(struct kvm_vcpu *vcpu)
>> +{
>> +	bool incpt;
>> +
>> +	if (kvm_cpu_cap_has(X86_FEATURE_SHSTK)) {
>> +		incpt = !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>> +
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_U_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_S_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL3_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB,
>> +					  MSR_TYPE_RW, incpt);
>> +		if (!incpt)
>> +			return;
> Hmm, I find this is unnecessarily confusing and brittle.  E.g. in the unlikely
> event more CET stuff comes along, this lurking return could cause problems.
>
> Why not handle S_CET and U_CET in a single common path?  IMO, this is less error
> prone, and more clearly captures the relationship between S/U_CET, SHSTK, and IBT.
> Updating MSR intercepts is not a hot path, so the overhead of checking guest CPUID
> multiple times should be a non-issue.  And eventually KVM should effectively cache
> all of those lookups, i.e. the cost will be negilible.
>
> 	bool incpt;
>
> 	if (kvm_cpu_cap_has(X86_FEATURE_SHSTK)) {
> 		incpt = !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>
> 		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP,
> 					  MSR_TYPE_RW, incpt);
> 		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP,
> 					  MSR_TYPE_RW, incpt);
> 		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP,
> 					  MSR_TYPE_RW, incpt);
> 		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL3_SSP,
> 					  MSR_TYPE_RW, incpt);
> 		vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB,
> 					  MSR_TYPE_RW, incpt);
> 	}
>
> 	if (kvm_cpu_cap_has(X86_FEATURE_SHSTK) ||
> 	    kvm_cpu_cap_has(X86_FEATURE_IBT)) {
> 		incpt = !guest_cpuid_has(vcpu, X86_FEATURE_IBT) &&
> 			!guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>
> 		vmx_set_intercept_for_msr(vcpu, MSR_IA32_U_CET,
> 					  MSR_TYPE_RW, incpt);
> 		vmx_set_intercept_for_msr(vcpu, MSR_IA32_S_CET,
> 					  MSR_TYPE_RW, incpt);
> 	}

It looks fine to me, will apply it, thanks!