Re: [PATCH 03/11] x86emul: handle AVX512-FP16 Map5 arithmetic insns

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

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

This...

> --- a/xen/arch/x86/x86_emulate/decode.c
> +++ b/xen/arch/x86/x86_emulate/decode.c
> @@ -1219,9 +1219,18 @@ int x86emul_decode(struct x86_emulate_st
>                          opcode |= MASK_INSR(0x0f3a, X86EMUL_OPC_EXT_MASK);
>                          d = twobyte_table[0x3a].desc;
>                          break;
> +
> +                    case evex_map5:
> +                        if ( !evex_encoded() )
> +                        {
>                      default:
> -                        rc = X86EMUL_UNRECOGNIZED;
> -                        goto done;
> +                            rc = X86EMUL_UNRECOGNIZED;
> +                            goto done;
> +                        }
> +                        opcode |= MASK_INSR(5, X86EMUL_OPC_EXT_MASK);
> +                        d = twobyte_table[b].desc;
> +                        s->simd_size = twobyte_table[b].size ?: simd_other;

... needs a comment here, and ...

> +                        break;
>                      }
>                  }
>                  else if ( s->ext < ext_8f08 + ARRAY_SIZE(xop_table) )
> @@ -1443,6 +1452,24 @@ int x86emul_decode(struct x86_emulate_st
>              }
>              break;
>  
> +        case ext_map5:
> +            switch ( b )
> +            {
> +            default:
> +                if ( !(s->evex.pfx & VEX_PREFIX_DOUBLE_MASK) )
> +                    s->fp16 = true;
> +                break;
> +
> +            case 0x2e: case 0x2f: /* v{,u}comish */
> +                if ( !s->evex.pfx )
> +                    s->fp16 = true;
> +                s->simd_size = simd_none;
> +                break;
> +            }
> +
> +            disp8scale = decode_disp8scale(twobyte_table[b].d8s, s);

... here.

Because otherwise the code reads as if it's buggy, with map5 referencing
the twobyte_table.

~Andrew