Re: [Xen-devel] [PATCH v3 06/25] x86emul: support most remaining AVX2 insns

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.02.18 at 20:45, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/12/17 14:03, Jan Beulich wrote:
>> @@ -2973,7 +2985,7 @@ x86_decode(
>>          }
>>          break;
>>  
>> -    case simd_scalar_fp:
>> +    case simd_scalar_fp: /* case simd_scalar_dq: */
> 
> I don't see this case label used, or introduced in any later patches. 
> Is it stale?

Oh, indeed it is. And it's been so long that I don't even recall what
it was used for.

>> @@ -6070,6 +6082,10 @@ x86_emulate(
>>      case X86EMUL_OPC_VEX_66(0x0f38, 0x40): /* vpmulld 
>> {x,y}mm/mem,{x,y}mm,{x,y}mm */
>>              if ( !vex.l )
>>                  goto simd_0f_avx;
>> +            /* fall through */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x45): /* vpsrlv{d,q} 
>> {x,y}mm/mem,{x,y}mm,{x,y}mm */
> 
> 0x46 / vpsrav{d,q}?  You add a decode for it above, but I don't see an
> introduced case.

See further down - it doesn't fit here very well because there's only
vpsravd, but no vpsravq (which only appears in AVX512).

>> @@ -7150,12 +7169,16 @@ x86_emulate(
>>          fic.insn_bytes = PFX_BYTES + 3;
>>          break;
>>  
>> -    case X86EMUL_OPC_VEX_66(0x0f38, 0x19): /* vbroadcastsd m64,ymm */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x19): /* vbroadcastsd xmm/m64,ymm */
>>      case X86EMUL_OPC_VEX_66(0x0f38, 0x1a): /* vbroadcastf128 m128,ymm */
>>          generate_exception_if(!vex.l, EXC_UD);
>>          /* fall through */
>> -    case X86EMUL_OPC_VEX_66(0x0f38, 0x18): /* vbroadcastss m32,{x,y}mm */
>> -        generate_exception_if(ea.type != OP_MEM, EXC_UD);
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x18): /* vbroadcastss xmm/m32,{x,y}mm 
>> */
> 
> It would help reviewability substantially if you split bugfixes of
> existing code out separately from introduction of new code, especially
> given the quantity of new additions here.  These comment changes are
> particularly deceptive.

This is not a bugfix - the register forms appear only in AVX2.
Normally I would have added their support right when the
instructions were added for AVX, but it was George who noticed
them missing after the AVX patch had gone in already. Changing
the code here still fits under the topic of the patch.

>>      case X86EMUL_OPC_VEX_66(0x0f38, 0x0c): /* vpermilps 
>> {x,y}mm/mem,{x,y}mm,{x,y}mm */
>>      case X86EMUL_OPC_VEX_66(0x0f38, 0x0d): /* vpermilpd 
>> {x,y}mm/mem,{x,y}mm,{x,y}mm */
>> @@ -7254,6 +7277,11 @@ x86_emulate(
>>          op_bytes = 8 << vex.l;
>>          goto simd_0f_ymm;
>>  
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x16): /* vpermps ymm/m256,ymm,ymm */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x36): /* vpermd ymm/m256,ymm,ymm */
>> +        generate_exception_if(!vex.l || vex.w, EXC_UD);
>> +        goto simd_0f_avx2;
>> +
> 
> Looking at these additions, the case labels look like they need sorting
> again.  Are you going to organise that in a later patch?

I don't understand. Together with context above and ...

>>      case X86EMUL_OPC_VEX_66(0x0f38, 0x20): /* vpmovsxbw xmm/mem,{x,y}mm */
>>      case X86EMUL_OPC_VEX_66(0x0f38, 0x21): /* vpmovsxbd xmm/mem,{x,y}mm */
>>      case X86EMUL_OPC_VEX_66(0x0f38, 0x22): /* vpmovsxbq xmm/mem,{x,y}mm */

... here I don't see what's wrong. The lowest entry in each block
of case statements wanting similar treatment rules: 0x16 is above
0x0d and below 0x20.

>> @@ -7370,6 +7398,80 @@ x86_emulate(
>>          generate_exception_if(vex.l, EXC_UD);
>>          goto simd_0f_avx;
>>  
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x58): /* vpbroadcastd xmm/m32,{x,y}mm 
>> */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x59): /* vpbroadcastq xmm/m64,{x,y}mm 
>> */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x78): /* vpbroadcastb xmm/m8,{x,y}mm */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x79): /* vpbroadcastw xmm/m16,{x,y}mm 
>> */
>> +        op_bytes = 1 << ((!(b & 0x20) * 2) + (b & 1));
>> +        /* fall through */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x46): /* vpsravd 
>> {x,y}mm/mem,{x,y}mm,{x,y}mm */
>> +        generate_exception_if(vex.w, EXC_UD);
> 
> Oh - here is vpsrav{d,q}.  Why is it not with its companions?  The
> manual does curiously omit mention of the W1 encoding for VEX (unlike
> its companions), but all 3 have W0 and W1 mentioned for EVEX encoding. 
> Judging by them all having identical behaviour, and this one not being
> declared as suffering a fault because of W, I expect that it is probably
> encoded as WIG.

Without trying out (which is unreliable as they might change things
between silicon revisions), I have to follow what the SDM and/or XED
say, and both say W0. We both know that things aren't always
consistent in the manual, so I prefer to use the tighter variant in case
of doubt - imo it's always better to relax things down the road than to
rely on catching faults raised from the stubs.

And trying it out (again; I'm sure I did so a year ago when this was
all put together), my Haswell faults for the W1 encoding.

> I've noticed lower down as well that you are inconsistent with vex.w
> handling compared to the manual as to whether you reject or ignore
> unspecified encodings.  Is this intentional, and if so, why?

I'm not intending to do anything inconsistently. It would help if you
could point out where you found such issues. Actually there's not a
whole lot of things left further down, and I can't spot any
inconsistency there.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel