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

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

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

[PATCH 1/5] x86_emulate-remove_unused, some cleanup: remove unused functions
[PATCH 2/5] x86_emulate-context, move all x86_emulate_memop() common variables
            between decode and execute to a structure decode_cache.

            struct decode_cache {
                u8 twobyte;
                u8 b;
                u8 lock_prefix;
                u8 rep_prefix;
                u8 op_bytes;
                u8 ad_bytes;
                struct operand src;
                struct operand dst;
                unsigned long *override_base;
                unsigned int d;
                unsigned long regs[NR_VCPU_REGS];
                unsigned long eip;
                /* modrm */
                u8 modrm;
                u8 modrm_mod;
                u8 modrm_reg;
                u8 modrm_rm;
                u8 use_modrm_ea;
                unsigned long modrm_ea;
                unsigned long modrm_val;
           };

[PATCH 3/5] x86_emulate-decode_insn, move all decoding process to function
            x86_decode_insn().
[PATCH 4/5] x86_emulate-split, emulate_instruction() calls now x86_decode_insn()
	    and x86_emulate_insn(). x86_emulate_insn() is x86_emulate_memop()
            without the decoding part.
[PATCH 5/5] x86_emulate-optimize, move emulate_ctxt to kvm_vcpu to keep emulate
            context when we exit from kvm module. Call x86_decode_insn() only
            when needed. Modify x86_emulate_insn() to not modify the context if
            it must be re-entered.

Signed-off-by: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

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

[Andi Kleen]

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.

-Andi

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

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

[Avi Kivity]

Andi Kleen wrote:
> 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.
>   

The two versions have already diverged too far for diff/apply style 
porting.  For logical porting (looking at a patch and writing a new one 
based on it) I think this patchset does not change things significantly.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

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

[Avi Kivity]

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

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.

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


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

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

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]

Avi Kivity wrote:
> 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.
>>
>>   
> 
> 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) ?

Regards,
Laurent
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

[Avi Kivity]

Laurent Vivier wrote:
> Avi Kivity wrote:
>   
>> 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.
>>>
>>>   
>>>       
>> 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) ?
>   

Running the split-emulator branch with 
'qemu/x86_64-softmmu/qemu-system-x86_64 /images/fc6.img -no-kvm-irqchip' 
I get

exception 14 (3)
rax 0000000000000000 rbx 0000000000000040 rcx 0000000000000180 rdx 
0000000000000005
rsi 00007fff128c50b0 rdi ffff8100078e8580 rsp ffff81000105fb60 rbp 
ffff8100008fc2c0
r8  0000000000000000 r9  0000000000000000 r10 0000000000000000 r11 
0000017f00000008
r12 0000000000000000 r13 0000000000000180 r14 00007fff128c50b0 r15 
0000000000000180
rip ffffffff8111769f rflags 00010206
cs 0010 (00000000/ffffffff p 1 dpl 0 db 0 s 1 type b l 1 g 1 avl 0)
ds 0000 (00000000/ffffffff p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
es 0000 (00000000/ffffffff p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
ss 0018 (00000000/ffffffff p 1 dpl 0 db 1 s 1 type 3 l 0 g 1 avl 0)
fs 0000 (2aaaab679710/ffffffff p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
gs 0000 (ffffffff813ec000/ffffffff p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
tr 0040 (ffff810001006000/0000206f p 1 dpl 0 db 0 s 0 type b l 0 g 0 avl 0)
ldt 0000 (00000000/ffffffff p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
gdt ffffffff81458000/80
idt ffffffff814bc000/fff
cr0 8005003b cr2 2aaaaaad33dc cr3 7934000 cr4 6e0 cr8 0 efer d01
Aborted

Running split-emulator~5 (i.e. with your patches backed out) is fine.  
Guest kernel is 2.6.22.2-42.fc6, x86_64.  Host kernel is kvm.git on the 
branch specified.

Running 'gdb vmlinux' on the guest kernel and disassebling the indicated 
rip may help in finding out which instruction is misemulated.


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


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

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

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 1361 bytes --]

Avi Kivity wrote:
> 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.
>>
>>   
> 
> 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.

I wrote:

      r = x86_decode_insn(&emulate_ctxt, &emulate_ops);
      if (r)
              return EMULATE_FAIL;

     vcpu->mmio_is_write = 0;
     vcpu->pio.string = 0;
     r = x86_emulate_insn(&emulate_ctxt, &emulate_ops);
     ...

It should be:

      vcpu->mmio_is_write = 0;
      vcpu->pio.string = 0;

      r = x86_decode_insn(&emulate_ctxt, &emulate_ops);
      if (r == 0) {
              r = x86_emulate_insn(&emulate_ctxt, &emulate_ops);
              if (vcpu->pio.string)
                      return EMULATE_DO_MMIO;
      }

      if ((r || vcpu->mmio_is_write) && run) {
      ...
      }

      if (r) {
      ...
      }

Laurent
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

[Avi Kivity]

Laurent Vivier wrote:
> Avi Kivity wrote:
>   
>> 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.
>>>
>>>   
>>>       
>> 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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


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

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

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 1436 bytes --]

Avi Kivity wrote:
> Laurent Vivier wrote:
>> Avi Kivity wrote:
>>   
>>> 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.
>>>>
>>>>   
>>>>       
>>> 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.

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

Some comments ?

Laurent
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

[Avi Kivity]

Laurent Vivier wrote:
> Avi Kivity wrote:
>   
>> Laurent Vivier wrote:
>>     
>>> Avi Kivity wrote:
>>>   
>>>       
>>>> 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.
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>> 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.
>>     
>
> 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").
>
>   

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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


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

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

[Laurent Vivier (Bull)]

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

Regards,
Laurent

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

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

[Avi Kivity]

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.

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

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

[Laurent Vivier]

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 ?
Should we remove it from after the emulate stage ?

Laurent

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

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

[Avi Kivity]

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/

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

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 2518 bytes --]

Avi Kivity wrote:
> 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.

Agree, I think it is better to merge something close to the original behavior
before improving it.

I try to post patches today.

Laurent
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel