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

On 17.08.2026 14:40, Andrew Cooper wrote:
> 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.

If covering _every_ bit pattern of ModR/M.rm is of interest, I simply find
I hard to see why also covering them empty-REX case should be of no
interest at all.

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

Oh, I see (now).

>>>>> --- /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:
>>>
>>> --- 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.

Yet then, as previously indicated, the possible overrun in run_tests()'
fetching will want covering. Hence why I suggested the particular other
place to put extra padding.

Jan