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@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -5929,6 +5929,7 @@ x86_emulate(
                                evex.w == ((b >> 4) & 1)),
                               X86_EXC_UD);
         d |= TwoOp;
+        op_bytes = 1; /* fake */
         /* fall through */
     case X86EMUL_OPC_EVEX_66(0x0f38, 0xc4): /* vpconflict{d,q} [xyz]mm/mem,[xyz]mm{k} */
         fault_suppression = false;

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@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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@suse.com>

This is the second such patch.  Does that mean there should be an
assertion somewhere?

~Andrew

>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -5929,6 +5929,7 @@ x86_emulate(
>                                 evex.w == ((b >> 4) & 1)),
>                                X86_EXC_UD);
>          d |= TwoOp;
> +        op_bytes = 1; /* fake */
>          /* fall through */
>      case X86EMUL_OPC_EVEX_66(0x0f38, 0xc4): /* vpconflict{d,q} [xyz]mm/mem,[xyz]mm{k} */
>          fault_suppression = false;

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@suse.com>
> 
> 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

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@suse.com>
>>
>> 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