* [PATCH v2 1/3] use consistent exit mechanism
2016-07-04 11:51 [PATCH v2 0/3] x86: instruction emulator adjustments Jan Beulich
@ 2016-07-04 11:55 ` Jan Beulich
2016-07-04 12:54 ` Andrew Cooper
2016-07-04 11:55 ` [PATCH v2 2/3] drop pointless and add useful default cases Jan Beulich
2016-07-04 11:56 ` [PATCH v2 3/3] x86emul: fold local variables Jan Beulich
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-07-04 11:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]
Similar code should use similar exit mechanisms (return vs goto).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use "goto" instead of "return", making things consistent right away
instead of only after a (series of) future patch(es) converting
more code to "return".
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2106,7 +2106,7 @@ x86_emulate(
generate_exception_if(mode_64bit() && !ext, EXC_UD, -1);
fail_if(ops->read_segment == NULL);
if ( (rc = ops->read_segment(src.val, ®, ctxt)) != 0 )
- return rc;
+ goto done;
/* 64-bit mode: PUSH defaults to a 64-bit operand. */
if ( mode_64bit() && (op_bytes == 4) )
op_bytes = 8;
@@ -2125,10 +2125,9 @@ x86_emulate(
if ( mode_64bit() && (op_bytes == 4) )
op_bytes = 8;
if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
- &dst.val, op_bytes, ctxt, ops)) != 0 )
+ &dst.val, op_bytes, ctxt, ops)) != 0 ||
+ (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 )
goto done;
- if ( (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 )
- return rc;
break;
case 0x0e: /* push %%cs */
[-- Attachment #2: x86emul-consistently-bail.patch --]
[-- Type: text/plain, Size: 1415 bytes --]
x86emul: use consistent exit mechanism
Similar code should use similar exit mechanisms (return vs goto).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use "goto" instead of "return", making things consistent right away
instead of only after a (series of) future patch(es) converting
more code to "return".
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2106,7 +2106,7 @@ x86_emulate(
generate_exception_if(mode_64bit() && !ext, EXC_UD, -1);
fail_if(ops->read_segment == NULL);
if ( (rc = ops->read_segment(src.val, ®, ctxt)) != 0 )
- return rc;
+ goto done;
/* 64-bit mode: PUSH defaults to a 64-bit operand. */
if ( mode_64bit() && (op_bytes == 4) )
op_bytes = 8;
@@ -2125,10 +2125,9 @@ x86_emulate(
if ( mode_64bit() && (op_bytes == 4) )
op_bytes = 8;
if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
- &dst.val, op_bytes, ctxt, ops)) != 0 )
+ &dst.val, op_bytes, ctxt, ops)) != 0 ||
+ (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 )
goto done;
- if ( (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 )
- return rc;
break;
case 0x0e: /* push %%cs */
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 2/3] drop pointless and add useful default cases
2016-07-04 11:51 [PATCH v2 0/3] x86: instruction emulator adjustments Jan Beulich
2016-07-04 11:55 ` [PATCH v2 1/3] use consistent exit mechanism Jan Beulich
@ 2016-07-04 11:55 ` Jan Beulich
2016-07-04 12:55 ` Andrew Cooper
2016-07-04 11:56 ` [PATCH v2 3/3] x86emul: fold local variables Jan Beulich
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-07-04 11:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
[-- Attachment #1: Type: text/plain, Size: 3144 bytes --]
There's no point in having default cases when all possible values have
respective case statements, or when there's just a "break" statement.
Otoh the two main switch() statements better get default cases added,
just to cover the case of someone altering one of the two lookup arrays
without suitably changing these switch statements.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: use ASSERT_UNREACHABLE() in favor of BUG(), and log a message once.
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1379,7 +1379,6 @@ decode_segment(uint8_t modrm_reg)
case 3: return x86_seg_ds;
case 4: return x86_seg_fs;
case 5: return x86_seg_gs;
- default: break;
}
return decode_segment_failed;
}
@@ -1503,6 +1502,19 @@ int x86emul_unhandleable_rw(
return X86EMUL_UNHANDLEABLE;
}
+static void internal_error(const char *which, uint8_t byte,
+ const struct cpu_user_regs *regs)
+{
+#ifdef __XEN__
+ static bool_t logged;
+
+ if ( !test_and_set_bool(logged) )
+ gprintk(XENLOG_ERR, "Internal error: %s/%02x [%04x:%08lx]\n",
+ which, byte, regs->cs, regs->eip);
+#endif
+ ASSERT_UNREACHABLE();
+}
+
int
x86_emulate(
struct x86_emulate_ctxt *ctxt,
@@ -2996,8 +3008,6 @@ x86_emulate(
case 7: /* fdivr */
emulate_fpu_insn_memsrc("fdivrs", src.val);
break;
- default:
- goto cannot_emulate;
}
}
break;
@@ -3128,8 +3138,6 @@ x86_emulate(
case 7: /* fidivr m32i */
emulate_fpu_insn_memsrc("fidivrl", src.val);
break;
- default:
- goto cannot_emulate;
}
}
break;
@@ -3352,8 +3360,6 @@ x86_emulate(
case 7: /* fidivr m16i */
emulate_fpu_insn_memsrc("fidivrs", src.val);
break;
- default:
- goto cannot_emulate;
}
}
break;
@@ -3431,8 +3437,6 @@ x86_emulate(
dst.type = OP_MEM;
emulate_fpu_insn_memdst("fistpll", dst.val);
break;
- default:
- goto cannot_emulate;
}
}
break;
@@ -3750,8 +3754,6 @@ x86_emulate(
}
break;
}
- default:
- goto cannot_emulate;
}
break;
@@ -3845,10 +3847,12 @@ x86_emulate(
goto push;
case 7:
generate_exception_if(1, EXC_UD, -1);
- default:
- goto cannot_emulate;
}
break;
+
+ default:
+ internal_error("primary", b, ctxt->regs);
+ goto cannot_emulate;
}
writeback:
@@ -4815,6 +4819,10 @@ x86_emulate(
break;
}
break;
+
+ default:
+ internal_error("secondary", b, ctxt->regs);
+ goto cannot_emulate;
}
goto writeback;
[-- Attachment #2: x86emul-default-cases.patch --]
[-- Type: text/plain, Size: 3194 bytes --]
x86emul: drop pointless and add useful default cases
There's no point in having default cases when all possible values have
respective case statements, or when there's just a "break" statement.
Otoh the two main switch() statements better get default cases added,
just to cover the case of someone altering one of the two lookup arrays
without suitably changing these switch statements.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: use ASSERT_UNREACHABLE() in favor of BUG(), and log a message once.
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1379,7 +1379,6 @@ decode_segment(uint8_t modrm_reg)
case 3: return x86_seg_ds;
case 4: return x86_seg_fs;
case 5: return x86_seg_gs;
- default: break;
}
return decode_segment_failed;
}
@@ -1503,6 +1502,19 @@ int x86emul_unhandleable_rw(
return X86EMUL_UNHANDLEABLE;
}
+static void internal_error(const char *which, uint8_t byte,
+ const struct cpu_user_regs *regs)
+{
+#ifdef __XEN__
+ static bool_t logged;
+
+ if ( !test_and_set_bool(logged) )
+ gprintk(XENLOG_ERR, "Internal error: %s/%02x [%04x:%08lx]\n",
+ which, byte, regs->cs, regs->eip);
+#endif
+ ASSERT_UNREACHABLE();
+}
+
int
x86_emulate(
struct x86_emulate_ctxt *ctxt,
@@ -2996,8 +3008,6 @@ x86_emulate(
case 7: /* fdivr */
emulate_fpu_insn_memsrc("fdivrs", src.val);
break;
- default:
- goto cannot_emulate;
}
}
break;
@@ -3128,8 +3138,6 @@ x86_emulate(
case 7: /* fidivr m32i */
emulate_fpu_insn_memsrc("fidivrl", src.val);
break;
- default:
- goto cannot_emulate;
}
}
break;
@@ -3352,8 +3360,6 @@ x86_emulate(
case 7: /* fidivr m16i */
emulate_fpu_insn_memsrc("fidivrs", src.val);
break;
- default:
- goto cannot_emulate;
}
}
break;
@@ -3431,8 +3437,6 @@ x86_emulate(
dst.type = OP_MEM;
emulate_fpu_insn_memdst("fistpll", dst.val);
break;
- default:
- goto cannot_emulate;
}
}
break;
@@ -3750,8 +3754,6 @@ x86_emulate(
}
break;
}
- default:
- goto cannot_emulate;
}
break;
@@ -3845,10 +3847,12 @@ x86_emulate(
goto push;
case 7:
generate_exception_if(1, EXC_UD, -1);
- default:
- goto cannot_emulate;
}
break;
+
+ default:
+ internal_error("primary", b, ctxt->regs);
+ goto cannot_emulate;
}
writeback:
@@ -4815,6 +4819,10 @@ x86_emulate(
break;
}
break;
+
+ default:
+ internal_error("secondary", b, ctxt->regs);
+ goto cannot_emulate;
}
goto writeback;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 3/3] x86emul: fold local variables
2016-07-04 11:51 [PATCH v2 0/3] x86: instruction emulator adjustments Jan Beulich
2016-07-04 11:55 ` [PATCH v2 1/3] use consistent exit mechanism Jan Beulich
2016-07-04 11:55 ` [PATCH v2 2/3] drop pointless and add useful default cases Jan Beulich
@ 2016-07-04 11:56 ` Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2016-07-04 11:56 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
[-- Attachment #1: Type: text/plain, Size: 2779 bytes --]
Declare some variables to they can be used by multiple pieces of code,
allowing some figure braces to be dropped (which don't align nicely
when used inside of case labeled statements).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3561,6 +3561,8 @@ x86_emulate(
case 0xf6 ... 0xf7: /* Grp3 */
switch ( modrm_reg & 7 )
{
+ unsigned long u[2], v;
+
case 0 ... 1: /* test */
goto test;
case 2: /* not */
@@ -3599,15 +3601,15 @@ x86_emulate(
_regs.edx = (uint32_t)(dst.val >> 32);
break;
#endif
- default: {
- unsigned long m[2] = { src.val, dst.val };
- if ( mul_dbl(m) )
+ default:
+ u[0] = src.val;
+ u[1] = dst.val;
+ if ( mul_dbl(u) )
_regs.eflags |= EFLG_OF|EFLG_CF;
- _regs.edx = m[1];
- dst.val = m[0];
+ _regs.edx = u[1];
+ dst.val = u[0];
break;
}
- }
break;
case 5: /* imul */
dst.type = OP_REG;
@@ -3643,20 +3645,18 @@ x86_emulate(
_regs.edx = (uint32_t)(dst.val >> 32);
break;
#endif
- default: {
- unsigned long m[2] = { src.val, dst.val };
- if ( imul_dbl(m) )
+ default:
+ u[0] = src.val;
+ u[1] = dst.val;
+ if ( imul_dbl(u) )
_regs.eflags |= EFLG_OF|EFLG_CF;
if ( b > 0x6b )
- _regs.edx = m[1];
- dst.val = m[0];
+ _regs.edx = u[1];
+ dst.val = u[0];
break;
}
- }
break;
- case 6: /* div */ {
- unsigned long u[2], v;
-
+ case 6: /* div */
dst.type = OP_REG;
dst.reg = (unsigned long *)&_regs.eax;
switch ( dst.bytes = src.bytes )
@@ -3703,10 +3703,7 @@ x86_emulate(
break;
}
break;
- }
- case 7: /* idiv */ {
- unsigned long u[2], v;
-
+ case 7: /* idiv */
dst.type = OP_REG;
dst.reg = (unsigned long *)&_regs.eax;
switch ( dst.bytes = src.bytes )
@@ -3754,7 +3751,6 @@ x86_emulate(
}
break;
}
- }
break;
case 0xf8: /* clc */
[-- Attachment #2: x86emul-fold-locals.patch --]
[-- Type: text/plain, Size: 2806 bytes --]
x86emul: fold local variables
Declare some variables to they can be used by multiple pieces of code,
allowing some figure braces to be dropped (which don't align nicely
when used inside of case labeled statements).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3561,6 +3561,8 @@ x86_emulate(
case 0xf6 ... 0xf7: /* Grp3 */
switch ( modrm_reg & 7 )
{
+ unsigned long u[2], v;
+
case 0 ... 1: /* test */
goto test;
case 2: /* not */
@@ -3599,15 +3601,15 @@ x86_emulate(
_regs.edx = (uint32_t)(dst.val >> 32);
break;
#endif
- default: {
- unsigned long m[2] = { src.val, dst.val };
- if ( mul_dbl(m) )
+ default:
+ u[0] = src.val;
+ u[1] = dst.val;
+ if ( mul_dbl(u) )
_regs.eflags |= EFLG_OF|EFLG_CF;
- _regs.edx = m[1];
- dst.val = m[0];
+ _regs.edx = u[1];
+ dst.val = u[0];
break;
}
- }
break;
case 5: /* imul */
dst.type = OP_REG;
@@ -3643,20 +3645,18 @@ x86_emulate(
_regs.edx = (uint32_t)(dst.val >> 32);
break;
#endif
- default: {
- unsigned long m[2] = { src.val, dst.val };
- if ( imul_dbl(m) )
+ default:
+ u[0] = src.val;
+ u[1] = dst.val;
+ if ( imul_dbl(u) )
_regs.eflags |= EFLG_OF|EFLG_CF;
if ( b > 0x6b )
- _regs.edx = m[1];
- dst.val = m[0];
+ _regs.edx = u[1];
+ dst.val = u[0];
break;
}
- }
break;
- case 6: /* div */ {
- unsigned long u[2], v;
-
+ case 6: /* div */
dst.type = OP_REG;
dst.reg = (unsigned long *)&_regs.eax;
switch ( dst.bytes = src.bytes )
@@ -3703,10 +3703,7 @@ x86_emulate(
break;
}
break;
- }
- case 7: /* idiv */ {
- unsigned long u[2], v;
-
+ case 7: /* idiv */
dst.type = OP_REG;
dst.reg = (unsigned long *)&_regs.eax;
switch ( dst.bytes = src.bytes )
@@ -3754,7 +3751,6 @@ x86_emulate(
}
break;
}
- }
break;
case 0xf8: /* clc */
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread