* [PATCH v2 0/3] x86: instruction emulator adjustments
@ 2016-07-04 11:51 Jan Beulich
2016-07-04 11:55 ` [PATCH v2 1/3] use consistent exit mechanism Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jan Beulich @ 2016-07-04 11:51 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
1: use consistent exit mechanism
2: drop pointless and add useful default cases
3: fold local variables
Signed-off-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
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 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
* Re: [PATCH v2 1/3] use consistent exit mechanism
2016-07-04 11:55 ` [PATCH v2 1/3] use consistent exit mechanism Jan Beulich
@ 2016-07-04 12:54 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2016-07-04 12:54 UTC (permalink / raw)
To: Jan Beulich, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 213 bytes --]
On 04/07/16 12:55, Jan Beulich wrote:
> Similar code should use similar exit mechanisms (return vs goto).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
[-- Attachment #1.2: Type: text/html, Size: 768 bytes --]
[-- Attachment #2: 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
* Re: [PATCH v2 2/3] drop pointless and add useful default cases
2016-07-04 11:55 ` [PATCH v2 2/3] drop pointless and add useful default cases Jan Beulich
@ 2016-07-04 12:55 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2016-07-04 12:55 UTC (permalink / raw)
To: Jan Beulich, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 491 bytes --]
On 04/07/16 12:55, Jan Beulich wrote:
> 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
[-- Attachment #1.2: Type: text/html, Size: 1037 bytes --]
[-- Attachment #2: 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
end of thread, other threads:[~2016-07-04 12:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 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 12:55 ` Andrew Cooper
2016-07-04 11:56 ` [PATCH v2 3/3] x86emul: fold local variables Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).