Re: [Xen-devel] [PATCH] x86emul: permit SAE for V{,U}COMIS{S,D}

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/12/2018 13:56, Jan Beulich wrote:
>>>> On 10.12.18 at 14:20, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 10/12/2018 11:32, Jan Beulich wrote:
>>> The avx512_vlen_check() invocation needs to be conditional.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> I'm not sure if I've asked before, but do LIG instructions really #UD
>> for L=3 ?  I don't see any documentation to this effect.
> At least on my Core i9 they do; I have a pending query with Intel
> as to the intentions in general and the lack of clear documentation,
> as well as to the behavior on the Knights line of processors (where
> there is no AVX512VL, and hence where special casing VL=128 and
> VL=256 but not VL=<whatever-3-will- mean> are at least
> questionable).

VL=3 will surely be 1024 bits wide, but I'd be interested to which
register mnemonic they choose to follow xmm/ymm/zmm.

I'll try to find some time to poke a Knights machine and see what happens.

>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -6179,7 +6179,8 @@ x86_emulate(
>>>                                 evex.w != evex.pfx),
>>>                                EXC_UD);
>>>          host_and_vcpu_must_have(avx512f);
>>> -        avx512_vlen_check(true);
>>> +        if ( !evex.br )
>> On the subject of ineligibility of the code, what about #define sae br ?
>>
>> That way, this would read "if ( !evex.sae ) check_vlen()"
> The three meanings of the bit can't reasonably all be conveyed
> by a acceptably short name. Of course we can introduce aliases
> like the above, but please recall that
> - "br" stands for _b_roadcast or _r_ounding, not _br_oadcast,

TBH, I'd even forgotten this.  I don't see it written anywhere.  Despite
what you claim, people will interpret it as _br_oadcast given a lack of
any information to the contrary.

> - we'd need another alias for the embedded-rounding case then.
> If you're convinced this is a good idea, I can do respective
> renaming both to what may already be committed as well as to
> the rest of the still pending series.
>
> But personally I'd rather not go that route, to make it easier to
> connect with one another all the uses/checks of that bit. This is
> in particular because for insns which allow neither broadcast nor
> rounding/SAE, I certainly don't want to check the same bit twice
> (via its different names).

The context-dependent meanings are:
* Broadcast
* Static Rounding
* Suppress All Exceptions

How about naming the field bsr for "broadcast/suppress/rounding" (which
breaks the _br_oadcast vs _b_roadcast/_r_ounding confusion), and
introducing a define for bcast, sae and rounding ?

/* EVEX.b (SDM nomenclature) has encoding-dependent meaning. */
#define bcast bsr
#define sae bsr
#define rounding bsr

That way, code with a single meaning can use the context-correct name,
and any cases (are there any?) which don't use one of these modes can
use the underlying field.  (I certainly don't suggest checking the same
bit with multiple names.  Amongst other things, I expect Coverity will
notice.)

I don't think it will cause confusion for correlating the uses of the
bit, because we will never be using more than a single name in one context.

To unblock the original patch (which shouldn't be conflated with this
suggested improvement), Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

~Andrew

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