[PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

[PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

[Sheng Yang]

[-- Attachment #1: Type: text/plain, Size: 5137 bytes --]

From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Thu, 15 Nov 2007 15:11:58 +0800
Subject: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

For CR2 is unreliable and unavailable in many condition, this patch
completely decode memory operand instead of using CR2 in x86 emulator.

Signed-off-by: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/kvm/x86.c         |    2 +-
 drivers/kvm/x86_emulate.c |   33 +++++++++++++++++++++------------
 drivers/kvm/x86_emulate.h |    2 +-
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
index aa6c3d8..85a0776 100644
--- a/drivers/kvm/x86.c
+++ b/drivers/kvm/x86.c
@@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 
 		vcpu->emulate_ctxt.vcpu = vcpu;
 		vcpu->emulate_ctxt.eflags = kvm_x86_ops->get_rflags(vcpu);
-		vcpu->emulate_ctxt.cr2 = cr2;
+		vcpu->emulate_ctxt.memop = 0;
 		vcpu->emulate_ctxt.mode =
 			(vcpu->emulate_ctxt.eflags & X86_EFLAGS_VM)
 			? X86EMUL_MODE_REAL : cs_l
diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index c020010..95536a8 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -880,6 +880,8 @@ done_prefixes:
 			break;
 		}
 		c->src.type = OP_MEM;
+		ctxt->memop = insn_fetch(u32, c->src.bytes, c->eip);
+		c->eip -= c->src.bytes; /* keep the page fault ip */
 		break;
 	case SrcImm:
 		c->src.type = OP_IMM;
@@ -918,14 +920,18 @@ done_prefixes:
 			 c->twobyte && (c->b == 0xb6 || c->b == 0xb7));
 		break;
 	case DstMem:
+		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
 		/*
 		 * For instructions with a ModR/M byte, switch to register
 		 * access if Mod = 3.
 		 */
-		if ((c->d & ModRM) && c->modrm_mod == 3)
+		if ((c->d & ModRM) && c->modrm_mod == 3) {
 			c->dst.type = OP_REG;
-		else
-			c->dst.type = OP_MEM;
+			break;
+		}
+		c->dst.type = OP_MEM;
+		ctxt->memop = insn_fetch(u32, c->dst.bytes, c->eip);
+		c->eip -= c->dst.bytes; /* keep the page fault ip */
 		break;
 	}
 
@@ -1091,13 +1097,13 @@ static inline int emulate_grp45(struct 
x86_emulate_ctxt *ctxt,
 
 static inline int emulate_grp9(struct x86_emulate_ctxt *ctxt,
 			       struct x86_emulate_ops *ops,
-			       unsigned long cr2)
+			       unsigned long memop)
 {
 	struct decode_cache *c = &ctxt->decode;
 	u64 old, new;
 	int rc;
 
-	rc = ops->read_emulated(cr2, &old, 8, ctxt->vcpu);
+	rc = ops->read_emulated(memop, &old, 8, ctxt->vcpu);
 	if (rc != 0)
 		return rc;
 
@@ -1112,7 +1118,7 @@ static inline int emulate_grp9(struct x86_emulate_ctxt 
*ctxt,
 		new = ((u64)c->regs[VCPU_REGS_RCX] << 32) |
 		       (u32) c->regs[VCPU_REGS_RBX];
 
-		rc = ops->cmpxchg_emulated(cr2, &old, &new, 8, ctxt->vcpu);
+		rc = ops->cmpxchg_emulated(memop, &old, &new, 8, ctxt->vcpu);
 		if (rc != 0)
 			return rc;
 		ctxt->eflags |= EFLG_ZF;
@@ -1175,7 +1181,7 @@ static inline int writeback(struct x86_emulate_ctxt 
*ctxt,
 int
 x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 {
-	unsigned long cr2 = ctxt->cr2;
+	unsigned long memop;
 	u64 msr_data;
 	unsigned long saved_eip;
 	struct decode_cache *c = &ctxt->decode;
@@ -1190,10 +1196,13 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct 
x86_emulate_ops *ops)
 	saved_eip = c->eip;
 
 	if (((c->d & ModRM) && (c->modrm_mod != 3)) || (c->d & MemAbs))
-		cr2 = c->modrm_ea;
+		memop = c->modrm_ea;
+	else if ((c->d & DstMask) != ImplicitOps)
+		memop = ctxt->memop;
+	else memop = 0;
 
 	if (c->src.type == OP_MEM) {
-		c->src.ptr = (unsigned long *)cr2;
+		c->src.ptr = (unsigned long *)memop;
 		c->src.val = 0;
 		rc = ops->read_emulated((unsigned long)c->src.ptr,
 					&c->src.val,
@@ -1209,7 +1218,7 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct 
x86_emulate_ops *ops)
 
 
 	if (c->dst.type == OP_MEM) {
-		c->dst.ptr = (unsigned long *)cr2;
+		c->dst.ptr = (unsigned long *)memop;
 		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
 		c->dst.val = 0;
 		if (c->d & BitOp) {
@@ -1648,7 +1657,7 @@ twobyte_insn:
 						  &ctxt->eflags);
 			break;
 		case 7: /* invlpg*/
-			emulate_invlpg(ctxt->vcpu, cr2);
+			emulate_invlpg(ctxt->vcpu, memop);
 			break;
 		default:
 			goto cannot_emulate;
@@ -1819,7 +1828,7 @@ twobyte_special_insn:
 		break;
 	}
 	case 0xc7:		/* Grp9 (cmpxchg8b) */
-		rc = emulate_grp9(ctxt, ops, cr2);
+		rc = emulate_grp9(ctxt, ops, memop);
 		if (rc != 0)
 			goto done;
 		break;
diff --git a/drivers/kvm/x86_emulate.h b/drivers/kvm/x86_emulate.h
index e34868b..5aacc76 100644
--- a/drivers/kvm/x86_emulate.h
+++ b/drivers/kvm/x86_emulate.h
@@ -149,7 +149,7 @@ struct x86_emulate_ctxt {
 
 	/* Linear faulting address (if emulating a page-faulting instruction). */
 	unsigned long eflags;
-	unsigned long cr2;
+	unsigned long memop;
 
 	/* Emulated execution mode, represented by an X86EMUL_MODE value. */
 	int mode;
-- 
1.5.2


[-- Attachment #2: 0002-KVM-x86-emulator-Discard-CR2-in-x86-emulator.patch --]
[-- Type: text/x-diff, Size: 5127 bytes --]

From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Thu, 15 Nov 2007 15:11:58 +0800
Subject: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

For CR2 is unreliable and unavailable in many condition, this patch
completely decode memory operand instead of using CR2 in x86 emulator.

Signed-off-by: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/kvm/x86.c         |    2 +-
 drivers/kvm/x86_emulate.c |   33 +++++++++++++++++++++------------
 drivers/kvm/x86_emulate.h |    2 +-
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
index aa6c3d8..85a0776 100644
--- a/drivers/kvm/x86.c
+++ b/drivers/kvm/x86.c
@@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 
 		vcpu->emulate_ctxt.vcpu = vcpu;
 		vcpu->emulate_ctxt.eflags = kvm_x86_ops->get_rflags(vcpu);
-		vcpu->emulate_ctxt.cr2 = cr2;
+		vcpu->emulate_ctxt.memop = 0;
 		vcpu->emulate_ctxt.mode =
 			(vcpu->emulate_ctxt.eflags & X86_EFLAGS_VM)
 			? X86EMUL_MODE_REAL : cs_l
diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index c020010..95536a8 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -880,6 +880,8 @@ done_prefixes:
 			break;
 		}
 		c->src.type = OP_MEM;
+		ctxt->memop = insn_fetch(u32, c->src.bytes, c->eip);
+		c->eip -= c->src.bytes; /* keep the page fault ip */
 		break;
 	case SrcImm:
 		c->src.type = OP_IMM;
@@ -918,14 +920,18 @@ done_prefixes:
 			 c->twobyte && (c->b == 0xb6 || c->b == 0xb7));
 		break;
 	case DstMem:
+		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
 		/*
 		 * For instructions with a ModR/M byte, switch to register
 		 * access if Mod = 3.
 		 */
-		if ((c->d & ModRM) && c->modrm_mod == 3)
+		if ((c->d & ModRM) && c->modrm_mod == 3) {
 			c->dst.type = OP_REG;
-		else
-			c->dst.type = OP_MEM;
+			break;
+		}
+		c->dst.type = OP_MEM;
+		ctxt->memop = insn_fetch(u32, c->dst.bytes, c->eip);
+		c->eip -= c->dst.bytes; /* keep the page fault ip */
 		break;
 	}
 
@@ -1091,13 +1097,13 @@ static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
 
 static inline int emulate_grp9(struct x86_emulate_ctxt *ctxt,
 			       struct x86_emulate_ops *ops,
-			       unsigned long cr2)
+			       unsigned long memop)
 {
 	struct decode_cache *c = &ctxt->decode;
 	u64 old, new;
 	int rc;
 
-	rc = ops->read_emulated(cr2, &old, 8, ctxt->vcpu);
+	rc = ops->read_emulated(memop, &old, 8, ctxt->vcpu);
 	if (rc != 0)
 		return rc;
 
@@ -1112,7 +1118,7 @@ static inline int emulate_grp9(struct x86_emulate_ctxt *ctxt,
 		new = ((u64)c->regs[VCPU_REGS_RCX] << 32) |
 		       (u32) c->regs[VCPU_REGS_RBX];
 
-		rc = ops->cmpxchg_emulated(cr2, &old, &new, 8, ctxt->vcpu);
+		rc = ops->cmpxchg_emulated(memop, &old, &new, 8, ctxt->vcpu);
 		if (rc != 0)
 			return rc;
 		ctxt->eflags |= EFLG_ZF;
@@ -1175,7 +1181,7 @@ static inline int writeback(struct x86_emulate_ctxt *ctxt,
 int
 x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 {
-	unsigned long cr2 = ctxt->cr2;
+	unsigned long memop;
 	u64 msr_data;
 	unsigned long saved_eip;
 	struct decode_cache *c = &ctxt->decode;
@@ -1190,10 +1196,13 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	saved_eip = c->eip;
 
 	if (((c->d & ModRM) && (c->modrm_mod != 3)) || (c->d & MemAbs))
-		cr2 = c->modrm_ea;
+		memop = c->modrm_ea;
+	else if ((c->d & DstMask) != ImplicitOps)
+		memop = ctxt->memop;
+	else memop = 0;
 
 	if (c->src.type == OP_MEM) {
-		c->src.ptr = (unsigned long *)cr2;
+		c->src.ptr = (unsigned long *)memop;
 		c->src.val = 0;
 		rc = ops->read_emulated((unsigned long)c->src.ptr,
 					&c->src.val,
@@ -1209,7 +1218,7 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 
 
 	if (c->dst.type == OP_MEM) {
-		c->dst.ptr = (unsigned long *)cr2;
+		c->dst.ptr = (unsigned long *)memop;
 		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
 		c->dst.val = 0;
 		if (c->d & BitOp) {
@@ -1648,7 +1657,7 @@ twobyte_insn:
 						  &ctxt->eflags);
 			break;
 		case 7: /* invlpg*/
-			emulate_invlpg(ctxt->vcpu, cr2);
+			emulate_invlpg(ctxt->vcpu, memop);
 			break;
 		default:
 			goto cannot_emulate;
@@ -1819,7 +1828,7 @@ twobyte_special_insn:
 		break;
 	}
 	case 0xc7:		/* Grp9 (cmpxchg8b) */
-		rc = emulate_grp9(ctxt, ops, cr2);
+		rc = emulate_grp9(ctxt, ops, memop);
 		if (rc != 0)
 			goto done;
 		break;
diff --git a/drivers/kvm/x86_emulate.h b/drivers/kvm/x86_emulate.h
index e34868b..5aacc76 100644
--- a/drivers/kvm/x86_emulate.h
+++ b/drivers/kvm/x86_emulate.h
@@ -149,7 +149,7 @@ struct x86_emulate_ctxt {
 
 	/* Linear faulting address (if emulating a page-faulting instruction). */
 	unsigned long eflags;
-	unsigned long cr2;
+	unsigned long memop;
 
 	/* Emulated execution mode, represented by an X86EMUL_MODE value. */
 	int mode;
-- 
1.5.2


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

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

[-- Attachment #4: 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 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

[Avi Kivity]

Sheng Yang wrote:
> From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Thu, 15 Nov 2007 15:11:58 +0800
> Subject: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator
>
> For CR2 is unreliable and unavailable in many condition, this patch
> completely decode memory operand instead of using CR2 in x86 emulator.
>
>
>   

One of my innermost wishes...

> diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
> index aa6c3d8..85a0776 100644
> --- a/drivers/kvm/x86.c
> +++ b/drivers/kvm/x86.c
> @@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>  
>  		vcpu->emulate_ctxt.vcpu = vcpu;
>  		vcpu->emulate_ctxt.eflags = kvm_x86_ops->get_rflags(vcpu);
> -		vcpu->emulate_ctxt.cr2 = cr2;
> +		vcpu->emulate_ctxt.memop = 0;
>   

We have c->modrm_ea which can be used for the memory operand.

> diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
> index c020010..95536a8 100644
> --- a/drivers/kvm/x86_emulate.c
> +++ b/drivers/kvm/x86_emulate.c
> @@ -880,6 +880,8 @@ done_prefixes:
>  			break;
>  		}
>  		c->src.type = OP_MEM;
> +		ctxt->memop = insn_fetch(u32, c->src.bytes, c->eip);
> +		c->eip -= c->src.bytes; /* keep the page fault ip */
>   

I don't understand this.  In the cases where the memory operand address 
is encoded in the instruction, we fetch it explicity.  When it isn't, 
this is broken.

>  		break;
>  	case SrcImm:
>  		c->src.type = OP_IMM;
> @@ -918,14 +920,18 @@ done_prefixes:
>  			 c->twobyte && (c->b == 0xb6 || c->b == 0xb7));
>  		break;
>  	case DstMem:
> +		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
>   

This seems unrelated; needs a separate patch.  Please check that movzx 
and movsx aren't affected by this.

>  		/*
>  		 * For instructions with a ModR/M byte, switch to register
>  		 * access if Mod = 3.
>  		 */
> -		if ((c->d & ModRM) && c->modrm_mod == 3)
> +		if ((c->d & ModRM) && c->modrm_mod == 3) {
>  			c->dst.type = OP_REG;
> -		else
> -			c->dst.type = OP_MEM;
> +			break;
> +		}
> +		c->dst.type = OP_MEM;
> +		ctxt->memop = insn_fetch(u32, c->dst.bytes, c->eip);
> +		c->eip -= c->dst.bytes; /* keep the page fault ip */
>   

Ditto.

 	


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


-------------------------------------------------------------------------
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 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

[Sheng Yang]

On Thursday 15 November 2007 18:15:20 Avi Kivity wrote:
> Sheng Yang wrote:
> > From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
> > From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Date: Thu, 15 Nov 2007 15:11:58 +0800
> > Subject: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator
> >
> > For CR2 is unreliable and unavailable in many condition, this patch
> > completely decode memory operand instead of using CR2 in x86 emulator.
>
> One of my innermost wishes...
>
> > diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
> > index aa6c3d8..85a0776 100644
> > --- a/drivers/kvm/x86.c
> > +++ b/drivers/kvm/x86.c
> > @@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
> >
> >  		vcpu->emulate_ctxt.vcpu = vcpu;
> >  		vcpu->emulate_ctxt.eflags = kvm_x86_ops->get_rflags(vcpu);
> > -		vcpu->emulate_ctxt.cr2 = cr2;
> > +		vcpu->emulate_ctxt.memop = 0;
>
> We have c->modrm_ea which can be used for the memory operand.

I don't think using the name modrm_ea is good for explicit encoding, so I add 
this. But I am think of is it better to be in decode_cache?

>
> > diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
> > index c020010..95536a8 100644
> > --- a/drivers/kvm/x86_emulate.c
> > +++ b/drivers/kvm/x86_emulate.c
> > @@ -880,6 +880,8 @@ done_prefixes:
> >  			break;
> >  		}
> >  		c->src.type = OP_MEM;
> > +		ctxt->memop = insn_fetch(u32, c->src.bytes, c->eip);
> > +		c->eip -= c->src.bytes; /* keep the page fault ip */
>
> I don't understand this.  In the cases where the memory operand address
> is encoded in the instruction, we fetch it explicity.  When it isn't,
> this is broken.

But we mark implicit encoding instructions as "ImplicitOps", so only explicit 
ones should get here. And my former patch deal with the implicit ones, and 
modrm_ea has priority to memop, so I think it's OK.

Maybe add  a judgment of modrm_ea here is better.

>
> >  		break;
> >  	case SrcImm:
> >  		c->src.type = OP_IMM;
> > @@ -918,14 +920,18 @@ done_prefixes:
> >  			 c->twobyte && (c->b == 0xb6 || c->b == 0xb7));
> >  		break;
> >  	case DstMem:
> > +		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
>
> This seems unrelated; needs a separate patch.  Please check that movzx
> and movsx aren't affected by this.

I need this for following insn_fetch(). And movzx/movsx is marked as DstReg, 
so they won't get here.

>
> >  		/*
> >  		 * For instructions with a ModR/M byte, switch to register
> >  		 * access if Mod = 3.
> >  		 */
> > -		if ((c->d & ModRM) && c->modrm_mod == 3)
> > +		if ((c->d & ModRM) && c->modrm_mod == 3) {
> >  			c->dst.type = OP_REG;
> > -		else
> > -			c->dst.type = OP_MEM;
> > +			break;
> > +		}
> > +		c->dst.type = OP_MEM;
> > +		ctxt->memop = insn_fetch(u32, c->dst.bytes, c->eip);
> > +		c->eip -= c->dst.bytes; /* keep the page fault ip */
>
> Ditto.



-- 
Thanks
Yang, Sheng

-------------------------------------------------------------------------
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 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

[Avi Kivity]

Sheng Yang wrote:
> On Thursday 15 November 2007 18:15:20 Avi Kivity wrote:
>   
>> Sheng Yang wrote:
>>     
>>> From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
>>> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> Date: Thu, 15 Nov 2007 15:11:58 +0800
>>> Subject: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator
>>>
>>> For CR2 is unreliable and unavailable in many condition, this patch
>>> completely decode memory operand instead of using CR2 in x86 emulator.
>>>       
>> One of my innermost wishes...
>>
>>     
>>> diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
>>> index aa6c3d8..85a0776 100644
>>> --- a/drivers/kvm/x86.c
>>> +++ b/drivers/kvm/x86.c
>>> @@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>>>
>>>  		vcpu->emulate_ctxt.vcpu = vcpu;
>>>  		vcpu->emulate_ctxt.eflags = kvm_x86_ops->get_rflags(vcpu);
>>> -		vcpu->emulate_ctxt.cr2 = cr2;
>>> +		vcpu->emulate_ctxt.memop = 0;
>>>       
>> We have c->modrm_ea which can be used for the memory operand.
>>     
>
> I don't think using the name modrm_ea is good for explicit encoding, so I add 
> this. 

I agree the name isn't good (we already use it for MemAbs decoding, 
too).  We can rename it later.
> thBut I am think of is it better to be in decode_cache?
>
>   

c-> is the decode cache.  Maybe I misunderstood you?

>>> diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
>>> index c020010..95536a8 100644
>>> --- a/drivers/kvm/x86_emulate.c
>>> +++ b/drivers/kvm/x86_emulate.c
>>> @@ -880,6 +880,8 @@ done_prefixes:
>>>  			break;
>>>  		}
>>>  		c->src.type = OP_MEM;
>>> +		ctxt->memop = insn_fetch(u32, c->src.bytes, c->eip);
>>> +		c->eip -= c->src.bytes; /* keep the page fault ip */
>>>       
>> I don't understand this.  In the cases where the memory operand address
>> is encoded in the instruction, we fetch it explicity.  When it isn't,
>> this is broken.
>>     
>
> But we mark implicit encoding instructions as "ImplicitOps", so only explicit 
> ones should get here. And my former patch deal with the implicit ones, and 
> modrm_ea has priority to memop, so I think it's OK.
>   

I still don't understand.  Which instruction benefits from this change?  
And shouldn't the be marked MemAbs instead?



-- 
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 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

[Sheng Yang]

On Friday 16 November 2007 14:41:50 Avi Kivity wrote:
> Sheng Yang wrote:
> > On Thursday 15 November 2007 18:15:20 Avi Kivity wrote:
> >> Sheng Yang wrote:
> >>> From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
> >>> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>> Date: Thu, 15 Nov 2007 15:11:58 +0800
> >>> Subject: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator
> >>>
> >>> For CR2 is unreliable and unavailable in many condition, this patch
> >>> completely decode memory operand instead of using CR2 in x86 emulator.
> >>
> >> One of my innermost wishes...
> >>
> >>> diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
> >>> index aa6c3d8..85a0776 100644
> >>> --- a/drivers/kvm/x86.c
> >>> +++ b/drivers/kvm/x86.c
> >>> @@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
> >>>
> >>>  		vcpu->emulate_ctxt.vcpu = vcpu;
> >>>  		vcpu->emulate_ctxt.eflags = kvm_x86_ops->get_rflags(vcpu);
> >>> -		vcpu->emulate_ctxt.cr2 = cr2;
> >>> +		vcpu->emulate_ctxt.memop = 0;
> >>
> >> We have c->modrm_ea which can be used for the memory operand.
> >
> > I don't think using the name modrm_ea is good for explicit encoding, so I
> > add this.
>
> I agree the name isn't good (we already use it for MemAbs decoding,
> too).  We can rename it later.
>
> > thBut I am think of is it better to be in decode_cache?
>
> c-> is the decode cache.  Maybe I misunderstood you?
>
> >>> diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
> >>> index c020010..95536a8 100644
> >>> --- a/drivers/kvm/x86_emulate.c
> >>> +++ b/drivers/kvm/x86_emulate.c
> >>> @@ -880,6 +880,8 @@ done_prefixes:
> >>>  			break;
> >>>  		}
> >>>  		c->src.type = OP_MEM;
> >>> +		ctxt->memop = insn_fetch(u32, c->src.bytes, c->eip);
> >>> +		c->eip -= c->src.bytes; /* keep the page fault ip */
> >>
> >> I don't understand this.  In the cases where the memory operand address
> >> is encoded in the instruction, we fetch it explicity.  When it isn't,
> >> this is broken.
> >
> > But we mark implicit encoding instructions as "ImplicitOps", so only
> > explicit ones should get here. And my former patch deal with the implicit
> > ones, and modrm_ea has priority to memop, so I think it's OK.
>
> I still don't understand.  Which instruction benefits from this change?
> And shouldn't the be marked MemAbs instead?

Yes, your are right. I found I made a wrong assumption. 

I will send the modified patch later, thx. 

-- 
Thanks
Yang, Sheng

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