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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/12/16 13:34, Jan Beulich wrote:
>>>> On 14.12.16 at 12:20, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel