Re: [PATCH v7 26/26] KVM: nVMX: Enable CET support for nested guest

["Yang, Weijiang" <weijiang.yang@intel.com>]

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?