Received-SPF: pass (google.com: domain of linux-kernel+bounces-42269-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 29 Jan 2024 15:04:39 +0800
From: Chao Gao <chao.gao@intel.com>
To: Yang Weijiang <weijiang.yang@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>
Subject: Re: [PATCH v9 26/27] KVM: nVMX: Enable CET support for nested guest
Message-ID: <ZbdOB5YWX8CGsEHC@chao-email>
References: <20240124024200.102792-1-weijiang.yang@intel.com>
 <20240124024200.102792-27-weijiang.yang@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240124024200.102792-27-weijiang.yang@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8ea4XY66LcznVACxBfD3Z0A5OlNEtlaRZ5frrkp/7GKTCo+UFdZtHQbDC1ue?=
 =?us-ascii?Q?DXYIiV5eybjtPmvD/An+jPLavwn11ZtA0Nyd3+JgjYykbggolgul3ttbN0vi?=
 =?us-ascii?Q?V5R4dRrP32vblR/n531UFmNlnreUAJlzCFPbW7zokda8EDBLYnMnNK7yu1Qc?=
 =?us-ascii?Q?Y5PqaRk2wRHdhUL9QwVIJLtbNlZVCNQz6JBzYQqJ/fjlqk3GOj9bqYKzIln4?=
 =?us-ascii?Q?NqJtej4xQGuUpGlAU3o5HLkLnakC3wsYdimhQdghTh4RjV17eQbBKBotMLn7?=
 =?us-ascii?Q?QQCVJTQMRsjBtzIe2ORM+qESbWTWcHZBYEQe3IW7dbxJehFWeAbJ8ApZFF/S?=
 =?us-ascii?Q?KpeMDuFUoIn9Qt/mlAtxu40rmvSRlTD92udAKRm1z2PEV0IsvvNHzBEU+aUo?=
 =?us-ascii?Q?i8kaCmIupu4W22NrR11NrMYC/2lDPdeSGn4O2MwWkaTTTPRVSKiN1FVpW+Q2?=
 =?us-ascii?Q?/oYqO/8y13dkaqZMbcgYy5A9XsEMDSzQKFxszNA5EsJeg4HHNgPINKRKWemZ?=
 =?us-ascii?Q?n+ye5I4PuMDz5kaYuPbSX0j/9rAmGdmhd9SDjAa74f6hhESEXleEsGIGK+yr?=
 =?us-ascii?Q?xpb3bXGAD2fruR6UXnvB4zJIl3Anpu5i3jbRVQ/5trjKSMeF4szq+ka1dqzJ?=
 =?us-ascii?Q?KrBZljAJcShLR7QminiMAn12UnRkeQlS7t9pRmw7mtDGvffDk2Dt/vdcrBB/?=
 =?us-ascii?Q?S77C5yvwC08LY1sbxVnGbXVLaWxLNGbkfJUmaBheHJ1Qx9j7QdWUrUUuj0He?=
 =?us-ascii?Q?ynzbDoP3cgVJpdyIThEubFuFwMNQ383drrhyfgFs8WoRX98/kR1jWg9AUwCC?=
 =?us-ascii?Q?jGbzpMFWW5A529k6Imcnq4ygK5UhJECYSiRetJkuMewk62wwAzsD1ulJuKhX?=
 =?us-ascii?Q?jrCZkDQ6IDtINQw4IGJ14dhbnpdm7PcJ/04A+Bp2UTWDHNifoxHQQa3QAgys?=
 =?us-ascii?Q?tyM9J9f+92u3rbuTHPQZHkSGory9YPg+powB5sf4G5QyinPsaVWmJcA/CqIw?=
 =?us-ascii?Q?fctqWqsXyOHg4aDvWetWRjS8VKzRC6CuJIq0WfVMzA1EB4i/icfYKzigl7+V?=
 =?us-ascii?Q?F6Q7lhrMeJzOaDUBI0aS4IQe3DcslnizDhVWbuXhDcooLa+5i0JBm9oci3st?=
 =?us-ascii?Q?l3H1PreVg5gG867UrvUNd042hTxXlZfLwwlGL/6GWurxQqO7pr3IsXq6+Zj4?=
 =?us-ascii?Q?ZytpVi3wGlEo4hR4nPxkImyktNysURWHCLDAAzGP875EJkTZuUpigvLPxe+k?=
 =?us-ascii?Q?eb3peeXejFtI0RzZjKI+lbc2y0RBTUeA9h58cYTgDP27q9ifF8tPuDUGTdqS?=
 =?us-ascii?Q?t3fMN8tPm042tr7mwXYH0ksXpxe2ZTrBYz6H5+DYFHzGJ5Drw2gyPuzILLx1?=
 =?us-ascii?Q?j//bp+ByaCCaP6MJP6BjD/U+UL4ORqCiIHObRUlZOftXFb7oUe1lvSABMOW0?=
 =?us-ascii?Q?DS8WefKyrdesqiy91pXDsN16cnpn0SGvjX8SkOReB9cHmfS7WMwdup4LXjU+?=
 =?us-ascii?Q?MmZ9uQ4Yd5d85lwVMYBMv6iXZ0Adzzjqlsuc9+bmEIZD/t6TAdyK65riqiA9?=
 =?us-ascii?Q?79msLl+McrhUTmILhTIqas14NFZbRRNMxd0pty6K?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4ecc2aad-d4a2-43be-5e4a-08dc2098951b
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 07:04:48.9607
 (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: jKLmXfTWffkJPJTdh3KMxS7Qel2gO4vxFEoKoHvYHExk6/hOR2vebqpT7gzomlM5Th3XN2HZyIDxfcRIRTCKHw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5200
X-OriginatorOrg: intel.com

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)) {

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.

> 	}
> 
> 	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.