| Message ID | 20210830125539.1768833-7-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: few more SMM fixes | expand |
On Mon, Aug 30, 2021, Maxim Levitsky wrote: > If L1 had invalid state on VM entry (can happen on SMM transactions > when we enter from real mode, straight to nested guest), > > then after we load 'host' state from VMCS12, the state has to become > valid again, but since we load the segment registers with > __vmx_set_segment we weren't always updating emulation_required. Because I'm an idiot. > Update emulation_required explicitly at end of load_vmcs12_host_state. Can you also add Reported-by: kernel test robot <oliver.sang@intel.com> which is how I ended up here, sort of. Fixes: 816be9e9be8d ("KVM: nVMX: Don't evaluate "emulation required" on nested VM-Exit") > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- The below is overkill. The state after VM-Exit _can't_ be invalid, which was the whole point of moving to __vmx_set_segment(), I just forgot the minor detail of clearing emulation_required. So just: diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index bc6327950657..55ac7211fb37 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4314,6 +4314,12 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, if (nested_vmx_load_msr(vcpu, vmcs12->vm_exit_msr_load_addr, vmcs12->vm_exit_msr_load_count)) nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_MSR_FAIL); + + /* + * All relevant vmcs12 host state is checked prior to VM-Entry, thus + * L1 guest can never be invalid after VM-Exit. + */ + to_vmx(vcpu)->emulation_required = false; } static inline u64 nested_vmx_get_vmcs01_guest_efer(struct vcpu_vmx *vmx) > arch/x86/kvm/vmx/nested.c | 2 ++ > arch/x86/kvm/vmx/vmx.c | 8 ++++---- > arch/x86/kvm/vmx/vmx.h | 1 + > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 1a05ae83dae5..f915e1ac589c 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4319,6 +4319,8 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, > if (nested_vmx_load_msr(vcpu, vmcs12->vm_exit_msr_load_addr, > vmcs12->vm_exit_msr_load_count)) > nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_MSR_FAIL); > + > + to_vmx(vcpu)->emulation_required = vmx_emulation_required(vcpu); > } > > static inline u64 nested_vmx_get_vmcs01_guest_efer(struct vcpu_vmx *vmx) > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 02d061f5956a..7ff1e1daeb0a 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1323,7 +1323,7 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu) > vmx_prepare_switch_to_host(to_vmx(vcpu)); > } > > -static bool emulation_required(struct kvm_vcpu *vcpu) > +bool vmx_emulation_required(struct kvm_vcpu *vcpu) > { > return emulate_invalid_guest_state && !vmx_guest_state_valid(vcpu); > } > @@ -1367,7 +1367,7 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) > vmcs_writel(GUEST_RFLAGS, rflags); > > if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) > - vmx->emulation_required = emulation_required(vcpu); > + vmx->emulation_required = vmx_emulation_required(vcpu); > } > > u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu) > @@ -3077,7 +3077,7 @@ void vmx_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0) > } > > /* depends on vcpu->arch.cr0 to be set to a new value */ > - vmx->emulation_required = emulation_required(vcpu); > + vmx->emulation_required = vmx_emulation_required(vcpu); > } > > static int vmx_get_max_tdp_level(void) > @@ -3330,7 +3330,7 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int > { > __vmx_set_segment(vcpu, var, seg); > > - to_vmx(vcpu)->emulation_required = emulation_required(vcpu); > + to_vmx(vcpu)->emulation_required = vmx_emulation_required(vcpu); > } > > static void vmx_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l) > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 4858c5fd95f2..3a587c51a8d1 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -359,6 +359,7 @@ void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu); > void vmx_set_host_fs_gs(struct vmcs_host_state *host, u16 fs_sel, u16 gs_sel, > unsigned long fs_base, unsigned long gs_base); > int vmx_get_cpl(struct kvm_vcpu *vcpu); > +bool vmx_emulation_required(struct kvm_vcpu *vcpu); > unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu); > void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags); > u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu); > -- > 2.26.3 >
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 1a05ae83dae5..f915e1ac589c 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4319,6 +4319,8 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, if (nested_vmx_load_msr(vcpu, vmcs12->vm_exit_msr_load_addr, vmcs12->vm_exit_msr_load_count)) nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_MSR_FAIL); + + to_vmx(vcpu)->emulation_required = vmx_emulation_required(vcpu); } static inline u64 nested_vmx_get_vmcs01_guest_efer(struct vcpu_vmx *vmx) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 02d061f5956a..7ff1e1daeb0a 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1323,7 +1323,7 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu) vmx_prepare_switch_to_host(to_vmx(vcpu)); } -static bool emulation_required(struct kvm_vcpu *vcpu) +bool vmx_emulation_required(struct kvm_vcpu *vcpu) { return emulate_invalid_guest_state && !vmx_guest_state_valid(vcpu); } @@ -1367,7 +1367,7 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) vmcs_writel(GUEST_RFLAGS, rflags); if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) - vmx->emulation_required = emulation_required(vcpu); + vmx->emulation_required = vmx_emulation_required(vcpu); } u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu) @@ -3077,7 +3077,7 @@ void vmx_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0) } /* depends on vcpu->arch.cr0 to be set to a new value */ - vmx->emulation_required = emulation_required(vcpu); + vmx->emulation_required = vmx_emulation_required(vcpu); } static int vmx_get_max_tdp_level(void) @@ -3330,7 +3330,7 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int { __vmx_set_segment(vcpu, var, seg); - to_vmx(vcpu)->emulation_required = emulation_required(vcpu); + to_vmx(vcpu)->emulation_required = vmx_emulation_required(vcpu); } static void vmx_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l) diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index 4858c5fd95f2..3a587c51a8d1 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -359,6 +359,7 @@ void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu); void vmx_set_host_fs_gs(struct vmcs_host_state *host, u16 fs_sel, u16 gs_sel, unsigned long fs_base, unsigned long gs_base); int vmx_get_cpl(struct kvm_vcpu *vcpu); +bool vmx_emulation_required(struct kvm_vcpu *vcpu); unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu); void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags); u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu);
If L1 had invalid state on VM entry (can happen on SMM transactions when we enter from real mode, straight to nested guest), then after we load 'host' state from VMCS12, the state has to become valid again, but since we load the segment registers with __vmx_set_segment we weren't always updating emulation_required. Update emulation_required explicitly at end of load_vmcs12_host_state. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/vmx/nested.c | 2 ++ arch/x86/kvm/vmx/vmx.c | 8 ++++---- arch/x86/kvm/vmx/vmx.h | 1 + 3 files changed, 7 insertions(+), 4 deletions(-)