Re: [Xen-devel] [PATCH] x86/emul: only emulate possibly operand sizes for POPA

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.11.12 at 18:10, Keir Fraser <keir@xxxxxxx> wrote:
> On 07/11/2012 16:08, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> This opcode neither support 1-byte operands, nor does it support 8-byte
>> ones (since the opcode is undefined in 64-bit mode). Simplify the code
>> accordingly.
>> 
>> 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
>> @@ -1996,13 +1996,10 @@ x86_emulate(
>>              if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
>>                                    &dst.val, op_bytes, ctxt, ops)) != 0 )
>>                  goto done;
>> -            switch ( op_bytes )
>> -            {
>> -            case 1: *(uint8_t  *)regs[i] = (uint8_t)dst.val; break;
>> -            case 2: *(uint16_t *)regs[i] = (uint16_t)dst.val; break;
>> -            case 4: *regs[i] = (uint32_t)dst.val; break; /* 64b: zero-ext */
>> -            case 8: *regs[i] = dst.val; break;
>> -            }
>> +            if ( op_bytes != 2 )
>> +                *regs[i] = (uint32_t)dst.val; /* 64b: zero-ext */
>> +            else
>> +                *(uint16_t *)regs[i] = (uint16_t)dst.val;
> 
> Would prefer:
>  if ( op_bytes == 2 )
>      *(uint16_t *)regs[i] = (uint16_t)dst.val;
>  else
>      *regs[i] = dst.val;
> 
> Handles the exceptional case immediately after its predicate.

I had it that way first, but compilers tend to prefer (in terms of
static branch prediction) the if() body over the else one. Doesn't
matter that much here of course, but I'm generally trying to
follow such guidelines even in non performance critical paths so
that in case code gets cloned elsewhere it doesn't require extra
reviewing or adjustment.

> And the cast
> from uint32_t, and 64b-related comment, are pointless and in fact misleading
> in the default case, since as you say the instruction is invalid in 64-bit
> mode.

And I considered that aspect too: Even if invalid in 64-bit mode, it
is valid in compatibility mode, and in that case the zero-extension
makes sense (as does the comment).

Jan

> Apart from that:
> Acked-by: Keir Fraser <keir@xxxxxxx>
> 
>  -- Keir
> 
>>          }
>>          break;
>>      }
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx 
>> http://lists.xen.org/xen-devel 




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