| Message ID | 20180913185448.106321-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Always reflect #NM VM-exits to L1 | expand |
> On 13 Sep 2018, at 21:54, Jim Mattson <jmattson@google.com> wrote: > > When bit 3 (corresponding to CR0.TS) of the VMCS12 cr0_guest_host_mask > field is clear, the VMCS12 guest_cr0 field does not necessarily hold > the current value of the L2 CR0.TS bit, so the code that checked for > L2's CR0.TS bit being set was incorrect. Moreover, I'm not sure that Surprised this wasn’t noticed when commit ccf9844e5d99 ("kvm, vmx: Really fix lazy FPU on nested guest”) was reviewed. Funny it didn’t even “Really” fixed lazy FPU on nested guest :P. Should always be careful with commit titles... > the CR0.TS check was adequate. (What if L2's CR0.EM was set, for > instance?) > > Fortunately, lazy FPU has gone away, so L0 has lost all interest in > intercepting #NM exceptions. See commit bd7e5b0899a4 ("KVM: x86: > remove code for lazy FPU handling"). Therefore, there is no longer any > question of which hypervisor gets first dibs. The #NM VM-exit should > always be reflected to L1. (Note that the corresponding bit must be > set in the VMCS12 exception_bitmap field for there to be an #NM > VM-exit at all.) > > Fixes: ccf9844e5d99c ("kvm, vmx: Really fix lazy FPU on nested guest”) > Reported-by: Abhiroop Dabral <adabral@paloaltonetworks.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > Tested-by: Abhiroop Dabral <adabral@paloaltonetworks.com> > --- > arch/x86/kvm/vmx.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 533a327372c8..49364b791eb9 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1610,11 +1610,6 @@ static inline bool is_page_fault(u32 intr_info) > return is_exception_n(intr_info, PF_VECTOR); > } > > -static inline bool is_no_device(u32 intr_info) > -{ > - return is_exception_n(intr_info, NM_VECTOR); > -} > - > static inline bool is_invalid_opcode(u32 intr_info) > { > return is_exception_n(intr_info, UD_VECTOR); > @@ -9639,9 +9634,6 @@ static bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason) > return false; > else if (is_page_fault(intr_info)) > return !vmx->vcpu.arch.apf.host_apf_reason && enable_ept; > - else if (is_no_device(intr_info) && > - !(vmcs12->guest_cr0 & X86_CR0_TS)) > - return false; > else if (is_debug(intr_info) && > vcpu->guest_debug & > (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)) > -- > 2.19.0.rc2.392.g5ba43deb5a-goog > Reviewed-by: Liran Alon <liran.alon@oracle.com> I think it will also be a good idea to add a regression kvm-unit-tests to vmx_tests.c as this should be fairly simple. -Liran
On Thu, Sep 13, 2018 at 3:53 PM, Liran Alon <liran.alon@oracle.com> wrote: > ... > Reviewed-by: Liran Alon <liran.alon@oracle.com> > > I think it will also be a good idea to add a regression kvm-unit-tests to vmx_tests.c > as this should be fairly simple. A kvm-unit-test has been submitted under separate cover ([kvm-unit-tests PATCH] x86: nvmx: Check #NM VM-exit reflection).
Ping. On Tue, Sep 25, 2018 at 1:34 PM, Jim Mattson <jmattson@google.com> wrote: > On Thu, Sep 13, 2018 at 3:53 PM, Liran Alon <liran.alon@oracle.com> wrote: >> ... >> Reviewed-by: Liran Alon <liran.alon@oracle.com> >> >> I think it will also be a good idea to add a regression kvm-unit-tests to vmx_tests.c >> as this should be fairly simple. > > A kvm-unit-test has been submitted under separate cover > ([kvm-unit-tests PATCH] x86: nvmx: Check #NM VM-exit reflection).
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 533a327372c8..49364b791eb9 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1610,11 +1610,6 @@ static inline bool is_page_fault(u32 intr_info) return is_exception_n(intr_info, PF_VECTOR); } -static inline bool is_no_device(u32 intr_info) -{ - return is_exception_n(intr_info, NM_VECTOR); -} - static inline bool is_invalid_opcode(u32 intr_info) { return is_exception_n(intr_info, UD_VECTOR); @@ -9639,9 +9634,6 @@ static bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason) return false; else if (is_page_fault(intr_info)) return !vmx->vcpu.arch.apf.host_apf_reason && enable_ept; - else if (is_no_device(intr_info) && - !(vmcs12->guest_cr0 & X86_CR0_TS)) - return false; else if (is_debug(intr_info) && vcpu->guest_debug & (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))
When bit 3 (corresponding to CR0.TS) of the VMCS12 cr0_guest_host_mask field is clear, the VMCS12 guest_cr0 field does not necessarily hold the current value of the L2 CR0.TS bit, so the code that checked for L2's CR0.TS bit being set was incorrect. Moreover, I'm not sure that the CR0.TS check was adequate. (What if L2's CR0.EM was set, for instance?) Fortunately, lazy FPU has gone away, so L0 has lost all interest in intercepting #NM exceptions. See commit bd7e5b0899a4 ("KVM: x86: remove code for lazy FPU handling"). Therefore, there is no longer any question of which hypervisor gets first dibs. The #NM VM-exit should always be reflected to L1. (Note that the corresponding bit must be set in the VMCS12 exception_bitmap field for there to be an #NM VM-exit at all.) Fixes: ccf9844e5d99c ("kvm, vmx: Really fix lazy FPU on nested guest") Reported-by: Abhiroop Dabral <adabral@paloaltonetworks.com> Signed-off-by: Jim Mattson <jmattson@google.com> Reviewed-by: Peter Shier <pshier@google.com> Tested-by: Abhiroop Dabral <adabral@paloaltonetworks.com> --- arch/x86/kvm/vmx.c | 8 -------- 1 file changed, 8 deletions(-)