Re: [PATCH] x86emul: convert op_bytes/opc checks in SIMD emulation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.08.2024 15:37, Andrew Cooper wrote:
> On 21/08/2024 2:30 pm, Jan Beulich wrote:
>> Delivering #UD for an internal shortcoming of the emulator isn't quite
>> right. Similarly BUG() is bigger a hammer than needed.
>>
>> Switch to using EXPECT() instead.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> To confirm, this is ASSERT_UNREACHABLE() (which fuzzing will now notice
> as an error), and unhandleable in release builds (which ultimately ends
> up in #UD)?

Yes, at least mostly. What exactly UNHANDLEABLE converts to depends on
the use site, I think.

> I think it would be helpful to at least note the fuzzing aspect in the
> commit message.

I've added "This way even for insns not covered by the test harness
fuzzers will have a chance of noticing issues, should any still exist
or new ones be introduced" to the 2nd paragraph.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -8114,13 +8114,13 @@ x86_emulate(
>>      }
>>      else if ( state->simd_size != simd_none )
>>      {
>> -        generate_exception_if(!op_bytes, X86_EXC_UD);
>>          generate_exception_if((vex.opcx && (d & TwoOp) &&
>>                                 (vex.reg != 0xf || (evex_encoded() && 
>> !evex.RX))),
>>                                X86_EXC_UD);
>>  
>> -        if ( !opc )
>> -            BUG();
>> +        EXPECT(op_bytes);
>> +        EXPECT(opc);
> 
> This is the only BUG() in x86_emulate.c, and it's right to get rid of it
> IMO.
> 
> Therefore, we should have a hunk removing it from
> tools/tests/x86_emulator/x86-emulate.h too, which will prevent
> reintroduction.
> 
> Maybe even undef BUG somewhere in x86_emulate/private.h?

Both of these actions can only be taken if the other BUG() in decode.c
also goes away. But yes, what you suggest is probably the best course of
action. I guess I'll do that in yet another patch, though.

Jan