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

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

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.

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

Ok.

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

Testing osize handling vs Imm8/Imm.

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

Excluding 66-prefix is easy, but excluding rex.w on jumps is hard and
would require extra logic.

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

Remember that RTM is available on Zen2 if you know which chickenbits to
clobber.

I've not tried.  I expect it's more likely that they behave consistently
than differently.

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

Binutils always produces AMD behaviour.  (As far as I can see.)

This is in the negative-tests section, which confirms that
x86_decode_lite() rejects the byte pattern.

If Binutils changes behaviour, the test will start failing.

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



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

Well, this for starters:

main.c: In function ‘run_tests’:
main.c:43:9: error: unknown type name ‘x86_decode_lite_t’
   43 |         x86_decode_lite_t r;
      |         ^~~~~~~~~~~~~~~~~
main.c:49:13: error: implicit declaration of function ‘x86_decode_lite’ [-Werror=implicit-function-declaration]
   49 |         r = x86_decode_lite(t->ip, t->ip + /* t->len */ 20);
      |             ^~~~~~~~~~~~~~~



~Andrew