Re: [Xen-devel] [PATCH v2 5/6] x86emul: correct EVEX decoding

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.18 at 12:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/08/18 15:25, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -650,7 +650,7 @@ union evex {
>>          uint8_t w:1;
>>          uint8_t opmsk:3;
>>          uint8_t RX:1;
>> -        uint8_t bcst:1;
>> +        uint8_t br:1;
>>          uint8_t lr:2;
>>          uint8_t z:1;
> 
> I'm afraid that some of the choices of field naming in here makes the
> code impossible to follow, due to their differences from the manual. 
> Particularly, the tail end of the structure would be easier to follow if
> it were:
> 
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index f38c73b..bc0d39b 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -648,10 +648,10 @@ union evex {
>          uint8_t mbs:1;
>          uint8_t reg:4;
>          uint8_t w:1;
> -        uint8_t opmsk:3;
> -        uint8_t RX:1;
> -        uint8_t br:1;
> -        uint8_t lr:2;
> +        uint8_t aaa:3;
> +        uint8_t V:1;
> +        uint8_t b:1;
> +        uint8_t l:2;
>          uint8_t z:1;
>      };
>  };
> 
> The manual refers to EVEX.RX as the combination of the R and X fields in
> the first byte, whereas the field currently named RX in Xen is V' in the
> manual.  It is unfortunate that Intel chose L'L for the vector notation,
> but l on its own is clearer than lr.

The naming isn't very good, yes, but what you suggest won't work:
What you name b and l are dual purpose fields, hence I really want
to retain their names (standing for "broadcast or rounding" and
"length or rounding" respectively). Furthermore you'll notice there
is a field named b already. And l alone would further risk mixing up
with VEX.L.

As much as I avoided introducing a field named vvvv, I also don't
view it sensible to introduce a field named aaa. If Intel considers
these reasonable names in their manuals - so be it. They aren't
reasonable at all imo in code.

V as a name would make sense only together with vvvv, but the
bit again has dual use, and in its secondary use V is misleading
rather than helpful.

Besides all of this I now have almost 20 more patches on top of this.
I really don't see myself renaming all those field references. The
patch here has been available for long enough to give such a
comment / make such suggestions, even more so that I'm changing
a single field here only anyway.

Best I can offer is to attach comments to the fields pointing out
their SDM names. But even that I would view as slightly odd a
request _here_, as both EVEX and VEX unions have been around
in our code for quite some time.

>> @@ -2760,13 +2760,11 @@ x86_decode(
>>                          evex.raw[1] = vex.raw[1];
>>                          evex.raw[2] = insn_fetch_type(uint8_t);
>>  
>> -                        generate_exception_if(evex.mbs || !evex.mbz, 
>> EXC_UD);
>> +                        generate_exception_if(!evex.mbs || evex.mbz, 
>> EXC_UD);
>> +                        generate_exception_if(!evex.opmsk && evex.z, 
>> EXC_UD);
> 
> Where does this check derive from?  I presume you've calculated it from
> Table 2-40 in the manual, but I don't see anything there which suggests
> the restriction applies universally.  Every check in that table is
> specific to certain classes of instruction.

The check doesn't derive from any tables, but from the fact that
"zeroing-masking" makes no sense without "masking", and from the
observed hardware behavior.

>> @@ -3404,6 +3402,7 @@ x86_emulate(
>>          d = (d & ~DstMask) | DstMem;
>>          /* Becomes a normal DstMem operation from here on. */
>>      case DstMem:
>> +        generate_exception_if(ea.type == OP_MEM && evex.z, EXC_UD);
> 
> I can't find any statement that all DstMem prohibit zero-masking.
> 
> There is a statement saying that the subset of DstMem instructions which
> require an encoded k register may not use zero-masking.

Not sure what you mean by "encoded k register". Various EVEX and
ModRM fields can encode a k register.

No current instruction allows zeroing-masking on a memory destination
(nor on a k-register destination, if that's what you mean, but that's not
the same as DstMem), and I can't currently foresee this to change
without a CPUID bit telling us, if even the most obvious candidates
(VMOVAP{S,D} and VMOVDQA{32,64}) don't allow this. Hence I prefer the
check to be in a central place instead of getting repeated in a number
of places.

Note that "generic checks" in the description is not meant to imply that
these exclusively follow what Intel lists in their "generic" tables. I've done
quite a bit of prereq work and classification over the last year, and this
is one of the patterns that resulted from that work without the SDM
explicitly stating it.

Jan


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