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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> +onebyte_row_9x:
> +        _ nop
> +        _ pause
> +        _ xchg %ax, %ax
> +        _ xchg %eax, %eax
> +        _ xchg %rax, %rax
> +        _ rex.w xchg %rax, %rax
> +        _ cltq
> +        _ cqto
> +        _ wait
> +        _ pushf
> +        _ popf
> +        _ sahf
> +        _ lahf
> +
> +onebyte_row_ax:

May I suggest onebyte_row_Ax?

> +DECL(tests_rel1)
> +disp8:
> +1:
> +        _ jo   1b
> +        _ jno  1b
> +        _ jb   1b
> +        _ jae  1b
> +        _ je   1b
> +        _ jne  1b
> +        _ jbe  1b
> +        _ ja   1b
> +        _ js   1b
> +        _ jns  1b
> +        _ jp   1b
> +        _ jnp  1b
> +        _ jl   1b
> +        _ jge  1b
> +        _ jle  1b
> +        _ jg   1b
> +        _ jmp  1b
> +
> +disp8_rex:
> +        _ rex.w jo   1b
> +        _ rex.w jno  1b
> +        _ rex.w jb   1b
> +        _ rex.w jae  1b
> +        _ rex.w je   1b
> +        _ rex.w jne  1b
> +        _ rex.w jbe  1b
> +        _ rex.w ja   1b
> +        _ rex.w js   1b
> +        _ rex.w jns  1b
> +        _ rex.w jp   1b
> +        _ rex.w jnp  1b
> +        _ rex.w jl   1b
> +        _ rex.w jge  1b
> +        _ rex.w jle  1b
> +        _ rex.w jg   1b
> +        _ rex.w jmp  1b
> +END(tests_rel1)

What's the idea behind the separate REX.W testing? It almost suggests that
tests with an operand size prefix also may want adding. Except that's
difficult, because of ...

> +DECL(tests_rel4)
> +disp32:
> +        _ call   other_section
> +        _ jmp    other_section
> +        _ jo     other_section
> +        _ jno    other_section
> +        _ jb     other_section
> +        _ jae    other_section
> +        _ je     other_section
> +        _ jne    other_section
> +        _ jbe    other_section
> +        _ ja     other_section
> +        _ js     other_section
> +        _ jns    other_section
> +        _ jp     other_section
> +        _ jnp    other_section
> +        _ jl     other_section
> +        _ jge    other_section
> +        _ jle    other_section
> +        _ jg     other_section
> +        _ xbegin other_section
> +
> +disp32_rex:
> +        _ rex.w call   other_section
> +        _ rex.w jmp    other_section
> +        _ rex.w jo     other_section
> +        _ rex.w jno    other_section
> +        _ rex.w jb     other_section
> +        _ rex.w jae    other_section
> +        _ rex.w je     other_section
> +        _ rex.w jne    other_section
> +        _ rex.w jbe    other_section
> +        _ rex.w ja     other_section
> +        _ rex.w js     other_section
> +        _ rex.w jns    other_section
> +        _ rex.w jp     other_section
> +        _ rex.w jnp    other_section
> +        _ rex.w jl     other_section
> +        _ rex.w jge    other_section
> +        _ rex.w jle    other_section
> +        _ rex.w jg     other_section
> +        _ rex.w xbegin other_section

... vendor differences here. Perhaps the decoder itself would better
reject handling of operand-size-prefixed branches.

> +opsize_branch: /* 66-prefixed branches are decoded differently by vendors */
> +        _ data16 call   other_section
> +        _ data16 jmp    other_section
> +        _ data16 jo     other_section
> +        _ data16 jno    other_section
> +        _ data16 jb     other_section
> +        _ data16 jae    other_section
> +        _ data16 je     other_section
> +        _ data16 jne    other_section
> +        _ data16 jbe    other_section
> +        _ data16 ja     other_section
> +        _ data16 js     other_section
> +        _ data16 jns    other_section
> +        _ data16 jp     other_section
> +        _ data16 jnp    other_section
> +        _ data16 jl     other_section
> +        _ data16 jge    other_section
> +        _ data16 jle    other_section
> +        _ data16 jg     other_section
> +        _ data16 xbegin other_section

Oh, you even cover the case here. For XBEGIN, however, this can only be pure
guesswork as to AMD behavior, I suppose.

I also don't see how you force which form you want.

> --- /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?

> --- /dev/null
> +++ b/tools/tests/x86-decode-lite/x86-emulate.h
> @@ -0,0 +1,27 @@
> +#ifndef X86_EMULATE_H
> +#define X86_EMULATE_H
> +
> +#include <assert.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <xen/asm/x86-defns.h>
> +#include <xen/asm/x86-vendors.h>
> +
> +#include <xen-tools/common-macros.h>
> +
> +#define ASSERT assert
> +
> +#define printk(...)
> +
> +#define likely
> +#define unlikely
> +#define cf_check
> +#define init_or_livepatch
> +#define init_or_livepatch_const
> +
> +#include "x86_emulate/x86_emulate.h"

Why does this end up being needed?

Jan