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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

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).

Jan