Re: [PATCH] x86emul: set (fake) operand size for AVX512CD broadcast insns

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.08.2024 12:49, Jan Beulich wrote:
> On 21.08.2024 12:35, Andrew Cooper wrote:
>> On 21/08/2024 10:28 am, Jan Beulich wrote:
>>> Back at the time I failed to pay attention to op_bytes still being zero
>>> when reaching the respective case block: With the ext0f38_table[]
>>> entries having simd_packed_int, the defaulting at the bottom of
>>> x86emul_decode() won't set the field to non-zero for F3-prefixed insns.
>>>
>>> Fixes: 37ccca740c26 ("x86emul: support AVX512CD insns")
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> This is the second such patch.
> 
> Indeed. The first one was a result of me doing some AVX10.2 work,
> covering new similar insn forms. That finding then prompted me to do
> an audit, resulting in this 2nd finding.
> 
>>   Does that mean there should be an assertion somewhere?
> 
> Not an assertion, as there actually is a check already:
> 
>     else if ( state->simd_size != simd_none )
>     {
>         generate_exception_if(!op_bytes, X86_EXC_UD);
> 
> That check is what is causing emulation to fail when op_bytes isn't set
> ahead of trying to invoke a SIMD insn via the shared logic near the end
> of the function. I don't think it needs to be any stronger than that.
> The reason this wasn't noticed so far is merely because no testing we
> have in place ever exercises these insns. Which is a shortcoming, yes,
> but one that's not very easy to overcome (as in: if we wanted to, that
> would likely mean writing quite a bit of new testing code, to cover
> everything that isn't covered right now).

Thinking about it - we have EXPECT(), which is perhaps better to use
here. That way, even if not covered by our testing, fuzzers will be in
the position to trigger it (on capable hardware only of course).

> For insns not accessing memory the value actually isn't needed / used.
> An alternative might therefore be to move that check into the OP_MEM
> conditional, and drop the fake setting of op_bytes (there are a few
> more similar to the one being added here).

Question then is whether to also move it, as indicated. From a
debugging perspective it's likely better to keep where it is, while
from a user perspective allowing reg-only insns to go through
(despite the internal error) might be preferable.

Jan