From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Serge E. Hallyn" <serge@hallyn.com>
Subject: Re: buggy emulate_int_real
Date: Tue, 12 Apr 2011 08:57:41 -0500
Message-ID: <20110412135740.GA6456@hallyn.com>
References: <20110408210900.GA26787@hallyn.com>
 <4DA16AA1.7010108@redhat.com>
 <20110412075319.GA28696@hallyn.com>
 <4DA406F8.4090701@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: KVM mailing list <kvm@vger.kernel.org>
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from hrndva-omtalb.mail.rr.com ([71.74.56.125]:55333 "EHLO
	hrndva-omtalb.mail.rr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1755535Ab1DLN5n (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 12 Apr 2011 09:57:43 -0400
Content-Disposition: inline
In-Reply-To: <4DA406F8.4090701@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Quoting Avi Kivity (avi@redhat.com):
> On 04/12/2011 10:53 AM, Serge E. Hallyn wrote:
> >Quoting Avi Kivity (avi@redhat.com):
> >>  On 04/09/2011 12:09 AM, Serge E. Hallyn wrote:
> >>  >Hi,
> >>  >
> >>  >at https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/747090, it was
> >>  >found that emulate_int_real() sometimes pushes the wrong eip when doing a
> >>  >int.  Whereas with non-kvm qemu we push the next instruction after the
> >>  >int, with kvm we push the addr of the instruction itself.
> >>  >
> >>
> >>
> >>  The code says:
> >>
> >>      c->src.val = c->eip;
> >>      emulate_push(ctxt, ops);
> >>      rc = writeback(ctxt, ops);
> >>      if (rc != X86EMUL_CONTINUE)
> >>          return rc;
> >>
> >>  which appears to be the address of the next instruction from my
> >>  reading of the code (see how insn_fetch() increments c->eip).
> >
> >Nevertheless removing commits
> >
> >	a92601bb707f6f49fd5563ef3d09928e70cc222e
> >	63995653ade16deacaea5b49ceaf6376314593ac
> >	6e154e56b4d7a6a28c54f0984e13d3f8defc4755
> >
> >changes the eip value being pushed.  If you look at
> >a92601bb707f6f49fd5563ef3d09928e70cc222e, you see:
> >
> >         if (vmx->rmode.vm86_active) {
> >-               vmx->rmode.irq.pending = true;
> >-               vmx->rmode.irq.vector = nr;
> >-               vmx->rmode.irq.rip = kvm_rip_read(vcpu);
> >-               if (kvm_exception_is_soft(nr))
> >-                       vmx->rmode.irq.rip +=
> >-                               vmx->vcpu.arch.event_exit_inst_len;
> >-               intr_info |= INTR_TYPE_SOFT_INTR;
> >-               vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, intr_info);
> >-               vmcs_write32(VM_ENTRY_INSTRUCTION_LEN, 1);
> >-               kvm_rip_write(vcpu, vmx->rmode.irq.rip - 1);
> >+               if (kvm_inject_realmode_interrupt(vcpu, nr) != EMULATE_DONE)
> >+                       kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> >                 return;
> >         }
> >
> >but kvm_inject_realmode_interrupt() does not appear to increment
> >vmx->rmode.irq.rip anywhere, as the code being replaced does.
> 
> Ah, I see now.  There are two cases, hard interrupt and soft
> interrupts.  I guess hard interrupts are handled fine, and the
> failing case is
> 
>   guest executes INTn instruction in guest mode
>   vmx intercepts a page fault (say due to access to the IDT or the stack)
>   kvm notes that a soft interrupt was in progress (vmx_complete_interrupts)
>   kvm handles the exception
>   reinject the interrupt while reentering the guest
> 
> so we do need something like
> 
>    if (soft)
>        vcpu->arch.emulate_ctxt.eip += inst_len;
> 
> in kvm_inject_realmode_interrupt().

But not always, so perhaps it would be better to still do this at
the callers?  For instance vmx_inject_nmi() didn't do it at all,
and the other two callers only did it for a soft interrupt.

-serge