use of saved_eip

use of saved_eip

[Kamble, Nitin A]

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

Hi Vivier, Avi,
  In order to debug faulures in my tree, I was looking at the saved_eip
changes coming from your commit. I did not understand the use of
saved_eip properly. like why is it used in the emulation of the pop
instruction. Can you please help me understand it's usage?
 
 
  
commit 5d9b36eec8ca6abe03da91efdfc7b5861525bd43
Author: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
Date:   Tue Sep 18 11:27:37 2007 +0200
 
    KVM: Call x86_decode_insn() only when needed
    
    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>
    Signed-off-by: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>

 
Thanks & Regards,
Nitin
Linux Open Source Technology Center, Intel Corporation
------------------------------------------------------------------------
--------
The Mind is like a parachute; it works much better when it's open.
 

[-- Attachment #1.2: Type: text/html, Size: 2800 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: use of saved_eip

[Laurent Vivier]

Kamble, Nitin A wrote:
> Hi Vivier, Avi,

Hi Nitin,
(BTW, my first name is Laurent)

>   In order to debug faulures in my tree, I was looking at the saved_eip 
> changes coming from your commit. I did not understand the use of 
> saved_eip properly. like why is it used in the emulation of the pop 
> instruction. Can you please help me understand it's usage?

in emulate_instruction(), we decode instructions and copy vcpu registers 
to ctxt (in x86_decode_insn()), then we really emulate the instruction 
(in x86_emulate_insn()).

In x86_emulate_insn(), if we have a REP prefix, we decrement ECX and set 
EIP to next instruction, then we try to emulate the instruction.
If the emulation fails (because this is a MMIO for instance) we have to 
restore the initial values of ECX and EIP because we will re-enter in 
x86_emulate_insn() once the IO has been managed by Qemu and thus ECX is 
decremented again and EIP set to next instruction again.

And you are right: _we_don't_have_to_do_that_for_the_pop_instruction_, 
it's a mistake because the REP prefix hasn't been processed at this 
level, it is managed (ECX and EIP are modified) later.

So, you can remove from pop_instruction:

1383                         if (c->rep_prefix) {
1384                                 c->regs[VCPU_REGS_RCX] = saved_rcx;
1385                                 c->eip = saved_eip;
1386                         }

Sorry for the inconvenience,

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: use of saved_eip

[Nitin A Kamble]

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

Hi Laurent,
  Sorry for calling by alst name. You 1st & last both names are totally
new to me. Are these french names?
  I understand your explanation. I was worried about code getting
misplaced due to automatic merges.
-- 
Thanks & Regards,
Nitin
Open Source Technology Center, Intel Corporation
-----------------------------------------------------------------
The mind is like a parachute; it works much better when it's open

On Wed, 2007-09-26 at 14:51 -0700, Laurent Vivier wrote:
> Kamble, Nitin A wrote:
> > Hi Vivier, Avi,
> 
> Hi Nitin,
> (BTW, my first name is Laurent)
> 
> >   In order to debug faulures in my tree, I was looking at the
> saved_eip
> > changes coming from your commit. I did not understand the use of
> > saved_eip properly. like why is it used in the emulation of the pop
> > instruction. Can you please help me understand it's usage?
> 
> in emulate_instruction(), we decode instructions and copy vcpu
> registers
> to ctxt (in x86_decode_insn()), then we really emulate the instruction
> (in x86_emulate_insn()).
> 
> In x86_emulate_insn(), if we have a REP prefix, we decrement ECX and
> set
> EIP to next instruction, then we try to emulate the instruction.
> If the emulation fails (because this is a MMIO for instance) we have
> to
> restore the initial values of ECX and EIP because we will re-enter in
> x86_emulate_insn() once the IO has been managed by Qemu and thus ECX
> is
> decremented again and EIP set to next instruction again.
> 
> And you are right: _we_don't_have_to_do_that_for_the_pop_instruction_,
> it's a mistake because the REP prefix hasn't been processed at this
> level, it is managed (ECX and EIP are modified) later.
> 
> So, you can remove from pop_instruction:
> 
> 1383                         if (c->rep_prefix) {
> 1384                                 c->regs[VCPU_REGS_RCX] =
> saved_rcx;
> 1385                                 c->eip = saved_eip;
> 1386                         }
> 
> Sorry for the inconvenience,
> 
> Laurent
> 
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- 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: use of saved_eip

[Laurent Vivier]

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

Nitin A Kamble wrote:
> Hi Laurent,
>   Sorry for calling by alst name. You 1st & last both names are totally
> new to me. Are these french names?

No problem. Yes, they are.

>   I understand your explanation. I was worried about code getting
> misplaced due to automatic merges.

In this case, it's not the fault of automatic merges but mine.

I'll post a patch to correct this.

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

[PATCH] On a pop instruction, don't restore ECX and EIP on error

[Laurent Vivier]

This patch corrects a mistake introduced by commit 5d9b36eec8ca6abe03da91efdfc7b5861525bd43
and reported by Nitin A Kamble.

The pop instruction restores ECX and EIP if read_std() fails and if we have a REP prefix,
but at this level ECX and EIP are not saved (and not modified). We don't have to restore it.

Signed-off-by: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
---
 drivers/kvm/x86_emulate.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 585cccf..1ad500c 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -1379,13 +1379,8 @@ special_insn:
 	pop_instruction:
 		if ((rc = ops->read_std(register_address(ctxt->ss_base,
 			c->regs[VCPU_REGS_RSP]), c->dst.ptr,
-			c->op_bytes, ctxt->vcpu)) != 0) {
-			if (c->rep_prefix) {
-				c->regs[VCPU_REGS_RCX] = saved_rcx;
-				c->eip = saved_eip;
-			}
+			c->op_bytes, ctxt->vcpu)) != 0)
 			goto done;
-		}
 
 		register_address_increment(c->regs[VCPU_REGS_RSP],
 					   c->op_bytes);
-- 
1.5.2.4


-------------------------------------------------------------------------
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] On a pop instruction, don't restore ECX and EIP on error

[Avi Kivity]

Laurent Vivier wrote:
> This patch corrects a mistake introduced by commit 5d9b36eec8ca6abe03da91efdfc7b5861525bd43
> and reported by Nitin A Kamble.
>
> The pop instruction restores ECX and EIP if read_std() fails and if we have a REP prefix,
> but at this level ECX and EIP are not saved (and not modified). We don't have to restore it.
>   

Applied, thanks.

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