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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

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

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.

~Andrew

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