Re: [Xen-devel] [PATCH v2 08/17] x86emul: fold/eliminate some local variables

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/14/2017 04:16 PM, Jan Beulich wrote:
> Make i switch-wide (at once making it unsigned, as it should have been)
> and introduce n (for immediate use in enter and aam/aad handling).
> Eliminate on-stack arrays in pusha/popa handling. Use ea.val instead of
> a custom variable in bound handling.
> 
> No (intended) functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3238,6 +3238,7 @@ x86_emulate(
>          struct segment_register cs, sreg;
>          struct cpuid_leaf cpuid_leaf;
>          uint64_t msr_val;
> +        unsigned int i, n;
>          unsigned long dummy;
>  
>      case 0x00 ... 0x05: add: /* add */
> @@ -3370,47 +3371,45 @@ x86_emulate(
>              goto done;
>          break;
>  
> -    case 0x60: /* pusha */ {
> -        int i;
> -        unsigned int regs[] = {
> -            _regs.eax, _regs.ecx, _regs.edx, _regs.ebx,
> -            _regs.esp, _regs.ebp, _regs.esi, _regs.edi };
> -
> +    case 0x60: /* pusha */
>          fail_if(!ops->write);
> +        ea.val = _regs.esp;
>          for ( i = 0; i < 8; i++ )
> +        {
> +            void *reg = decode_register(i, &_regs, 0);
> +
>              if ( (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
> -                                  &regs[i], op_bytes, ctxt)) != 0 )
> -            goto done;
> +                                  reg != &_regs.esp ? reg : &ea.val,
> +                                  op_bytes, ctxt)) != 0 )
> +                goto done;
> +        }
>          break;
> -    }
> -
> -    case 0x61: /* popa */ {
> -        int i;
> -        unsigned int dummy_esp, *regs[] = {
> -            &_regs.edi, &_regs.esi, &_regs.ebp, &dummy_esp,
> -            &_regs.ebx, &_regs.edx, &_regs.ecx, &_regs.eax };
>  
> +    case 0x61: /* popa */
>          for ( i = 0; i < 8; i++ )
>          {
> +            void *reg = decode_register(7 - i, &_regs, 0);
> +
>              if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
>                                    &dst.val, op_bytes, ctxt, ops)) != 0 )
>                  goto done;
> +            if ( reg == &_regs.r(sp) )
> +                continue;
>              if ( op_bytes == 2 )
> -                *(uint16_t *)regs[i] = (uint16_t)dst.val;
> +                *(uint16_t *)reg = dst.val;
>              else
> -                *regs[i] = dst.val; /* 64b: zero-ext done by read_ulong() */
> +                *(unsigned long *)reg = dst.val;
>          }
>          break;
> -    }
>  
>      case 0x62: /* bound */ {
> -        unsigned long src_val2;
>          int lb, ub, idx;
> +
>          generate_exception_if(src.type != OP_MEM, EXC_UD);
>          if ( (rc = read_ulong(src.mem.seg, truncate_ea(src.mem.off + 
> op_bytes),

This is the bit where the context is wrong; is this meant to be applied
on top of my AFL series?

 -George

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