[PATCH 0/4] KVM: x86 emulator: Split decoder into separate functions

[PATCH 0/4] KVM: x86 emulator: Split decoder into separate functions

[Takuya Yoshikawa]

Takuya Yoshikawa (4):
  KVM: x86 emulator: Introduce ctxt->op_prefix for 0x66 prefix
  KVM: x86 emulator: Make prefix decoding a separate function
  KVM: x86 emulator: Make opcode decoding a separate function
  KVM: x86 emulator: Avoid pushing back ModRM byte in decode_opcode()

 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |  124 ++++++++++++++++++++++++------------
 2 files changed, 84 insertions(+), 41 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] KVM: x86 emulator: Introduce ctxt->op_prefix for 0x66 prefix

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

This is needed to make prefix decoding a separate function.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Cc: Takuya Yoshikawa <takuya.yoshikawa@gmail.com>
---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |    7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 1ac46c22..75783a7 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -261,6 +261,7 @@ struct x86_emulate_ctxt {
 	u8 intercept;
 	u8 lock_prefix;
 	u8 rep_prefix;
+	u8 op_prefix;
 	u8 op_bytes;
 	u8 ad_bytes;
 	u8 rex_prefix;
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index d5729a9..5a49290 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3879,7 +3879,6 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 	int rc = X86EMUL_CONTINUE;
 	int mode = ctxt->mode;
 	int def_op_bytes, def_ad_bytes, goffset, simd_prefix;
-	bool op_prefix = false;
 	struct opcode opcode;
 
 	ctxt->memop.type = OP_NONE;
@@ -3916,7 +3915,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 	for (;;) {
 		switch (ctxt->b = insn_fetch(u8, ctxt)) {
 		case 0x66:	/* operand-size override */
-			op_prefix = true;
+			ctxt->op_prefix = ctxt->b;
 			/* switch between 2/4 bytes */
 			ctxt->op_bytes = def_op_bytes ^ 6;
 			break;
@@ -3997,9 +3996,9 @@ done_prefixes:
 			opcode = opcode.u.group[goffset];
 			break;
 		case Prefix:
-			if (ctxt->rep_prefix && op_prefix)
+			if (ctxt->rep_prefix && ctxt->op_prefix)
 				return EMULATION_FAILED;
-			simd_prefix = op_prefix ? 0x66 : ctxt->rep_prefix;
+			simd_prefix = ctxt->rep_prefix | ctxt->op_prefix;
 			switch (simd_prefix) {
 			case 0x00: opcode = opcode.u.gprefix->pfx_no; break;
 			case 0x66: opcode = opcode.u.gprefix->pfx_66; break;
-- 
1.7.5.4

[PATCH 2/4] KVM: x86 emulator: Make prefix decoding a separate function

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Instruction decoding can be split into separate parts, and this is the
first one which treats the instruction prefixes.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Cc: Takuya Yoshikawa <takuya.yoshikawa@gmail.com>
---
 arch/x86/kvm/emulate.c |   58 +++++++++++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 5a49290..b22238b 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3874,22 +3874,21 @@ done:
 	return rc;
 }
 
-int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
+/**
+ * decode_prefixes - decode instruction prefixes
+ * @ctxt: emulation context
+ *
+ * Fetches instruction prefixes and sets some ctxt fields based on them.
+ * The byte next to the last prefix is also fetched into ctxt->b.
+ *
+ * Returns X86EMUL_CONTINUE on success.
+ */
+static int decode_prefixes(struct x86_emulate_ctxt *ctxt)
 {
 	int rc = X86EMUL_CONTINUE;
-	int mode = ctxt->mode;
-	int def_op_bytes, def_ad_bytes, goffset, simd_prefix;
-	struct opcode opcode;
+	int def_op_bytes, def_ad_bytes;
 
-	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;
-	if (insn_len > 0)
-		memcpy(ctxt->fetch.data, insn, insn_len);
-
-	switch (mode) {
+	switch (ctxt->mode) {
 	case X86EMUL_MODE_REAL:
 	case X86EMUL_MODE_VM86:
 	case X86EMUL_MODE_PROT16:
@@ -3905,7 +3904,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 		break;
 #endif
 	default:
-		return EMULATION_FAILED;
+		return X86EMUL_UNHANDLEABLE;
 	}
 
 	ctxt->op_bytes = def_op_bytes;
@@ -3920,7 +3919,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 			ctxt->op_bytes = def_op_bytes ^ 6;
 			break;
 		case 0x67:	/* address-size override */
-			if (mode == X86EMUL_MODE_PROT64)
+			if (ctxt->mode == X86EMUL_MODE_PROT64)
 				/* switch between 4/8 bytes */
 				ctxt->ad_bytes = def_ad_bytes ^ 12;
 			else
@@ -3938,7 +3937,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 			set_seg_override(ctxt, ctxt->b & 7);
 			break;
 		case 0x40 ... 0x4f: /* REX */
-			if (mode != X86EMUL_MODE_PROT64)
+			if (ctxt->mode != X86EMUL_MODE_PROT64)
 				goto done_prefixes;
 			ctxt->rex_prefix = ctxt->b;
 			continue;
@@ -3954,15 +3953,34 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 		}
 
 		/* Any legacy prefix after a REX prefix nullifies its effect. */
-
 		ctxt->rex_prefix = 0;
 	}
 
 done_prefixes:
-
 	/* REX prefix. */
 	if (ctxt->rex_prefix & 8)
 		ctxt->op_bytes = 8;	/* REX.W */
+done:
+	return rc;
+}
+
+int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
+{
+	int rc = X86EMUL_CONTINUE;
+	int goffset, simd_prefix;
+	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;
+	if (insn_len > 0)
+		memcpy(ctxt->fetch.data, insn, insn_len);
+
+	rc = decode_prefixes(ctxt);
+	if (rc != X86EMUL_CONTINUE)
+		goto done;
 
 	/* Opcode byte(s). */
 	opcode = opcode_table[ctxt->b];
@@ -4025,11 +4043,11 @@ done_prefixes:
 	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
 		return EMULATION_FAILED;
 
-	if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
+	if (ctxt->mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
 		ctxt->op_bytes = 8;
 
 	if (ctxt->d & Op3264) {
-		if (mode == X86EMUL_MODE_PROT64)
+		if (ctxt->mode == X86EMUL_MODE_PROT64)
 			ctxt->op_bytes = 8;
 		else
 			ctxt->op_bytes = 4;
-- 
1.7.5.4

[PATCH 3/4] KVM: x86 emulator: Make opcode decoding a separate function

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

This is the second part of the instruction decoding which treats the
opcode.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Cc: Takuya Yoshikawa <takuya.yoshikawa@gmail.com>
---
 arch/x86/kvm/emulate.c |   66 +++++++++++++++++++++++++++++++----------------
 1 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b22238b..e87570e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3964,32 +3964,28 @@ done:
 	return rc;
 }
 
-int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
+/**
+ * decode_opcode - decode opcode
+ * @ctxt: emulation context
+ *
+ * Decodes opcode bytes and reads opcode from table.
+ *
+ * Returns X86EMUL_CONTINUE on success.
+ */
+static int decode_opcode(struct x86_emulate_ctxt *ctxt)
 {
 	int rc = X86EMUL_CONTINUE;
 	int goffset, simd_prefix;
 	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;
-	if (insn_len > 0)
-		memcpy(ctxt->fetch.data, insn, insn_len);
-
-	rc = decode_prefixes(ctxt);
-	if (rc != X86EMUL_CONTINUE)
-		goto done;
-
-	/* Opcode byte(s). */
-	opcode = opcode_table[ctxt->b];
 	/* Two-byte opcode? */
 	if (ctxt->b == 0x0f) {
 		ctxt->twobyte = 1;
 		ctxt->b = insn_fetch(u8, ctxt);
 		opcode = twobyte_table[ctxt->b];
-	}
+	} else
+		opcode = opcode_table[ctxt->b];
+
 	ctxt->d = opcode.flags;
 
 	while (ctxt->d & GroupMask) {
@@ -4015,7 +4011,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 			break;
 		case Prefix:
 			if (ctxt->rep_prefix && ctxt->op_prefix)
-				return EMULATION_FAILED;
+				return X86EMUL_UNHANDLEABLE;
 			simd_prefix = ctxt->rep_prefix | ctxt->op_prefix;
 			switch (simd_prefix) {
 			case 0x00: opcode = opcode.u.gprefix->pfx_no; break;
@@ -4025,23 +4021,47 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 			}
 			break;
 		default:
-			return EMULATION_FAILED;
+			return X86EMUL_UNHANDLEABLE;
 		}
 
 		ctxt->d &= ~(u64)GroupMask;
 		ctxt->d |= opcode.flags;
 	}
 
+	/* Unrecognised? */
+	if (ctxt->d == 0 || (ctxt->d & Undefined))
+		return X86EMUL_UNHANDLEABLE;
+
+	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
+		return X86EMUL_UNHANDLEABLE;
+
 	ctxt->execute = opcode.u.execute;
 	ctxt->check_perm = opcode.check_perm;
 	ctxt->intercept = opcode.intercept;
 
-	/* Unrecognised? */
-	if (ctxt->d == 0 || (ctxt->d & Undefined))
-		return EMULATION_FAILED;
+done:
+	return rc;
+}
 
-	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
-		return EMULATION_FAILED;
+int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
+{
+	int rc = X86EMUL_CONTINUE;
+
+	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;
+	if (insn_len > 0)
+		memcpy(ctxt->fetch.data, insn, insn_len);
+
+	rc = decode_prefixes(ctxt);
+	if (rc != X86EMUL_CONTINUE)
+		goto done;
+
+	rc = decode_opcode(ctxt);
+	if (rc != X86EMUL_CONTINUE)
+		goto done;
 
 	if (ctxt->mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
 		ctxt->op_bytes = 8;
-- 
1.7.5.4

[PATCH 4/4] KVM: x86 emulator: Avoid pushing back ModRM byte in decode_opcode()

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Although ModRM byte is read for group decoding, it is soon pushed back
to make decode_modrm() fetch it later.

We should consistently read it, only once, in decode_opcode() instead.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Cc: Takuya Yoshikawa <takuya.yoshikawa@gmail.com>
---
 arch/x86/kvm/emulate.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e87570e..8729773 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -972,7 +972,6 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
 		ctxt->modrm_rm = base_reg = (ctxt->rex_prefix & 1) << 3; /* REG.B */
 	}
 
-	ctxt->modrm = insn_fetch(u8, ctxt);
 	ctxt->modrm_mod |= (ctxt->modrm & 0xc0) >> 6;
 	ctxt->modrm_reg |= (ctxt->modrm & 0x38) >> 3;
 	ctxt->modrm_rm |= (ctxt->modrm & 0x07);
@@ -3969,6 +3968,7 @@ done:
  * @ctxt: emulation context
  *
  * Decodes opcode bytes and reads opcode from table.
+ * The ModRM byte, if exists, is also fetched into ctxt->modrm.
  *
  * Returns X86EMUL_CONTINUE on success.
  */
@@ -3977,6 +3977,8 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
 	int rc = X86EMUL_CONTINUE;
 	int goffset, simd_prefix;
 	struct opcode opcode;
+	bool modrm_fetched = false;
+	u64 gtype;
 
 	/* Two-byte opcode? */
 	if (ctxt->b == 0x0f) {
@@ -3988,17 +3990,18 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
 
 	ctxt->d = opcode.flags;
 
-	while (ctxt->d & GroupMask) {
-		switch (ctxt->d & GroupMask) {
-		case Group:
+	while ((gtype = ctxt->d & GroupMask)) {
+		if (!modrm_fetched && gtype != Prefix) {
 			ctxt->modrm = insn_fetch(u8, ctxt);
-			--ctxt->_eip;
+			modrm_fetched = true;
+		}
+
+		switch (gtype) {
+		case Group:
 			goffset = (ctxt->modrm >> 3) & 7;
 			opcode = opcode.u.group[goffset];
 			break;
 		case GroupDual:
-			ctxt->modrm = insn_fetch(u8, ctxt);
-			--ctxt->_eip;
 			goffset = (ctxt->modrm >> 3) & 7;
 			if ((ctxt->modrm >> 6) == 3)
 				opcode = opcode.u.gdual->mod3[goffset];
@@ -4039,6 +4042,8 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
 	ctxt->check_perm = opcode.check_perm;
 	ctxt->intercept = opcode.intercept;
 
+	if (!modrm_fetched && (ctxt->d & ModRM))
+		ctxt->modrm = insn_fetch(u8, ctxt);
 done:
 	return rc;
 }
-- 
1.7.5.4

Re: [PATCH 4/4] KVM: x86 emulator: Avoid pushing back ModRM byte in decode_opcode()

[Avi Kivity]

On 04/23/2012 06:37 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
>
> Although ModRM byte is read for group decoding, it is soon pushed back
> to make decode_modrm() fetch it later.
>
> We should consistently read it, only once, in decode_opcode() instead.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Cc: Takuya Yoshikawa <takuya.yoshikawa@gmail.com>
> ---
>  arch/x86/kvm/emulate.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index e87570e..8729773 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -972,7 +972,6 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
>  		ctxt->modrm_rm = base_reg = (ctxt->rex_prefix & 1) << 3; /* REG.B */
>  	}
>  
> -	ctxt->modrm = insn_fetch(u8, ctxt);
>  	ctxt->modrm_mod |= (ctxt->modrm & 0xc0) >> 6;
>  	ctxt->modrm_reg |= (ctxt->modrm & 0x38) >> 3;
>  	ctxt->modrm_rm |= (ctxt->modrm & 0x07);
> @@ -3969,6 +3968,7 @@ done:
>   * @ctxt: emulation context
>   *
>   * Decodes opcode bytes and reads opcode from table.
> + * The ModRM byte, if exists, is also fetched into ctxt->modrm.
>   *
>   * Returns X86EMUL_CONTINUE on success.
>   */
> @@ -3977,6 +3977,8 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
>  	int rc = X86EMUL_CONTINUE;
>  	int goffset, simd_prefix;
>  	struct opcode opcode;
> +	bool modrm_fetched = false;
> +	u64 gtype;
>  
>  	/* Two-byte opcode? */
>  	if (ctxt->b == 0x0f) {
> @@ -3988,17 +3990,18 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
>  
>  	ctxt->d = opcode.flags;
>  
> -	while (ctxt->d & GroupMask) {
> -		switch (ctxt->d & GroupMask) {
> -		case Group:
> +	while ((gtype = ctxt->d & GroupMask)) {
> +		if (!modrm_fetched && gtype != Prefix) {
>  			ctxt->modrm = insn_fetch(u8, ctxt);
> -			--ctxt->_eip;
> +			modrm_fetched = true;
> +		}
> +
> +		switch (gtype) {
> +		case Group:
>  			goffset = (ctxt->modrm >> 3) & 7;
>  			opcode = opcode.u.group[goffset];
>  			break;
>  		case GroupDual:
> -			ctxt->modrm = insn_fetch(u8, ctxt);
> -			--ctxt->_eip;
>  			goffset = (ctxt->modrm >> 3) & 7;
>  			if ((ctxt->modrm >> 6) == 3)
>  				opcode = opcode.u.gdual->mod3[goffset];
> @@ -4039,6 +4042,8 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
>  	ctxt->check_perm = opcode.check_perm;
>  	ctxt->intercept = opcode.intercept;
>  
> +	if (!modrm_fetched && (ctxt->d & ModRM))
> +		ctxt->modrm = insn_fetch(u8, ctxt);

Instead of adding yet another conditional, how about doing something like

   if ((c->d & ModRM) || (gtype == Group) || (gtype == GroupDual))
          ctxt->modrm = insn_fetch(u8, ctxt);

somewhere early?

In fact even that is too much.  All groups have ModRM somewhere in their
encoding; all we have to do is move it to the main tables (opcode_table
or twobyte_table) and just move the existing modrm fetch before group
parsing.

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

Re: [PATCH 0/4] KVM: x86 emulator: Split decoder into separate functions

[Avi Kivity]

On 04/23/2012 06:31 PM, Takuya Yoshikawa wrote:
> Takuya Yoshikawa (4):
>   KVM: x86 emulator: Introduce ctxt->op_prefix for 0x66 prefix
>   KVM: x86 emulator: Make prefix decoding a separate function
>   KVM: x86 emulator: Make opcode decoding a separate function
>

I don't see the benefit for the first three patches.  They add more
state, and don't encourage any reuse.  The current code is complicated,
but just splitting it doesn't really fix that.

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

Re: [PATCH 4/4] KVM: x86 emulator: Avoid pushing back ModRM byte in decode_opcode()

[Takuya Yoshikawa]

On Tue, 24 Apr 2012 17:10:08 +0300
Avi Kivity <avi@redhat.com> wrote:

> > +	if (!modrm_fetched && (ctxt->d & ModRM))
> > +		ctxt->modrm = insn_fetch(u8, ctxt);
> 
> Instead of adding yet another conditional, how about doing something like
> 
>    if ((c->d & ModRM) || (gtype == Group) || (gtype == GroupDual))
>           ctxt->modrm = insn_fetch(u8, ctxt);
> 
> somewhere early?

Looks nicer than mine.

> In fact even that is too much.  All groups have ModRM somewhere in their
> encoding; all we have to do is move it to the main tables (opcode_table
> or twobyte_table) and just move the existing modrm fetch before group
> parsing.

I see.

Then, the best way to go seems to be drop patch-4 and work on the ModRM
fetch issue separately.

Modifying the tables needs some care.

Thanks,
	Takuya

Re: [PATCH 0/4] KVM: x86 emulator: Split decoder into separate functions

[Takuya Yoshikawa]

On Tue, 24 Apr 2012 17:11:54 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 04/23/2012 06:31 PM, Takuya Yoshikawa wrote:
> > Takuya Yoshikawa (4):
> >   KVM: x86 emulator: Introduce ctxt->op_prefix for 0x66 prefix
> >   KVM: x86 emulator: Make prefix decoding a separate function
> >   KVM: x86 emulator: Make opcode decoding a separate function
> >
> 
> I don't see the benefit for the first three patches.  They add more
> state, and don't encourage any reuse.  The current code is complicated,
> but just splitting it doesn't really fix that.

I thought up these when some people tried to use some opcode/prefix
bytes in wrong stages.

Although I personally like splitting a long function into named
sub-functions, it's just a matter of taste.

OK, then I will focus on modrm fetching.

Thanks,
	Takuya