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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/08/18 15:25, Jan Beulich wrote:
> Fix an inverted pair of checks, drop an incorrect instance of #UD
> raising for non-64-bit mode, and add further generic checks.
>
> Note: Other than SDM Vol 2 rev 067 states, EVEX.V' is _not_ ignored
>       outside of 64-bit mode when the field does not encode a register.
>       Just like EVEX.VVVV is required to be 0b1111 in that case, EVEX.V'
>       is required to be 1 there.
>
> Also rename the bcst field to br, as #UD generation for individual insns
> will need to consider both of its possible meanings.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- 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.

>      };
> @@ -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.

>  
>                          if ( !mode_64bit() )
> -                        {
> -                            generate_exception_if(!evex.RX, EXC_UD);
>                              evex.R = 1;
> -                        }
>  
>                          vex.opcx = evex.opcx;
>                          break;
> @@ -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.

~Andrew

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