Re: [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite()
Andrew Cooper <[email protected]> Tue, 4 Aug 2026 20:37:15 +0100
| Newsgroups | gmane.comp.emulators.xen.devel |
|---|---|
| Message-ID | <[email protected]> |
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