Re: [Xen-devel] [PATCH v3 12/18] x86emul: add tables for 0f38 and 0f3a extension space

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.02.17 at 17:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/02/17 11:14, Jan Beulich wrote:
>> @@ -2207,12 +2231,12 @@ x86_decode_twobyte(
>>          switch ( modrm_reg & 7 )
>>          {
>>          case 2: /* {,v}ldmxcsr */
>> -            state->desc = DstImplicit | SrcMem | ModRM | Mov;
>> +            state->desc = DstImplicit | SrcMem | Mov;
>>              op_bytes = 4;
>>              break;
>>  
>>          case 3: /* {,v}stmxcsr */
>> -            state->desc = DstMem | SrcImplicit | ModRM | Mov;
>> +            state->desc = DstMem | SrcImplicit | Mov;
>>              op_bytes = 4;
>>              break;
>>          }
> 
> Shouldn't this be folded into patch 7?

I don't think so - the re-purposing of the ModRM bit starts only in the
patch here.

>> @@ -2571,6 +2595,25 @@ x86_decode(
>>                  }
>>                  break;
>>              }
>> +            break;
>> +
>> +        case vex_0f38:
>> +            d = ext0f38_table[b].to_memory ? DstMem | SrcReg
>> +                                           : DstReg | SrcMem;
>> +            if ( ext0f38_table[b].two_op )
>> +                d |= TwoOp;
>> +            if ( ext0f38_table[b].vsib )
>> +                d |= vSIB;
> 
> What prevents vSIB becoming set for a non-vex encoded 0f38 instruction?

Coding discipline. The intention here is to have this bit available for
future use in the ext0f3a_table[] case as well as, once they need
adding, any XOP ones. At that point consumers of the bit would
need to become sensitive to the extension space an insn is in, but
for now we can keep the code simple in this regard.

>> @@ -7382,7 +7438,7 @@ x86_insn_modrm(const struct x86_emulate_
>>  {
>>      check_state(state);
>>  
>> -    if ( !(state->desc & ModRM) )
>> +    if ( state->modrm_mod > 3 )
> 
> Speaking of, there is still a bug with some existing x86_insn_modrm()
> callsites.
> 
> Consider this a ping on the "Re: [Xen-devel] [PATCH] x86/svm: Adjust
> ModRM Mode check in is_invlpg()" thread.

Not sure what you mean here - I did provide my ack to your
patch, and even made the condition on it conditional (with the
alternative of reverting).

Jan


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