| Message ID | 1251802098-23819-1-git-send-email-m.gamal005@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 01, 2009 at 12:48:18PM +0200, Mohammed Gamal wrote: > - Change returned handle_invalid_guest_state() to return relevant exit codes > - Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit() > - Return to userspace instead of repeatedly trying to emulate instructions that have already failed > > Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> Mohammed, The handle_invalid_guest_state loop is potentially problematic. It would be more appropriate to use the __vcpu_run loop. Can't you set vmx->emulation_required depending on the result of one call to emulate_instruction and get rid of the while (!guest_state_valid(vcpu)) loop? > --- > arch/x86/kvm/vmx.c | 44 ++++++++++++++++++++------------------------ > 1 files changed, 20 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 78101dd..6265098 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -107,7 +107,6 @@ struct vcpu_vmx { > } rmode; > int vpid; > bool emulation_required; > - enum emulation_result invalid_state_emulation_result; > > /* Support for vnmi-less CPUs */ > int soft_vnmi_blocked; > @@ -3318,35 +3317,37 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu) > return 1; > } > > -static void handle_invalid_guest_state(struct kvm_vcpu *vcpu) > +static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > enum emulation_result err = EMULATE_DONE; > - > - local_irq_enable(); > - preempt_enable(); > + int ret = 1; > > while (!guest_state_valid(vcpu)) { > err = emulate_instruction(vcpu, 0, 0, 0); > > - if (err == EMULATE_DO_MMIO) > - break; > + if (err == EMULATE_DO_MMIO) { > + ret = 0; > + goto out; > + } > > if (err != EMULATE_DONE) { > kvm_report_emulation_failure(vcpu, "emulation failure"); > - break; > + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; > + ret = 0; > + goto out; > } > > if (signal_pending(current)) > - break; > + goto out; > if (need_resched()) > schedule(); > } > > - preempt_disable(); > - local_irq_disable(); > - > - vmx->invalid_state_emulation_result = err; > + vmx->emulation_required = 0; > +out: > + return ret; > } > > /* > @@ -3402,13 +3403,9 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu) > > trace_kvm_exit(exit_reason, kvm_rip_read(vcpu)); > > - /* If we need to emulate an MMIO from handle_invalid_guest_state > - * we just return 0 */ > - if (vmx->emulation_required && emulate_invalid_guest_state) { > - if (guest_state_valid(vcpu)) > - vmx->emulation_required = 0; > - return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO; > - } > + /* If guest state is invalid, start emulating */ > + if (vmx->emulation_required && emulate_invalid_guest_state) > + return handle_invalid_guest_state(vcpu); > > /* Access CR3 don't cause VMExit in paging mode, so we need > * to sync with guest real CR3. */ > @@ -3603,11 +3600,10 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) > if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked)) > vmx->entry_time = ktime_get(); > > - /* Handle invalid guest state instead of entering VMX */ > - if (vmx->emulation_required && emulate_invalid_guest_state) { > - handle_invalid_guest_state(vcpu); > + /* Don't enter VMX if guest state is invalid, let the exit handler > + start emulation until we arrive back to a valid state */ > + if (vmx->emulation_required && emulate_invalid_guest_state) > return; > - } > > if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty)) > vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]); > -- > 1.6.0.4 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: > On Tue, Sep 01, 2009 at 12:48:18PM +0200, Mohammed Gamal wrote: >> - Change returned handle_invalid_guest_state() to return relevant exit codes >> - Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit() >> - Return to userspace instead of repeatedly trying to emulate instructions that have already failed >> >> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> > > Mohammed, > > The handle_invalid_guest_state loop is potentially problematic. It would > be more appropriate to use the __vcpu_run loop. > > Can't you set vmx->emulation_required depending on the result > of one call to emulate_instruction and get rid of the while > (!guest_state_valid(vcpu)) loop? > Invalid state emulation is VMX-specfic, while the __vcpu_run loop is independent of the virtualization extension (defined in x86.c), no? AMD SVM can comforably run hosts in big-real mode and thus it doesn't have the notion of a guest going to an invalid state because of mode switching, so I don't think it'd be a good idea to move emulation into a generic layer. Please correct me if I am wrong -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 01, 2009 at 02:14:17PM +0200, Mohammed Gamal wrote: > On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: > > On Tue, Sep 01, 2009 at 12:48:18PM +0200, Mohammed Gamal wrote: > >> - Change returned handle_invalid_guest_state() to return relevant exit codes > >> - Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit() > >> - Return to userspace instead of repeatedly trying to emulate instructions that have already failed > >> > >> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> > > > > Mohammed, > > > > The handle_invalid_guest_state loop is potentially problematic. It would > > be more appropriate to use the __vcpu_run loop. > > > > Can't you set vmx->emulation_required depending on the result > > of one call to emulate_instruction and get rid of the while > > (!guest_state_valid(vcpu)) loop? > > > > Invalid state emulation is VMX-specfic, while the __vcpu_run loop is > independent of the virtualization extension (defined in x86.c), no? > AMD SVM can comforably run hosts in big-real mode and thus it doesn't > have the notion of a guest going to an invalid state because of mode > switching, so I don't think it'd be a good idea to move emulation into > a generic layer. Please correct me if I am wrong Right. But all i am asking is to emulate one instruction at a time in handle_invalid_guest_state, instead of looping until guest_state_valid(vcpu). So you get rid of schedule(), the check for signal_pending, etc. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 1, 2009 at 2:18 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: > On Tue, Sep 01, 2009 at 02:14:17PM +0200, Mohammed Gamal wrote: >> On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: >> > On Tue, Sep 01, 2009 at 12:48:18PM +0200, Mohammed Gamal wrote: >> >> - Change returned handle_invalid_guest_state() to return relevant exit codes >> >> - Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit() >> >> - Return to userspace instead of repeatedly trying to emulate instructions that have already failed >> >> >> >> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> >> > >> > Mohammed, >> > >> > The handle_invalid_guest_state loop is potentially problematic. It would >> > be more appropriate to use the __vcpu_run loop. >> > >> > Can't you set vmx->emulation_required depending on the result >> > of one call to emulate_instruction and get rid of the while >> > (!guest_state_valid(vcpu)) loop? >> > >> >> Invalid state emulation is VMX-specfic, while the __vcpu_run loop is >> independent of the virtualization extension (defined in x86.c), no? >> AMD SVM can comforably run hosts in big-real mode and thus it doesn't >> have the notion of a guest going to an invalid state because of mode >> switching, so I don't think it'd be a good idea to move emulation into >> a generic layer. Please correct me if I am wrong > > Right. But all i am asking is to emulate one instruction at a > time in handle_invalid_guest_state, instead of looping until > guest_state_valid(vcpu). > > So you get rid of schedule(), the check for signal_pending, etc. But we'll still need to enter the guest when it's in a valid state, so we need to move that loop somewhere, and now that we still have a loop we'll also still need to do the pending signals and scheduling checks, no? I'd appreciate any suggestions you have to alleviate this. > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 01, 2009 at 03:08:55PM +0200, Mohammed Gamal wrote: > On Tue, Sep 1, 2009 at 2:18 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: > > On Tue, Sep 01, 2009 at 02:14:17PM +0200, Mohammed Gamal wrote: > >> On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: > >> > On Tue, Sep 01, 2009 at 12:48:18PM +0200, Mohammed Gamal wrote: > >> >> - Change returned handle_invalid_guest_state() to return relevant exit codes > >> >> - Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit() > >> >> - Return to userspace instead of repeatedly trying to emulate instructions that have already failed > >> >> > >> >> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> > >> > > >> > Mohammed, > >> > > >> > The handle_invalid_guest_state loop is potentially problematic. It would > >> > be more appropriate to use the __vcpu_run loop. > >> > > >> > Can't you set vmx->emulation_required depending on the result > >> > of one call to emulate_instruction and get rid of the while > >> > (!guest_state_valid(vcpu)) loop? > >> > > >> > >> Invalid state emulation is VMX-specfic, while the __vcpu_run loop is > >> independent of the virtualization extension (defined in x86.c), no? > >> AMD SVM can comforably run hosts in big-real mode and thus it doesn't > >> have the notion of a guest going to an invalid state because of mode > >> switching, so I don't think it'd be a good idea to move emulation into > >> a generic layer. Please correct me if I am wrong > > > > Right. But all i am asking is to emulate one instruction at a > > time in handle_invalid_guest_state, instead of looping until > > guest_state_valid(vcpu). > > > > So you get rid of schedule(), the check for signal_pending, etc. > > But we'll still need to enter the guest when it's in a valid state, so > we need to move that loop somewhere, Sure, just set vmx->emulation_required = guest_state_valid(vcpu). When the state is good, the entry handler will vmentry. > and now that we still have a loop > we'll also still need to do the pending signals and scheduling checks, > no? Point is you can use the __vcpu_run loop. In the latest patch you do: + /* Don't enter VMX if guest state is invalid, let the exit handler + start emulation until we arrive back to a valid state */ + if (vmx->emulation_required && emulate_invalid_guest_state) return; And then emulate in the exit handler. > I'd appreciate any suggestions you have to alleviate this. I fail to see why you need an internal loop if you can use the external (__vcpu_run) one. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 1, 2009 at 3:29 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: > On Tue, Sep 01, 2009 at 03:08:55PM +0200, Mohammed Gamal wrote: >> On Tue, Sep 1, 2009 at 2:18 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: >> > On Tue, Sep 01, 2009 at 02:14:17PM +0200, Mohammed Gamal wrote: >> >> On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote: >> >> > On Tue, Sep 01, 2009 at 12:48:18PM +0200, Mohammed Gamal wrote: >> >> >> - Change returned handle_invalid_guest_state() to return relevant exit codes >> >> >> - Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit() >> >> >> - Return to userspace instead of repeatedly trying to emulate instructions that have already failed >> >> >> >> >> >> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> >> >> > >> >> > Mohammed, >> >> > >> >> > The handle_invalid_guest_state loop is potentially problematic. It would >> >> > be more appropriate to use the __vcpu_run loop. >> >> > >> >> > Can't you set vmx->emulation_required depending on the result >> >> > of one call to emulate_instruction and get rid of the while >> >> > (!guest_state_valid(vcpu)) loop? >> >> > >> >> >> >> Invalid state emulation is VMX-specfic, while the __vcpu_run loop is >> >> independent of the virtualization extension (defined in x86.c), no? >> >> AMD SVM can comforably run hosts in big-real mode and thus it doesn't >> >> have the notion of a guest going to an invalid state because of mode >> >> switching, so I don't think it'd be a good idea to move emulation into >> >> a generic layer. Please correct me if I am wrong >> > >> > Right. But all i am asking is to emulate one instruction at a >> > time in handle_invalid_guest_state, instead of looping until >> > guest_state_valid(vcpu). >> > >> > So you get rid of schedule(), the check for signal_pending, etc. >> >> But we'll still need to enter the guest when it's in a valid state, so >> we need to move that loop somewhere, > > Sure, just set vmx->emulation_required = guest_state_valid(vcpu). When > the state is good, the entry handler will vmentry. > >> and now that we still have a loop >> we'll also still need to do the pending signals and scheduling checks, >> no? > > Point is you can use the __vcpu_run loop. > > In the latest patch you do: > > + Â Â Â /* Don't enter VMX if guest state is invalid, let the exit handler > + Â Â Â Â Â start emulation until we arrive back to a valid state */ > + Â Â Â if (vmx->emulation_required && emulate_invalid_guest_state) > Â Â Â Â Â Â Â Â return; > > And then emulate in the exit handler. > >> I'd appreciate any suggestions you have to alleviate this. > > I fail to see why you need an internal loop if you can use the external > (__vcpu_run) one. Because it's not just used by VMX. So I don't think it'd be wise to use it for something that's VMX-specific. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 01, 2009 at 03:32:29PM +0200, Mohammed Gamal wrote: > > + Â Â Â if (vmx->emulation_required && emulate_invalid_guest_state) > > Â Â Â Â Â Â Â Â return; > > > > And then emulate in the exit handler. > > > >> I'd appreciate any suggestions you have to alleviate this. > > > > I fail to see why you need an internal loop if you can use the external > > (__vcpu_run) one. > > Because it's not just used by VMX. So I don't think it'd be wise to > use it for something that's VMX-specific. OK, it can be done incrementally. This is already an improvement. Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/01/2009 04:32 PM, Mohammed Gamal wrote: >>> I'd appreciate any suggestions you have to alleviate this. >>> >> I fail to see why you need an internal loop if you can use the external >> (__vcpu_run) one. >> > Because it's not just used by VMX. So I don't think it'd be wise to > use it for something that's VMX-specific. > The loop is there anyway. The only question is whether vmx_handle_exit() emulates on instruction or many. Emulating one instruction is slower, but will get interrupt injection more accurate (once we have emulated real mode interrupt injection).
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 78101dd..6265098 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -107,7 +107,6 @@ struct vcpu_vmx { } rmode; int vpid; bool emulation_required; - enum emulation_result invalid_state_emulation_result; /* Support for vnmi-less CPUs */ int soft_vnmi_blocked; @@ -3318,35 +3317,37 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu) return 1; } -static void handle_invalid_guest_state(struct kvm_vcpu *vcpu) +static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) { struct vcpu_vmx *vmx = to_vmx(vcpu); enum emulation_result err = EMULATE_DONE; - - local_irq_enable(); - preempt_enable(); + int ret = 1; while (!guest_state_valid(vcpu)) { err = emulate_instruction(vcpu, 0, 0, 0); - if (err == EMULATE_DO_MMIO) - break; + if (err == EMULATE_DO_MMIO) { + ret = 0; + goto out; + } if (err != EMULATE_DONE) { kvm_report_emulation_failure(vcpu, "emulation failure"); - break; + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; + ret = 0; + goto out; } if (signal_pending(current)) - break; + goto out; if (need_resched()) schedule(); } - preempt_disable(); - local_irq_disable(); - - vmx->invalid_state_emulation_result = err; + vmx->emulation_required = 0; +out: + return ret; } /* @@ -3402,13 +3403,9 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu) trace_kvm_exit(exit_reason, kvm_rip_read(vcpu)); - /* If we need to emulate an MMIO from handle_invalid_guest_state - * we just return 0 */ - if (vmx->emulation_required && emulate_invalid_guest_state) { - if (guest_state_valid(vcpu)) - vmx->emulation_required = 0; - return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO; - } + /* If guest state is invalid, start emulating */ + if (vmx->emulation_required && emulate_invalid_guest_state) + return handle_invalid_guest_state(vcpu); /* Access CR3 don't cause VMExit in paging mode, so we need * to sync with guest real CR3. */ @@ -3603,11 +3600,10 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked)) vmx->entry_time = ktime_get(); - /* Handle invalid guest state instead of entering VMX */ - if (vmx->emulation_required && emulate_invalid_guest_state) { - handle_invalid_guest_state(vcpu); + /* Don't enter VMX if guest state is invalid, let the exit handler + start emulation until we arrive back to a valid state */ + if (vmx->emulation_required && emulate_invalid_guest_state) return; - } if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty)) vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);
- Change returned handle_invalid_guest_state() to return relevant exit codes - Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit() - Return to userspace instead of repeatedly trying to emulate instructions that have already failed Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> --- arch/x86/kvm/vmx.c | 44 ++++++++++++++++++++------------------------ 1 files changed, 20 insertions(+), 24 deletions(-)