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

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

[Jan Beulich]

The avx512_vlen_check() invocation needs to be conditional.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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 )
+            avx512_vlen_check(true);
         get_fpu(X86EMUL_FPU_zmm);
 
         opc = init_evex(stub);



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

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

[Andrew Cooper]

On 10/12/2018 11:32, Jan Beulich wrote:
> The avx512_vlen_check() invocation needs to be conditional.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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.

>
> --- 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()"

~Andrew

> +            avx512_vlen_check(true);
>          get_fpu(X86EMUL_FPU_zmm);
>  
>          opc = init_evex(stub);
>
>


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

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

[Jan Beulich]

>>> On 10.12.18 at 14:20, <andrew.cooper3@citrix.com> 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@suse.com>
> 
> 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).

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

Jan



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

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

[Andrew Cooper]

On 10/12/2018 13:56, Jan Beulich wrote:
>>>> On 10.12.18 at 14:20, <andrew.cooper3@citrix.com> 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@suse.com>
>> 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@citrix.com>

~Andrew

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

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

[Jan Beulich]

>>> On 18.12.18 at 15:28, <andrew.cooper3@citrix.com> wrote:
> On 10/12/2018 13:56, Jan Beulich wrote:
>>>>> On 10.12.18 at 14:20, <andrew.cooper3@citrix.com> 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@suse.com>
>>> 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.

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

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)?

Jan


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

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

[Andrew Cooper]

On 18/12/2018 15:22, Jan Beulich wrote:
>>>> On 18.12.18 at 15:28, <andrew.cooper3@citrix.com> wrote:
>> On 10/12/2018 13:56, Jan Beulich wrote:
>>>>>> On 10.12.18 at 14:20, <andrew.cooper3@citrix.com> 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@suse.com>
>>>> 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@citrix.com>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 19.12.18 at 13:02, <andrew.cooper3@citrix.com> wrote:
> On 18/12/2018 15:22, Jan Beulich wrote:
>>>>> On 18.12.18 at 15:28, <andrew.cooper3@citrix.com> wrote:
>>> On 10/12/2018 13:56, Jan Beulich wrote:
>>>>>>> On 10.12.18 at 14:20, <andrew.cooper3@citrix.com> 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@suse.com>
>>>>> 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@citrix.com>
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel