Re: [PATCH 0/5] Split the emulator: decode & execute

Avi Kivity
2007-09-18T05:59:57Z

Laurent Vivier wrote:
> Avi Kivity wrote:
>
>> Laurent Vivier (Bull) wrote:
>>
>>>> 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;
>>> }
>>> ...
>>>
>>>
>> 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.

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

(there are a couple of edge cases that can be fixed with the
decode/execute split, like a single read that crosses a page boundary
being split into two. I think Red Hat 7 doesn't work on kvm because it
issues a read that is partially in RAM and partially in mmio space).

--
Any sufficiently difficult bug is indistinguishable from a feature.