|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite()
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.
>>> +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.
I see, albeit I very much hope osize would never, ever have an effect on Imm8
encodings, as far as the size of the immediate goes.
>> 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.
To exclude 66 is all I was suggesting. REX.W isn't treated differently by
the vendors, afaik, likely simply because it's meaningless altogether for
these insns.
>>> +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.)
By default, yes. Quite some time ago CALL and JMP were covered more
correctly, via the -mamd64 / -mintel64 cmdline options. Not very long ago
I realized we had never extended that to Jcc.
> 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.
Changing the default behavior seems extremely unlikely to me. Changing
the non default behavior, otoh, has happened (and if need be could
happen again).
Anyway, all of this is becoming moot if 66 was rejected on branches.
>>> --- /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:
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.
>>> --- /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);
> | ^~~~~~~~~~~~~~~
Hmm, yes, that should have been obvious, if only I didn't expect decode-lite
to be largely (up to entirely) independent of the core emulator, irrespective
of its placement in the same dir. x86_emulate/x86_emulate.h is a pretty
involved header, which I think would be nice to avoid growing more
dependencies on. Then again it looks as if about every object file already
depends on it (which imo is bad).
Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |