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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel