Re: [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/08/2026 7:45 am, Jan Beulich wrote:
> On 04.08.2026 21:37, Andrew Cooper wrote:
>> On 03/08/2026 5:03 pm, Jan Beulich wrote:
>>> On 03.08.2026 09:20, Andrew Cooper wrote:
>>>> --- /dev/null
>>>> +++ b/tools/tests/x86-decode-lite/insns.S
>>>> @@ -0,0 +1,703 @@
>>>> +#include "macro-magic.h"
>>>> +
>>>> +        .code64
>>>> +
>>>> +        .allow_index_reg
>>>> +
>>>> +        .text
>>>> +
>>>> +DECL(tests_rel0)
>>>> +modrm:
>>>> +        /* Mod=0, Reg=0, RM {0..f} */
>>>> +        _ add %al, (%rax)
>>>> +        _ add %al, (%rcx)
>>>> +        _ add %al, (%rdx)
>>>> +        _ add %al, (%rbx)
>>>> +        _ add %al, (%rsp) /* SIB */
>>>> +        /*add %al, (%rbp)    RIP --> tests_rel4 */
>>>> +        _ add %al, (%rsi)
>>>> +        _ add %al, (%rdi)
>>>> +        _ add %al, (%r8)
>>>> +        _ add %al, (%r9)
>>>> +        _ add %al, (%r10)
>>>> +        _ add %al, (%r11)
>>>> +        _ add %al, (%r12) /* SIB */
>>>> +        /*add %al, (%r13)    RIP --> tests_rel4 */
>>>> +        _ add %al, (%r14)
>>>> +        _ add %al, (%r15)
>>>> +
>>>> +        /* Mod=1, Reg=0, RM {0..f} */
>>>> +        _ add %al, 0x01(%rax)
>>>> +        _ add %al, 0x01(%rcx)
>>>> +        _ add %al, 0x01(%rdx)
>>>> +        _ add %al, 0x01(%rbx)
>>>> +        _ add %al, 0x01(%rsp) /* SIB */
>>>> +        _ add %al, 0x01(%rbp)
>>>> +        _ add %al, 0x01(%rsi)
>>>> +        _ add %al, 0x01(%rdi)
>>>> +        _ add %al, 0x01(%r8)
>>>> +        _ add %al, 0x01(%r9)
>>>> +        _ add %al, 0x01(%r10)
>>>> +        _ add %al, 0x01(%r11)
>>>> +        _ add %al, 0x01(%r12) /* SIB */
>>>> +        _ add %al, 0x01(%r13)
>>>> +        _ add %al, 0x01(%r14)
>>>> +        _ add %al, 0x01(%r15)
>>>> +
>>>> +        /* Mod=2, Reg=0, RM {0..f} */
>>>> +        _ add %al, 0x7f000001(%rax)
>>>> +        _ add %al, 0x7f000001(%rcx)
>>>> +        _ add %al, 0x7f000001(%rdx)
>>>> +        _ add %al, 0x7f000001(%rbx)
>>>> +        _ add %al, 0x7f000001(%rsp) /* SIB */
>>>> +        _ add %al, 0x7f000001(%rbp)
>>>> +        _ add %al, 0x7f000001(%rsi)
>>>> +        _ add %al, 0x7f000001(%rdi)
>>>> +        _ add %al, 0x7f000001(%r8)
>>>> +        _ add %al, 0x7f000001(%r9)
>>>> +        _ add %al, 0x7f000001(%r10)
>>>> +        _ add %al, 0x7f000001(%r11)
>>>> +        _ add %al, 0x7f000001(%r12) /* SIB */
>>>> +        _ add %al, 0x7f000001(%r13)
>>>> +        _ add %al, 0x7f000001(%r14)
>>>> +        _ add %al, 0x7f000001(%r15)
>>>> +
>>>> +        /* Mod=3, Reg=0, RM {0..f} */
>>>> +        _ add %al, %al
>>>> +        _ add %al, %cl
>>>> +        _ add %al, %dl
>>>> +        _ add %al, %bl
>>>> +        _ add %al, %ah
>>>> +        _ add %al, %ch
>>>> +        _ add %al, %dh
>>>> +        _ add %al, %dl
>>> Perhaps also include %bpl, %sil, and %dil?
>> They're not relevant to this test, and interfere with the intentional
>> pattern set up.
> Hmm, how does a particular pattern matter here? I don't think you test those
> cases (or more generally an empty REX prefix) anywhere else.

There's nothing structurally interesting about those; I'm not testing
the assembler, and x86_decode_lite() doesn't decode registers.

The ModRM byte has multiple structurally interesting interactions with
REX prefixes, hence the coverage of Mod and RM value.

The patten makes it trivial to look at the disassembled result and check
the coverage.  (And spot the bug that's hiding in plain sight above.)

>>>> --- /dev/null
>>>> +++ b/tools/tests/x86-decode-lite/main.c
>>>> @@ -0,0 +1,111 @@
>>>> +/*
>>>> + * Userspace test harness for x86_decode_lite().
>>>> + */
>>>> +#include <stdio.h>
>>>> +
>>>> +#include "x86-emulate.h"
>>>> +
>>>> +static unsigned int nr_failures;
>>>> +#define fail(t, fmt, ...)                                       \
>>>> +({                                                              \
>>>> +    const unsigned char *insn = (t)->ip;                        \
>>>> +                                                                \
>>>> +    nr_failures++;                                              \
>>>> +                                                                \
>>>> +    (void)printf("  Fail '%s' [%02x", (t)->name, *insn);        \
>>>> +    for ( unsigned int i = 1; i < (t)->len; i++ )               \
>>>> +        printf(" %02x", insn[i]);                               \
>>>> +    printf("]\n");                                              \
>>>> +                                                                \
>>>> +    (void)printf(fmt, ##__VA_ARGS__);                           \
>>>> +})
>>>> +
>>>> +struct test {
>>>> +    const char *name;
>>>> +    void *ip;
>>>> +    unsigned long len;
>>>> +};
>>>> +
>>>> +extern const struct test
>>>> +/* Defined in insns.S, ends with sentinel */
>>>> +    tests_rel0[], /* No relocatable entry */
>>>> +    tests_rel1[], /* disp8 */
>>>> +    tests_rel4[], /* disp32 or RIP-relative */
>>>> +    tests_unsup[]; /* Unsupported instructions */
>>>> +
>>>> +static inline void run_tests(const struct test *tests, unsigned int rel_sz)
>>>> +{
>>>> +    printf("Test rel%u\n", rel_sz);
>>>> +
>>>> +    for ( unsigned int i = 0; tests[i].name; ++i )
>>>> +    {
>>>> +        const struct test *t = &tests[i];
>>>> +        x86_decode_lite_t r;
>>>> +
>>>> +        /*
>>>> +         * Don't end strictly at t->len.  This provides better diagnostics if
>>>> +         * too many bytes end up getting consumed.
>>>> +         */
>>>> +        r = x86_decode_lite(t->ip, t->ip + /* t->len */ 20);
>>> For the excess bytes to at least be legitimate to access (not causing UB),
>>> shouldn't finish_arr emit enough filler bytes?
>> finish_arr is the wrong place, but I've folded in:
>>
>> diff --git a/tools/tests/x86-decode-lite/insns.S b/tools/tests/x86-decode-lite/insns.S
>> index e52c2934c8d8..dc017016b2d2 100644
>> --- a/tools/tests/x86-decode-lite/insns.S
>> +++ b/tools/tests/x86-decode-lite/insns.S
>> @@ -695,6 +695,13 @@ unsup_insn: /* Instructions that would complicated decode, or shouldn't be used
>>  
>>  END(tests_unsup)
>>  
>> +        /*
>> +         * For improved diagnostics, we allow some overreading of the
>> +         * instruction under test.  Ensure there are good bytes to read.
>> +         */
>> +overread_padding:
>> +        .skip 20
>> +
>>          /* This is here to cause jmps to use their disp32 form. */
>>          .section .text.other_section, "ax", @progbits
>>  other_section:
> How would this help? run_tests() is never invoked with tests_unsup[] as
> argument. And run_tests_unsup() wants to only fetch up to t->len.

Oh, in which case nothing is needed at all.  I'll take it back out.

~Andrew