Messages in this thread | | | Date | Tue, 12 Dec 2023 16:56:13 +0800 | Subject | Re: [PATCH v7 26/26] KVM: nVMX: Enable CET support for nested guest | From | "Yang, Weijiang" <> |
| |
On 12/8/2023 11:22 PM, Maxim Levitsky wrote: > On Fri, 2023-12-08 at 23:15 +0800, Yang, Weijiang wrote: >> On 12/7/2023 1:24 AM, Maxim Levitsky wrote: >>> On Wed, 2023-12-06 at 17:22 +0800, Yang, Weijiang wrote: >>>> 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. >>> SDM spec from September 2023: >>> >>> 28.3.1 Saving Control Registers, Debug Registers, and MSRs >>> >>> "If the processor supports the 1-setting of the “load CET” VM-entry control, the contents of the IA32_S_CET and >>> IA32_INTERRUPT_SSP_TABLE_ADDR MSRs are saved into the corresponding fields. On processors that do not >>> support Intel 64 architecture, bits 63:32 of these MSRs are not saved." >>> >>> Honestly it's not 100% clear if the “load CET” should be set to 1 to trigger the restore, or that this control just needs to be >>> supported on the CPU. >>> It does feel like you are right here, that CPU always saves the guest state, but allows to not load it on VM entry via >>> “load CET” VM entry control. >>> >>> IMHO its best to check what the bare metal does by rigging a test by patching the host kernel to not set the 'load CET' control, >>> and see if the CPU still updates the guest CET fields on the VM exit. >> OK, I'll do some tests to see what's happening, thanks! >>>>> 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? >>> SDM. >>> >>> In fact, now that I am thinking about this again, it should be OK to unconditionally copy the CET fields from vmcs12 to vmcs02, because as long as the >>> VM_ENTRY_LOAD_CET_STATE is not set, the CPU should care about their values in the vmcs02. > I noticed a typo. I meant that the CPU should't care about their values in the vmcs02. > >>> And about the other way around, assuming that I made a mistake as I said above, then the other way around is indeed unconditional. >>> >>> >>> Sorry for a bit of a confusion. >> NP, I also double check it with HW Arch and get it back. >> Thanks for raising these questions!
I got reply from HW Arch, the guest CET state is saved unconditionally:
"On the state save side, uCode doesn’t check for an exit control (or the load CET VM-entry control), but rather since it supports (as of TGL/SPR) CET, it unconditionally saves the state to the VMCS guest-state area. "
> Thanks to you too! > > > Best regards, > Maxim Levitsky > >>> Best regards, >>> Maxim Levitsky >>> >>> >
| |