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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/12/16 14:09, Jan Beulich wrote:
> Now that segment registers are numbered naturally this can be easily
> done to achieve some code size reduction.
>
> Also consistently use X86EMUL_OKAY in the code being touched.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Nice improvement in principle, but...

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

~Andrew

>              ctxt->retire.mov_ss = true;
>          break;


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