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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.08.2022 20:14, Andrew Cooper wrote:
> 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.

What SrcImm / SrcImmByte are you talking about here? This twobyte_table[]
entry doesn't have such.

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

First of all this follows pre-existing code, where 0F38 is handled in a
similar manner. The primary reason for the two step approach though is
that we want to pick up the ModRM flag here, which the other table
doesn't have. Instead other tables might use its aliases (vSIB only for
now, which - yes - doesn't have a use yet in Map6, but this might
change going forward).

I also wonder why you comment on this here, but you didn't for patch 3,
where you've only asked that I add comments (which of course I will do).

Jan