* [PATCH] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding
@ 2016-10-14 16:13 Andrew Cooper
2016-10-24 9:16 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2016-10-14 16:13 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
This avoids needing a translation table between hardware ordering and Xen's
ordering.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/x86_emulate/x86_emulate.c | 35 +++++++++++++++-------------------
xen/arch/x86/x86_emulate/x86_emulate.h | 4 ++--
2 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 38147c5..32c45c4 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1557,22 +1557,6 @@ decode_register(
return p;
}
-#define decode_segment_failed x86_seg_tr
-static enum x86_segment
-decode_segment(uint8_t modrm_reg)
-{
- switch ( modrm_reg )
- {
- case 0: return x86_seg_es;
- case 1: return x86_seg_cs;
- case 2: return x86_seg_ss;
- case 3: return x86_seg_ds;
- case 4: return x86_seg_fs;
- case 5: return x86_seg_gs;
- }
- return decode_segment_failed;
-}
-
static bool is_aligned(enum x86_segment seg, unsigned long offs,
unsigned int size, struct x86_emulate_ctxt *ctxt,
const struct x86_emulate_ops *ops)
@@ -2980,8 +2964,8 @@ x86_emulate(
break;
case 0x8c: /* mov Sreg,r/m */
- seg = decode_segment(modrm_reg);
- generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
+ seg = modrm_reg;
+ generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1);
store_selector:
fail_if(ops->read_segment == NULL);
if ( (rc = ops->read_segment(seg, &sreg, ctxt)) != 0 )
@@ -2992,8 +2976,8 @@ x86_emulate(
break;
case 0x8e: /* mov r/m,Sreg */
- seg = decode_segment(modrm_reg);
- generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
+ seg = modrm_reg;
+ generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1);
generate_exception_if(seg == x86_seg_cs, EXC_UD, -1);
if ( (rc = load_seg(seg, src.val, 0, NULL, ctxt, ops)) != 0 )
goto done;
@@ -5520,4 +5504,15 @@ x86_insn_length(const struct x86_emulate_state *state,
return state->eip - ctxt->regs->eip;
}
+static void __init __maybe_unused build_assertions(void)
+{
+ /* Check the values against SReg3 encoding in opcode/ModRM bytes. */
+ BUILD_BUG_ON(x86_seg_es != 0);
+ BUILD_BUG_ON(x86_seg_cs != 1);
+ BUILD_BUG_ON(x86_seg_ss != 2);
+ BUILD_BUG_ON(x86_seg_ds != 3);
+ BUILD_BUG_ON(x86_seg_fs != 4);
+ BUILD_BUG_ON(x86_seg_gs != 5);
+}
+
#endif
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 641711e..00ceade 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -29,11 +29,11 @@ struct x86_emulate_ctxt;
/* Comprehensive enumeration of x86 segment registers. */
enum x86_segment {
- /* General purpose. */
+ /* General purpose. Matches the SReg3 encoding in opcode/ModRM bytes. */
+ x86_seg_es,
x86_seg_cs,
x86_seg_ss,
x86_seg_ds,
- x86_seg_es,
x86_seg_fs,
x86_seg_gs,
/* System. */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding
2016-10-14 16:13 [PATCH] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding Andrew Cooper
@ 2016-10-24 9:16 ` Jan Beulich
2016-10-24 9:28 ` Andrew Cooper
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2016-10-24 9:16 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 14.10.16 at 18:13, <andrew.cooper3@citrix.com> wrote:
> @@ -2980,8 +2964,8 @@ x86_emulate(
> break;
>
> case 0x8c: /* mov Sreg,r/m */
> - seg = decode_segment(modrm_reg);
> - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
> + seg = modrm_reg;
You need to mask with 7 here and ...
> @@ -2992,8 +2976,8 @@ x86_emulate(
> break;
>
> case 0x8e: /* mov r/m,Sreg */
> - seg = decode_segment(modrm_reg);
> - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
> + seg = modrm_reg;
... here - I've just checked that at least Intel ignores REX.R.
> @@ -5520,4 +5504,15 @@ x86_insn_length(const struct x86_emulate_state *state,
> return state->eip - ctxt->regs->eip;
> }
>
> +static void __init __maybe_unused build_assertions(void)
> +{
> + /* Check the values against SReg3 encoding in opcode/ModRM bytes. */
> + BUILD_BUG_ON(x86_seg_es != 0);
> + BUILD_BUG_ON(x86_seg_cs != 1);
> + BUILD_BUG_ON(x86_seg_ss != 2);
> + BUILD_BUG_ON(x86_seg_ds != 3);
> + BUILD_BUG_ON(x86_seg_fs != 4);
> + BUILD_BUG_ON(x86_seg_gs != 5);
> +}
If we really want this, I think it should be enabled for the test tool
too.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding
2016-10-24 9:16 ` Jan Beulich
@ 2016-10-24 9:28 ` Andrew Cooper
2016-10-24 10:06 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2016-10-24 9:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 24/10/16 10:16, Jan Beulich wrote:
>>>> On 14.10.16 at 18:13, <andrew.cooper3@citrix.com> wrote:
>> @@ -2980,8 +2964,8 @@ x86_emulate(
>> break;
>>
>> case 0x8c: /* mov Sreg,r/m */
>> - seg = decode_segment(modrm_reg);
>> - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
>> + seg = modrm_reg;
> You need to mask with 7 here and ...
>
>> @@ -2992,8 +2976,8 @@ x86_emulate(
>> break;
>>
>> case 0x8e: /* mov r/m,Sreg */
>> - seg = decode_segment(modrm_reg);
>> - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
>> + seg = modrm_reg;
> ... here - I've just checked that at least Intel ignores REX.R.
Both points are covered by by the is_x86_user_segment() check which you
have cropped out of context.
One option, if you would prefer, is to reverse the check and assignment,
passing modrm_reg into is_x86_user_segment(), but I don't see any need
for other code changes.
>
>> @@ -5520,4 +5504,15 @@ x86_insn_length(const struct x86_emulate_state *state,
>> return state->eip - ctxt->regs->eip;
>> }
>>
>> +static void __init __maybe_unused build_assertions(void)
>> +{
>> + /* Check the values against SReg3 encoding in opcode/ModRM bytes. */
>> + BUILD_BUG_ON(x86_seg_es != 0);
>> + BUILD_BUG_ON(x86_seg_cs != 1);
>> + BUILD_BUG_ON(x86_seg_ss != 2);
>> + BUILD_BUG_ON(x86_seg_ds != 3);
>> + BUILD_BUG_ON(x86_seg_fs != 4);
>> + BUILD_BUG_ON(x86_seg_gs != 5);
>> +}
> If we really want this, I think it should be enabled for the test tool
> too.
I wonder whether I can get away with having it as a static inline in
x86_emulate.h
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding
2016-10-24 9:28 ` Andrew Cooper
@ 2016-10-24 10:06 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2016-10-24 10:06 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 24.10.16 at 11:28, <andrew.cooper3@citrix.com> wrote:
> On 24/10/16 10:16, Jan Beulich wrote:
>>>>> On 14.10.16 at 18:13, <andrew.cooper3@citrix.com> wrote:
>>> @@ -2980,8 +2964,8 @@ x86_emulate(
>>> break;
>>>
>>> case 0x8c: /* mov Sreg,r/m */
>>> - seg = decode_segment(modrm_reg);
>>> - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
>>> + seg = modrm_reg;
>> You need to mask with 7 here and ...
>>
>>> @@ -2992,8 +2976,8 @@ x86_emulate(
>>> break;
>>>
>>> case 0x8e: /* mov r/m,Sreg */
>>> - seg = decode_segment(modrm_reg);
>>> - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
>>> + seg = modrm_reg;
>> ... here - I've just checked that at least Intel ignores REX.R.
>
> Both points are covered by by the is_x86_user_segment() check which you
> have cropped out of context.
Very definitely not:
+ seg = modrm_reg;
+ generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1);
You raise an exception if REX.R is set, as that causes seg > x86_seg_gs.
> One option, if you would prefer, is to reverse the check and assignment,
> passing modrm_reg into is_x86_user_segment(), but I don't see any need
> for other code changes.
That wouldn't help at all.
>>> @@ -5520,4 +5504,15 @@ x86_insn_length(const struct x86_emulate_state *state,
>>> return state->eip - ctxt->regs->eip;
>>> }
>>>
>>> +static void __init __maybe_unused build_assertions(void)
>>> +{
>>> + /* Check the values against SReg3 encoding in opcode/ModRM bytes. */
>>> + BUILD_BUG_ON(x86_seg_es != 0);
>>> + BUILD_BUG_ON(x86_seg_cs != 1);
>>> + BUILD_BUG_ON(x86_seg_ss != 2);
>>> + BUILD_BUG_ON(x86_seg_ds != 3);
>>> + BUILD_BUG_ON(x86_seg_fs != 4);
>>> + BUILD_BUG_ON(x86_seg_gs != 5);
>>> +}
>> If we really want this, I think it should be enabled for the test tool
>> too.
>
> I wonder whether I can get away with having it as a static inline in
> x86_emulate.h
I think that would do.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-10-24 10:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-14 16:13 [PATCH] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding Andrew Cooper
2016-10-24 9:16 ` Jan Beulich
2016-10-24 9:28 ` Andrew Cooper
2016-10-24 10:06 ` 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).