KVM: x86 emulator: Discard CR2 in x86 emulator

From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
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.

drivers/kvm/x86_emulate.c +0 -1

 		/*
 		 * For instructions with a ModR/M byte, switch to register
 		 * access if Mod = 3.
 		 */
-		if ((c->d & ModRM) && c->modrm_mod == 3)

Quoted 7 lines · Sheng Yang

From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
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...

Quoted 6 lines

drivers/kvm/x86.c +0 -0

index aa6c3d8..85a0776 100644
@@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 

drivers/kvm/x86.c int emulate_instruction(struct kvm_vcpu *vcpu, · line 1296

Quoted 4 lines

drivers/kvm/x86.c +1 -1

 		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.

drivers/kvm/x86_emulate.c done_prefixes: · line 884

Quoted 11 lines

drivers/kvm/x86_emulate.c +2 -0

index c020010..95536a8 100644
@@ -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.

drivers/kvm/x86_emulate.c done_prefixes: · line 923

Quoted 9 lines

drivers/kvm/x86_emulate.c +1 -0

 		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.

Quoted 5 lines

drivers/kvm/x86_emulate.c +0 -0

 		/*
 		 * For instructions with a ModR/M byte, switch to register
 		 * access if Mod = 3.
 		 */

drivers/kvm/x86_emulate.c done_prefixes: · line 934

Quoted 10 lines

drivers/kvm/x86_emulate.c +6 -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.

Quoted 19 lines · On Thursday 15 November 2007 18:15:20 Avi Kivity wrote:

Quoted 8 lines · Sheng Yang

From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
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...

Quoted 6 lines

drivers/kvm/x86.c +0 -0

index aa6c3d8..85a0776 100644
@@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 

Quoted 7 lines

drivers/kvm/x86.c int emulate_instruction(struct kvm_vcpu *vcpu,

Quoted 5 lines

drivers/kvm/x86.c +1 -1

 		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.

thBut I am think of is it better to be in decode_cache?

Quoted 6 lines

Quoted 6 lines

drivers/kvm/x86_emulate.c +0 -0

index c020010..95536a8 100644
@@ -880,6 +880,8 @@ done_prefixes:
 			break;

Quoted 9 lines

drivers/kvm/x86_emulate.c done_prefixes:

Quoted 5 lines

drivers/kvm/x86_emulate.c +2 -0

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

Quoted 25 lines · Sheng Yang

Quoted 17 lines · On Thursday 15 November 2007 18:15:20 Avi Kivity wrote:

Quoted 7 lines · Sheng Yang

From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
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...

Quoted 6 lines

drivers/kvm/x86.c +0 -0

index aa6c3d8..85a0776 100644
@@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 

Quoted 6 lines

drivers/kvm/x86.c int emulate_instruction(struct kvm_vcpu *vcpu, · line 1296

Quoted 4 lines

drivers/kvm/x86.c +1 -1

 		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.

Quoted 2 lines

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.

Quoted 1 line

thBut I am think of is it better to be in decode_cache?

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

Quoted 15 lines

Quoted 14 lines

drivers/kvm/x86_emulate.c done_prefixes: · line 884

Quoted 10 lines

drivers/kvm/x86_emulate.c +2 -0

index c020010..95536a8 100644
@@ -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.

Quoted 3 lines

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?