| Message ID | 20180221022440.9861-2-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-02-20 at 21:24 -0500, Krish Sadhukhan wrote: > According to Intel SDM 26.2.1.1, the following rules should be enforced > on vmentry: > > * If the "NMI exiting" VM-execution control is 0, "Virtual NMIs" > VM-execution control must be 0. > * If the “virtual NMIs” VM-execution control is 0, the “NMI-window > exiting” VM-execution control must be 0. > > This patch enforces these rules when entering an L2 guest. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Liran Alon <liran.alon@oracle.com> > Reviewed-by: Reviewed-by: Jim Mattson <jmattson@google.com> s/^Reviewed-by: Reviewed-by:/Reviewed-by:/ > --- > arch/x86/kvm/vmx.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 8eba631..24b88db 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1366,6 +1366,16 @@ static inline bool nested_cpu_has_preemption_timer(struct vmcs12 *vmcs12) > PIN_BASED_VMX_PREEMPTION_TIMER; > } > > +static inline bool nested_cpu_has_nmi_exiting(struct vmcs12 *vmcs12) > +{ > + return vmcs12->pin_based_vm_exec_control & PIN_BASED_NMI_EXITING; > +} > + > +static inline bool nested_cpu_has_virtual_nmis(struct vmcs12 *vmcs12) > +{ > + return vmcs12->pin_based_vm_exec_control & PIN_BASED_VIRTUAL_NMIS; > +} > + > static inline int nested_cpu_has_ept(struct vmcs12 *vmcs12) > { > return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_EPT); > @@ -5667,8 +5677,7 @@ static bool nested_exit_intr_ack_set(struct kvm_vcpu *vcpu) > > static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu) > { > - return get_vmcs12(vcpu)->pin_based_vm_exec_control & > - PIN_BASED_NMI_EXITING; > + return nested_cpu_has_nmi_exiting(get_vmcs12(vcpu)); > } > > static void enable_irq_window(struct kvm_vcpu *vcpu) > @@ -10752,6 +10761,19 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, > return 0; > } > > +static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12) > +{ > + if (!nested_cpu_has_nmi_exiting(vmcs12) && > + nested_cpu_has_virtual_nmis(vmcs12)) > + return -EINVAL; > + > + if (!nested_cpu_has_virtual_nmis(vmcs12) && > + nested_cpu_has(vmcs12, CPU_BASED_VIRTUAL_NMI_PENDING)) > + return -EINVAL; > + > + return 0; > +} > + > static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -10796,6 +10818,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > vmx->nested.nested_vmx_entry_ctls_high)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > + if (nested_vmx_check_nmi_controls(vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > if (nested_cpu_has_vmfunc(vmcs12)) { > if (vmcs12->vm_function_control & > ~vmx->nested.nested_vmx_vmfunc_controls) On a related note, which VMentry checks do we decide to do in software and which ones we defer to hardware? The spec has like a dizillion checks that are enfored by hardware on VMEntry, which ones do we decide that it makes sense to validate in software before-hand? Regards. Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On Tue, Feb 20, 2018 at 9:51 PM, KarimAllah Ahmed <karahmed@amazon.de> wrote: > On a related note, which VMentry checks do we decide to do in software > and which ones we defer to hardware? Guest-state checks and some control field checks are currently deferred to hardware. Some checks simply aren't done at all (like the ones covered in this patch). > The spec has like a dizillion checks that are enfored by hardware on > VMEntry, which ones do we decide that it makes sense to validate in > software before-hand? We should probably defer only the guest-state checks to hardware. One problem with the current implementation is that we start loading L2 vCPU state before VM-entry to vmcs02. If VM-entry to vmcs02 fails early, with VMfailValid, the vCPU should still contain L1 state consistent with the VMLAUNCH or VMRESUME instruction and execution should just fall through to the next L1 instruction. At present, we have no way of backing out the L2 vCPU state that has already been loaded.
2018-02-21 09:20-0800, Jim Mattson: > On Tue, Feb 20, 2018 at 9:51 PM, KarimAllah Ahmed <karahmed@amazon.de> wrote: > > > On a related note, which VMentry checks do we decide to do in software > > and which ones we defer to hardware? > > Guest-state checks and some control field checks are currently > deferred to hardware. Some checks simply aren't done at all (like the > ones covered in this patch). > > > The spec has like a dizillion checks that are enfored by hardware on > > VMEntry, which ones do we decide that it makes sense to validate in > > software before-hand? > > We should probably defer only the guest-state checks to hardware. One > problem with the current implementation is that we start loading L2 > vCPU state before VM-entry to vmcs02. If VM-entry to vmcs02 fails > early, with VMfailValid, the vCPU should still contain L1 state > consistent with the VMLAUNCH or VMRESUME instruction and execution > should just fall through to the next L1 instruction. At present, we > have no way of backing out the L2 vCPU state that has already been > loaded. I agree, I'll look into that. Applied the patch and the test, thanks.
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 8eba631..24b88db 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1366,6 +1366,16 @@ static inline bool nested_cpu_has_preemption_timer(struct vmcs12 *vmcs12) PIN_BASED_VMX_PREEMPTION_TIMER; } +static inline bool nested_cpu_has_nmi_exiting(struct vmcs12 *vmcs12) +{ + return vmcs12->pin_based_vm_exec_control & PIN_BASED_NMI_EXITING; +} + +static inline bool nested_cpu_has_virtual_nmis(struct vmcs12 *vmcs12) +{ + return vmcs12->pin_based_vm_exec_control & PIN_BASED_VIRTUAL_NMIS; +} + static inline int nested_cpu_has_ept(struct vmcs12 *vmcs12) { return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_EPT); @@ -5667,8 +5677,7 @@ static bool nested_exit_intr_ack_set(struct kvm_vcpu *vcpu) static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu) { - return get_vmcs12(vcpu)->pin_based_vm_exec_control & - PIN_BASED_NMI_EXITING; + return nested_cpu_has_nmi_exiting(get_vmcs12(vcpu)); } static void enable_irq_window(struct kvm_vcpu *vcpu) @@ -10752,6 +10761,19 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, return 0; } +static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12) +{ + if (!nested_cpu_has_nmi_exiting(vmcs12) && + nested_cpu_has_virtual_nmis(vmcs12)) + return -EINVAL; + + if (!nested_cpu_has_virtual_nmis(vmcs12) && + nested_cpu_has(vmcs12, CPU_BASED_VIRTUAL_NMI_PENDING)) + return -EINVAL; + + return 0; +} + static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) { struct vcpu_vmx *vmx = to_vmx(vcpu); @@ -10796,6 +10818,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) vmx->nested.nested_vmx_entry_ctls_high)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_vmx_check_nmi_controls(vmcs12)) + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_cpu_has_vmfunc(vmcs12)) { if (vmcs12->vm_function_control & ~vmx->nested.nested_vmx_vmfunc_controls)
According to Intel SDM 26.2.1.1, the following rules should be enforced on vmentry: * If the "NMI exiting" VM-execution control is 0, "Virtual NMIs" VM-execution control must be 0. * If the “virtual NMIs” VM-execution control is 0, the “NMI-window exiting” VM-execution control must be 0. This patch enforces these rules when entering an L2 guest. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Reviewed-by: Liran Alon <liran.alon@oracle.com> Reviewed-by: Reviewed-by: Jim Mattson <jmattson@google.com> --- arch/x86/kvm/vmx.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)