[PATCH] x86/emul: Simplfy L{ES,DS,SS,FS,GS} handling

[PATCH] x86/emul: Simplfy L{ES,DS,SS,FS,GS} handling

[Andrew Cooper]

%ss, %fs and %gs can be calculated by directly masking the opcode.  %es and
%ds cant, but the calculation isn't hard.

Use seg rather than dst.val for storing the calculated segment, which is
appropriately typed.  The mode_64() check can be repositioned and simplified
to drop the ext check.  Replace opencoding of X86EMUL_OKAY.

Finally, introduce assertions each time we calculate a user segment to load
(rather than using constants) which don't have other validity checks.  This
includes the POP %sreg case.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 40 +++++++++++++++-------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 1b5becf..2fb99e9 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2765,6 +2765,7 @@ x86_emulate(
         if ( mode_64bit() && (op_bytes == 4) )
             op_bytes = 8;
         seg = (b >> 3) & 7;
+        ASSERT(is_x86_user_segment(seg));
         if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), &dst.val,
                               op_bytes, ctxt, ops)) != X86EMUL_OKAY ||
              (rc = load_seg(seg, dst.val, 0, NULL, ctxt, ops)) != X86EMUL_OKAY )
@@ -3393,25 +3394,32 @@ x86_emulate(
         _regs.eip = dst.val;
         break;
 
-    case 0xc4: /* les */ {
+    case 0xc4: /* les */
+    case 0xc5: /* lds */
+    {
         unsigned long sel;
-        dst.val = x86_seg_es;
-    les: /* dst.val identifies the segment */
-        generate_exception_if(mode_64bit() && !ext, EXC_UD);
+
+        generate_exception_if(mode_64bit(), EXC_UD);
+        seg = (b & 1) * 3; /* es = 0, ds = 3 */
+        goto les;
+
+    case X86EMUL_OPC(0x0f, 0xb2): /* lss */
+    case X86EMUL_OPC(0x0f, 0xb4): /* lfs */
+    case X86EMUL_OPC(0x0f, 0xb5): /* lgs */
+        seg = b & 7;
+
+    les:
         generate_exception_if(src.type != OP_MEM, EXC_UD);
         if ( (rc = read_ulong(src.mem.seg, src.mem.off + src.bytes,
-                              &sel, 2, ctxt, ops)) != 0 )
+                              &sel, 2, ctxt, ops)) != X86EMUL_OKAY )
             goto done;
-        if ( (rc = load_seg(dst.val, sel, 0, NULL, ctxt, ops)) != 0 )
+        ASSERT(is_x86_user_segment(seg));
+        if ( (rc = load_seg(seg, sel, 0, NULL, ctxt, ops)) != X86EMUL_OKAY )
             goto done;
         dst.val = src.val;
         break;
     }
 
-    case 0xc5: /* lds */
-        dst.val = x86_seg_ds;
-        goto les;
-
     case 0xc8: /* enter imm16,imm8 */ {
         uint8_t depth = imm2 & 31;
         int i;
@@ -5228,22 +5236,10 @@ x86_emulate(
         }
         break;
 
-    case X86EMUL_OPC(0x0f, 0xb2): /* lss */
-        dst.val = x86_seg_ss;
-        goto les;
-
     case X86EMUL_OPC(0x0f, 0xb3): btr: /* btr */
         emulate_2op_SrcV_nobyte("btr", src, dst, _regs.eflags);
         break;
 
-    case X86EMUL_OPC(0x0f, 0xb4): /* lfs */
-        dst.val = x86_seg_fs;
-        goto les;
-
-    case X86EMUL_OPC(0x0f, 0xb5): /* lgs */
-        dst.val = x86_seg_gs;
-        goto les;
-
     case X86EMUL_OPC(0x0f, 0xb6): /* movzx rm8,r{16,32,64} */
         /* Recompute DstReg as we may have decoded AH/BH/CH/DH. */
         dst.reg   = decode_register(modrm_reg, &_regs, 0);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/emul: Simplfy L{ES, DS, SS, FS, GS} handling

[Jan Beulich]

>>> On 14.12.16 at 12:20, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -2765,6 +2765,7 @@ x86_emulate(
>          if ( mode_64bit() && (op_bytes == 4) )
>              op_bytes = 8;
>          seg = (b >> 3) & 7;
> +        ASSERT(is_x86_user_segment(seg));

Kind of pointless, don't you think? It's guaranteed by the set of
case statements ahead of here.

> @@ -3393,25 +3394,32 @@ x86_emulate(
>          _regs.eip = dst.val;
>          break;
>  
> -    case 0xc4: /* les */ {
> +    case 0xc4: /* les */
> +    case 0xc5: /* lds */
> +    {
>          unsigned long sel;

Since you're touching this anyway, and since you're eliminating the
use of dst.val here, may I ask that you eliminate this local variable
at once, using dst.val instead?

> -        dst.val = x86_seg_es;
> -    les: /* dst.val identifies the segment */
> -        generate_exception_if(mode_64bit() && !ext, EXC_UD);
> +
> +        generate_exception_if(mode_64bit(), EXC_UD);
> +        seg = (b & 1) * 3; /* es = 0, ds = 3 */
> +        goto les;
> +
> +    case X86EMUL_OPC(0x0f, 0xb2): /* lss */
> +    case X86EMUL_OPC(0x0f, 0xb4): /* lfs */
> +    case X86EMUL_OPC(0x0f, 0xb5): /* lgs */
> +        seg = b & 7;
> +
> +    les:

My general line of thinking about where to place case labels was
- next to each other for opcodes sharing all of their code, or allowing
  plain fall-through,
- in numeric order when plain fall-through is not possible.
Hence I'd prefer if the three could stay at the place where LSS was.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/emul: Simplfy L{ES, DS, SS, FS, GS} handling

[Andrew Cooper]

On 14/12/16 13:34, Jan Beulich wrote:
>>>> On 14.12.16 at 12:20, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -2765,6 +2765,7 @@ x86_emulate(
>>          if ( mode_64bit() && (op_bytes == 4) )
>>              op_bytes = 8;
>>          seg = (b >> 3) & 7;
>> +        ASSERT(is_x86_user_segment(seg));
> Kind of pointless, don't you think? It's guaranteed by the set of
> case statements ahead of here.

Well, I didn't think so at the time.  All other uses either have seg
used from a constant, or has previously had a user check in a
generate_exception_if() clause.

>
>> @@ -3393,25 +3394,32 @@ x86_emulate(
>>          _regs.eip = dst.val;
>>          break;
>>  
>> -    case 0xc4: /* les */ {
>> +    case 0xc4: /* les */
>> +    case 0xc5: /* lds */
>> +    {
>>          unsigned long sel;
> Since you're touching this anyway, and since you're eliminating the
> use of dst.val here, may I ask that you eliminate this local variable
> at once, using dst.val instead?

Good point.

>
>> -        dst.val = x86_seg_es;
>> -    les: /* dst.val identifies the segment */
>> -        generate_exception_if(mode_64bit() && !ext, EXC_UD);
>> +
>> +        generate_exception_if(mode_64bit(), EXC_UD);
>> +        seg = (b & 1) * 3; /* es = 0, ds = 3 */
>> +        goto les;
>> +
>> +    case X86EMUL_OPC(0x0f, 0xb2): /* lss */
>> +    case X86EMUL_OPC(0x0f, 0xb4): /* lfs */
>> +    case X86EMUL_OPC(0x0f, 0xb5): /* lgs */
>> +        seg = b & 7;
>> +
>> +    les:
> My general line of thinking about where to place case labels was
> - next to each other for opcodes sharing all of their code, or allowing
>   plain fall-through,
> - in numeric order when plain fall-through is not possible.
> Hence I'd prefer if the three could stay at the place where LSS was.

Ok.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel