Re: [Xen-devel] [PATCH 2/5] x86emul: consolidate segment register handling

[Andrew Cooper]

On 30/09/16 13:15, Jan Beulich wrote:
 On 30.09.16 at 12:39,  wrote:
>> On 08/09/16 14:43, Jan Beulich wrote:
>>> @@ -2861,21 +2860,20 @@ x86_emulate(
>>>  dst.val = src.val;
>>>  break;
>>>  
>>> -case 0x8c: /* mov Sreg,r/m */ {
>>> -struct segment_register reg;
>>> -enum x86_segment seg = decode_segment(modrm_reg);
>>> +case 0x8c: /* mov Sreg,r/m */
>>> +seg = decode_segment(modrm_reg);
>>>  generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
>>> +store_seg:
>> store_seg is easy to confuse with load_seg.  How about read_selector
>> instead?
> Well - it was specifically meant to match up with load_seg. I dislike
> read_selector (specifically because the read part of the operation
> was done by the time we get here), but if you like store_selector
> better than store_seg, I could use that one.

Fine by me.

~Andrew

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

Re: [Xen-devel] [PATCH 2/5] x86emul: consolidate segment register handling

[Jan Beulich]

>>> On 30.09.16 at 12:39,  wrote:
> On 08/09/16 14:43, Jan Beulich wrote:
>> @@ -2861,21 +2860,20 @@ x86_emulate(
>>  dst.val = src.val;
>>  break;
>>  
>> -case 0x8c: /* mov Sreg,r/m */ {
>> -struct segment_register reg;
>> -enum x86_segment seg = decode_segment(modrm_reg);
>> +case 0x8c: /* mov Sreg,r/m */
>> +seg = decode_segment(modrm_reg);
>>  generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
>> +store_seg:
> 
> store_seg is easy to confuse with load_seg.  How about read_selector
> instead?

Well - it was specifically meant to match up with load_seg. I dislike
read_selector (specifically because the read part of the operation
was done by the time we get here), but if you like store_selector
better than store_seg, I could use that one.

> It also occurs to me that you might have an easier time with both this
> patch and patch 1 if you reversed their order.

Yes, but that's the order they came to life in, and I then didn't
want to bother switching them around.

>  Either way, Reviewed-by:
> Andrew Cooper 

Thanks, Jan


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

Re: [Xen-devel] [PATCH 2/5] x86emul: consolidate segment register handling

[Andrew Cooper]

On 08/09/16 14:43, Jan Beulich wrote:
> @@ -2861,21 +2860,20 @@ x86_emulate(
>  dst.val = src.val;
>  break;
>  
> -case 0x8c: /* mov Sreg,r/m */ {
> -struct segment_register reg;
> -enum x86_segment seg = decode_segment(modrm_reg);
> +case 0x8c: /* mov Sreg,r/m */
> +seg = decode_segment(modrm_reg);
>  generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
> +store_seg:

store_seg is easy to confuse with load_seg.  How about read_selector
instead?

It also occurs to me that you might have an easier time with both this
patch and patch 1 if you reversed their order.  Either way, Reviewed-by:
Andrew Cooper 

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

[Xen-devel] [PATCH 2/5] x86emul: consolidate segment register handling

[Jan Beulich]

Use a single set of variables throughout the huge switch() statement,
allowing to funnel SLDT/STR into the mov-from-sreg code path.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2494,7 +2494,8 @@ x86_emulate(
 
 switch ( ctxt->opcode )
 {
-struct segment_register cs;
+enum x86_segment seg;
+struct segment_register cs, sreg;
 
 case 0x00 ... 0x05: add: /* add */
 emulate_2op_SrcV("add", src, dst, _regs.eflags);
@@ -2530,22 +2531,20 @@ x86_emulate(
 dst.type = OP_NONE;
 break;
 
-case 0x06: /* push %%es */ {
-struct segment_register reg;
+case 0x06: /* push %%es */
 src.val = x86_seg_es;
 push_seg:
 generate_exception_if(mode_64bit() && !ext, EXC_UD, -1);
 fail_if(ops->read_segment == NULL);
-if ( (rc = ops->read_segment(src.val, , ctxt)) != 0 )
+if ( (rc = ops->read_segment(src.val, , ctxt)) != 0 )
 goto done;
 /* 64-bit mode: PUSH defaults to a 64-bit operand. */
 if ( mode_64bit() && (op_bytes == 4) )
 op_bytes = 8;
 if ( (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
-  , op_bytes, ctxt)) != 0 )
+  , op_bytes, ctxt)) != 0 )
 goto done;
 break;
-}
 
 case 0x07: /* pop %%es */
 src.val = x86_seg_es;
@@ -2861,21 +2860,20 @@ x86_emulate(
 dst.val = src.val;
 break;
 
-case 0x8c: /* mov Sreg,r/m */ {
-struct segment_register reg;
-enum x86_segment seg = decode_segment(modrm_reg);
+case 0x8c: /* mov Sreg,r/m */
+seg = decode_segment(modrm_reg);
 generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
+store_seg:
 fail_if(ops->read_segment == NULL);
-if ( (rc = ops->read_segment(seg, , ctxt)) != 0 )
+if ( (rc = ops->read_segment(seg, , ctxt)) != 0 )
 goto done;
-dst.val = reg.sel;
+dst.val = sreg.sel;
 if ( dst.type == OP_MEM )
 dst.bytes = 2;
 break;
-}
 
-case 0x8e: /* mov r/m,Sreg */ {
-enum x86_segment seg = decode_segment(modrm_reg);
+case 0x8e: /* mov r/m,Sreg */
+seg = decode_segment(modrm_reg);
 generate_exception_if(seg == decode_segment_failed, 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 )
@@ -2884,7 +2882,6 @@ x86_emulate(
 ctxt->retire.flags.mov_ss = 1;
 dst.type = OP_NONE;
 break;
-}
 
 case 0x8d: /* lea */
 generate_exception_if(ea.type != OP_MEM, EXC_UD, -1);
@@ -2941,17 +2938,15 @@ x86_emulate(
 }
 break;
 
-case 0x9a: /* call (far, absolute) */ {
-struct segment_register reg;
-
+case 0x9a: /* call (far, absolute) */
 ASSERT(!mode_64bit());
 fail_if(ops->read_segment == NULL);
 
-if ( (rc = ops->read_segment(x86_seg_cs, , ctxt)) ||
+if ( (rc = ops->read_segment(x86_seg_cs, , ctxt)) ||
  (rc = load_seg(x86_seg_cs, imm2, 0, , ctxt, ops)) ||
  (validate_far_branch(, imm1),
   rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
-  , op_bytes, ctxt)) ||
+  , op_bytes, ctxt)) ||
  (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
   &_regs.eip, op_bytes, ctxt)) ||
  (rc = ops->write_segment(x86_seg_cs, , ctxt)) )
@@ -2959,7 +2954,6 @@ x86_emulate(
 
 _regs.eip = imm1;
 break;
-}
 
 case 0x9b:  /* wait/fwait */
 host_and_vcpu_must_have(fpu);
@@ -4178,13 +4172,12 @@ x86_emulate(
 
 if ( (modrm_reg & 7) == 3 ) /* call */
 {
-struct segment_register reg;
 fail_if(ops->read_segment == NULL);
-if ( (rc = ops->read_segment(x86_seg_cs, , ctxt)) ||
+if ( (rc = ops->read_segment(x86_seg_cs, , ctxt)) ||
  (rc = load_seg(x86_seg_cs, sel, 0, , ctxt, ops)) ||
  (validate_far_branch(, src.val),
   rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
-  , op_bytes, ctxt)) ||
+  , op_bytes, ctxt)) ||
  (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
   &_regs.eip, op_bytes, ctxt)) ||
  (rc = ops->write_segment(x86_seg_cs, , ctxt)) )
@@ -4205,34 +4198,24 @@ x86_emulate(
 }
 break;
 
-case X86EMUL_OPC(0x0f, 0x00): /* Grp6 */ {
-enum x86_segment seg = (modrm_reg & 1) ? x86_seg_tr : x86_seg_ldtr;
-
+case X86EMUL_OPC(0x0f, 0x00): /* Grp6 */
+seg = (modrm_reg & 1) ? x86_seg_tr :