On 21/08/2024 4:03 pm, Jan Beulich wrote:
> 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.coop...@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>> 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.
Sure, but it's a well defined non-fatal exit path.
>
>> 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.
Acked-by: Andrew Cooper <andrew.coop...@citrix.com>
>
>>> --- 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.
Is that BUG() local to your tree? I cant see it in staging.
Deferring this to a later patch is fine.
~Andrew
