Re: buggy emulate_int_real

[Jan Kiszka <jan.kiszka@web.de>]

[-- 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 --]