Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <73119078-7483-42e0-bb1f-b696932b6cd2@intel.com>
Date:   Wed, 6 Dec 2023 17:22:29 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 26/26] KVM: nVMX: Enable CET support for nested guest
Content-Language: en-US
To:     Maxim Levitsky <mlevitsk@redhat.com>
CC:     <seanjc@google.com>, <pbonzini@redhat.com>,
        <dave.hansen@intel.com>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <peterz@infradead.org>,
        <chao.gao@intel.com>, <rick.p.edgecombe@intel.com>,
        <john.allen@amd.com>
References: <20231124055330.138870-1-weijiang.yang@intel.com>
 <20231124055330.138870-27-weijiang.yang@intel.com>
 <2e280f545e8b15500fc4a2a77f6000a51f6f8bbd.camel@redhat.com>
 <e7d399a2-a4ff-4e27-af09-a8611985648a@intel.com>
 <8a2216b0c1a945e33a18a981cbce7737a07de52d.camel@redhat.com>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <8a2216b0c1a945e33a18a981cbce7737a07de52d.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bjR1d1c0b1dFanBPN2JuQ1FFcEdWRmF5Ym9nQ1JHc1dNaC9yS0dBaXF3Vktu?=
 =?utf-8?B?Q3JXYzMrWkc3NndldTFQc3R3VnVFNjEvQ0pncHZ4dEZ3L2p5aTJLNXRtaTM0?=
 =?utf-8?B?QTlMMjlpbmQ0OURoRFEwU1pZU2sycjZVOFRvR05EN2Y1alp1dWc5UWJ5UXNm?=
 =?utf-8?B?NERHWldnaXpTeUVHR2hkSGcxNi9ZWVlwbmlCSkhpQnpOcVdENzFJWlRaWHVW?=
 =?utf-8?B?dU40eWRmUkVVWGVCSnM4bGozR09EQ3JvNXdRZzRjMzN1U09DR1ExRE1lamJW?=
 =?utf-8?B?emZYTVl6R05WZGwrZmtOdDZsK29QU3duL3lyVDU3TFR4SFhRYUxWdDIvazVi?=
 =?utf-8?B?YUsxcEQ0RUI1NFgybEY1UzZpNC9XNFJGK2lxcnBKaTBSWFJUZ3ExdG13MGlx?=
 =?utf-8?B?TlVCajh0NXNmUkJSQkVBRE01L2kvTWhzV3VZbFZGRGpsRzVWOUV4aDF0d0Nl?=
 =?utf-8?B?TVI3Z0Ztemt6UjVCNm9PZkR2dEJldVlUNmxySXhrQk1acWRoRmp5VVQ5UzdN?=
 =?utf-8?B?aEdZaWw4SytBRmhYamRMV3poUVlqWUlHbXFMZWtodmt4OWl0bnJiQlhQN0w4?=
 =?utf-8?B?SzgzQUFzUTlTUjFFN1dJUllmRXRrRjY4ZnlsZlhVeXBGVEJoTDVLSUNLOEJ0?=
 =?utf-8?B?VURPN01Ga1V1YXJmY25UUzYwdzFpNkdMU1M0bDNXK2VrZUt3Wjk0UFc5ZGFC?=
 =?utf-8?B?VU9td2lFcTlxVzJWSXdRZGNycGlJT2tuOXdMa3dDanQ0Vk5Rd3ZhNG9yeHVK?=
 =?utf-8?B?R2JrVDhRTVBMS3NHUTlUZkdybmJlOHZncGhHZVRqWmRXQzhxYUVNamh0eUE2?=
 =?utf-8?B?Uk4rWTQxdWhuSnBIend3cDdETmV0NUJrRmhMNUw2MUJIOEVxUERManZaazI5?=
 =?utf-8?B?NVh6V2daZXUrWVo3SEpoSkZES2hhNTNPYnVINmc4dXBrZ1ZHdjJaMWxjZUhM?=
 =?utf-8?B?elQxRHE3c2R2eVJ2b2MxTzN3TTVTV1pLemsrN202TC9sQ3RDckp2aEdMSkdw?=
 =?utf-8?B?TUZxQnl6RGJzZlBQM0xsb2xyWlZLRjU5Z3d2MHFObnNiK3FMbWxOY2VncUc4?=
 =?utf-8?B?dnl2TUhBK0JLRk5Rd0pwaXVGbENtSUJ5ZS9KM1JlL1JVWmh2V0J5eVpuclll?=
 =?utf-8?B?a2VyRXpEb3RvYUt6ZE4rd2RJY0dUdkRlYzJwNnc5Z2diYVhLajVHbytyWUpv?=
 =?utf-8?B?K3J0WXp2OXYrQ1BRMjdBdmhGVEVjSVJiZ1NseTlVMTQrTGVzR3dsN051cGow?=
 =?utf-8?B?YmwzcTgvMXlGbjhQeEQ4OHZNWEtPOHpOWUg5b2M0bFB0VE5OUzVLN2VrQWtk?=
 =?utf-8?B?eXpUVEdsY3cvaFNFVk4rNzdnQ1Z0Y0tKVmNtY2tBVFRNK2Y3VnhFT2x1cVBy?=
 =?utf-8?B?MlQ4dTZxcnVNcnhPU1hQTGxhQkROYVJGZytuNS9yMUdhRGlCTWt3enFjQzdN?=
 =?utf-8?B?Qzh0SUN0SzJVSkZML0NVU0t6ZkxqbEN3VFNJcnpNUEVUQzNMYzhwZUQyUWJt?=
 =?utf-8?B?ZFVjdVlTS0RGT0VoeXVXZDdBWEgrRTdmRTVDemIvdWM1Q0xZQWE3bThNOXB2?=
 =?utf-8?B?czRsVWEyRk81dzE4QjBpNkZtVy9qaXhYNnRIVjdmelpDZVlSMllkTlZwSGFR?=
 =?utf-8?B?T2dpTzdrZTExNzBGeUhoQVdzNm5LUEdYaGY1S1N2SVJvcDZOdThxSVRwbjZU?=
 =?utf-8?B?Wmg0QS8za2E3aUxyOW9FTnFYY2U0bkJORFcrUEhSM3FuQ2xJUi9SU2EwRm5w?=
 =?utf-8?B?OVZJaU1DMUZZQ3lHTTdHWUh0eWd5K3hlMlluWU9qK0R4bGlGTFcvSXVReFJW?=
 =?utf-8?B?ZmN2MmhWUm03OXZYMFdKaEhidkljTWxYdTdBcWFQWk50YjA4UnUxNWdFUEhy?=
 =?utf-8?B?bTkzOFlLSXVWOU82K3ovT2RkbnF3bTQyOWhJc0VHbFRzbnJwY1NQK05DSjJo?=
 =?utf-8?B?WjNIZzRMK2VjeE1KL1UxTWlUaEtjOVZVRi9RbVdEdkZueWcwbFJ0bXFPT2FM?=
 =?utf-8?B?OVM3UjA2djdPUjFWKzlIL1FxTXF5YXlsOCswcnA2RkxENVdGb2JZbG12emlF?=
 =?utf-8?B?MWt2b3lWOU9vTjRyVi9TN2taR3IvYWZ5RkV2MTZQNVhKdy9oVng1RGRJRVN2?=
 =?utf-8?B?QWVjcWhOMnJYU0MrRVF0SG85dlltNTViRXZ0R1hBY3M3cEZQRHVUMENKaXNR?=
 =?utf-8?B?ZGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 63344200-ea4a-4f55-6acc-08dbf63ce4df
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2023 09:22:40.4964
 (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: EOyCTUIHfOFH693Jl1b3DGGY79NvtLTBOdvS0NYLS3bVqbDkC+0WeqCu7afTqjkiXGdLOp4Fv8ySOtGMcRs7Eg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7522
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 06 Dec 2023 01:22:57 -0800 (PST)

On 12/5/2023 6:12 PM, Maxim Levitsky wrote:
> On Mon, 2023-12-04 at 16:50 +0800, Yang, Weijiang wrote:

[...]

>>>>    	vmx->nested.force_msr_bitmap_recalc = false;
>>>> @@ -2469,6 +2491,18 @@ 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 (vmx->nested.nested_run_pending &&
>>> I don't think that nested.nested_run_pending check is needed.
>>> prepare_vmcs02_rare is not going to be called unless the nested run is pending.
>> But there're other paths along to call prepare_vmcs02_rare(), e.g., vmx_set_nested_state()-> nested_vmx_enter_non_root_mode()-> prepare_vmcs02_rare(), especially when L1 instead of L2 was running. In this case, nested.nested_run_pending == false,
>> we don't need to update vmcs02's fields at the point until L2 is being resumed.
> - If we restore VM from migration stream when L2 is *not running*, then prepare_vmcs02_rare won't be called,
> because nested_vmx_enter_non_root_mode will not be called, because in turn there is no nested vmcs to load.
>
> - If we restore VM from migration stream when L2 is *about to run* (KVM emulated the VMRESUME/VMLAUNCH,
> but we didn't do the actual hardware VMLAUNCH/VMRESUME on vmcs02, then the 'nested_run_pending' will be true, it will be restored
> from the migration stream.
>
> - If we migrate while nested guest was run once but didn't VMEXIT to L1 yet, then yes, nested.nested_run_pending will be false indeed,
> but we still need to setup vmcs02, otherwise it will be left with default zero values.

Thanks a lot for recapping these cases! I overlooked some nested flags before. It makes sense to remove nested.nested_run_pending.
> Remember that prior to setting nested state the VM wasn't running even once usually, unlike when the guest enters nested state normally.
>
>>>> +		    (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);
>>>> +		}
>>>>    	}
>>>>    
>>>>    	if (nested_cpu_has_xsaves(vmcs12))
>>>> @@ -4300,6 +4334,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);
>>>> +	}
>>> The above code should be conditional on VM_ENTRY_LOAD_CET_STATE - if the guest (L2) state
>>> was loaded, then it must be updated on exit - this is usually how VMX works.
>> I think this is not for L2 VM_ENTRY_LOAD_CET_STATE, it happens in prepare_vmcs02_rare(). IIUC, the guest registers will be saved into VMCS fields unconditionally when vm-exit happens,
>> so these fields for L2 guest should be synced to L1 unconditionally.
> "the guest registers will be saved into VMCS fields unconditionally"
> This is not true, unless there is a bug.

I checked the latest SDM, there's no such kind of wording regarding CET entry/exit control bits. The wording comes from
the individual CET spec.:
"10.6 VM Exit
On processors that support CET, the VM exit saves the state of IA32_S_CET, SSP and IA32_INTERRUPT_SSP_TABLE_ADDR MSR to the VMCS guest-state area unconditionally."
But since it doesn't appear in SDM, I shouldn't take it for granted.

> the vmcs12 VM_ENTRY_LOAD_CET_STATE should be passed through as is to vmcs02, so if the nested guest doesn't set this bit
> the entry/exit using vmcs02 will not touch the CET state, which is unusual but allowed by the spec I think - a nested hypervisor can opt for example to save/load
> this state manually or use msr load/store lists instead.

Right although the use case should be rare, will modify the code to check VM_ENTRY_LOAD_CET_STATE. Thanks!
> Regardless of this,
> if the guest didn't set VM_ENTRY_LOAD_CET_STATE, then vmcs12 guest fields should neither be loaded on VM entry (copied to vmcs02) nor updated on VM exit,
> (that is copied back to vmcs12) this is what is written in the VMX spec.

What's the VMX spec. your're referring to here?