| Message ID | 1251470963-14542-1-git-send-email-m.gamal005@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/28/2009 05:49 PM, 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> > --- > arch/x86/kvm/vmx.c | 24 ++++++++++++++---------- > 1 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 78101dd..e422470 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3318,10 +3318,11 @@ 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; > + int ret = 1; > > local_irq_enable(); > preempt_enable(); > These are now wrong, since handle_invalid_exit() is called with interrupts and preemption enabled. > @@ -3329,11 +3330,16 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu) > while (!guest_state_valid(vcpu)) { > err = emulate_instruction(vcpu, 0, 0, 0); > > - if (err == EMULATE_DO_MMIO) > + if (err == EMULATE_DO_MMIO) { > + ret = 0; > break; > + } > > if (err != EMULATE_DONE) { > kvm_report_emulation_failure(vcpu, "emulation failure"); > + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; > + ret = 0; > break; > } > > @@ -3347,6 +3353,7 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu) > local_irq_disable(); > > vmx->invalid_state_emulation_result = err; > + return ret; > } > > /* > @@ -3405,9 +3412,12 @@ static int vmx_handle_exit(struct kvm_vcpu *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)) > + if (guest_state_valid(vcpu)) { > vmx->emulation_required = 0; > - return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO; > + return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO; > This looks fishy. Can't say exactly why but vmx_handle_exit() should only depend on the current guest execution, not the previous guest execution. > + } else { > + return handle_invalid_guest_state(vcpu); > + } > } > > /* Access CR3 don't cause VMExit in paging mode, so we need > @@ -3603,12 +3613,6 @@ 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); > - return; > - } > - > Don't we still need to return here? Otherwise we attempt guest entry needlessly. > if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty)) > vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]); > if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty)) >
On 08/31/2009 04:39 PM, Mohammed Gamal wrote: >>> local_irq_enable(); >>> preempt_enable(); >>> >>> >> These are now wrong, since handle_invalid_exit() is called with interrupts >> and preemption enabled. >> >> > Do you mean vmx_handle_exit() ? > Yes. >>> /* >>> @@ -3405,9 +3412,12 @@ static int vmx_handle_exit(struct kvm_vcpu *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)) >>> + if (guest_state_valid(vcpu)) { >>> vmx->emulation_required = 0; >>> - return vmx->invalid_state_emulation_result != >>> EMULATE_DO_MMIO; >>> + return vmx->invalid_state_emulation_result != >>> EMULATE_DO_MMIO; >>> >>> >> This looks fishy. Can't say exactly why but vmx_handle_exit() should only >> depend on the current guest execution, not the previous guest execution. >> > I still can't quite get the problem here! > The design of the main loop is that you can have a save/restore cycle (or live migration) after __vcpu_run(). So abything in __vcpu_run() and the function it calls can only depend on state set previously in __vcpu_run(), or on state that is loaded from KVM_SET_REGS and similar ioctls. In this case I think vmx->invalid_state_emulation_result is not set previously to its use within __vcpu_run(). >>> /* Access CR3 don't cause VMExit in paging mode, so we need >>> @@ -3603,12 +3613,6 @@ 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); >>> - return; >>> - } >>> - >>> >>> >> Don't we still need to return here? Otherwise we attempt guest entry >> needlessly. >> > But how would a vmexit be triggered (thus calling vmx_handle_exit() )? > vmx_handle_exit() will be called in any case. > I personally prefer if we can start emulation before attempting guest > entry, but how can we tell vmx_vcpu_run() to return to userspace if it > doesn't return a value, I don't feel that changing it to return a > value would be a wise thing to do too, no? > vmx_vcpu_run() can simply return, setting an internal vmx flag. Then vmx_handle_exit() can notice the flag, emulate, and handle the result of this emulation.
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 78101dd..e422470 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3318,10 +3318,11 @@ 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; + int ret = 1; local_irq_enable(); preempt_enable(); @@ -3329,11 +3330,16 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu) while (!guest_state_valid(vcpu)) { err = emulate_instruction(vcpu, 0, 0, 0); - if (err == EMULATE_DO_MMIO) + if (err == EMULATE_DO_MMIO) { + ret = 0; break; + } if (err != EMULATE_DONE) { kvm_report_emulation_failure(vcpu, "emulation failure"); + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; + ret = 0; break; } @@ -3347,6 +3353,7 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu) local_irq_disable(); vmx->invalid_state_emulation_result = err; + return ret; } /* @@ -3405,9 +3412,12 @@ static int vmx_handle_exit(struct kvm_vcpu *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)) + if (guest_state_valid(vcpu)) { vmx->emulation_required = 0; - return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO; + return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO; + } else { + return handle_invalid_guest_state(vcpu); + } } /* Access CR3 don't cause VMExit in paging mode, so we need @@ -3603,12 +3613,6 @@ 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); - return; - } - if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty)) vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]); if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty))
- 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 | 24 ++++++++++++++---------- 1 files changed, 14 insertions(+), 10 deletions(-)