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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/11/2018 14:35, Jan Beulich wrote:
>>>> 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.

It is amazing how you get used to the code when you stare at it for
weeks on end.  In this case however, I expect it might be easier to spot
if the \ were aligned on the RHS.  (I still find aligned \ far easier to
read.)

>
>>> @@ -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[].

Oh - I was looking for that, but failed to spot it.  Where is that?

>  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.

I hoped as much, but in the absence of finding any suitable code, I
though I'd ask.

>
>>> @@ -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.

BSD style already specifies that function parameters are aligned
vertically after the (, so this case is fine without.

The problematic case is bare block continuations (especially on return
statements) where the BSD style is 4 spaces in from the outer block.

>
>>> @@ -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.

An { ASSERT_UNREACHABLE(); return X86_UNHANDLEABLE; } block would be
better than BUG(), because at least it won't crash a release
hypervisor.  (At least being unsigned division, we don't have the -1
case to worry about).

~Andrew

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