Re: [Xen-devel] [PATCH] x86emul: fold SReg PUSH/POP cases

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.12.16 at 16:54, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/12/16 14:09, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -2670,51 +2670,40 @@ x86_emulate(
>>          break;
>>  
>>      case 0x06: /* push %%es */
>> -        src.val = x86_seg_es;
>> -    push_seg:
>> -        generate_exception_if(mode_64bit() && !ext, EXC_UD);
>> +    case 0x0e: /* push %%cs */
>> +    case 0x16: /* push %%ss */
>> +    case 0x1e: /* push %%ds */
>> +        generate_exception_if(mode_64bit(), EXC_UD);
>> +        /* fall through */
>> +    case X86EMUL_OPC(0x0f, 0xa0): /* push %%fs */
>> +    case X86EMUL_OPC(0x0f, 0xa8): /* push %%gs */
>>          fail_if(ops->read_segment == NULL);
>> -        if ( (rc = ops->read_segment(src.val, &sreg, ctxt)) != 0 )
>> +        if ( (rc = ops->read_segment((b >> 3) & 7, &sreg,
>> +                                     ctxt)) != X86EMUL_OKAY )
>>              goto done;
>>          src.val = sreg.sel;
>>          goto push;
>>  
>>      case 0x07: /* pop %%es */
>> -        src.val = x86_seg_es;
>> -    pop_seg:
>> -        generate_exception_if(mode_64bit() && !ext, EXC_UD);
>> +    case 0x17: /* pop %%ss */
>> +    case 0x1f: /* pop %%ds */
>> +        generate_exception_if(mode_64bit(), EXC_UD);
>> +        /* fall through */
>> +    case X86EMUL_OPC(0x0f, 0xa1): /* pop %%fs */
>> +    case X86EMUL_OPC(0x0f, 0xa9): /* pop %%gs */
>>          fail_if(ops->write_segment == NULL);
>>          /* 64-bit mode: POP defaults to a 64-bit operand. */
>>          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 ||
>> -             (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 )
>> +        if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), &dst.val,
>> +                              op_bytes, ctxt, ops)) != X86EMUL_OKAY ||
>> +             (rc = load_seg((b >> 3) & 7, dst.val, 0, NULL, ctxt,
>> +                            ops)) != X86EMUL_OKAY )
>>              goto done;
>> -        if ( src.val == x86_seg_ss )
>> +        if ( b == 0x07 )
> 
> This would be less error prone by setting
> 
> seg = (b >> 3) & 7;
> 
> similarly to the mov %sreg blocks.
> 
> Doing so wouldn't accidentally break this by setting mov_ss for a pop %es.

Oops - that's a pretty kind way of saying that I've introduced a bug.

Jan


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