Split the emulator: decode & execute

These patches split the emulator in two parts: one to decode the instruction, the other to execute it. The decode part is then called only when needed.

Quoted 3 lines · On Wednesday 29 August 2007 18:39:45 Laurent Vivier wrote:

These patches split the emulator in two parts: one to decode the instruction, the other to execute it. The decode part is then called only when needed.

Is the same change done to the Xen version? It might be better to make it easier to port future Xen changes over by keeping the code similar; at least as long as this emulator is far from being a complete x86 implementation.

These patches split the emulator in two parts: one to decode the instruction, the other to execute it. The decode part is then called only when needed.

Quoted 5 lines · Laurent Vivier

These patches split the emulator in two parts: one to decode the instruction,
the other to execute it. The decode part is then called only when needed.

Patchset looks good, but fails booting FC6 x86-64 on Intel. It may be a merge error (did not apply cleanly due to other changes). I pushed this as a 'split-emulator' branch on the kvm.git repository.

Quoted 8 lines · Avi Kivity

Quoted 6 lines · Laurent Vivier

These patches split the emulator in two parts: one to decode the instruction,
the other to execute it. The decode part is then called only when needed.

Quoted 5 lines

Patchset looks good, but fails booting FC6 x86-64 on Intel. It may be a merge error (did not apply cleanly due to other changes). I pushed this as a 'split-emulator' branch on the kvm.git repository.

I'm not able to reproduce the problem. Could I have more details (like guest kernel version and how it fails) ?

Quoted 5 lines · Laurent Vivier

These patches split the emulator in two parts: one to decode the instruction,
the other to execute it. The decode part is then called only when needed.

Patchset looks good, but fails booting FC6 x86-64 on Intel. It may be a merge error (did not apply cleanly due to other changes). I pushed this as a 'split-emulator' branch on the kvm.git repository.

Quoted 8 lines · Avi Kivity

Quoted 6 lines · Laurent Vivier

These patches split the emulator in two parts: one to decode the instruction,
the other to execute it. The decode part is then called only when needed.

Quoted 5 lines

Patchset looks good, but fails booting FC6 x86-64 on Intel. It may be a merge error (did not apply cleanly due to other changes). I pushed this as a 'split-emulator' branch on the kvm.git repository.

I think I found the bug (not a merge error...): I just supposed that an instruction fetch cannot failed.

Quoted 10 lines · Laurent Vivier

Quoted 8 lines · Avi Kivity

Quoted 6 lines · Laurent Vivier

These patches split the emulator in two parts: one to decode the instruction,
the other to execute it. The decode part is then called only when needed.

Quoted 9 lines

Quoted 5 lines

Patchset looks good, but fails booting FC6 x86-64 on Intel. It may be a merge error (did not apply cleanly due to other changes). I pushed this as a 'split-emulator' branch on the kvm.git repository.

I think I found the bug (not a merge error...): I just supposed that an instruction fetch cannot failed.

Interesting. I don't see how an instruction fetch can fail on uniprocessor. Can you give details of the failure?

Instruction fetches can fail on SMP so a fix is certainly needed.

Quoted 26 lines · Avi Kivity

Quoted 13 lines · Laurent Vivier

Quoted 10 lines · Avi Kivity

Quoted 7 lines · Laurent Vivier

These patches split the emulator in two parts: one to decode the instruction,
the other to execute it. The decode part is then called only when needed.

Quoted 11 lines

Quoted 6 lines

Patchset looks good, but fails booting FC6 x86-64 on Intel. It may be a merge error (did not apply cleanly due to other changes). I pushed this as a 'split-emulator' branch on the kvm.git repository.

I think I found the bug (not a merge error...): I just supposed that an instruction fetch cannot failed.

Quoted 5 lines

Interesting. I don't see how an instruction fetch can fail on uniprocessor. Can you give details of the failure?

Instruction fetches can fail on SMP so a fix is certainly needed.

OK, I spoke too fast.

x86_decode_insn() fails because it is not able to decode:

0xffffffff8110b7ef <__copy_user_nocache+47>: movnti %r11,(%rdi)
or
0xffffffff8110b7ef <__copy_user_nocache+47>: 0x4c 0x0f 0xc3 0x1f

0x4c is decoded as a REX prefix.
0x0f is decoded as a Two-byte opcode
but 0xc3 is unknown in twobyte_table, so we exit because of an unrecognized opcode ("Cannot emulate").

Quoted 1 line · Laurent Vivier

Avi Kivity wrote:

Quoted 5 lines

Quoted 5 lines

Interesting. I don't see how an instruction fetch can fail on uniprocessor. Can you give details of the failure?

Instruction fetches can fail on SMP so a fix is certainly needed.

Quoted 7 lines

OK, I spoke too fast.

x86_decode_insn() fails because it is not able to decode:

0xffffffff8110b7ef <__copy_user_nocache+47>: movnti %r11,(%rdi)
or
0xffffffff8110b7ef <__copy_user_nocache+47>: 0x4c 0x0f 0xc3

Quoted 4 lines

0x4c is decoded as a REX prefix.
0x0f is decoded as a Two-byte opcode
but 0xc3 is unknown in twobyte_table, so we exit because of an

Quoted 3 lines

opcode ("Cannot emulate").

Not being able to emulate is sometimes legitimate. In the case of writing to a write-protected guest page table, we simply un-write-protect it and go back to the guest (which should now execute the instruction natively).

Perhaps the logic that deals with this (the call to kvm_mmu_unprotect_page_virt() in emulate_instruction()) was broken by your changes.

Quoted 10 lines

Not being able to emulate is sometimes legitimate. In the case of writing to a write-protected guest page table, we simply un-write-protect it and go back to the guest (which should now execute the instruction natively).

Perhaps the logic that deals with this (the call to kvm_mmu_unprotect_page_virt() in emulate_instruction()) was broken by your changes.

In fact this case is managed in the error cases of emulate_instruction(). My first patch removes this management for instruction decoding because I supposed it cannot generate such errors. So what I proposed in my last email seems to be the good solution :

emulate_instruction()
...

        r = x86_decode_insn(&vcpu->emulate_ctxt, &emulate_ops);
        if (r == 0)
                r = x86_emulate_insn(&vcpu->emulate_ctxt, &emulate_ops);
...
        if (r) {
                if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
                        return EMULATE_DONE;
                if (!vcpu->mmio_needed) {
                        kvm_report_emulation_failure(vcpu, "mmio");
                        return EMULATE_FAIL;
                }
                return EMULATE_DO_MMIO;
        }

...

Quoted 31 lines · Laurent Vivier (Bull)

Quoted 10 lines

Not being able to emulate is sometimes legitimate. In the case of writing to a write-protected guest page table, we simply un-write-protect it and go back to the guest (which should now execute the instruction natively).

Perhaps the logic that deals with this (the call to kvm_mmu_unprotect_page_virt() in emulate_instruction()) was broken by your changes.

In fact this case is managed in the error cases of emulate_instruction(). My first patch removes this management for instruction decoding because I supposed it cannot generate such errors. So what I proposed in my last email seems to be the good solution :

emulate_instruction()
...

        r = x86_decode_insn(&vcpu->emulate_ctxt, &emulate_ops);
        if (r == 0)
                r = x86_emulate_insn(&vcpu->emulate_ctxt, &emulate_ops);
...
        if (r) {
                if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
                        return EMULATE_DONE;
                if (!vcpu->mmio_needed) {
                        kvm_report_emulation_failure(vcpu, "mmio");
                        return EMULATE_FAIL;
                }
                return EMULATE_DO_MMIO;
        }

Quoted 2 lines

...

Yes. But pushing the kvm_mmu_unprotect_page() to immediately after the decode stage may be better.

Quoted 35 lines · Avi Kivity

Quoted 33 lines · Laurent Vivier (Bull)

Quoted 10 lines

Not being able to emulate is sometimes legitimate. In the case of writing to a write-protected guest page table, we simply un-write-protect it and go back to the guest (which should now execute the instruction natively).

Perhaps the logic that deals with this (the call to kvm_mmu_unprotect_page_virt() in emulate_instruction()) was broken by your changes.

In fact this case is managed in the error cases of emulate_instruction(). My first patch removes this management for instruction decoding because I supposed it cannot generate such errors. So what I proposed in my last email seems to be the good solution :

emulate_instruction()
...

        r = x86_decode_insn(&vcpu->emulate_ctxt, &emulate_ops);
        if (r == 0)
                r = x86_emulate_insn(&vcpu->emulate_ctxt, &emulate_ops);
...
        if (r) {
                if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
                        return EMULATE_DONE;
                if (!vcpu->mmio_needed) {
                        kvm_report_emulation_failure(vcpu, "mmio");
                        return EMULATE_FAIL;
                }
                return EMULATE_DO_MMIO;
        }

...

Quoted 4 lines

Yes. But pushing the kvm_mmu_unprotect_page() to immediately after the decode stage may be better.

OK, but is this the only error case we can have in the decode stage ?

Should we remove it from after the emulate stage ?

Quoted 38 lines · Laurent Vivier

Quoted 36 lines · Avi Kivity

Quoted 34 lines · Laurent Vivier (Bull)

Quoted 10 lines

Not being able to emulate is sometimes legitimate. In the case of writing to a write-protected guest page table, we simply un-write-protect it and go back to the guest (which should now execute the instruction natively).

Perhaps the logic that deals with this (the call to kvm_mmu_unprotect_page_virt() in emulate_instruction()) was broken by your changes.

In fact this case is managed in the error cases of emulate_instruction(). My first patch removes this management for instruction decoding because I supposed it cannot generate such errors. So what I proposed in my last email seems to be the good solution :

emulate_instruction()
...

        r = x86_decode_insn(&vcpu->emulate_ctxt, &emulate_ops);
        if (r == 0)
                r = x86_emulate_insn(&vcpu->emulate_ctxt,
&emulate_ops);
...
        if (r) {
                if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
                        return EMULATE_DONE;
                if (!vcpu->mmio_needed) {
                        kvm_report_emulation_failure(vcpu, "mmio");
                        return EMULATE_FAIL;
                }
                return EMULATE_DO_MMIO;
        }

...

Quoted 7 lines

Quoted 4 lines

Yes. But pushing the kvm_mmu_unprotect_page() to immediately after the decode stage may be better.

OK, but is this the only error case we can have in the decode stage ?

Decode can actually have fetch faults in smp (due to the instruction lengthening during decode, or due to the page tables changing with npt/ept).

I think these are the only two errors possible for decode: can't decode and can't fetch.

Quoted 3 lines

Should we remove it from after the emulate stage ?

Instruction execution shouldn't cause decode failures, so yes, that error shouldn't be emitted there.

But we can defer these fine tunings until later. Let's merge something that works first.