Emulator speedups - avoid initializations where possible

RFC PATCH Superseded Bandan Das 2 participants 10 messages Apr 11, 2014 – Apr 14, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Bandan Das

author

2014-04-11 00:03 UTC #0 lore raw

v2 cover Emulator speedups - avoid initializations where possible

While initializing emulation context structure, kvm memsets to 0 a number of fields some of which are redundant since they get set eventually in x86_decode_insn. Cleanup unnecessary initializations and remove some fields.

This is on top of Paolo's RFC
KVM: x86: speedups for emulator memory accesses
https://lkml.org/lkml/2014/4/1/494

Here are the new realmode.flat numbers with improvement wrt unpatched kernel -

639 cycles/emulated jump instruction (4.3%) 776 cycles/emulated move instruction (7.5%) 791 cycles/emulated arithmetic instruction (11%) 943 cycles/emulated memory load instruction (5.2%) 948 cycles/emulated memory store instruction (7.6%) 929 cycles/emulated memory RMW instruction (9.0%)

v1 numbers -
639 cycles/emulated jump instruction
786 cycles/emulated move instruction
802 cycles/emulated arithmetic instruction
936 cycles/emulated memory load instruction
970 cycles/emulated memory store instruction
1000 cycles/emulated memory RMW instruction

v2:
All thanks and credit to Paolo!
- 1/6 - no change
- 2/6 - new patch, inercept and check_perm replaced with checks for bits in ctxt->d
- 3/6 - new patch, remove if condition in decode_rm and rearrange bit operations
- 4/6 - remove else conditions from v1 and misc cleanups
- 5/6 - new patch, remove seg_override and related fields and functions
- 6/6 - new patch, remove memopp and move rip_relative to a local variable in decode_modrm

Bandan Das (6):
KVM: emulate: move init_decode_cache to emulate.c
KVM: emulate: Remove ctxt->intercept and ctxt->check_perm checks
KVM: emulate: cleanup decode_rm
KVM: emulate: clean up initializations in init_decode_cache
KVM: emulate: rework seg_override
KVM: emulate: remove memopp and rip_relative

 arch/x86/include/asm/kvm_emulate.h | 20 ++++-----
 arch/x86/kvm/emulate.c             | 85 ++++++++++++++++++--------------------
 arch/x86/kvm/x86.c                 | 13 ------
 3 files changed, 50 insertions(+), 68 deletions(-)

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-04-11 00:03 UTC #1 lore raw

RFC PATCH v2 1/6 KVM: emulate: move init_decode_cache to emulate.c

Core emulator functions all belong in emulator.c, x86 should have no knowledge of emulator internals

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/include/asm/kvm_emulate.h |  1 +
 arch/x86/kvm/emulate.c             | 13 +++++++++++++
 arch/x86/kvm/x86.c                 | 13 -------------
 3 files changed, 14 insertions(+), 13 deletions(-)

arch/x86/include/asm/kvm_emulate.h +1 -0

index d085f73..ad4cca8 100644
@@ -436,6 +436,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);
 #define EMULATION_OK 0
 #define EMULATION_RESTART 1
 #define EMULATION_INTERCEPTED 2
+void init_decode_cache(struct x86_emulate_ctxt *ctxt);
 int x86_emulate_insn(struct x86_emulate_ctxt *ctxt);
 int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
 			 u16 tss_selector, int idt_index, int reason,

arch/x86/kvm/emulate.c +13 -0

index 8ca292c..8e2b866 100644
@@ -4550,6 +4550,19 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *))
 	return X86EMUL_CONTINUE;
 }
 
+void init_decode_cache(struct x86_emulate_ctxt *ctxt)
+{
+	memset(&ctxt->opcode_len, 0,
+	       (void *)&ctxt->_regs - (void *)&ctxt->opcode_len);
+
+	ctxt->fetch.start = 0;
+	ctxt->fetch.end = 0;
+	ctxt->io_read.pos = 0;
+	ctxt->io_read.end = 0;
+	ctxt->mem_read.pos = 0;
+	ctxt->mem_read.end = 0;
+}
+
 int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 {
 	const struct x86_emulate_ops *ops = ctxt->ops;

arch/x86/kvm/x86.c +0 ~~-13~~

index 4fad05d..122410d 100644
@@ -4951,19 +4951,6 @@ static void inject_emulated_exception(struct kvm_vcpu *vcpu)
 		kvm_queue_exception(vcpu, ctxt->exception.vector);
 }
 
-static void init_decode_cache(struct x86_emulate_ctxt *ctxt)
-{
-	memset(&ctxt->opcode_len, 0,
-	       (void *)&ctxt->_regs - (void *)&ctxt->opcode_len);
-
-	ctxt->fetch.start = 0;
-	ctxt->fetch.end = 0;
-	ctxt->io_read.pos = 0;
-	ctxt->io_read.end = 0;
-	ctxt->mem_read.pos = 0;
-	ctxt->mem_read.end = 0;
-}
-
 static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
 {
 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-04-11 00:03 UTC #2 lore raw

RFC PATCH v2 2/6 KVM: emulate: Remove ctxt->intercept and ctxt->check_perm checks

In reply to Bandan Das

The same information can be gleaned from ctxt->d and avoids having to zero/NULL initialize intercept and check_perm

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/kvm/emulate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/kvm/emulate.c +4 -4

index 8e2b866..a6d9892 100644
@@ -4615,7 +4615,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 				fetch_possible_mmx_operand(ctxt, &ctxt->dst);
 		}
 
-		if (unlikely(ctxt->guest_mode) && ctxt->intercept) {
+		if (unlikely(ctxt->guest_mode) && (ctxt->d & Intercept)) {
 			rc = emulator_check_intercept(ctxt, ctxt->intercept,
 						      X86_ICPT_PRE_EXCEPT);
 			if (rc != X86EMUL_CONTINUE)

@@ -4635,13 +4635,13 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 		}
 
 		/* Do instruction specific permission checks */
-		if (ctxt->check_perm) {
+		if (ctxt->d & CheckPerm) {
 			rc = ctxt->check_perm(ctxt);
 			if (rc != X86EMUL_CONTINUE)
 				goto done;
 		}
 
-		if (unlikely(ctxt->guest_mode) && ctxt->intercept) {
+		if (unlikely(ctxt->guest_mode) && (ctxt->d & Intercept)) {
 			rc = emulator_check_intercept(ctxt, ctxt->intercept,
 						      X86_ICPT_POST_EXCEPT);
 			if (rc != X86EMUL_CONTINUE)

@@ -4685,7 +4685,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 
 special_insn:
 
-	if (unlikely(ctxt->guest_mode) && ctxt->intercept) {
+	if (unlikely(ctxt->guest_mode) && (ctxt->d & Intercept)) {
 		rc = emulator_check_intercept(ctxt, ctxt->intercept,
 					      X86_ICPT_POST_MEMACCESS);
 		if (rc != X86EMUL_CONTINUE)

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-04-11 00:03 UTC #3 lore raw

RFC PATCH v2 3/6 KVM: emulate: cleanup decode_rm

In reply to Bandan Das

Remove the if conditional - that will help us avoid an "else initialize to 0" Also, rearrange operators for slightly better code.

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/kvm/emulate.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

arch/x86/kvm/emulate.c +7 -9

index a6d9892..9cbaba2 100644
@@ -1064,19 +1064,17 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
 			struct operand *op)
 {
 	u8 sib;
-	int index_reg = 0, base_reg = 0, scale;
+	int index_reg, base_reg, scale;
 	int rc = X86EMUL_CONTINUE;
 	ulong modrm_ea = 0;
 
-	if (ctxt->rex_prefix) {
-		ctxt->modrm_reg = (ctxt->rex_prefix & 4) << 1;	/* REX.R */
-		index_reg = (ctxt->rex_prefix & 2) << 2; /* REX.X */
-		ctxt->modrm_rm = base_reg = (ctxt->rex_prefix & 1) << 3; /* REG.B */
-	}
+	index_reg = (ctxt->rex_prefix << 2) & 8; /* REX.X */
+	base_reg = (ctxt->rex_prefix << 3) & 8; /* REG.B */
 
-	ctxt->modrm_mod |= (ctxt->modrm & 0xc0) >> 6;
-	ctxt->modrm_reg |= (ctxt->modrm & 0x38) >> 3;
-	ctxt->modrm_rm |= (ctxt->modrm & 0x07);
+	ctxt->modrm_mod = (ctxt->modrm & 0xc0) >> 6;
+	ctxt->modrm_reg = ((ctxt->rex_prefix << 1) & 8) | /* REX.R */
+		((ctxt->modrm & 0x38) >> 3);
+	ctxt->modrm_rm = base_reg | (ctxt->modrm & 0x07);
 	ctxt->modrm_seg = VCPU_SREG_DS;
 
 	if (ctxt->modrm_mod == 3) {

Signature · 1 line

1.8.3.1

Paolo Bonzini

maintainer

2014-04-13 12:51 UTC #4 lore raw

Re: [RFC PATCH v2 3/6] KVM: emulate: cleanup decode_rm

Il 10/04/2014 20:03, Bandan Das ha scritto:

arch/x86/kvm/emulate.c static int decode_modrm(struct x86_emulate_ctxt *ctxt, · line 1072

Quoted 7 lines

arch/x86/kvm/emulate.c +2 -5

-	if (ctxt->rex_prefix) {
-		ctxt->modrm_reg = (ctxt->rex_prefix & 4) << 1;	/* REX.R */
-		index_reg = (ctxt->rex_prefix & 2) << 2; /* REX.X */
-		ctxt->modrm_rm = base_reg = (ctxt->rex_prefix & 1) << 3; /* REG.B */
-	}
+	index_reg = (ctxt->rex_prefix << 2) & 8; /* REX.X */
+	base_reg = (ctxt->rex_prefix << 3) & 8; /* REG.B */

This is REX.B (preexisting typo), and please leave REX.R here too for clarity.

Also, the function is "decode_modrm", not "decode_rm" (in the commit message).

Otherwise the series seems okay from a somewhat cursory review.

Paolo

Quoted 10 lines

arch/x86/kvm/emulate.c +4 -3

-	ctxt->modrm_mod |= (ctxt->modrm & 0xc0) >> 6;
-	ctxt->modrm_reg |= (ctxt->modrm & 0x38) >> 3;
-	ctxt->modrm_rm |= (ctxt->modrm & 0x07);
+	ctxt->modrm_mod = (ctxt->modrm & 0xc0) >> 6;
+	ctxt->modrm_reg = ((ctxt->rex_prefix << 1) & 8) | /* REX.R */
+		((ctxt->modrm & 0x38) >> 3);
+	ctxt->modrm_rm = base_reg | (ctxt->modrm & 0x07);
 	ctxt->modrm_seg = VCPU_SREG_DS;

Bandan Das

author

2014-04-11 00:03 UTC #5 lore raw

RFC PATCH v2 4/6 KVM: emulate: clean up initializations in init_decode_cache

In reply to Bandan Das

A lot of initializations are unnecessary as they get set to appropriate values before actually being used. Optimize placement of fields in x86_emulate_ctxt

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/include/asm/kvm_emulate.h | 20 +++++++++++---------
 arch/x86/kvm/emulate.c             |  7 ++-----
 2 files changed, 13 insertions(+), 14 deletions(-)

arch/x86/include/asm/kvm_emulate.h +11 -9

index ad4cca8..d2ec45d 100644
@@ -315,30 +315,32 @@ struct x86_emulate_ctxt {
 	u8 opcode_len;
 	u8 b;
 	u8 intercept;
-	u8 lock_prefix;
-	u8 rep_prefix;
 	u8 op_bytes;
 	u8 ad_bytes;
-	u8 rex_prefix;
 	struct operand src;
 	struct operand src2;
 	struct operand dst;
-	bool has_seg_override;
-	u8 seg_override;
-	u64 d;
 	int (*execute)(struct x86_emulate_ctxt *ctxt);
 	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
+	bool has_seg_override;
+	bool rip_relative;
+	u8 rex_prefix;
+	u8 lock_prefix;
+	u8 rep_prefix;
+	u8 seg_override;
+	/* bitmaps of registers in _regs[] that can be read */
+	u32 regs_valid;
+	/* bitmaps of registers in _regs[] that have been written */
+	u32 regs_dirty;
 	/* modrm */
 	u8 modrm;
 	u8 modrm_mod;
 	u8 modrm_reg;
 	u8 modrm_rm;
 	u8 modrm_seg;
-	bool rip_relative;
+	u64 d;
 	unsigned long _eip;
 	struct operand memop;
-	u32 regs_valid;  /* bitmaps of registers in _regs[] that can be read */
-	u32 regs_dirty;  /* bitmaps of registers in _regs[] that have been written */
 	/* Fields above regs are cleared together. */
 	unsigned long _regs[NR_VCPU_REGS];
 	struct operand *memopp;

arch/x86/kvm/emulate.c +2 -5

index 9cbaba2..5517dd2 100644
@@ -4550,14 +4550,11 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *))
 
 void init_decode_cache(struct x86_emulate_ctxt *ctxt)
 {
-	memset(&ctxt->opcode_len, 0,
-	       (void *)&ctxt->_regs - (void *)&ctxt->opcode_len);
+	memset(&ctxt->has_seg_override, 0,
+	       (void *)&ctxt->modrm - (void *)&ctxt->has_seg_override);
 
-	ctxt->fetch.start = 0;
-	ctxt->fetch.end = 0;
 	ctxt->io_read.pos = 0;
 	ctxt->io_read.end = 0;
-	ctxt->mem_read.pos = 0;
 	ctxt->mem_read.end = 0;
 }

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-04-11 00:03 UTC #6 lore raw

RFC PATCH v2 5/6 KVM: emulate: rework seg_override

In reply to Bandan Das

x86_decode_insn already sets a default for seg_override, so remove it from the zeroed area. Also replace set/get functions with direct access to the field.

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/include/asm/kvm_emulate.h |  3 +--
 arch/x86/kvm/emulate.c             | 41 +++++++++++++++-----------------------
 2 files changed, 17 insertions(+), 27 deletions(-)

arch/x86/include/asm/kvm_emulate.h +1 -2

index d2ec45d..08a7696 100644
@@ -322,12 +322,10 @@ struct x86_emulate_ctxt {
 	struct operand dst;
 	int (*execute)(struct x86_emulate_ctxt *ctxt);
 	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
-	bool has_seg_override;
 	bool rip_relative;
 	u8 rex_prefix;
 	u8 lock_prefix;
 	u8 rep_prefix;
-	u8 seg_override;
 	/* bitmaps of registers in _regs[] that can be read */
 	u32 regs_valid;
 	/* bitmaps of registers in _regs[] that have been written */

@@ -338,6 +336,7 @@ struct x86_emulate_ctxt {
 	u8 modrm_reg;
 	u8 modrm_rm;
 	u8 modrm_seg;
+	u8 seg_override;
 	u64 d;
 	unsigned long _eip;
 	struct operand memop;

arch/x86/kvm/emulate.c +16 ~~-25~~

98 lines not on this page · raw message

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-04-11 00:03 UTC #7 lore raw

RFC PATCH v2 6/6 KVM: emulate: remove memopp and rip_relative

In reply to Bandan Das

Move typecast on "out of range value" of mem.ea to decode_modrm. rip_relative is only set in decode_modrm, change it to a local var

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/include/asm/kvm_emulate.h |  2 --
 arch/x86/kvm/emulate.c             | 18 +++++++-----------
 2 files changed, 7 insertions(+), 13 deletions(-)

arch/x86/include/asm/kvm_emulate.h +0 -2

index 08a7696..813254b 100644
@@ -322,7 +322,6 @@ struct x86_emulate_ctxt {
 	struct operand dst;
 	int (*execute)(struct x86_emulate_ctxt *ctxt);
 	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
-	bool rip_relative;
 	u8 rex_prefix;
 	u8 lock_prefix;
 	u8 rep_prefix;

@@ -342,7 +341,6 @@ struct x86_emulate_ctxt {
 	struct operand memop;
 	/* Fields above regs are cleared together. */
 	unsigned long _regs[NR_VCPU_REGS];
-	struct operand *memopp;
 	struct fetch_cache fetch;
 	struct read_cache io_read;
 	struct read_cache mem_read;

arch/x86/kvm/emulate.c +7 ~~-11~~

index b8affac..fcdf5b7 100644
@@ -1053,6 +1053,7 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
 	int index_reg, base_reg, scale;
 	int rc = X86EMUL_CONTINUE;
 	ulong modrm_ea = 0;
+	bool rip_relative = false;
 
 	index_reg = (ctxt->rex_prefix << 2) & 8; /* REX.X */
 	base_reg = (ctxt->rex_prefix << 3) & 8; /* REG.B */

@@ -1155,7 +1156,7 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
 				modrm_ea += reg_read(ctxt, index_reg) << scale;
 		} else if ((ctxt->modrm_rm & 7) == 5 && ctxt->modrm_mod == 0) {
 			if (ctxt->mode == X86EMUL_MODE_PROT64)
-				ctxt->rip_relative = 1;
+				rip_relative = true;
 		} else {
 			base_reg = ctxt->modrm_rm;
 			modrm_ea += reg_read(ctxt, base_reg);

@@ -1175,6 +1176,9 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
 		}
 	}
 	op->addr.mem.ea = modrm_ea;
+	ctxt->memop.addr.mem.ea = (u32)ctxt->memop.addr.mem.ea;
+	if (rip_relative)
+		ctxt->memop.addr.mem.ea += ctxt->_eip;
 done:
 	return rc;
 }

@@ -4088,7 +4092,6 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
 		ctxt->memop.bytes = (ctxt->d & ByteOp) ? 1 : ctxt->op_bytes;
 	mem_common:
 		*op = ctxt->memop;
-		ctxt->memopp = op;
 		if (ctxt->d & BitOp)
 			fetch_bit_operand(ctxt);
 		op->orig_val = op->val;

@@ -4240,7 +4243,6 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 	struct opcode opcode;
 
 	ctxt->memop.type = OP_NONE;
-	ctxt->memopp = NULL;
 	ctxt->_eip = ctxt->eip;
 	ctxt->fetch.start = ctxt->_eip;
 	ctxt->fetch.end = ctxt->fetch.start + insn_len;

@@ -4441,9 +4443,6 @@ done_prefixes:
 
 	ctxt->memop.addr.mem.seg = ctxt->seg_override;
 
-	if (ctxt->memop.type == OP_MEM && ctxt->ad_bytes != 8)
-		ctxt->memop.addr.mem.ea = (u32)ctxt->memop.addr.mem.ea;
-
 	/*
 	 * Decode and fetch the source operand: register, memory
 	 * or immediate.

@@ -4464,9 +4463,6 @@ done_prefixes:
 	rc = decode_operand(ctxt, &ctxt->dst, (ctxt->d >> DstShift) & OpMask);
 
 done:
-	if (ctxt->memopp && ctxt->memopp->type == OP_MEM && ctxt->rip_relative)
-		ctxt->memopp->addr.mem.ea += ctxt->_eip;
-
 	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
 }

@@ -4541,8 +4537,8 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *))
 
 void init_decode_cache(struct x86_emulate_ctxt *ctxt)
 {
-	memset(&ctxt->rip_relative, 0,
-	       (void *)&ctxt->modrm - (void *)&ctxt->rip_relative);
+	memset(&ctxt->rex_prefix, 0,
+	       (void *)&ctxt->modrm - (void *)&ctxt->rex_prefix);
 
 	ctxt->io_read.pos = 0;
 	ctxt->io_read.end = 0;

Signature · 1 line

1.8.3.1

Paolo Bonzini

maintainer

2014-04-13 12:48 UTC #8 lore raw

Re: [RFC PATCH v2 6/6] KVM: emulate: remove memopp and rip_relative

Il 10/04/2014 20:03, Bandan Das ha scritto:

Quoted 1 line

/* Fields above regs are cleared together. */

This comment is not accurate anymore after patch 4. Since you're fixing it, please add another comment saying where the cleared fields start, too.

arch/x86/kvm/emulate.c static int decode_modrm(struct x86_emulate_ctxt *ctxt, · line 1179

Quoted 1 line

arch/x86/kvm/emulate.c +1 -0
+	ctxt->memop.addr.mem.ea = (u32)ctxt->memop.addr.mem.ea;

This is missing "if (ctxt->ad_bytes != 8)".

arch/x86/kvm/emulate.c static int decode_modrm(struct x86_emulate_ctxt *ctxt, · line 1181

Quoted 2 lines

arch/x86/kvm/emulate.c +2 -0

+	if (rip_relative)
+		ctxt->memop.addr.mem.ea += ctxt->_eip;

Paolo

Bandan Das

author

2014-04-14 14:01 UTC #9 lore raw

Re: [RFC PATCH v2 6/6] KVM: emulate: remove memopp and rip_relative

Quoted 6 lines · Paolo Bonzini

Il 10/04/2014 20:03, Bandan Das ha scritto:
Quoted 1 line
/* Fields above regs are cleared together. */
This comment is not accurate anymore after patch 4. Since you're fixing it, please add another comment saying where the cleared fields start, too.

Oops, I forgot to change the comment appropriately. Will fix in next version.

Bandan

Quoted 8 lines

arch/x86/kvm/emulate.c static int decode_modrm(struct x86_emulate_ctxt *ctxt, · line 1179
Quoted 1 line
arch/x86/kvm/emulate.c +1 -0
+	ctxt->memop.addr.mem.ea = (u32)ctxt->memop.addr.mem.ea;
This is missing "if (ctxt->ad_bytes != 8)".
arch/x86/kvm/emulate.c static int decode_modrm(struct x86_emulate_ctxt *ctxt, · line 1181
Quoted 2 lines
arch/x86/kvm/emulate.c +2 -0
+	if (rip_relative)
+		ctxt->memop.addr.mem.ea += ctxt->_eip;
Paolo