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

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

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.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/