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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.11.18 at 17:26, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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:
>>>> @@ -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?

                    switch ( b )
                    {
                    case 0x62:
                        opcode = X86EMUL_OPC_EVEX_;
                        evex.raw[0] = vex.raw[0];
                        evex.raw[1] = vex.raw[1];
                        evex.raw[2] = insn_fetch_type(uint8_t);

(in the middle of ModRM processing in x86_decode())

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

So considering the second aspect I've mentioned, would you
nevertheless prefer the parens here (and perhaps elsewhere)
to be dropped?

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

Will do, perhaps introducing (in a prereq patch) an IMPOSSIBLE()
construct abstracting this away, as there's already one such
instance in the code.

Jan



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