Re: [PATCH 05/11] x86emul: handle AVX512-FP16 fma-like insns

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 15/06/2022 11:28, Jan Beulich wrote:
> The Map6 encoding space is a very sparse clone of the "0f38" one. Once
> again re-use that table, as the entries corresponding to invalid opcodes
> in Map6 are simply benign with simd_size forced to other than simd_none
> (preventing undue memory reads in SrcMem handling early in
> x86_emulate()).

Again, this needs communicating in the code.

>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/x86_emulate/decode.c
> +++ b/xen/arch/x86/x86_emulate/decode.c
> @@ -1231,6 +1231,16 @@ int x86emul_decode(struct x86_emulate_st
>                          d = twobyte_table[b].desc;
>                          s->simd_size = twobyte_table[b].size ?: simd_other;
>                          break;
> +
> +                    case evex_map6:
> +                        if ( !evex_encoded() )
> +                        {
> +                            rc = X86EMUL_UNRECOGNIZED;
> +                            goto done;
> +                        }
> +                        opcode |= MASK_INSR(6, X86EMUL_OPC_EXT_MASK);
> +                        d = twobyte_table[0x38].desc;

So the manual says that map spaces 2, 3, 5 and 6 are regular maps (insn
length doesn't depend on the opcode byte), with map 3 being the only one
which takes an imm byte.

I think this means that SrcImm and SrcImmByte will cause x86_decode() to
get the wrong instruction length.

> +                        break;
>                      }
>                  }
>                  else if ( s->ext < ext_8f08 + ARRAY_SIZE(xop_table) )
> @@ -1479,6 +1489,24 @@ int x86emul_decode(struct x86_emulate_st
>              disp8scale = decode_disp8scale(twobyte_table[b].d8s, s);
>              break;
>  
> +        case ext_map6:
> +            d = ext0f38_table[b].to_mem ? DstMem | SrcReg
> +                                        : DstReg | SrcMem;
> +            if ( ext0f38_table[b].two_op )
> +                d |= TwoOp;

... but here we discard the table desc and construct it from first
principles.

Why are we processing it twice?

~Andrew