Re: [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite()
Jan Beulich <[email protected]> Mon, 3 Aug 2026 18:03:26 +0200
| Newsgroups | gmane.comp.emulators.xen.devel |
|---|---|
| Message-ID | <[email protected]> |
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