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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.12.18 at 13:02, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/12/2018 15:22, Jan Beulich wrote:
>>>>> On 18.12.18 at 15:28, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> 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 ?
>> Well, yes, I'd been considering "brs" (I dislike "bsr" for its collision
>> with the same name insn mnemonic).
>>
>>> /* 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.
>> Well, it's the common case that the field has two meanings: SAE
>> or ER with all register operands and BROADCAST with a memory
>> one. Exceptions are when either broadcast or SAE/ER are not
>> permitted for a particular major opcode.
> 
> Lovely... The SDM uses {er}, naming it "embedded rounding", for the
> field referred to as Static Rounding in the EVEX chapter.  I think I'll
> ask Intel to fix this.

Right, both names are used.

> How do we distinguish between SAE and ER then?  It looks like ER implies
> SAE, and they are both only usable by scalar or full-width float operations.

This is an insn property. ER indeed implies SAE (that's spelled out
somewhere), but I think the reality is the other way around: It's
always ER, but in some case there's simply nothing to round, in
which case EVEX.L'L will be ignored, and the insn will be specified
to allow {sae} rather than {er}.

>>> 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>
>> Thanks. Question then is - are you convinced enough of your
>> proposal for me to re-work things before posting v7 of the
>> main series? And if so are you fine with "brs" instead of "bsr"
>> (and perhaps "er" instead of "rounding", to be closer to SDM
>> terminology)?
> 
> brs is fine as an alternative bsr.  It retains the important property of
> not being able to be confused as "broadcast".

Okay, I'll switch to that then in order to get v7 out.

> I suppose that at the point that we have sae, er is also fine, and as
> you point out, it is closer to SDM terminology.

I'm not going to introduce any aliases, due to - as said - them not
being usable in the common case because of the bit having dual
meaning until you've split memory and register operand cases.

Jan


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