Re: [Xen-devel] [PATCH v4 05/44] x86emul: support basic AVX512 moves

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.11.18 at 18:12, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 25/09/2018 14:28, Jan Beulich wrote:
>> +#define avx512_vlen_check(lig) do { \
>> +    switch ( evex.lr ) \
>> +    { \
>> +    default: \
>> +        generate_exception(EXC_UD); \
>> +    case 2: \
>> +        break; \
>> +    case 0: case 1: \
>> +        if (!(lig)) \
> 
> if ( !(lig) )

Oops. By now I've looked at this many dozen times, and I've
never noticed the style issue.

>> @@ -3272,6 +3387,7 @@ x86_emulate(
>>      b = ctxt->opcode;
>>      d = state.desc;
>>  #define state (&state)
>> +    elem_bytes = 4 << evex.w;
> 
> evex.w isn't filled by this point, is it?  We only fill evex.lr in the
> !evex_encoded() case AFAICT.

Well, that's another bit of (pre-existing) trickery: When we decode
these special prefixes (VEX, XOP, and EVEX) we first read the bytes
into vex.raw[]. The code dealing with the EVEX case then copies
the two vex.raw[] bytes into evex.raw[]. Thus the common fields
(between the prefix encodings) are filled uniformly, and uses
through vex are fine also for the EVEX case. I think this is better
than littering around many ?: expressions.

As an aside - if the above didn't work, none of the testing would
have succeeded.

>> @@ -6348,6 +6521,41 @@ x86_emulate(
>>          ASSERT(!state->simd_size);
>>          break;
>>  
>> +    case X86EMUL_OPC_EVEX_66(0x0f, 0x6e): /* vmov{d,q} r/m,xmm */
>> +    case X86EMUL_OPC_EVEX_66(0x0f, 0x7e): /* vmov{d,q} xmm,r/m */
>> +        generate_exception_if((evex.lr || evex.opmsk || evex.br ||
>> +                               evex.reg != 0xf || !evex.RX),
> 
> Are the inner brackets necessary?

I'd be happy to drop them - I've put them there mostly for you,
who you want whatever tool to properly deal with indentation on
such wrapped lines. Since I don't know the exact rules that tool
follows, I may have gone too far, but then again I think the
resulting different indentation between the two lines above and
the next line (holding the other macro argument) isn't unhelpful.

>> @@ -8819,6 +9070,44 @@ x86_emulate(
>>                                    !is_aligned(ea.mem.seg, ea.mem.off, 
>> op_bytes,
>>                                                ctxt, ops),
>>                                    EXC_GP, 0);
>> +
>> +            if ( evex.br )
>> +            {
>> +                ASSERT((d & DstMask) != DstMem);
>> +                op_bytes = elem_bytes;
>> +            }
>> +            if ( evex.opmsk )
>> +            {
>> +                ASSERT(!(op_bytes % elem_bytes));
>> +                full = ~0ULL >> (64 - op_bytes / elem_bytes);
> 
> I think we want a path which checks elem_bytes != 0 which is
> release-build safe.  This feels like an XSA waiting to happen.

Nothing _ever_ sets (or should set) elem_bytes to zero, and it gets
initialized to a non-zero value right in this patch. When writing this
code I indeed did think about adding a check against zero, but I
couldn't figure what half way sensible action (other than BUG()ing)
I could take in that case. Yet BUG() is in no way better than hitting
#DE on the division.

Jan



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