Re: [PATCH 3/3] VMX: Enhance invalid guest state emulation

Avi Kivity
2009-08-31T12:35:14Z

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))
>

--
error compiling committee.c: too many arguments to function