Received-SPF: pass (google.com: domain of linux-kernel+bounces-44057-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <073cc3da-63ef-4913-9c20-f4fa090751cd@intel.com>
Date: Tue, 30 Jan 2024 15:38:36 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v9 26/27] KVM: nVMX: Enable CET support for nested guest
To: Chao Gao <chao.gao@intel.com>
CC: <seanjc@google.com>, <pbonzini@redhat.com>, <dave.hansen@intel.com>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
	<yuan.yao@linux.intel.com>, <peterz@infradead.org>,
	<rick.p.edgecombe@intel.com>, <mlevitsk@redhat.com>, <john.allen@amd.com>
References: <20240124024200.102792-1-weijiang.yang@intel.com>
 <20240124024200.102792-27-weijiang.yang@intel.com>
 <ZbdOB5YWX8CGsEHC@chao-email>
Content-Language: en-US
From: "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZbdOB5YWX8CGsEHC@chao-email>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ajM4Z092end2YkNXcXU1dUs1RENZVnRHaDVvUDMyTFVsb1h2Q2JzNGR4VVpY?=
 =?utf-8?B?eWZQaUlhK09scGtib0VHbi81UWRGK2ZoalVZelVsd0dPanNOR2RUWDFNeGVh?=
 =?utf-8?B?K2puSjlnTmtzZHkrbDJGMjFleEZXcXVDbHF2Sm9RbXRCUGlUaURRVWtBN0xL?=
 =?utf-8?B?SHloZlcwOVAza21CN1I4TkpxcDZRYUFjQnRWaXVCOGpNZUVxR2dJUmhFQ3ZJ?=
 =?utf-8?B?aGJTWFo0eTFDSW0wN2YyWDIyUkxOYjhCbzlISkQya0M1NGVkVnJRenFHcVZ2?=
 =?utf-8?B?dHl4OVBkWE1KVjNsZ3lGUE9idzU4V3NFZTJqTy9Wc2Q5UjltdWhleXVReVhx?=
 =?utf-8?B?WmdWQlIzTlU1U3hlUEg1Y0JycG5JQ0tVcGpZRWdYUWp6endqY2c5alM4ODhC?=
 =?utf-8?B?OFZqS2ZyRGdvaTdUVklFWVE4UERRbGZyKzFJUDc2V1ZxQ2xJZG1qeDIwcDRT?=
 =?utf-8?B?cGEwc25xblgxaWRkbUZzTVA2QU80a1ZJTzBsYmRHcmVYN011WXV2VzhzTDBL?=
 =?utf-8?B?bFNuc0lyWFNVMVgwOEI3K0hJTmdKR3hBc0o0QUhHYjNEQ2pLT0RBcUxJUTRE?=
 =?utf-8?B?TGtoOUt3Z3FEOXdqRGtFUVRpaWFtVmlwMy9SK1JZemlrekxrZ29IZVpZdkZ1?=
 =?utf-8?B?MmNzSzEybDBYQUtmdkxmQVZNa2FQK1pRbDZsb3BSNjdJSXBWTjZXRVdKMXFC?=
 =?utf-8?B?dnZHbGh2ZmNndnlvVjZzL3BZSnB6Z0dFUmVKOVFuclhiSjVxa09GQW1MdW9Z?=
 =?utf-8?B?bFF3bVN5UFlaYTlLWGpaTFgzNWpkclpQRWEyTi9MUngxSXRkS3IxMlRmUG04?=
 =?utf-8?B?djB6S2JydkU4a1Rxc2JZNGFIL3hpQW1YaEsxSlQ3N3ZwV0cyeEdtVllNZndp?=
 =?utf-8?B?NkhJZ3U1S2xxN29EZHBNMFIvWU9WYnZIOTRrR2FhTEtXbXQ5UjlTL29xY0p6?=
 =?utf-8?B?WEFoZzJvUmRhOElSN2dLSlZRZUI0eTBqWWxMVjNrWTNoNDdOMjF3eEV6L0JD?=
 =?utf-8?B?RzdxTXA4ZkhJVGtMNHFlOE43V01HSkhEUWtBUjNZdkc2TGNrdzdTNnNzQldy?=
 =?utf-8?B?WFpzU2NnbG9lZk9vRVU4MHRuS1pTTWZKQUEwSjlpUG1nNDV4NUFkT2VhTHBv?=
 =?utf-8?B?NXJSTXZlLzVLL1NaR1ZReHFkZDk0dDlBZlRKK0FIZVQvaVorT2ZtQkIrUkx2?=
 =?utf-8?B?VGU5OU9Dckl2aXdaMzZBRDcybmM1OTI4ZlQwdkFIVy9mNHRMUC94NlBpL0k5?=
 =?utf-8?B?eUtsQnFHYktINDlVRGRrcE93VGxRaFhmMjY3VEdwNjI3YzJQcUFVM3Yxa1pt?=
 =?utf-8?B?NUk0L2laaTZkZkpPZUw3TGg2Qm5OWFhjTzBrYitFVW5SK0ZieWhmd2NtSHBx?=
 =?utf-8?B?UGhZaG9jQ2xidEhteE5aRDQyYTYxTFNzbzFINHdSMWN5cHp5MXd6OG5qVzRa?=
 =?utf-8?B?SmYxZG05ckx2U0tvNTVQeXhOdFJNQVo3QjVYNmZwaktjYkVtNkRjc2NLSk5B?=
 =?utf-8?B?dzNBYWdDVmR4cTNDQ0hwKzVmdzdqN0d4VG45VmV0TXp1aWdjS2JIcWhZZGtT?=
 =?utf-8?B?VisyN045MnV4b21VOVlSVGlZKytWRDd4c3ZMVmxCT3lpdGJ6SGFMZllha1dt?=
 =?utf-8?B?TEw3YTJFYnVMSHpQbUdvSlhLNThnNUg3TTllNDRzTWVoTFdmdGdhbEFSUDM1?=
 =?utf-8?B?NVdqbEtsNWx1KzcyK0lyc1FRV28wQ09MZXVlOGMzZmpTQUlkeUo3a0xNV0R6?=
 =?utf-8?B?WEx1c0tNUit1eUNzdE9mRGw2YXVjbVRSNDdvWGpMdERER0JrWXBnM3p0dFps?=
 =?utf-8?B?TXFXMXlSUjB3Uis1NVg1VGg1by9EMk42RmI4R01MbFk2b1JJbGovaGQyNGV0?=
 =?utf-8?B?WFJFSlZFdGNTSE1xaGdvU3NlcVEzSXNtc1BKMjZCMW5MaldiK2I5MTB3bWxa?=
 =?utf-8?B?d1hnRXN1RVpwK1M5cEhoMjRPNVk3enovU2tNREJhUUtFV1QyclFVankrMmxH?=
 =?utf-8?B?ZElrdHdzNkwvZzBveWxFQ0k1eVdFQTlrU0czSnFHQmttWnBsbkRZdFA0OEND?=
 =?utf-8?B?aXdhOVJsSG1kQmlxNGJyU21DOXJBc3dZU3hBT2JONnNxbndlRkZ5ZllLUExC?=
 =?utf-8?B?U291T09GK21Da25FVVNpUEZOb0luSi9Xd0ozekFibnVxdElYWlI0a3RNSjlN?=
 =?utf-8?B?R2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 728c258e-b963-4376-7233-08dc21667ff7
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jan 2024 07:38:50.0266
 (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: +s3sz6E1cNvb651If1kl2NAillTEaH3Rq2UZfwHmQubP6CUtc6oWPU/C4kYOIQ4q6HkKgv2QkuHueosesIvdhg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6269
X-OriginatorOrg: intel.com

On 1/29/2024 3:04 PM, Chao Gao wrote:
> On Tue, Jan 23, 2024 at 06:41:59PM -0800, Yang Weijiang wrote:
>> Set up CET MSRs, related VM_ENTRY/EXIT control bits and fixed CR4 setting
>> to enable CET for nested VM.
>>
>> vmcs12 and vmcs02 needs to be synced when L2 exits to L1 or when L1 wants
>> to resume L2, that way correct CET states can be observed by one another.
>>
>> Suggested-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>> ---
>> arch/x86/kvm/vmx/nested.c | 57 +++++++++++++++++++++++++++++++++++++--
>> arch/x86/kvm/vmx/vmcs12.c |  6 +++++
>> arch/x86/kvm/vmx/vmcs12.h | 14 +++++++++-
>> arch/x86/kvm/vmx/vmx.c    |  2 ++
>> 4 files changed, 76 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index 468a7cf75035..e330897a7e5e 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -691,6 +691,28 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
>> 	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
>> 					 MSR_IA32_FLUSH_CMD, MSR_TYPE_W);
>>
>> +	/* Pass CET MSRs to nested VM if L0 and L1 are set to pass-through. */
>> +	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
>> +					 MSR_IA32_U_CET, MSR_TYPE_RW);
>> +
>> +	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
>> +					 MSR_IA32_S_CET, MSR_TYPE_RW);
>> +
>> +	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
>> +					 MSR_IA32_PL0_SSP, MSR_TYPE_RW);
>> +
>> +	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
>> +					 MSR_IA32_PL1_SSP, MSR_TYPE_RW);
>> +
>> +	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
>> +					 MSR_IA32_PL2_SSP, MSR_TYPE_RW);
>> +
>> +	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
>> +					 MSR_IA32_PL3_SSP, MSR_TYPE_RW);
>> +
>> +	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
>> +					 MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW);
>> +
>> 	kvm_vcpu_unmap(vcpu, &vmx->nested.msr_bitmap_map, false);
>>
>> 	vmx->nested.force_msr_bitmap_recalc = false;
>> @@ -2506,6 +2528,17 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
>> 		if (kvm_mpx_supported() && vmx->nested.nested_run_pending &&
>> 		    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS))
>> 			vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs);
>> +
>> +		if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE) {
>> +			if (guest_can_use(&vmx->vcpu, X86_FEATURE_SHSTK)) {
>> +				vmcs_writel(GUEST_SSP, vmcs12->guest_ssp);
>> +				vmcs_writel(GUEST_INTR_SSP_TABLE,
>> +					    vmcs12->guest_ssp_tbl);
>> +			}
>> +			if (guest_can_use(&vmx->vcpu, X86_FEATURE_SHSTK) ||
>> +			    guest_can_use(&vmx->vcpu, X86_FEATURE_IBT))
>> +				vmcs_writel(GUEST_S_CET, vmcs12->guest_s_cet);
>> +		}
> I think you need to move this hunk outside the outmost if-statement, i.e.,
>
> 	if (!hv_evmcs || !(hv_evmcs->hv_clean_fields &
> 			   HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP1)) {

Yes, I should move them to the  end of the function, will do it, thanks!

>
> otherwise, the whole block may be skipped (e.g., when evmcs is enabled and
> GUEST_GRP1 is clean), leaving CET state not context-switched.
>
> And if VM_ENTRY_LOAD_CET_STATE of vmcs12 is cleared, L1's values should be
> propagated to vmcs02 on nested VMenter; see pre_vmenter_debugctl in struct
> nested_vmx. I believe we need similar handling for the three CET fields.

The code used to be there, but I thought it's not the valid usage model, so removed them.
After a second thought, I think I shouldn't assume L1's CET usage, just check the flags and
sync CET states between L2<-->L1. Will add the handling, thanks!

>
>> 	}
>>
>> 	if (nested_cpu_has_xsaves(vmcs12))
>> @@ -4344,6 +4377,15 @@ static void sync_vmcs02_to_vmcs12_rare(struct kvm_vcpu *vcpu,
>> 	vmcs12->guest_pending_dbg_exceptions =
>> 		vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS);
>>
>> +	if (guest_can_use(&vmx->vcpu, X86_FEATURE_SHSTK)) {
>> +		vmcs12->guest_ssp = vmcs_readl(GUEST_SSP);
>> +		vmcs12->guest_ssp_tbl = vmcs_readl(GUEST_INTR_SSP_TABLE);
>> +	}
>> +	if (guest_can_use(&vmx->vcpu, X86_FEATURE_SHSTK) ||
>> +	    guest_can_use(&vmx->vcpu, X86_FEATURE_IBT)) {
>> +		vmcs12->guest_s_cet = vmcs_readl(GUEST_S_CET);
>> +	}
> unnecessary braces.

Will remove it.