Re: buggy emulate_int_real

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

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