Re: [Xen-devel] [PATCH 1/3] x86emul: support load forms of {, V}MOV{D, Q}

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.01.17 at 23:31, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/12/16 09:55, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -4995,6 +4995,12 @@ x86_emulate(
>>                                           /* vmovntdq ymm,m256 */
>>          fail_if(ea.type != OP_MEM);
>>          /* fall through */
>> +    case X86EMUL_OPC(0x0f, 0x6e):        /* movd r/m32,mm */
>> +                                         /* movq r/m64,mm */
>> +    case X86EMUL_OPC_66(0x0f, 0x6e):     /* movd r/m32,xmm */
>> +                                         /* movq r/m64,xmm */
>> +    case X86EMUL_OPC_VEX_66(0x0f, 0x6e): /* vmovd r/m32,xmm */
>> +                                         /* vmovq r/m64,xmm */
>>      case X86EMUL_OPC(0x0f, 0x6f):        /* movq mm/m64,mm */
>>      case X86EMUL_OPC_66(0x0f, 0x6f):     /* movdqa xmm/m128,xmm */
>>      case X86EMUL_OPC_F3(0x0f, 0x6f):     /* movdqu xmm/m128,xmm */
>> @@ -5008,6 +5014,8 @@ x86_emulate(
>>                                           /* movq xmm,r/m64 */
>>      case X86EMUL_OPC_VEX_66(0x0f, 0x7e): /* vmovd xmm,r/m32 */
>>                                           /* vmovq xmm,r/m64 */
>> +    case X86EMUL_OPC_F3(0x0f, 0x7e):     /* movq xmm/m64,xmm */
>> +    case X86EMUL_OPC_VEX_F3(0x0f, 0x7e): /* vmovq xmm/m64,xmm */
>>      case X86EMUL_OPC(0x0f, 0x7f):        /* movq mm,mm/m64 */
>>      case X86EMUL_OPC_66(0x0f, 0x7f):     /* movdqa xmm,xmm/m128 */
>>      case X86EMUL_OPC_VEX_66(0x0f, 0x7f): /* vmovdqa xmm,xmm/m128 */
>> @@ -5019,6 +5027,7 @@ x86_emulate(
>>      case X86EMUL_OPC_VEX_66(0x0f, 0xd6): /* vmovq xmm,xmm/m64 */
>>      {
>>          uint8_t *buf = get_stub(stub);
>> +        bool load = false;
> 
> I am afraid that this logic is already almost-opaque, and these change
> make it even more complicated to follow.  Sufficiently so that I can't
> review it; I have tried and failed to look at the end result and
> conclude whether it is correct or not.
> 
> (I have no specific reasons to think that it isn't correct, but I can't
> claim to have reviewed it with integrity.)
> 
> This block of code in particular has a higher security risk than most in
> x86_emulate(), because of the risk of accidentally executing a stub with
> a modrm which selects anything other than SIMD register or (%rax) r/m
> destination.
> 
> Therefore, I'd like to see what we can do to make the logic easier to
> follow.
> 
> 
> This block deals with 3 kinds of operations, load / move / store,
> depending on the whether the source operand is memory, both operands are
> registers, or the destination operand is memory.  Beyond that however,
> there doesn't appear to be any consistency to the required adjustments
> to make the stub safe.
> 
> At a start, vex.pfx continues to be a source of confusion as it refers
> to legacy prefixes rather than the vex prefix, and vex.opcx being the
> entity which refers to legacy/vex/evex/xop encoding.  Renaming these
> constants alone would be a help.
> 
> I wonder if doing things like this would help?
> 
> enum { LOAD, MOVE, STORE } type;

With what I'm in the process of doing right now, some of this
should become easier (without the need for an enum like the one
above). I can see to re-order patches once the widening of
SSE/AVX support has grown far enough, but right now I have to
admit that my plan was to leave this particular case statement
mostly alone for the time being (whereas I've already shrunk
down the one dealing with MOVAPS & Co).

Jan

> switch ( b )
> {
>     case ...
>         type = LOAD;
>         break;
>     case ...
>         type = MOVE;
>         break;
>     case ...
>         type = STORE;
>         break;
> }
> 
> In particular, having a 128 line case statement (before this patch, 149
> after), with most semantics based on b == one of 5 (before, 7 after)
> different raw numbers, is too much cognitive context to hold.
> 
> ~Andrew




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