buggy emulate_int_real

buggy emulate_int_real

[Serge E. Hallyn]

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.

I thought it'd be simple to fix (bump the value being pushed :), but my
attempts at that have failed.  Well, the right value seemed to get pushed,
but kvm started to act rather funky.  So I just removed commits

a92601bb707f6f49fd5563ef3d09928e70cc222e
    KVM: VMX: Emulated real mode interrupt injection

63995653ade16deacaea5b49ceaf6376314593ac
    KVM: Add kvm_inject_realmode_interrupt() wrapper

6e154e56b4d7a6a28c54f0984e13d3f8defc4755
    KVM: x86 emulator: Add into, int, and int3 instructions (opcodes 0xcc-0xce)

and now it behaves as I'd expect.  There were a few commits tweaking these
functions, and I have not checked whether reverting some of those helps.

Anyone happen to know what exactly is going on?

thanks,
-serge

Re: buggy emulate_int_real

[Avi Kivity]

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

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

Re: buggy emulate_int_real

[Serge E. Hallyn]

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.

-serge

Re: buggy emulate_int_real

[Avi Kivity]

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

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

Re: buggy emulate_int_real

[Serge E. Hallyn]

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

Re: buggy emulate_int_real

[Serge E. Hallyn]

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

Oops, right.  Disregard last email pls :)

So is 'kvm_exception_is_soft(irq)' a reliable check?

(building a test kernel right now)

thanks,
-serge

Re: buggy emulate_int_real

[Avi Kivity]

On 04/12/2011 04:57 PM, Serge E. Hallyn wrote:
> 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?

Possibly.

> For instance vmx_inject_nmi() didn't do it at all,
> and the other two callers only did it for a soft interrupt.

NMIs are always hard.

Perhaps have a new helper for this.

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

Re: buggy emulate_int_real

[Avi Kivity]

On 04/12/2011 05:12 PM, Serge E. Hallyn wrote:
> 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().
>
> Oops, right.  Disregard last email pls :)
>
> So is 'kvm_exception_is_soft(irq)' a reliable check?
>

No, need to check vcpu->arch.interrupt.soft instead.  Not sure about 
kvm_exception_is_soft().  Jan?

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

Re: buggy emulate_int_real

[Jan Kiszka]

On 2011-04-12 16:16, Avi Kivity wrote:
> On 04/12/2011 05:12 PM, Serge E. Hallyn wrote:
>> 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().
>>
>> Oops, right.  Disregard last email pls :)
>>
>> So is 'kvm_exception_is_soft(irq)' a reliable check?
>>
> 
> No, need to check vcpu->arch.interrupt.soft instead.  Not sure about 
> kvm_exception_is_soft().  Jan?

Jumping late on this, I don't understand the question. Reliable /wrt what?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: buggy emulate_int_real

[Serge E. Hallyn]

Quoting Jan Kiszka (jan.kiszka@siemens.com):
> On 2011-04-12 16:16, Avi Kivity wrote:
> > On 04/12/2011 05:12 PM, Serge E. Hallyn wrote:
> >> 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().
> >>
> >> Oops, right.  Disregard last email pls :)
> >>
> >> So is 'kvm_exception_is_soft(irq)' a reliable check?
> >>
> > 
> > No, need to check vcpu->arch.interrupt.soft instead.  Not sure about 
> > kvm_exception_is_soft().  Jan?
> 
> Jumping late on this, I don't understand the question. Reliable /wrt what?

As to whether we are supposed to increment eip or not.

thanks,
-serge

Re: buggy emulate_int_real

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4000 bytes --]

On 2011-04-12 20:31, Serge E. Hallyn wrote:
> Quoting Jan Kiszka (jan.kiszka@siemens.com):
>> On 2011-04-12 16:16, Avi Kivity wrote:
>>> On 04/12/2011 05:12 PM, Serge E. Hallyn wrote:
>>>> 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().
>>>>
>>>> Oops, right.  Disregard last email pls :)
>>>>
>>>> So is 'kvm_exception_is_soft(irq)' a reliable check?
>>>>
>>>
>>> No, need to check vcpu->arch.interrupt.soft instead.  Not sure about 
>>> kvm_exception_is_soft().  Jan?
>>
>> Jumping late on this, I don't understand the question. Reliable /wrt what?
> 
> As to whether we are supposed to increment eip or not.

From a brief refresh-reading, I would say

if (interrupt.soft || kvm_exception_is_soft(nr))
    increment_eip_by_inst_len

but only for those interrupts/exceptions which were raised by the
triggering instructions, _not_ for exceptions raise while processing
them (e.g. a page fault while accessing the IDT).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: buggy emulate_int_real

[Serge E. Hallyn]

Quoting Jan Kiszka (jan.kiszka@web.de):
> From a brief refresh-reading, I would say
> 
> if (interrupt.soft || kvm_exception_is_soft(nr))
>     increment_eip_by_inst_len
> 
> but only for those interrupts/exceptions which were raised by the
> triggering instructions, _not_ for exceptions raise while processing
> them (e.g. a page fault while accessing the IDT).
> 
> Jan

Ok, thanks guys.  I'm currently trying a compile of the following.

>From a3c4d5b67eb6d91ef772bc0e04adcd7d9711275f Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serge.hallyn@canonical.com>
Date: Tue, 12 Apr 2011 22:18:40 +0100
Subject: [PATCH 1/1] kvm: fix push of wrong eip when doing softint

When doing a soft int, we need to bump eip before pushing it to
the stack.  Otherwise we'll do the int a second time.

Signed-off-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
---
 arch/x86/kvm/vmx.c |   12 +++++++++---
 arch/x86/kvm/x86.c |    3 ++-
 arch/x86/kvm/x86.h |    2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index bf89ec2..6761464 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1053,7 +1053,10 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 	}
 
 	if (vmx->rmode.vm86_active) {
-		if (kvm_inject_realmode_interrupt(vcpu, nr) != EMULATE_DONE)
+		int inc_eip = 0;
+		if (kvm_exception_is_soft(nr))
+			inc_eip = vcpu.arch.event_exit_inst_len;
+		if (kvm_inject_realmode_interrupt(vcpu, nr, inc_eip) != EMULATE_DONE)
 			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
 		return;
 	}
@@ -2871,7 +2874,10 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu)
 
 	++vcpu->stat.irq_injections;
 	if (vmx->rmode.vm86_active) {
-		if (kvm_inject_realmode_interrupt(vcpu, irq) != EMULATE_DONE)
+		int inc_eip = 0;
+		if (vcpu->arch.interrupt.soft)
+			inc_eip = vcpu.arch.event_exit_inst_len;
+		if (kvm_inject_realmode_interrupt(vcpu, irq, inc_eip) != EMULATE_DONE)
 			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
 		return;
 	}
@@ -2905,7 +2911,7 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
 
 	++vcpu->stat.nmi_injections;
 	if (vmx->rmode.vm86_active) {
-		if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR) != EMULATE_DONE)
+		if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR, 0) != EMULATE_DONE)
 			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
 		return;
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bcc0efc..8911622 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4293,7 +4293,7 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
 	memcpy(c->regs, vcpu->arch.regs, sizeof c->regs);
 }
 
-int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq)
+int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
 {
 	struct decode_cache *c = &vcpu->arch.emulate_ctxt.decode;
 	int ret;
@@ -4303,6 +4303,7 @@ int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq)
 	vcpu->arch.emulate_ctxt.decode.op_bytes = 2;
 	vcpu->arch.emulate_ctxt.decode.ad_bytes = 2;
 	vcpu->arch.emulate_ctxt.decode.eip = vcpu->arch.emulate_ctxt.eip;
+	vcpu->arch.emulate_ctxt.decode.eip += inc_eip;
 	ret = emulate_int_real(&vcpu->arch.emulate_ctxt, &emulate_ops, irq);
 
 	if (ret != X86EMUL_CONTINUE)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index c600da8..e407ed3 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -77,7 +77,7 @@ static inline u32 bit(int bitno)
 
 void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
-int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq);
+int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
 
 void kvm_write_tsc(struct kvm_vcpu *vcpu, u64 data);
 
-- 
1.7.0.4

Re: buggy emulate_int_real

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serge@hallyn.com):
> Quoting Jan Kiszka (jan.kiszka@web.de):
> > From a brief refresh-reading, I would say
> > 
> > if (interrupt.soft || kvm_exception_is_soft(nr))
> >     increment_eip_by_inst_len
> > 
> > but only for those interrupts/exceptions which were raised by the
> > triggering instructions, _not_ for exceptions raise while processing
> > them (e.g. a page fault while accessing the IDT).
> > 
> > Jan
> 
> Ok, thanks guys.  I'm currently trying a compile of the following.

With the obvious fixes, that turns into the below, which seems to be
working for me:

>From 555696f355ec2db9f4919f5e363355a2671b15a5 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serge.hallyn@canonical.com>
Date: Tue, 12 Apr 2011 22:18:40 +0100
Subject: [PATCH 1/1] kvm: fix push of wrong eip when doing softint

When doing a soft int, we need to bump eip before pushing it to
the stack.  Otherwise we'll do the int a second time.

Signed-off-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
---
 arch/x86/kvm/vmx.c |   12 +++++++++---
 arch/x86/kvm/x86.c |    3 ++-
 arch/x86/kvm/x86.h |    2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index bf89ec2..3aad96c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1053,7 +1053,10 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 	}
 
 	if (vmx->rmode.vm86_active) {
-		if (kvm_inject_realmode_interrupt(vcpu, nr) != EMULATE_DONE)
+		int inc_eip = 0;
+		if (kvm_exception_is_soft(nr))
+			inc_eip = vcpu->arch.event_exit_inst_len;
+		if (kvm_inject_realmode_interrupt(vcpu, nr, inc_eip) != EMULATE_DONE)
 			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
 		return;
 	}
@@ -2871,7 +2874,10 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu)
 
 	++vcpu->stat.irq_injections;
 	if (vmx->rmode.vm86_active) {
-		if (kvm_inject_realmode_interrupt(vcpu, irq) != EMULATE_DONE)
+		int inc_eip = 0;
+		if (vcpu->arch.interrupt.soft)
+			inc_eip = vcpu->arch.event_exit_inst_len;
+		if (kvm_inject_realmode_interrupt(vcpu, irq, inc_eip) != EMULATE_DONE)
 			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
 		return;
 	}
@@ -2905,7 +2911,7 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
 
 	++vcpu->stat.nmi_injections;
 	if (vmx->rmode.vm86_active) {
-		if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR) != EMULATE_DONE)
+		if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR, 0) != EMULATE_DONE)
 			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
 		return;
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bcc0efc..8911622 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4293,7 +4293,7 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
 	memcpy(c->regs, vcpu->arch.regs, sizeof c->regs);
 }
 
-int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq)
+int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
 {
 	struct decode_cache *c = &vcpu->arch.emulate_ctxt.decode;
 	int ret;
@@ -4303,6 +4303,7 @@ int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq)
 	vcpu->arch.emulate_ctxt.decode.op_bytes = 2;
 	vcpu->arch.emulate_ctxt.decode.ad_bytes = 2;
 	vcpu->arch.emulate_ctxt.decode.eip = vcpu->arch.emulate_ctxt.eip;
+	vcpu->arch.emulate_ctxt.decode.eip += inc_eip;
 	ret = emulate_int_real(&vcpu->arch.emulate_ctxt, &emulate_ops, irq);
 
 	if (ret != X86EMUL_CONTINUE)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index c600da8..e407ed3 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -77,7 +77,7 @@ static inline u32 bit(int bitno)
 
 void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
-int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq);
+int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
 
 void kvm_write_tsc(struct kvm_vcpu *vcpu, u64 data);
 
-- 
1.7.0.4

Re: buggy emulate_int_real

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4312 bytes --]

On 2011-04-13 00:39, Serge E. Hallyn wrote:
> Quoting Serge E. Hallyn (serge@hallyn.com):
>> Quoting Jan Kiszka (jan.kiszka@web.de):
>>> From a brief refresh-reading, I would say
>>>
>>> if (interrupt.soft || kvm_exception_is_soft(nr))
>>>     increment_eip_by_inst_len
>>>
>>> but only for those interrupts/exceptions which were raised by the
>>> triggering instructions, _not_ for exceptions raise while processing
>>> them (e.g. a page fault while accessing the IDT).
>>>
>>> Jan
>>
>> Ok, thanks guys.  I'm currently trying a compile of the following.
> 
> With the obvious fixes, that turns into the below, which seems to be
> working for me:
> 
> From 555696f355ec2db9f4919f5e363355a2671b15a5 Mon Sep 17 00:00:00 2001
> From: Serge E. Hallyn <serge.hallyn@canonical.com>
> Date: Tue, 12 Apr 2011 22:18:40 +0100
> Subject: [PATCH 1/1] kvm: fix push of wrong eip when doing softint
> 
> When doing a soft int, we need to bump eip before pushing it to
> the stack.  Otherwise we'll do the int a second time.
> 
> Signed-off-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> ---
>  arch/x86/kvm/vmx.c |   12 +++++++++---
>  arch/x86/kvm/x86.c |    3 ++-
>  arch/x86/kvm/x86.h |    2 +-
>  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index bf89ec2..3aad96c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1053,7 +1053,10 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
>  	}
>  
>  	if (vmx->rmode.vm86_active) {
> -		if (kvm_inject_realmode_interrupt(vcpu, nr) != EMULATE_DONE)
> +		int inc_eip = 0;
> +		if (kvm_exception_is_soft(nr))
> +			inc_eip = vcpu->arch.event_exit_inst_len;
> +		if (kvm_inject_realmode_interrupt(vcpu, nr, inc_eip) != EMULATE_DONE)
>  			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>  		return;
>  	}
> @@ -2871,7 +2874,10 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu)
>  
>  	++vcpu->stat.irq_injections;
>  	if (vmx->rmode.vm86_active) {
> -		if (kvm_inject_realmode_interrupt(vcpu, irq) != EMULATE_DONE)
> +		int inc_eip = 0;
> +		if (vcpu->arch.interrupt.soft)
> +			inc_eip = vcpu->arch.event_exit_inst_len;
> +		if (kvm_inject_realmode_interrupt(vcpu, irq, inc_eip) != EMULATE_DONE)
>  			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>  		return;
>  	}
> @@ -2905,7 +2911,7 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>  
>  	++vcpu->stat.nmi_injections;
>  	if (vmx->rmode.vm86_active) {
> -		if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR) != EMULATE_DONE)
> +		if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR, 0) != EMULATE_DONE)
>  			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>  		return;
>  	}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index bcc0efc..8911622 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4293,7 +4293,7 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
>  	memcpy(c->regs, vcpu->arch.regs, sizeof c->regs);
>  }
>  
> -int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq)
> +int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
>  {
>  	struct decode_cache *c = &vcpu->arch.emulate_ctxt.decode;
>  	int ret;
> @@ -4303,6 +4303,7 @@ int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq)
>  	vcpu->arch.emulate_ctxt.decode.op_bytes = 2;
>  	vcpu->arch.emulate_ctxt.decode.ad_bytes = 2;
>  	vcpu->arch.emulate_ctxt.decode.eip = vcpu->arch.emulate_ctxt.eip;
> +	vcpu->arch.emulate_ctxt.decode.eip += inc_eip;

I would fold this into the previous line.

>  	ret = emulate_int_real(&vcpu->arch.emulate_ctxt, &emulate_ops, irq);
>  
>  	if (ret != X86EMUL_CONTINUE)
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index c600da8..e407ed3 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -77,7 +77,7 @@ static inline u32 bit(int bitno)
>  
>  void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
>  void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
> -int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq);
> +int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
>  
>  void kvm_write_tsc(struct kvm_vcpu *vcpu, u64 data);
>  

Looks consistent to me.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: buggy emulate_int_real

[Serge E. Hallyn]

Quoting Jan Kiszka (jan.kiszka@web.de):
> Looks consistent to me.

It's working for me, so if there are no further comments I'll improve
the changelog and send to lkml.

thanks,
-serge

Re: buggy emulate_int_real

[Avi Kivity]

On 04/13/2011 04:24 PM, Serge E. Hallyn wrote:
> Quoting Jan Kiszka (jan.kiszka@web.de):
> >  Looks consistent to me.
>
> It's working for me, so if there are no further comments

Looks good to me too.

> I'll improve
> the changelog and send to lkml.
>

kvm@, surely.

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

Re: buggy emulate_int_real

[Serge E. Hallyn]

Quoting Avi Kivity (avi@redhat.com):
> On 04/13/2011 04:24 PM, Serge E. Hallyn wrote:
> >Quoting Jan Kiszka (jan.kiszka@web.de):
> >>  Looks consistent to me.
> >
> >It's working for me, so if there are no further comments
> 
> Looks good to me too.
> 
> >I'll improve
> >the changelog and send to lkml.
> >
> 
> kvm@, surely.

Oh?  Didn't realize that was the proper path - absolutely.

thanks,
-serge