* [PATCH v3 0/5] x86/alternatives: Adjust all insn-relative fields
@ 2026-08-03 7:20 Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite() Andrew Cooper
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Andrew Cooper @ 2026-08-03 7:20 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Teddy Astie
Alternatives have had a reasonably severe restriction since their
introduction. This has been the source of several bugs, and several
inefficiencies particularly in the speculative safety paths.
https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2724019865
v3:
* Run through Gitlab CI.
* Fix up Eclair issues, Clang issues, and older-binutils issues.
Andrew Cooper (5):
x86/emul: Introduce x86_decode_lite()
tests/x86: Introduce a userspace test harness for x86_decode_lite()
x86/alternative: Walk all replacements during self tests
x86/alternative: Relocate all insn-relative fields
x86/spec-ctrl: Introduce and use DO_COND_BHB_SEQ
tools/tests/Makefile | 1 +
tools/tests/x86-decode-lite/.gitignore | 1 +
tools/tests/x86-decode-lite/Makefile | 56 ++
tools/tests/x86-decode-lite/insns.S | 703 ++++++++++++++++++++++
tools/tests/x86-decode-lite/macro-magic.h | 62 ++
tools/tests/x86-decode-lite/main.c | 111 ++++
tools/tests/x86-decode-lite/x86-emulate.h | 27 +
xen/arch/x86/alternative.c | 102 +++-
xen/arch/x86/hvm/vmx/entry.S | 12 +-
xen/arch/x86/include/asm/spec_ctrl_asm.h | 43 +-
xen/arch/x86/x86_emulate/Makefile | 6 +
xen/arch/x86/x86_emulate/decode-lite.c | 330 ++++++++++
xen/arch/x86/x86_emulate/x86_emulate.h | 14 +
13 files changed, 1434 insertions(+), 34 deletions(-)
create mode 100644 tools/tests/x86-decode-lite/.gitignore
create mode 100644 tools/tests/x86-decode-lite/Makefile
create mode 100644 tools/tests/x86-decode-lite/insns.S
create mode 100644 tools/tests/x86-decode-lite/macro-magic.h
create mode 100644 tools/tests/x86-decode-lite/main.c
create mode 100644 tools/tests/x86-decode-lite/x86-emulate.h
create mode 100644 xen/arch/x86/x86_emulate/decode-lite.c
--
2.39.5
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite()
2026-08-03 7:20 [PATCH v3 0/5] x86/alternatives: Adjust all insn-relative fields Andrew Cooper
@ 2026-08-03 7:20 ` Andrew Cooper
2026-08-03 9:14 ` Andrew Cooper
2026-08-03 15:26 ` Jan Beulich
2026-08-03 7:20 ` [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite() Andrew Cooper
` (3 subsequent siblings)
4 siblings, 2 replies; 14+ messages in thread
From: Andrew Cooper @ 2026-08-03 7:20 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Teddy Astie
In order to relocate all IP-relative fields in an alternative replacement
block, we need to decode the instructions enough to obtain their length and
any relative fields.
Full x86_decode() is far too heavyweight, so introduce a minimal form which
can make several simplifying assumptions.
This a mostly-complete decoder for integer instruction in the onebyte and
twobyte maps. Some instructions are intentionally unrecognised, as finding
them in an alternative is more likely to be a bug than intentional. Some
instruction groups and prefixes are unimplemented to reduce decode complexity.
This logic can decode all alternative blocks that exist in Xen right now.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger@xenproject.org>
CC: Teddy Astie <teddy.astie@vates.tech>
v3:
* Rearrange decode tables to satisfy comment requests without splitting
* Recognise UDB now it's used by Xen
* Fix MISRA violations
* Misc other changes
v2:
* Switch to 0 on failure, rel_sz in bytes
* Mostly complete the integer instructions; paird with userspace harness
* Put in .init when !CONFIG_LIVEPATCH
---
xen/arch/x86/x86_emulate/Makefile | 6 +
xen/arch/x86/x86_emulate/decode-lite.c | 330 +++++++++++++++++++++++++
xen/arch/x86/x86_emulate/x86_emulate.h | 14 ++
3 files changed, 350 insertions(+)
create mode 100644 xen/arch/x86/x86_emulate/decode-lite.c
diff --git a/xen/arch/x86/x86_emulate/Makefile b/xen/arch/x86/x86_emulate/Makefile
index 295e602f6b86..679bddbb1584 100644
--- a/xen/arch/x86/x86_emulate/Makefile
+++ b/xen/arch/x86/x86_emulate/Makefile
@@ -17,3 +17,9 @@ obj-y += decode.o
obj-$(CONFIG_HVM) += fpu.o
obj-y += util.o
obj-y += util-xen.o
+
+ifeq ($(CONFIG_LIVEPATCH),y)
+obj-y += decode-lite.o
+else
+obj-bin-y += decode-lite.init.o
+endif
diff --git a/xen/arch/x86/x86_emulate/decode-lite.c b/xen/arch/x86/x86_emulate/decode-lite.c
new file mode 100644
index 000000000000..131cc07d5516
--- /dev/null
+++ b/xen/arch/x86/x86_emulate/decode-lite.c
@@ -0,0 +1,330 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifdef __XEN__
+# include <xen/init.h>
+# include <xen/livepatch.h>
+#endif
+
+#include "private.h"
+
+#undef ModRM
+
+/*
+ * Bare minimum x86 instruction decoder to parse the alternative replacement
+ * instructions and locate the IP-relative references that may need updating.
+ *
+ * These are:
+ * - disp8/32 from near direct branches
+ * - RIP-relative memory references
+ *
+ * The following simplifications are used:
+ * - All code is 64bit, the instruction stream is well formed and safe to
+ * read.
+ * - Instruction groups and prefixes not used by Xen's current alternatives
+ * are not implemented in order to reduce the decode complexity.
+ * - Certain instructions are intentionally not recognised, when it is more
+ * likely for their presence to be an error than intentional.
+ *
+ * Inputs:
+ * @ip The position to start decoding from.
+ * @end End of the replacement block. Exceeding this is considered an error.
+ *
+ * Returns: x86_decode_lite_t
+ * - On failure, length of 0.
+ * - On success, length > 0. For rel_sz > 0, rel points at the relative
+ * field in the instruction stream.
+ */
+x86_decode_lite_t init_or_livepatch x86_decode_lite(void *ip, void *end)
+{
+#define Imm8 (1 << 0)
+#define Imm (1 << 1)
+#define Moffs (1 << 2)
+#define Branch (1 << 5) /* Near direct branches, which have a displacement */
+#define ModRM (1 << 6)
+#define Known (1 << 7)
+
+ static const uint8_t init_or_livepatch_const onebyte[256] = {
+
+#define ALU_OPS(x) \
+ [(x) + 0] = (Known|ModRM), \
+ [(x) + 1] = (Known|ModRM), \
+ [(x) + 2] = (Known|ModRM), \
+ [(x) + 3] = (Known|ModRM), \
+ [(x) + 4] = (Known|Imm8), \
+ [(x) + 5] = (Known|Imm)
+
+ ALU_OPS(0x00) /* ADD */, ALU_OPS(0x08) /* OR */,
+ ALU_OPS(0x10) /* ADC */, ALU_OPS(0x18) /* SBB */,
+ ALU_OPS(0x20) /* AND */, ALU_OPS(0x28) /* SUB */,
+ ALU_OPS(0x30) /* XOR */, ALU_OPS(0x38) /* CMP */,
+
+#undef ALU_OPS
+
+ [0x50 ... 0x5f] = (Known), /* PUSH/POP %reg */
+
+ [0x62] = 0, /* BOUND, but also EVEX prefix, not implemented. */
+ [0x63] = (Known|ModRM), /* MOVSxd */
+
+ [0x68] = (Known|Imm), /* PUSH $imm */
+ [0x69] = (Known|ModRM|Imm), /* IMUL $imm */
+ [0x6a] = (Known|Imm8), /* PUSH $imm8 */
+ [0x6b] = (Known|ModRM|Imm8), /* PUSH $imm8 */
+ [0x6c ... 0x6f] = (Known), /* INS/OUTS */
+ [0x70 ... 0x7f] = (Known|Branch|Imm8), /* Jcc disp8 */
+ [0x80] = (Known|ModRM|Imm8), /* Grp1 */
+ [0x81] = (Known|ModRM|Imm), /* Grp1 */
+
+ [0x83] = (Known|ModRM|Imm8), /* Grp1 */
+ [0x84 ... 0x8e] = (Known|ModRM), /* TEST/XCHG/MOV/MOV-SREG/LEA */
+ [0x8f] = 0, /* Grp1A - POP but also XOP prefix, not implemented. */
+ [0x90 ... 0x99] = (Known), /* NOP/XCHG %rAX/CLTQ/CQTO */
+
+ [0x9b ... 0x9f] = (Known), /* FWAIT/PUSHF/POPF/SAHF/LAHF */
+ [0xa0 ... 0xa3] = (Known|Moffs), /* MOVABS */
+ [0xa4 ... 0xa7] = (Known), /* MOVS/CMPS */
+ [0xa8] = (Known|Imm8), /* TEST %al */
+ [0xa9] = (Known|Imm), /* TEST %rAX */
+ [0xaa ... 0xaf] = (Known), /* STOS/LODS/SCAS */
+ [0xb0 ... 0xb7] = (Known|Imm8), /* MOV $imm8, %reg */
+ [0xb8 ... 0xbf] = (Known|Imm), /* MOV $imm{16,32,64}, %reg */
+ [0xc0 ... 0xc1] = (Known|ModRM|Imm8), /* Grp2 (ROL..SAR $imm8, %reg) */
+
+ [0xc3] = (Known), /* RET */
+ [0xc4 ... 0xc5] = 0, /* LES/LDS but also VEX prefixes, not implemented. */
+ [0xc6] = (Known|ModRM|Imm8), /* Grp11, Further ModRM decode */
+ [0xc7] = (Known|ModRM|Imm), /* Grp11, Further ModRM decode */
+
+ [0xcb ... 0xcc] = (Known), /* LRET/INT3 */
+ [0xcd] = (Known|Imm8), /* INT $imm8 */
+
+ [0xd0 ... 0xd3] = (Known|ModRM), /* Grp2 (ROL..SAR {$1,%cl}, %reg) */
+
+ [0xd6] = (Known), /* UDB */
+
+ [0xe4 ... 0xe7] = (Known|Imm8), /* IN/OUT $imm8 */
+ [0xe8 ... 0xe9] = (Known|Branch|Imm), /* CALL/JMP disp32 */
+
+ [0xeb] = (Known|Branch|Imm8), /* JMP disp8 */
+ [0xec ... 0xef] = (Known), /* IN/OUT %dx */
+
+ [0xf1] = (Known), /* ICEBP */
+
+ [0xf4] = (Known), /* HLT */
+ [0xf5] = (Known), /* CMC */
+ [0xf6 ... 0xf7] = (Known|ModRM), /* Grp3, Further ModRM decode */
+ [0xf8 ... 0xfd] = (Known), /* CLC ... STD */
+ [0xfe ... 0xff] = (Known|ModRM), /* Grp4 */
+ };
+ static const uint8_t init_or_livepatch_const twobyte[256] = {
+ [0x00 ... 0x03] = (Known|ModRM), /* Grp6/Grp7/LAR/LSL */
+
+ [0x0b] = (Known), /* UD2 */
+
+ [0x18 ... 0x1f] = (Known|ModRM), /* Grp16 (Hint Nop) */
+ [0x20 ... 0x23] = (Known|ModRM), /* MOV %cr/%dr */
+
+ [0x30 ... 0x33] = (Known), /* WRMSR/RDTSC/RDMSR/RDPMC */
+
+ [0x40 ... 0x4f] = (Known|ModRM), /* CMOVcc */
+
+ [0x80 ... 0x8f] = (Known|Branch|Imm), /* Jcc disp32 */
+ [0x90 ... 0x9f] = (Known|ModRM), /* SETcc */
+
+ [0xa0 ... 0xa2] = (Known), /* PUSH/POP %fs/CPUID */
+ [0xa3] = (Known|ModRM), /* BT */
+ [0xa4] = (Known|ModRM|Imm8), /* SHLD $imm8 */
+ [0xa5] = (Known|ModRM), /* SHLD %cl */
+
+ [0xa8 ... 0xa9] = (Known), /* PUSH/POP %gs */
+
+ [0xab] = (Known|ModRM), /* BTS */
+ [0xac] = (Known|ModRM|Imm8), /* SHRD $imm8 */
+ [0xad ... 0xaf] = (Known|ModRM), /* SHRD %cl/Grp15/IMUL */
+
+ [0xb0 ... 0xb9] = (Known|ModRM), /* CMPXCHG/LSS/BTR/LFS/LGS/MOVZxx/POPCNT/UD1 */
+ [0xba] = (Known|ModRM|Imm8), /* Grp8 */
+ [0xbb ... 0xbf] = (Known|ModRM), /* BTC/BSF/BSR/MOVSX */
+ [0xc0 ... 0xc1] = (Known|ModRM), /* XADD */
+ [0xc7] = (Known|ModRM), /* Grp9 */
+ [0xc8 ... 0xcf] = (Known), /* BSWAP */
+ };
+
+ void *start = ip, *rel = NULL;
+ unsigned int opc, rel_sz = 0;
+ uint8_t b, d, rex = 0, osize = 4;
+
+#define OPC_TWOBYTE (1 << 8)
+
+ /* Mutates IP, uses END. */
+#define FETCH(ty) \
+ ({ \
+ ty _val; \
+ \
+ if ( (ip + sizeof(ty)) > end ) \
+ goto overrun; \
+ _val = *(ty *)ip; \
+ ip += sizeof(ty); \
+ _val; \
+ })
+
+ for ( ;; ) /* Prefixes */
+ {
+ switch ( b = FETCH(uint8_t) )
+ {
+ case 0x26: /* ES override */
+ case 0x2e: /* CS override */
+ case 0x36: /* DS override */
+ case 0x3e: /* SS override */
+ case 0x64: /* FS override */
+ case 0x65: /* GS override */
+ case 0xf0: /* LOCK */
+ case 0xf2: /* REPNE */
+ case 0xf3: /* REP */
+ break;
+
+ case 0x66: /* Operand size override */
+ osize = 2;
+ break;
+
+ /* case 0x67: Address size override, not implemented */
+
+ case 0x40 ... 0x4f: /* REX */
+ rex = b;
+ continue;
+
+ default:
+ goto prefixes_done;
+ }
+ rex = 0; /* REX cancelled by subsequent legacy prefix. */
+ }
+ prefixes_done:
+
+ if ( rex & REX_W )
+ osize = 8;
+
+ /* Fetch the main opcode byte(s) */
+ if ( b == 0x0f )
+ {
+ b = FETCH(uint8_t);
+ opc = OPC_TWOBYTE | b;
+
+ d = twobyte[b];
+ }
+ else
+ {
+ opc = b;
+ d = onebyte[b];
+ }
+
+ if ( unlikely(!(d & Known)) )
+ goto unknown;
+
+ if ( d & ModRM )
+ {
+ uint8_t modrm = FETCH(uint8_t);
+ uint8_t mod = modrm >> 6;
+ uint8_t reg = (modrm >> 3) & 7;
+ uint8_t rm = modrm & 7;
+
+ /* ModRM/SIB decode */
+ if ( mod == 0 && rm == 5 ) /* RIP relative */
+ {
+ rel = ip;
+ rel_sz = 4;
+ FETCH(int32_t);
+ }
+ else if ( mod != 3 && rm == 4 ) /* SIB */
+ {
+ uint8_t sib = FETCH(uint8_t);
+ uint8_t base = sib & 7;
+
+ if ( mod == 0 && base == 5 )
+ goto disp32;
+ }
+
+ if ( mod == 1 ) /* disp8 */
+ FETCH(int8_t);
+ else if ( mod == 2 ) /* disp32 */
+ {
+ disp32:
+ FETCH(int32_t);
+ }
+
+ /* ModRM based decode adjustements */
+ switch ( opc )
+ {
+ case 0xc7: /* Grp11 XBEGIN is a near direct branch. */
+ if ( modrm == 0xf8 )
+ d |= Branch;
+ break;
+
+ case 0xf6: /* Grp3 TEST(s) have extra Imm8 */
+ if ( reg == 0 || reg == 1 )
+ d |= Imm8;
+ break;
+
+ case 0xf7: /* Grp3 TEST(s) have extra Imm */
+ if ( reg == 0 || reg == 1 )
+ d |= Imm;
+ break;
+ }
+ }
+
+ if ( d & Branch )
+ {
+ /*
+ * We don't tolerate 66-prefixed call/jmp in alternatives. Some are
+ * genuinely decoded differently between Intel and AMD CPUs.
+ *
+ * We also don't implement APX instructions, so don't have to cope
+ * with JMPABS which is the first branch to have an 8-byte immediate.
+ */
+ if ( osize < 4 )
+ goto bad_osize;
+
+ rel = ip;
+ rel_sz = (d & Imm8) ? 1 : 4;
+ }
+
+ if ( d & (Imm | Imm8 | Moffs) )
+ {
+ if ( d & Imm8 )
+ osize = 1;
+ else if ( d & Moffs )
+ osize = 8;
+ else if ( osize == 8 && !(opc >= 0xb8 && opc <= 0xbf) )
+ osize = 4;
+
+ switch ( osize )
+ {
+ case 1: FETCH(uint8_t); break;
+ case 2: FETCH(uint16_t); break;
+ case 4: FETCH(uint32_t); break;
+ case 8: FETCH(uint64_t); break;
+ default: goto bad_osize;
+ }
+ }
+
+ return (x86_decode_lite_t){ ip - start, rel_sz, rel };
+
+ bad_osize:
+ printk(XENLOG_ERR "%s() Bad osize %u in %*ph\n",
+ __func__, osize,
+ (int)(unsigned long)(end - start), start);
+ return (x86_decode_lite_t){ 0, 0, NULL };
+
+ unknown:
+ printk(XENLOG_ERR "%s() Unknown opcode in %*ph <%02x> %*ph\n",
+ __func__,
+ (int)(unsigned long)(ip - 1 - start), start, b,
+ (int)(unsigned long)(end - ip), ip);
+ return (x86_decode_lite_t){ 0, 0, NULL };
+
+ overrun:
+ printk(XENLOG_ERR "%s() Decode overrun, got %*ph\n",
+ __func__,
+ (int)(unsigned long)(end - start), start);
+ return (x86_decode_lite_t){ 0, 0, NULL };
+
+#undef FETCH
+}
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 0fd20747dc43..566a8297d8a5 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -835,4 +835,18 @@ static inline void x86_emul_reset_event(struct x86_emulate_ctxt *ctxt)
ctxt->event = (struct x86_event){};
}
+/*
+ * x86_decode_lite(). Very minimal decoder for managing alternatives.
+ *
+ * @len is 0 on error, or nonzero on success. If the instruction has a
+ * relative field, @rel_sz is nonzero, and @rel points at the field.
+ */
+typedef struct {
+ uint8_t len;
+ uint8_t rel_sz; /* bytes: 0, 1 or 4 */
+ void *rel;
+} x86_decode_lite_t;
+
+x86_decode_lite_t x86_decode_lite(void *ip, void *end);
+
#endif /* __X86_EMULATE_H__ */
--
2.39.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite()
2026-08-03 7:20 [PATCH v3 0/5] x86/alternatives: Adjust all insn-relative fields Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite() Andrew Cooper
@ 2026-08-03 7:20 ` Andrew Cooper
2026-08-03 16:03 ` Jan Beulich
2026-08-03 7:20 ` [PATCH v3 3/5] x86/alternative: Walk all replacements during self tests Andrew Cooper
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2026-08-03 7:20 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Teddy Astie
All the interesting behaviour is in insns.S.
There are 4 interesting cases; "not an instruction we tolerate", and one we do
tolerate, split by no relation, disp8 or disp32. The DECL()/END() macros
start and terminate the tests_*[] arrays used by C.
Between DECL()/END(), a macro named _ adds an entry into the array, including
a name and the length of the instruction according to the assembler, while
being as visually unintrusive as possible.
Plain labels are ad-hoc and there to aid legibility during disassembly. In a
couple of cases, the macro named n (for name) allows for choosing a name
manually, and is used for cases where the assembler doesn't like the mnemonic.
Clang IAS doesn't like the convience macro, and Binutils of around 2.30 don't
like sysexitl or movsxd with a 32bit operand. As it's only Ubuntu 18.04
affected by this, skip building the harness in old environments.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger@xenproject.org>
CC: Teddy Astie <teddy.astie@vates.tech>
v3:
* Force disable Clang IAS. It doesn't like the _ macro.
* Support 32bit builds
v2:
* New
---
tools/tests/Makefile | 1 +
tools/tests/x86-decode-lite/.gitignore | 1 +
tools/tests/x86-decode-lite/Makefile | 56 ++
tools/tests/x86-decode-lite/insns.S | 703 ++++++++++++++++++++++
tools/tests/x86-decode-lite/macro-magic.h | 62 ++
tools/tests/x86-decode-lite/main.c | 111 ++++
tools/tests/x86-decode-lite/x86-emulate.h | 27 +
7 files changed, 961 insertions(+)
create mode 100644 tools/tests/x86-decode-lite/.gitignore
create mode 100644 tools/tests/x86-decode-lite/Makefile
create mode 100644 tools/tests/x86-decode-lite/insns.S
create mode 100644 tools/tests/x86-decode-lite/macro-magic.h
create mode 100644 tools/tests/x86-decode-lite/main.c
create mode 100644 tools/tests/x86-decode-lite/x86-emulate.h
diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index fc0ed8091510..ca4f0c707638 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -14,6 +14,7 @@ SUBDIRS-y += xenstore
SUBDIRS-$(CONFIG_X86) += cpu-policy
SUBDIRS-$(CONFIG_X86) += tsx
+SUBDIRS-$(CONFIG_X86) += x86-decode-lite
ifneq ($(clang),y)
SUBDIRS-$(CONFIG_X86) += x86_emulator
endif
diff --git a/tools/tests/x86-decode-lite/.gitignore b/tools/tests/x86-decode-lite/.gitignore
new file mode 100644
index 000000000000..e726b493c993
--- /dev/null
+++ b/tools/tests/x86-decode-lite/.gitignore
@@ -0,0 +1 @@
+test-x86-decode-lite
diff --git a/tools/tests/x86-decode-lite/Makefile b/tools/tests/x86-decode-lite/Makefile
new file mode 100644
index 000000000000..dc33d5fd173a
--- /dev/null
+++ b/tools/tests/x86-decode-lite/Makefile
@@ -0,0 +1,56 @@
+XEN_ROOT = $(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+TARGET :=
+
+# Clang IAS doesn't like the convenience macros we use
+$(call cc-option-add,CFLAGS,CC,-no-integrated-as)
+
+# Binutils around 2.30 have mutually exclusive expectations of instruction
+# suffix validities compared to later versions. Among the distro we test,
+# this only excludes Ubuntu 18.04.
+ifeq ($(shell echo 'asm(".code64;sysexitl");' | $(CC) -x c -c -o /dev/null 2>/dev/null - && echo y),y)
+TARGET += test-x86-decode-lite
+endif
+
+.PHONY: all
+all: $(TARGET)
+
+.PHONY: run
+run: $(TARGET)
+ ./$<
+
+.PHONY: clean
+clean:
+ $(RM) -- *.o $(TARGET) $(DEPS_RM)
+
+.PHONY: distclean
+distclean: clean
+ $(RM) -- *~
+
+.PHONY: install
+install: all
+ $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)/tests
+ $(if $(TARGET),$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)/tests)
+
+.PHONY: uninstall
+uninstall:
+ $(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET)
+
+.PHONY: uninstall
+uninstall:
+
+vpath decode-lite.c $(XEN_ROOT)/xen/arch/x86/x86_emulate
+
+CFLAGS += $(CFLAGS_xeninclude) -I. -I$(XEN_ROOT)/xen/arch/x86
+CFLAGS += $(APPEND_CFLAGS)
+
+
+LDFLAGS += $(APPEND_LDFLAGS)
+
+%.o: Makefile
+
+$(TARGET): main.o insns.o decode-lite.o
+ $(CC) -o $@ $^ $(LDFLAGS)
+
+-include $(DEPS_INCLUDE)
diff --git a/tools/tests/x86-decode-lite/insns.S b/tools/tests/x86-decode-lite/insns.S
new file mode 100644
index 000000000000..8b299cfb594e
--- /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
+ _ add %al, %r8b
+ _ add %al, %r9b
+ _ add %al, %r10b
+ _ add %al, %r11b
+ _ add %al, %r12b
+ _ add %al, %r13b
+ _ add %al, %r14b
+ _ add %al, %r15b
+
+sib:
+ /* Mod=0, Reg=0, RM=4, SIB S=3, I=0, B {0..f} */
+ _ add %al, (%rax, %rax, 8)
+ _ add %al, (%rcx, %rax, 8)
+ _ add %al, (%rdx, %rax, 8)
+ _ add %al, (%rbx, %rax, 8)
+ _ add %al, (%rsp, %rax, 8)
+ _ add %al, ( , %rax, 8) /* "none", %rbp encoded with mod=1/2 */
+ _ add %al, (%rsi, %rax, 8)
+ _ add %al, (%rdi, %rax, 8)
+ _ add %al, (%r8, %rax, 8)
+ _ add %al, (%r9, %rax, 8)
+ _ add %al, (%r10, %rax, 8)
+ _ add %al, (%r11, %rax, 8)
+ _ add %al, (%r12, %rax, 8)
+ _ rex.b add %al,(,%rax, 8) /* "none", %r13 encoded with mod=1/2 */
+ _ add %al, (%r14, %rax, 8)
+ _ add %al, (%r15, %rax, 8)
+
+ /* Mod=1, Reg=0, RM=4, SIB S=3, I=0, B {0..f} */
+ _ add %al, 0x01(%rax, %rax, 8)
+ _ add %al, 0x01(%rcx, %rax, 8)
+ _ add %al, 0x01(%rdx, %rax, 8)
+ _ add %al, 0x01(%rbx, %rax, 8)
+ _ add %al, 0x01(%rsp, %rax, 8)
+ _ add %al, 0x01(%rbp, %rax, 8)
+ _ add %al, 0x01(%rsi, %rax, 8)
+ _ add %al, 0x01(%rdi, %rax, 8)
+ _ add %al, 0x01(%r8, %rax, 8)
+ _ add %al, 0x01(%r9, %rax, 8)
+ _ add %al, 0x01(%r10, %rax, 8)
+ _ add %al, 0x01(%r11, %rax, 8)
+ _ add %al, 0x01(%r12, %rax, 8)
+ _ add %al, 0x01(%r13, %rax, 8)
+ _ add %al, 0x01(%r14, %rax, 8)
+ _ add %al, 0x01(%r15, %rax, 8)
+
+ /* Mod=2, Reg=0, RM=4, SIB S=3, I=0, B {0..f} */
+ _ add %al, 0x7f000001(%rax, %rax, 8)
+ _ add %al, 0x7f000001(%rcx, %rax, 8)
+ _ add %al, 0x7f000001(%rdx, %rax, 8)
+ _ add %al, 0x7f000001(%rbx, %rax, 8)
+ _ add %al, 0x7f000001(%rsp, %rax, 8)
+ _ add %al, 0x7f000001(%rbp, %rax, 8)
+ _ add %al, 0x7f000001(%rsi, %rax, 8)
+ _ add %al, 0x7f000001(%rdi, %rax, 8)
+ _ add %al, 0x7f000001(%r8, %rax, 8)
+ _ add %al, 0x7f000001(%r9, %rax, 8)
+ _ add %al, 0x7f000001(%r10, %rax, 8)
+ _ add %al, 0x7f000001(%r11, %rax, 8)
+ _ add %al, 0x7f000001(%r12, %rax, 8)
+ _ add %al, 0x7f000001(%r13, %rax, 8)
+ _ add %al, 0x7f000001(%r14, %rax, 8)
+ _ add %al, 0x7f000001(%r15, %rax, 8)
+
+ /* Mod=0, Reg=0, RM=4, SIB S=3, I=4, B {0..f} */
+ _ add %al, (%rax, %riz, 8)
+ _ add %al, (%rcx, %riz, 8)
+ _ add %al, (%rdx, %riz, 8)
+ _ add %al, (%rbx, %riz, 8)
+ _ add %al, (%rsp, %riz, 8)
+ _ add %al, ( , %riz, 8) /* %rbp encoded with mod=1/2 */
+ _ add %al, (%rsi, %riz, 8)
+ _ add %al, (%rdi, %riz, 8)
+ _ add %al, (%r8, %riz, 8)
+ _ add %al, (%r9, %riz, 8)
+ _ add %al, (%r10, %riz, 8)
+ _ add %al, (%r11, %riz, 8)
+ _ add %al, (%r12, %riz, 8)
+ _ rex.b add %al,(,%riz, 8) /* %r13 encoded with mod=1/2 */
+ _ add %al, (%r14, %riz, 8)
+ _ add %al, (%r15, %riz, 8)
+
+ /* Mod=1, Reg=0, RM=4, SIB S=3, I=4, B {0..f} */
+ _ add %al, 0x01(%rax, %riz, 8)
+ _ add %al, 0x01(%rcx, %riz, 8)
+ _ add %al, 0x01(%rdx, %riz, 8)
+ _ add %al, 0x01(%rbx, %riz, 8)
+ _ add %al, 0x01(%rsp, %riz, 8)
+ _ add %al, 0x01(%rbp, %riz, 8)
+ _ add %al, 0x01(%rsi, %riz, 8)
+ _ add %al, 0x01(%rdi, %riz, 8)
+ _ add %al, 0x01(%r8, %riz, 8)
+ _ add %al, 0x01(%r9, %riz, 8)
+ _ add %al, 0x01(%r10, %riz, 8)
+ _ add %al, 0x01(%r11, %riz, 8)
+ _ add %al, 0x01(%r12, %riz, 8)
+ _ add %al, 0x01(%r13, %riz, 8)
+ _ add %al, 0x01(%r14, %riz, 8)
+ _ add %al, 0x01(%r15, %riz, 8)
+
+ /* Mod=2, Reg=0, RM=4, SIB S=3, I=4, B {0..f} */
+ _ add %al, 0x7f000001(%rax, %riz, 8)
+ _ add %al, 0x7f000001(%rcx, %riz, 8)
+ _ add %al, 0x7f000001(%rdx, %riz, 8)
+ _ add %al, 0x7f000001(%rbx, %riz, 8)
+ _ add %al, 0x7f000001(%rsp, %riz, 8)
+ _ add %al, 0x7f000001(%rbp, %riz, 8)
+ _ add %al, 0x7f000001(%rsi, %riz, 8)
+ _ add %al, 0x7f000001(%rdi, %riz, 8)
+ _ add %al, 0x7f000001(%r8, %riz, 8)
+ _ add %al, 0x7f000001(%r9, %riz, 8)
+ _ add %al, 0x7f000001(%r10, %riz, 8)
+ _ add %al, 0x7f000001(%r11, %riz, 8)
+ _ add %al, 0x7f000001(%r12, %riz, 8)
+ _ add %al, 0x7f000001(%r13, %riz, 8)
+ _ add %al, 0x7f000001(%r14, %riz, 8)
+ _ add %al, 0x7f000001(%r15, %riz, 8)
+
+ .macro alu_ops op
+ _ \op %al, (%rax)
+ _ \op %eax, (%rax)
+ _ \op (%rax), %al
+ _ \op (%rax), %eax
+ _ \op $1, %al
+ _ \op $0x7f000001, %eax
+
+ /* Vary osize on imm fields */
+ _ data16 \op $1, %al
+ _ rex.w \op $1, %al
+ _ data16 rex.w \op $1, %al
+
+ _ \op $0x7f01, %ax
+ _ \op $0x7f000001, %rax
+ _ data16 \op $0x7f000001, %rax
+ .endm
+
+onebyte_row_0x:
+ alu_ops add
+ alu_ops or
+
+onebyte_row_1x:
+ alu_ops adc
+ alu_ops sbb
+
+onebyte_row_2x:
+ alu_ops and
+ .code32
+ _ es nop
+ .code64
+ alu_ops sub
+ _ cs nop
+
+onebyte_row_3x:
+ alu_ops xor
+ .code32
+ _ ss nop
+ .code64
+ alu_ops cmp
+ _ ds nop
+
+/* onebyte_row_4x --> rex prefixes */
+
+onebyte_row_5x:
+ _ push %rax
+ _ push %rcx
+ _ push %rdx
+ _ push %rbx
+ _ push %rsp
+ _ push %rbp
+ _ push %rsi
+ _ push %rdi
+ _ pop %rax
+ _ pop %rcx
+ _ pop %rdx
+ _ pop %rbx
+ _ pop %rsp
+ _ pop %rbp
+ _ pop %rsi
+ _ pop %rdi
+
+onebyte_row_6x:
+ /*pusha,popa,bound --> not supported */
+ _ movsxd (%rax), %eax
+ _ movslq (%rax), %rax
+ _ fs nop
+ _ gs nop
+ _ data16 nop
+ /* addr32 --> not supported */
+ _ pushq $0x7f000001
+ _ pushw $0x7f01
+ _ rex.w pushq $0x7f000001
+ _ imul $0x7f01, %ax, %ax
+ _ imul $0x7f000001, %eax, %eax
+ _ imul $0x7f000001, %rax, %rax
+ _ pushq $0
+ _ pushw $0
+ _ rex.w pushq $0
+ _ imul $0, %ax, %ax
+ _ imul $0, %eax, %eax
+ _ imul $0, %rax, %rax
+ _ insb
+ _ insw
+ _ insl
+ _ outsb
+ _ outsw
+ _ outsl
+
+/* onebyte_row_7x: --> Jcc disp8 */
+
+onebyte_row_8x:
+ _ add $0, %cl /* Grp1 */
+ _ data16 add $0, %cl
+ _ rex.w add $0, %cl
+ _ add $0x7f01, %cx
+ _ add $0x7f000001, %ecx
+ _ add $0x7f000001, %rcx
+ _ add $0, %cx
+ _ add $0, %ecx
+ _ add $0, %rcx
+ _ test %cl, %cl
+ _ test %ecx, %ecx
+ _ xchg %cl, %cl
+ _ xchg %ecx, %ecx
+ _ mov %cl, (%rax)
+ _ mov %ecx, (%rax)
+ _ mov (%rax), %cl
+ _ mov (%rax), %ecx
+ _ mov %cs, (%rax)
+ _ lea (%rax), %eax
+ _ mov (%rax), %cs
+ /*pop mem --> Grp1a, Not supported (XOP prefix adjacent) */
+
+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:
+ _ mov 0x8000000000000001, %al
+ _ mov 0x8000000000000001, %ax
+ _ mov 0x8000000000000001, %eax
+ _ mov 0x8000000000000001, %rax
+ _ mov %al, 0x8000000000000001
+ _ mov %ax, 0x8000000000000001
+ _ mov %eax, 0x8000000000000001
+ _ mov %rax, 0x8000000000000001
+ _ movsb
+ _ movsl
+ _ cmpsb
+ _ cmpsl
+ _ test $0, %al
+ _ test $0x80000001, %eax
+ _ test $0x7f000001, %rax
+ _ stosb
+ _ stosl
+ _ lodsb
+ _ lodsl
+ _ scasb
+ _ scasl
+
+onebyte_row_bx:
+ _ mov $0, %al
+ _ mov $0, %cl
+ _ mov $0x7f01, %ax
+ _ mov $0x7f01, %cx
+ _ mov $0x7f000001, %eax
+ _ mov $0x7f000001, %ecx
+ _ mov $0x7f00000000000001, %rax
+ _ mov $0x7f00000000000001, %rcx
+
+onebyte_row_cx:
+ _ rol $0, %al /* Grp2 */
+ _ rol $0, %ax
+ _ rol $0, %eax
+ _ rol $0, %rax
+ /*ret $0 --> not supported */
+ _ ret
+ /*les,lds --> not supported */
+ _ movb $0, (%rax) /* Grp11 */
+ _ movw $0, (%rax)
+ _ movl $0, (%rax)
+ _ movq $0, (%rax)
+ /*xbegin (Grp11) --> disp32 */
+ /*enter,leave,lretq $0 --> not supported */
+ _ lretq
+ _ int3
+ _ int $0
+ /*into,iret --> not supported */
+
+onebyte_row_dx:
+ _ rol $1, %al /* Grp2 */
+ _ rol $1, %ax
+ _ rol $1, %eax
+ _ rol $1, %rax
+ _ rol %cl, %al
+ _ rol %cl, %ax
+ _ rol %cl, %eax
+ _ rol %cl, %rax
+ /*aam,aad --> not supported */
+ n "udb" .byte 0xd6
+ /*xlat,d8...df --> not supported */
+
+onebyte_row_ex:
+ /*loop{ne,e,},jrcxz --> not supported */
+ _ in $0, %al
+ _ in $0, %eax
+ _ out %al, $0
+ _ out %eax, $0
+ /*call,jmp --> disp32 */
+ /*ljmp --> not supported */
+ /*jmp --> disp8 */
+ _ in %dx, %al
+ _ in %dx, %eax
+ _ out %al, %dx
+ _ out %eax, %dx
+
+onebyte_row_fx:
+ _ lock addb $0, (%rax)
+ n "icebp" .byte 0xf1 /* icebp */
+ _ repne nop
+ _ repe nop
+ _ hlt
+ _ cmc
+ _ test $0, %cl /* Grp3, /0 has extra Imm{8,} */
+ _ not %cl
+ _ test $0x7f01, %cx
+ _ not %cx
+ _ test $0x7f000001, %ecx
+ _ not %ecx
+ _ test $0x7f000001, %rcx
+ _ not %rcx
+ _ clc
+ _ stc
+ _ cli
+ _ sti
+ _ cld
+ _ std
+ _ inc %cl /* Grp4 */
+ _ dec %cl
+ _ inc %ecx /* Grp5 */
+ _ dec %ecx
+ _ call *(%rax)
+ _ lcall *(%rax)
+ _ jmp *(%rax)
+ _ ljmp *(%rax)
+ _ push (%rax)
+
+twobyte_row_0x:
+ _ sldt (%rax) /* Grp6 */
+ _ sgdt (%rax) /* Grp7 */
+ _ lar (%rax), %eax
+ _ lsl (%rax), %eax
+ _ ud2a
+
+twobyte_row_1x:
+ _ prefetchnta (%rax) /* Grp16 (Hint Nop) */
+ _ nopl (%rax)
+
+twobyte_row_2x:
+ _ mov %cr0, %rax
+ _ mov %dr0, %rax
+ _ mov %rax, %cr0
+ _ mov %rax, %dr0
+
+twobyte_row_3x:
+ _ wrmsr
+ _ rdtsc
+ _ rdmsr
+ _ rdpmc
+
+twobyte_row_4x:
+ _ cmovo (%rax), %eax
+ _ cmovg (%rax), %eax
+
+/* twobyte_row_8x: --> Jcc disp32 */
+
+twobyte_row_9x:
+ _ seto (%rax)
+ _ setg (%rax)
+
+twobyte_row_ax:
+ _ push %fs
+ _ pop %fs
+ _ cpuid
+ _ bt %eax, (%rax)
+ _ shld $0, %ax, (%rax)
+ _ shld $0, %eax, (%rax)
+ _ shld $0, %rax, (%rax)
+ _ shld %cl, %ax, (%rax)
+ _ shld %cl, %eax, (%rax)
+ _ shld %cl, %rax, (%rax)
+ _ push %gs
+ _ pop %gs
+ /*rsm --> not supported */
+ _ bts %eax, (%rax)
+ _ shrd $0, %ax, (%rax)
+ _ shrd $0, %eax, (%rax)
+ _ shrd $0, %rax, (%rax)
+ _ shrd %cl, %ax, (%rax)
+ _ shrd %cl, %eax, (%rax)
+ _ shrd %cl, %rax, (%rax)
+ _ fxsave (%rax) /* Grp15 */
+ _ imul (%rax), %eax
+
+twobyte_row_bx:
+ _ cmpxchg %al, (%rax)
+ _ cmpxchg %eax, (%rax)
+ _ lss (%rax), %eax
+ _ btr %eax, (%rax)
+ _ lfs (%rax), %eax
+ _ lgs (%rax), %eax
+ _ movzbl (%rax), %eax
+ _ movzwl (%rax), %eax
+ _ popcnt (%rax), %eax
+ _ ud1 (%rax), %eax /* Grp10 */
+ _ bt $0, %ax /* Grp8 */
+ _ bt $0, %eax
+ _ bt $0, %rax
+ _ btc %eax, (%rax)
+ _ bsf (%rax), %eax
+ _ bsr (%rax), %eax
+ _ movsbl (%rax), %eax
+ _ movswl (%rax), %eax
+
+twobyte_row_cx:
+ _ xadd %al, (%rax)
+ _ xadd %eax, (%rax)
+ _ cmpxchg8b (%rax) /* Grp9 */
+ _ bswap %eax
+ _ bswap %edi
+
+END(tests_rel0)
+
+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)
+
+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
+
+riprel:
+ _ add %al, 0(%rip)
+ _ rex.b add %al, 0(%rip)
+
+ _ addb $1, 0(%rip)
+ _ rex.b addb $1, 0(%rip)
+
+ _ addl $0x7f000001, 0(%rip)
+ _ rex.b addl $0x7f000001, 0(%rip)
+END(tests_rel4)
+
+DECL(tests_unsup)
+
+unsup_prefix: /* Prefixes unimplemented for simplicity. */
+ _ vaddpd %zmm0, %zmm0, %zmm0 /* 0x62 EVEX */
+ _ addr32 nop /* 0x67 Address size override */
+ _ bextr $0, %eax, %eax /* 0x8f XOP */
+ _ bextr %eax, %eax, %eax /* 0xc4 VEX3 */
+ _ vaddpd %ymm0, %ymm0, %ymm0 /* 0xc5 VEX2 */
+ n "jmpabs 0" .byte 0xd5, 0x00, 0xa1, 0x01, 0, 0, 0, 0, 0, 0, 0x80 /* 0xd5 REX2 */
+ _ fadds (%rax) /* 0xd8 ... 0xdf ESCAPE (x87) */
+ _ femms /* 0x0f,0x0e ... 0x0f 3DNOW */
+
+unsup_branch:
+1:
+ _ loopne 1b
+ _ loope 1b
+ _ loop 1b
+ _ jrcxz 1b
+
+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
+
+not_64bit: /* Not valid/encodable in 64bit mode */
+ .code32
+ _ push %es
+ _ pop %es
+ _ push %cs
+ _ push %ss
+ _ pop %ss
+ _ push %ds
+ _ pop %ds
+ _ daa
+ _ das
+ _ aaa
+ _ aas
+ _ pusha
+ _ popa
+ _ bound %eax, (%eax)
+ /*arpl %ax, %ax --> movsxd in 64bit mode */
+ /* Grp1 */
+ _ lcall $-1, $-1
+ _ les (%eax), %eax
+ _ lds (%eax), %eax
+ _ into
+ _ aam $0
+ _ aad $0 /* Also REX2, also not supported */
+ _ ljmp $-1, $-1
+ .code64
+
+unsup_insn: /* Instructions that would complicated decode, or shouldn't be used */
+ _ ret $0
+ _ enter $0, $0
+ _ leave
+ _ lretq $0
+ _ iretq
+ _ xlat
+ _ clts
+ _ wbinvd
+ _ syscall
+ _ sysretl
+ _ invd
+ _ sysenter
+ _ sysexitl
+ _ rsm
+
+END(tests_unsup)
+
+ /* This is here to cause jmps to use their disp32 form. */
+ .section .text.other_section, "ax", @progbits
+other_section:
+ int3
+
+ /* Mark this file as not needing executable stacks. */
+ .section .note.GNU-stack, "", @progbits
diff --git a/tools/tests/x86-decode-lite/macro-magic.h b/tools/tests/x86-decode-lite/macro-magic.h
new file mode 100644
index 000000000000..b3c8aae39acd
--- /dev/null
+++ b/tools/tests/x86-decode-lite/macro-magic.h
@@ -0,0 +1,62 @@
+#ifndef X86_DECODE_LITE_LINKAGE_H
+#define X86_DECODE_LITE_LINKAGE_H
+
+#ifdef __i386__
+# define PTR_ALIGN 4
+# define PTR .long
+#else
+# define PTR_ALIGN 8
+# define PTR .quad
+#endif
+
+
+/* Start a 'struct test' array */
+.macro start_arr aname
+ .pushsection .data.rel.ro.\aname, "aw", @progbits
+ .globl \aname
+ .align PTR_ALIGN
+ .type \aname, STT_OBJECT
+\aname:
+ .popsection
+
+ /* Declare a macro wrapping \aname */
+ .macro pushsection_arr
+ .pushsection .data.rel.ro.\aname, "aw", @progbits
+ .endm
+.endm
+
+/* Macro 'n' to wrap the metadata of an instruction. Name can be different. */
+.macro n name:req insn:vararg
+ /* Emit the instruction, with start & end markers. */
+.Ls\@: \insn
+.Le\@:
+
+ /* Emit \name as a string. */
+ .pushsection .rodata.str1, "aMS", @progbits, 1
+.Ln\@: .asciz "\name"
+ .popsection
+
+ /* Emit an entry into the array. */
+ pushsection_arr
+ PTR .Ln\@, .Ls\@, .Le\@ - .Ls\@
+ .popsection
+.endm
+
+/* Macro '_' where the name is the instruction itself. */
+.macro _ insn:vararg
+ n "\insn" \insn
+.endm
+
+/* Finish a 'struct test' array */
+.macro finish_arr aname
+ pushsection_arr
+ PTR 0, 0, 0
+ .size \aname, . - \aname
+ .popsection
+ .purgem pushsection_arr
+.endm
+
+#define DECL(aname) start_arr aname
+#define END(aname) finish_arr aname
+
+#endif /* X86_DECODE_LITE_LINKAGE_H */
diff --git a/tools/tests/x86-decode-lite/main.c b/tools/tests/x86-decode-lite/main.c
new file mode 100644
index 000000000000..cdae7de8e90e
--- /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);
+
+ if ( r.len == 0 )
+ {
+ fail(t, " Failed to decode instruction\n");
+
+ if ( r.rel_sz != 0 || r.rel )
+ fail(t, " Rel/sz despite no decode\n");
+
+ continue;
+ }
+
+ if ( r.len != t->len )
+ {
+ fail(t, " Expected length %lu, got %u\n",
+ t->len, r.len);
+ continue;
+ }
+
+ if ( r.rel_sz != rel_sz )
+ {
+ fail(t, " Expected relocation size %u, got %u\n",
+ rel_sz, r.rel_sz);
+ continue;
+ }
+
+ if ( r.rel_sz &&
+ (r.rel < t->ip ||
+ r.rel > t->ip + t->len ||
+ r.rel + r.rel_sz > t->ip + t->len) )
+ {
+ fail(t, " Rel [%p,+%u) outside insn [%p,+%lu)\n",
+ r.rel, r.rel_sz, t->ip, t->len);
+ continue;
+ }
+ }
+}
+
+static void run_tests_unsup(const struct test *tests)
+{
+ printf("Test unsup\n");
+
+ for ( unsigned int i = 0; tests[i].name; ++i )
+ {
+ const struct test *t = &tests[i];
+ x86_decode_lite_t r = x86_decode_lite(t->ip, t->ip + t->len);
+
+ if ( r.len )
+ fail(t, " Got len %u\n", r.len);
+ }
+}
+
+int main(int argc, char **argv)
+{
+ printf("Tests for x86_decode_lite()\n");
+
+ run_tests(tests_rel0, 0);
+ run_tests(tests_rel1, 1);
+ run_tests(tests_rel4, 4);
+ run_tests_unsup(tests_unsup);
+
+ return !!nr_failures;
+}
diff --git a/tools/tests/x86-decode-lite/x86-emulate.h b/tools/tests/x86-decode-lite/x86-emulate.h
new file mode 100644
index 000000000000..558dab1b768e
--- /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"
+
+#endif /* X86_EMULATE_H */
--
2.39.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/5] x86/alternative: Walk all replacements during self tests
2026-08-03 7:20 [PATCH v3 0/5] x86/alternatives: Adjust all insn-relative fields Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite() Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite() Andrew Cooper
@ 2026-08-03 7:20 ` Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 4/5] x86/alternative: Relocate all insn-relative fields Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 5/5] x86/spec-ctrl: Introduce and use DO_COND_BHB_SEQ Andrew Cooper
4 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2026-08-03 7:20 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Jan Beulich, Roger Pau Monné,
Teddy Astie
When self tests are active, walk all alternative replacements with
x86_decode_lite().
This checks that we can decode all instructions, and also lets us check that
disp8's don't leave the replacement block as such a case will definitely
malfunction.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger@xenproject.org>
CC: Teddy Astie <teddy.astie@vates.tech>
v2:
* Rebase over API changes in patch 1
* Use +%lu and drop casts
* Swap to CONFIG_SELF_TESTS
---
xen/arch/x86/alternative.c | 52 ++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 5ed0c2672589..fd03147bdd12 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -16,6 +16,7 @@
#include <asm/traps.h>
#include <asm/nmi.h>
#include <asm/nops.h>
+#include <asm/x86_emulate.h>
#include <xen/livepatch.h>
#define MAX_PATCH_LEN (255-1)
@@ -586,6 +587,57 @@ static void __init _alternative_instructions(unsigned int what)
void __init alternative_instructions(void)
{
arch_init_ideal_nops();
+
+ /*
+ * Walk all replacement instructions with x86_decode_lite(). This checks
+ * both that we can decode all instructions within the replacement, and
+ * that any near branch with a disp8 stays within the alternative itself.
+ */
+ if ( IS_ENABLED(CONFIG_SELF_TESTS) )
+ {
+ struct alt_instr *a;
+
+ for ( a = __alt_instructions;
+ a < __alt_instructions_end; ++a )
+ {
+ void *repl = ALT_REPL_PTR(a);
+ void *ip = repl, *end = ip + a->repl_len;
+
+ if ( !a->repl_len )
+ continue;
+
+ for ( x86_decode_lite_t res; ip < end; ip += res.len )
+ {
+ const int8_t *d8;
+ const void *target;
+
+ res = x86_decode_lite(ip, end);
+
+ if ( res.len == 0 )
+ {
+ printk("Alt for %ps [%*ph]\n",
+ ALT_ORIG_PTR(a), a->repl_len, repl);
+ panic(" Unable to decode instruction at +%lu in alternative\n",
+ ip - repl);
+ }
+
+ if ( res.rel_sz != 1 )
+ continue;
+
+ d8 = res.rel;
+ target = ip + res.len + *d8;
+
+ if ( target < repl || target > end )
+ {
+ printk("Alt for %ps [%*ph]\n",
+ ALT_ORIG_PTR(a), a->repl_len, repl);
+ panic(" 'JMP/Jcc disp8' at +%lu leaves alternative block\n",
+ ip - repl);
+ }
+ }
+ }
+ }
+
_alternative_instructions(ALT_INSNS);
}
--
2.39.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 4/5] x86/alternative: Relocate all insn-relative fields
2026-08-03 7:20 [PATCH v3 0/5] x86/alternatives: Adjust all insn-relative fields Andrew Cooper
` (2 preceding siblings ...)
2026-08-03 7:20 ` [PATCH v3 3/5] x86/alternative: Walk all replacements during self tests Andrew Cooper
@ 2026-08-03 7:20 ` Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 5/5] x86/spec-ctrl: Introduce and use DO_COND_BHB_SEQ Andrew Cooper
4 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2026-08-03 7:20 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Teddy Astie
Right now, relocation of displacements is restricted to finding 0xe8/e9 as the
first byte of the replacement, but this is overly restrictive.
Use x86_decode_lite() to find and adjust all insn-relative fields.
As with disp8's not leaving the replacemnet block, some disp32's don't either.
e.g. the RSB stuffing loop. These stay unmodified.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger@xenproject.org>
CC: Teddy Astie <teddy.astie@vates.tech>
v3:
* Rebase over the split-out of altcall. Substantially simpler.
---
xen/arch/x86/alternative.c | 50 +++++++++++++++++++++++++++++++-------
1 file changed, 41 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index fd03147bdd12..a4a65597b2fc 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -349,15 +349,47 @@ static int init_or_livepatch _apply_alternatives(struct alt_instr *start,
memcpy(buf, repl, a->repl_len);
- /* 0xe8/0xe9 are relative branches; fix the offset. */
- if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
- *(int32_t *)(buf + 1) += repl - orig;
- else if ( IS_ENABLED(CONFIG_RETURN_THUNK) &&
- a->repl_len > 5 && buf[a->repl_len - 5] == 0xe9 &&
- ((long)repl + a->repl_len +
- *(int32_t *)(buf + a->repl_len - 4) ==
- (long)__x86_return_thunk) )
- *(int32_t *)(buf + a->repl_len - 4) += repl - orig;
+ /*
+ * Walk buf[] and adjust any insn-relative operands which leave the
+ * replacement block.
+ */
+ if ( a->repl_len )
+ {
+ uint8_t *ip = buf, *repl_end = ip + a->repl_len;
+
+ for ( x86_decode_lite_t res; ip < repl_end; ip += res.len )
+ {
+ int32_t *d32;
+ const uint8_t *target;
+
+ res = x86_decode_lite(ip, repl_end);
+
+ if ( res.len == 0 )
+ {
+ printk("Alt for %ps [%*ph]\n"
+ " Unable to decode instruction at +%lu in alternative\n",
+ ALT_ORIG_PTR(a), a->repl_len, repl, ip - repl);
+ return -EINVAL;
+ }
+
+ if ( res.rel_sz != 4 )
+ continue;
+
+ d32 = res.rel;
+ target = ip + res.len + *d32;
+
+ if ( target >= buf && target <= repl_end )
+ {
+ /*
+ * Target doesn't leave the replacement block. e.g. RSB
+ * stuffing. Leave it unmodified.
+ */
+ continue;
+ }
+
+ *d32 += repl - orig;
+ }
+ }
a->priv = 1;
--
2.39.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 5/5] x86/spec-ctrl: Introduce and use DO_COND_BHB_SEQ
2026-08-03 7:20 [PATCH v3 0/5] x86/alternatives: Adjust all insn-relative fields Andrew Cooper
` (3 preceding siblings ...)
2026-08-03 7:20 ` [PATCH v3 4/5] x86/alternative: Relocate all insn-relative fields Andrew Cooper
@ 2026-08-03 7:20 ` Andrew Cooper
4 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2026-08-03 7:20 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Teddy Astie
Now that alternatives can fix up call displacements even when they're not the
first instruction of the replacement, move the SCF_entry_bhb conditional
inside the replacement block.
This removes a conditional branch from the fastpaths of BHI-unaffected
hardware.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger@xenproject.org>
CC: Teddy Astie <teddy.astie@vates.tech>
---
xen/arch/x86/hvm/vmx/entry.S | 12 +++----
xen/arch/x86/include/asm/spec_ctrl_asm.h | 43 +++++++++++++-----------
2 files changed, 30 insertions(+), 25 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index cebc70064048..76508c0de2f3 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -59,12 +59,12 @@ FUNC(vmx_asm_vmexit_handler)
* Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs
* itself so must be after we've perfomed all the RET-safety we can.
*/
- testb $SCF_entry_bhb, CPUINFO_scf(%rsp)
- jz .L_skip_bhb
- ALTERNATIVE_2 "", \
- "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \
- "call clear_bhb_tsx", X86_SPEC_BHB_TSX
-.L_skip_bhb:
+ .macro VMX_BHB_SEQ fn:req
+ DO_COND_BHB_SEQ \fn scf=CPUINFO_scf(%rsp)
+ .endm
+ ALTERNATIVE_2 "", \
+ "VMX_BHB_SEQ fn=clear_bhb_loops", X86_SPEC_BHB_LOOPS, \
+ "VMX_BHB_SEQ fn=clear_bhb_tsx", X86_SPEC_BHB_TSX
ALTERNATIVE "lfence", "", X86_SPEC_NO_LFENCE_ENTRY_VMX
/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index abb64ad2b7f9..780ec57f4553 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -92,6 +92,21 @@
.L\@_skip:
.endm
+.macro DO_COND_BHB_SEQ fn:req, scf=%bl
+/*
+ * Requires SCF (defaults to %rbx), fn=clear_bhb_{loops,tsx}
+ * Clobbers %rax, %rcx
+ *
+ * Conditionally use a BHB clearing software sequence.
+ */
+ testb $SCF_entry_bhb, \scf
+ jz .L\@_skip_bhb
+
+ call \fn
+
+.L\@_skip_bhb:
+.endm
+
.macro DO_OVERWRITE_RSB tmp=rax, xu
/*
* Requires nothing
@@ -277,12 +292,9 @@
* Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs
* itself so must be after we've perfomed all the RET-safety we can.
*/
- testb $SCF_entry_bhb, %bl
- jz .L\@_skip_bhb
- ALTERNATIVE_2 "", \
- "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \
- "call clear_bhb_tsx", X86_SPEC_BHB_TSX
-.L\@_skip_bhb:
+ ALTERNATIVE_2 "", \
+ "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \
+ "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX
ALTERNATIVE "lfence", "", X86_SPEC_NO_LFENCE_ENTRY_PV
.endm
@@ -322,12 +334,9 @@
ALTERNATIVE "", __stringify(DO_SPEC_CTRL_ENTRY maybexen=1), \
X86_FEATURE_SC_MSR_PV
- testb $SCF_entry_bhb, %bl
- jz .L\@_skip_bhb
- ALTERNATIVE_2 "", \
- "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \
- "call clear_bhb_tsx", X86_SPEC_BHB_TSX
-.L\@_skip_bhb:
+ ALTERNATIVE_2 "", \
+ "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \
+ "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX
ALTERNATIVE "lfence", "", X86_SPEC_NO_LFENCE_ENTRY_INTR
.endm
@@ -433,13 +442,9 @@
* Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs
* itself so must be after we've perfomed all the RET-safety we can.
*/
- testb $SCF_entry_bhb, %bl
- jz .L\@_skip_bhb
-
- ALTERNATIVE_2 "", \
- "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \
- "call clear_bhb_tsx", X86_SPEC_BHB_TSX
-.L\@_skip_bhb:
+ ALTERNATIVE_2 "", \
+ "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \
+ "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX
lfence
.endm
--
2.39.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite()
2026-08-03 7:20 ` [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite() Andrew Cooper
@ 2026-08-03 9:14 ` Andrew Cooper
2026-08-03 15:26 ` Jan Beulich
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2026-08-03 9:14 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Teddy Astie
On 03/08/2026 8:20 am, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/x86_emulate/decode-lite.c b/xen/arch/x86/x86_emulate/decode-lite.c
> new file mode 100644
> index 000000000000..131cc07d5516
> --- /dev/null
> +++ b/xen/arch/x86/x86_emulate/decode-lite.c
> @@ -0,0 +1,330 @@
> +
> + if ( d & (Imm | Imm8 | Moffs) )
> + {
> + if ( d & Imm8 )
> + osize = 1;
> + else if ( d & Moffs )
> + osize = 8;
> + else if ( osize == 8 && !(opc >= 0xb8 && opc <= 0xbf) )
> + osize = 4;
GCC 12 does transform this into sub $0xb8; cmp $7.
> +
> + switch ( osize )
> + {
> + case 1: FETCH(uint8_t); break;
> + case 2: FETCH(uint16_t); break;
> + case 4: FETCH(uint32_t); break;
> + case 8: FETCH(uint64_t); break;
> + default: goto bad_osize;
> + }
On further consideration:
switch ( osize )
{
case 1:
case 2:
case 4:
case 8:
if ( ip + osize > end )
goto overrun;
ip += osize;
break;
default: goto bad_osize;
}
drops nearly 10% of the function:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-91 (-91)
Function old new delta
x86_decode_lite 972 881 -91
GCC clearly can't reason about the relationship between osize and
sizeof(type), and needs the help.
I also tried the further simplification:
if ( osize > 8 || (osize & (osize - 1)) != 0 )
goto bad_osize;
if ( ip + osize > end )
goto overrun;
ip += osize;
but interestingly this delta grows the function by 30 bytes. It only
seems to add the block checking osize, meaning that GCC managed to
optimise away all of the switch dispatch previously. In hindsight this
is probably quite easy; because we're 64bit only, osize only ever has
constant values that GCC can see.
Anyway, I've folded in the first optimisation.
~Andrew
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite()
2026-08-03 7:20 ` [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite() Andrew Cooper
2026-08-03 9:14 ` Andrew Cooper
@ 2026-08-03 15:26 ` Jan Beulich
2026-08-04 15:39 ` Jan Beulich
2026-08-04 18:56 ` Andrew Cooper
1 sibling, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2026-08-03 15:26 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Teddy Astie, Xen-devel
> --- /dev/null
> +++ b/xen/arch/x86/x86_emulate/decode-lite.c
> @@ -0,0 +1,330 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifdef __XEN__
> +# include <xen/init.h>
> +# include <xen/livepatch.h>
> +#endif
> +
> +#include "private.h"
> +
> +#undef ModRM
> +
> +/*
> + * Bare minimum x86 instruction decoder to parse the alternative replacement
> + * instructions and locate the IP-relative references that may need updating.
> + *
> + * These are:
> + * - disp8/32 from near direct branches
> + * - RIP-relative memory references
> + *
> + * The following simplifications are used:
> + * - All code is 64bit, the instruction stream is well formed and safe to
> + * read.
> + * - Instruction groups and prefixes not used by Xen's current alternatives
> + * are not implemented in order to reduce the decode complexity.
> + * - Certain instructions are intentionally not recognised, when it is more
> + * likely for their presence to be an error than intentional.
> + *
> + * Inputs:
> + * @ip The position to start decoding from.
> + * @end End of the replacement block. Exceeding this is considered an error.
Why do you mention replacement blocks here? Are we entirely set on this
code not possibly gaining any purpose beyond the scanning of those?
> + * Returns: x86_decode_lite_t
> + * - On failure, length of 0.
> + * - On success, length > 0. For rel_sz > 0, rel points at the relative
> + * field in the instruction stream.
> + */
> +x86_decode_lite_t init_or_livepatch x86_decode_lite(void *ip, void *end)
Is there a reason the parameters can't be pointer-to-const? Hmm,
apparently for x86_decode_lite_t's "rel" field not be plaing void *, "ip"
needs to be this way as well. But not "end", I don't think.
> +{
> +#define Imm8 (1 << 0)
> +#define Imm (1 << 1)
> +#define Moffs (1 << 2)
> +#define Branch (1 << 5) /* Near direct branches, which have a displacement */
> +#define ModRM (1 << 6)
> +#define Known (1 << 7)
> +
> + static const uint8_t init_or_livepatch_const onebyte[256] = {
> +
> +#define ALU_OPS(x) \
> + [(x) + 0] = (Known|ModRM), \
> + [(x) + 1] = (Known|ModRM), \
> + [(x) + 2] = (Known|ModRM), \
> + [(x) + 3] = (Known|ModRM), \
> + [(x) + 4] = (Known|Imm8), \
> + [(x) + 5] = (Known|Imm)
> +
> + ALU_OPS(0x00) /* ADD */, ALU_OPS(0x08) /* OR */,
> + ALU_OPS(0x10) /* ADC */, ALU_OPS(0x18) /* SBB */,
> + ALU_OPS(0x20) /* AND */, ALU_OPS(0x28) /* SUB */,
> + ALU_OPS(0x30) /* XOR */, ALU_OPS(0x38) /* CMP */,
> +
> +#undef ALU_OPS
> +
> + [0x50 ... 0x5f] = (Known), /* PUSH/POP %reg */
> +
> + [0x62] = 0, /* BOUND, but also EVEX prefix, not implemented. */
> + [0x63] = (Known|ModRM), /* MOVSxd */
> +
> + [0x68] = (Known|Imm), /* PUSH $imm */
> + [0x69] = (Known|ModRM|Imm), /* IMUL $imm */
> + [0x6a] = (Known|Imm8), /* PUSH $imm8 */
> + [0x6b] = (Known|ModRM|Imm8), /* PUSH $imm8 */
> + [0x6c ... 0x6f] = (Known), /* INS/OUTS */
> + [0x70 ... 0x7f] = (Known|Branch|Imm8), /* Jcc disp8 */
> + [0x80] = (Known|ModRM|Imm8), /* Grp1 */
> + [0x81] = (Known|ModRM|Imm), /* Grp1 */
> +
> + [0x83] = (Known|ModRM|Imm8), /* Grp1 */
> + [0x84 ... 0x8e] = (Known|ModRM), /* TEST/XCHG/MOV/MOV-SREG/LEA */
> + [0x8f] = 0, /* Grp1A - POP but also XOP prefix, not implemented. */
POP doesn't look all that unlikely to be used in inline assembly, and
hence in alternatives. That said, of course using it with a memory
operand requires quite a bit of care. I don't see you excluding the
PUSH counterpart, though - being consistent for any such pairs would
seem somewhat desirable.
> + [0x90 ... 0x99] = (Known), /* NOP/XCHG %rAX/CLTQ/CQTO */
> +
> + [0x9b ... 0x9f] = (Known), /* FWAIT/PUSHF/POPF/SAHF/LAHF */
> + [0xa0 ... 0xa3] = (Known|Moffs), /* MOVABS */
> + [0xa4 ... 0xa7] = (Known), /* MOVS/CMPS */
> + [0xa8] = (Known|Imm8), /* TEST %al */
> + [0xa9] = (Known|Imm), /* TEST %rAX */
> + [0xaa ... 0xaf] = (Known), /* STOS/LODS/SCAS */
> + [0xb0 ... 0xb7] = (Known|Imm8), /* MOV $imm8, %reg */
> + [0xb8 ... 0xbf] = (Known|Imm), /* MOV $imm{16,32,64}, %reg */
> + [0xc0 ... 0xc1] = (Known|ModRM|Imm8), /* Grp2 (ROL..SAR $imm8, %reg) */
> +
> + [0xc3] = (Known), /* RET */
> + [0xc4 ... 0xc5] = 0, /* LES/LDS but also VEX prefixes, not implemented. */
This may bite us sooner or later, due to the VEX-encoded integer insns
that there are. Of course as long as we don't use this function on
compiled code, and as long as my "x86: allow Kconfig control over psABI
level" doesn't come close to going in, that's merely a theoretical
concern.
Same goes for not supporting the 3-byte opcodes, which also encode
certain integer insns.
> + [0xc6] = (Known|ModRM|Imm8), /* Grp11, Further ModRM decode */
> + [0xc7] = (Known|ModRM|Imm), /* Grp11, Further ModRM decode */
> +
> + [0xcb ... 0xcc] = (Known), /* LRET/INT3 */
> + [0xcd] = (Known|Imm8), /* INT $imm8 */
> +
> + [0xd0 ... 0xd3] = (Known|ModRM), /* Grp2 (ROL..SAR {$1,%cl}, %reg) */
> +
> + [0xd6] = (Known), /* UDB */
I guess you consider XLAT, LOOP*, and J*CXZ as too odd to use in alternatives?
Decoding-wise they're rather easy to implement.
> + [0xe4 ... 0xe7] = (Known|Imm8), /* IN/OUT $imm8 */
> + [0xe8 ... 0xe9] = (Known|Branch|Imm), /* CALL/JMP disp32 */
> +
> + [0xeb] = (Known|Branch|Imm8), /* JMP disp8 */
> + [0xec ... 0xef] = (Known), /* IN/OUT %dx */
> +
> + [0xf1] = (Known), /* ICEBP */
> +
> + [0xf4] = (Known), /* HLT */
> + [0xf5] = (Known), /* CMC */
> + [0xf6 ... 0xf7] = (Known|ModRM), /* Grp3, Further ModRM decode */
> + [0xf8 ... 0xfd] = (Known), /* CLC ... STD */
> + [0xfe ... 0xff] = (Known|ModRM), /* Grp4 */
> + };
> + static const uint8_t init_or_livepatch_const twobyte[256] = {
> + [0x00 ... 0x03] = (Known|ModRM), /* Grp6/Grp7/LAR/LSL */
Leaving out INVD is surely find, but WBINVD?
> + [0x0b] = (Known), /* UD2 */
> +
> + [0x18 ... 0x1f] = (Known|ModRM), /* Grp16 (Hint Nop) */
> + [0x20 ... 0x23] = (Known|ModRM), /* MOV %cr/%dr */
> +
> + [0x30 ... 0x33] = (Known), /* WRMSR/RDTSC/RDMSR/RDPMC */
> +
> + [0x40 ... 0x4f] = (Known|ModRM), /* CMOVcc */
> +
> + [0x80 ... 0x8f] = (Known|Branch|Imm), /* Jcc disp32 */
> + [0x90 ... 0x9f] = (Known|ModRM), /* SETcc */
> +
> + [0xa0 ... 0xa2] = (Known), /* PUSH/POP %fs/CPUID */
> + [0xa3] = (Known|ModRM), /* BT */
> + [0xa4] = (Known|ModRM|Imm8), /* SHLD $imm8 */
> + [0xa5] = (Known|ModRM), /* SHLD %cl */
> +
> + [0xa8 ... 0xa9] = (Known), /* PUSH/POP %gs */
> +
> + [0xab] = (Known|ModRM), /* BTS */
> + [0xac] = (Known|ModRM|Imm8), /* SHRD $imm8 */
> + [0xad ... 0xaf] = (Known|ModRM), /* SHRD %cl/Grp15/IMUL */
> +
> + [0xb0 ... 0xb9] = (Known|ModRM), /* CMPXCHG/LSS/BTR/LFS/LGS/MOVZxx/POPCNT/UD1 */
> + [0xba] = (Known|ModRM|Imm8), /* Grp8 */
> + [0xbb ... 0xbf] = (Known|ModRM), /* BTC/BSF/BSR/MOVSX */
> + [0xc0 ... 0xc1] = (Known|ModRM), /* XADD */
What about MOVNTI?
> + [0xc7] = (Known|ModRM), /* Grp9 */
> + [0xc8 ... 0xcf] = (Known), /* BSWAP */
> + };
What about UD0?
> + void *start = ip, *rel = NULL;
> + unsigned int opc, rel_sz = 0;
> + uint8_t b, d, rex = 0, osize = 4;
> +
> +#define OPC_TWOBYTE (1 << 8)
> +
> + /* Mutates IP, uses END. */
> +#define FETCH(ty) \
> + ({ \
> + ty _val; \
> + \
> + if ( (ip + sizeof(ty)) > end ) \
> + goto overrun; \
> + _val = *(ty *)ip; \
> + ip += sizeof(ty); \
> + _val; \
> + })
> +
> + for ( ;; ) /* Prefixes */
> + {
> + switch ( b = FETCH(uint8_t) )
> + {
> + case 0x26: /* ES override */
> + case 0x2e: /* CS override */
> + case 0x36: /* DS override */
> + case 0x3e: /* SS override */
> + case 0x64: /* FS override */
> + case 0x65: /* GS override */
> + case 0xf0: /* LOCK */
> + case 0xf2: /* REPNE */
> + case 0xf3: /* REP */
> + break;
> +
> + case 0x66: /* Operand size override */
> + osize = 2;
> + break;
> +
> + /* case 0x67: Address size override, not implemented */
> +
> + case 0x40 ... 0x4f: /* REX */
> + rex = b;
> + continue;
> +
> + default:
> + goto prefixes_done;
> + }
> + rex = 0; /* REX cancelled by subsequent legacy prefix. */
> + }
> + prefixes_done:
> +
> + if ( rex & REX_W )
> + osize = 8;
> +
> + /* Fetch the main opcode byte(s) */
> + if ( b == 0x0f )
> + {
> + b = FETCH(uint8_t);
> + opc = OPC_TWOBYTE | b;
> +
> + d = twobyte[b];
> + }
> + else
> + {
> + opc = b;
> + d = onebyte[b];
> + }
> +
> + if ( unlikely(!(d & Known)) )
> + goto unknown;
> +
> + if ( d & ModRM )
> + {
> + uint8_t modrm = FETCH(uint8_t);
> + uint8_t mod = modrm >> 6;
> + uint8_t reg = (modrm >> 3) & 7;
> + uint8_t rm = modrm & 7;
> +
> + /* ModRM/SIB decode */
> + if ( mod == 0 && rm == 5 ) /* RIP relative */
> + {
> + rel = ip;
> + rel_sz = 4;
> + FETCH(int32_t);
FETCH() here but ...
> + }
> + else if ( mod != 3 && rm == 4 ) /* SIB */
> + {
> + uint8_t sib = FETCH(uint8_t);
> + uint8_t base = sib & 7;
> +
> + if ( mod == 0 && base == 5 )
> + goto disp32;
... goto here?
> + }
> +
> + if ( mod == 1 ) /* disp8 */
> + FETCH(int8_t);
> + else if ( mod == 2 ) /* disp32 */
> + {
> + disp32:
> + FETCH(int32_t);
> + }
In several cases the FETCH()ed value isn't used. Compilers as well as Eclair
(and alike) are happy with that? And compilers also manage to eliminate the
memory accesses then?
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -835,4 +835,18 @@ static inline void x86_emul_reset_event(struct x86_emulate_ctxt *ctxt)
> ctxt->event = (struct x86_event){};
> }
>
> +/*
> + * x86_decode_lite(). Very minimal decoder for managing alternatives.
> + *
> + * @len is 0 on error, or nonzero on success. If the instruction has a
> + * relative field, @rel_sz is nonzero, and @rel points at the field.
> + */
> +typedef struct {
> + uint8_t len;
> + uint8_t rel_sz; /* bytes: 0, 1 or 4 */
Perhaps use bitfields in favor of fixed-width integers, seeing what
./CODING_STYLE says?
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite()
2026-08-03 7:20 ` [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite() Andrew Cooper
@ 2026-08-03 16:03 ` Jan Beulich
2026-08-04 19:37 ` Andrew Cooper
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2026-08-03 16:03 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Teddy Astie, Xen-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite()
2026-08-03 15:26 ` Jan Beulich
@ 2026-08-04 15:39 ` Jan Beulich
2026-08-04 18:56 ` Andrew Cooper
1 sibling, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2026-08-04 15:39 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Teddy Astie, Xen-devel
On 03.08.2026 17:26, Jan Beulich wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/x86_emulate/decode-lite.c
>> @@ -0,0 +1,330 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifdef __XEN__
>> +# include <xen/init.h>
>> +# include <xen/livepatch.h>
>> +#endif
>> +
>> +#include "private.h"
>> +
>> +#undef ModRM
>> +
>> +/*
>> + * Bare minimum x86 instruction decoder to parse the alternative replacement
>> + * instructions and locate the IP-relative references that may need updating.
>> + *
>> + * These are:
>> + * - disp8/32 from near direct branches
>> + * - RIP-relative memory references
>> + *
>> + * The following simplifications are used:
>> + * - All code is 64bit, the instruction stream is well formed and safe to
>> + * read.
>> + * - Instruction groups and prefixes not used by Xen's current alternatives
>> + * are not implemented in order to reduce the decode complexity.
>> + * - Certain instructions are intentionally not recognised, when it is more
>> + * likely for their presence to be an error than intentional.
>> + *
>> + * Inputs:
>> + * @ip The position to start decoding from.
>> + * @end End of the replacement block. Exceeding this is considered an error.
>
> Why do you mention replacement blocks here? Are we entirely set on this
> code not possibly gaining any purpose beyond the scanning of those?
>
>> + * Returns: x86_decode_lite_t
>> + * - On failure, length of 0.
>> + * - On success, length > 0. For rel_sz > 0, rel points at the relative
>> + * field in the instruction stream.
>> + */
>> +x86_decode_lite_t init_or_livepatch x86_decode_lite(void *ip, void *end)
>
> Is there a reason the parameters can't be pointer-to-const? Hmm,
> apparently for x86_decode_lite_t's "rel" field not be plaing void *, "ip"
> needs to be this way as well. But not "end", I don't think.
>
>> +{
>> +#define Imm8 (1 << 0)
>> +#define Imm (1 << 1)
>> +#define Moffs (1 << 2)
>> +#define Branch (1 << 5) /* Near direct branches, which have a displacement */
>> +#define ModRM (1 << 6)
>> +#define Known (1 << 7)
>> +
>> + static const uint8_t init_or_livepatch_const onebyte[256] = {
>> +
>> +#define ALU_OPS(x) \
>> + [(x) + 0] = (Known|ModRM), \
>> + [(x) + 1] = (Known|ModRM), \
>> + [(x) + 2] = (Known|ModRM), \
>> + [(x) + 3] = (Known|ModRM), \
>> + [(x) + 4] = (Known|Imm8), \
>> + [(x) + 5] = (Known|Imm)
>> +
>> + ALU_OPS(0x00) /* ADD */, ALU_OPS(0x08) /* OR */,
>> + ALU_OPS(0x10) /* ADC */, ALU_OPS(0x18) /* SBB */,
>> + ALU_OPS(0x20) /* AND */, ALU_OPS(0x28) /* SUB */,
>> + ALU_OPS(0x30) /* XOR */, ALU_OPS(0x38) /* CMP */,
>> +
>> +#undef ALU_OPS
>> +
>> + [0x50 ... 0x5f] = (Known), /* PUSH/POP %reg */
>> +
>> + [0x62] = 0, /* BOUND, but also EVEX prefix, not implemented. */
>> + [0x63] = (Known|ModRM), /* MOVSxd */
>> +
>> + [0x68] = (Known|Imm), /* PUSH $imm */
>> + [0x69] = (Known|ModRM|Imm), /* IMUL $imm */
>> + [0x6a] = (Known|Imm8), /* PUSH $imm8 */
>> + [0x6b] = (Known|ModRM|Imm8), /* PUSH $imm8 */
>> + [0x6c ... 0x6f] = (Known), /* INS/OUTS */
>> + [0x70 ... 0x7f] = (Known|Branch|Imm8), /* Jcc disp8 */
>> + [0x80] = (Known|ModRM|Imm8), /* Grp1 */
>> + [0x81] = (Known|ModRM|Imm), /* Grp1 */
>> +
>> + [0x83] = (Known|ModRM|Imm8), /* Grp1 */
>> + [0x84 ... 0x8e] = (Known|ModRM), /* TEST/XCHG/MOV/MOV-SREG/LEA */
>> + [0x8f] = 0, /* Grp1A - POP but also XOP prefix, not implemented. */
>
> POP doesn't look all that unlikely to be used in inline assembly, and
> hence in alternatives. That said, of course using it with a memory
> operand requires quite a bit of care. I don't see you excluding the
> PUSH counterpart, though - being consistent for any such pairs would
> seem somewhat desirable.
>
>> + [0x90 ... 0x99] = (Known), /* NOP/XCHG %rAX/CLTQ/CQTO */
>> +
>> + [0x9b ... 0x9f] = (Known), /* FWAIT/PUSHF/POPF/SAHF/LAHF */
>> + [0xa0 ... 0xa3] = (Known|Moffs), /* MOVABS */
>> + [0xa4 ... 0xa7] = (Known), /* MOVS/CMPS */
>> + [0xa8] = (Known|Imm8), /* TEST %al */
>> + [0xa9] = (Known|Imm), /* TEST %rAX */
>> + [0xaa ... 0xaf] = (Known), /* STOS/LODS/SCAS */
>> + [0xb0 ... 0xb7] = (Known|Imm8), /* MOV $imm8, %reg */
>> + [0xb8 ... 0xbf] = (Known|Imm), /* MOV $imm{16,32,64}, %reg */
>> + [0xc0 ... 0xc1] = (Known|ModRM|Imm8), /* Grp2 (ROL..SAR $imm8, %reg) */
>> +
>> + [0xc3] = (Known), /* RET */
>> + [0xc4 ... 0xc5] = 0, /* LES/LDS but also VEX prefixes, not implemented. */
>
> This may bite us sooner or later, due to the VEX-encoded integer insns
> that there are. Of course as long as we don't use this function on
> compiled code, and as long as my "x86: allow Kconfig control over psABI
> level" doesn't come close to going in, that's merely a theoretical
> concern.
Actually perhaps sooner - we're meaning to use MSR-IMM insns after all, if
I'm not mistaken.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite()
2026-08-03 15:26 ` Jan Beulich
2026-08-04 15:39 ` Jan Beulich
@ 2026-08-04 18:56 ` Andrew Cooper
2026-08-05 6:24 ` Jan Beulich
1 sibling, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2026-08-04 18:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Teddy Astie, Xen-devel
On 03/08/2026 4:26 pm, Jan Beulich wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/x86_emulate/decode-lite.c
>> @@ -0,0 +1,330 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifdef __XEN__
>> +# include <xen/init.h>
>> +# include <xen/livepatch.h>
>> +#endif
>> +
>> +#include "private.h"
>> +
>> +#undef ModRM
>> +
>> +/*
>> + * Bare minimum x86 instruction decoder to parse the alternative replacement
>> + * instructions and locate the IP-relative references that may need updating.
>> + *
>> + * These are:
>> + * - disp8/32 from near direct branches
>> + * - RIP-relative memory references
>> + *
>> + * The following simplifications are used:
>> + * - All code is 64bit, the instruction stream is well formed and safe to
>> + * read.
>> + * - Instruction groups and prefixes not used by Xen's current alternatives
>> + * are not implemented in order to reduce the decode complexity.
>> + * - Certain instructions are intentionally not recognised, when it is more
>> + * likely for their presence to be an error than intentional.
>> + *
>> + * Inputs:
>> + * @ip The position to start decoding from.
>> + * @end End of the replacement block. Exceeding this is considered an error.
> Why do you mention replacement blocks here? Are we entirely set on this
> code not possibly gaining any purpose beyond the scanning of those?
It's just the end of the instruction stream wanting decoding. I'll
adjust the comment.
>> +{
>> +#define Imm8 (1 << 0)
>> +#define Imm (1 << 1)
>> +#define Moffs (1 << 2)
>> +#define Branch (1 << 5) /* Near direct branches, which have a displacement */
>> +#define ModRM (1 << 6)
>> +#define Known (1 << 7)
>> +
>> + static const uint8_t init_or_livepatch_const onebyte[256] = {
>> +
>> +#define ALU_OPS(x) \
>> + [(x) + 0] = (Known|ModRM), \
>> + [(x) + 1] = (Known|ModRM), \
>> + [(x) + 2] = (Known|ModRM), \
>> + [(x) + 3] = (Known|ModRM), \
>> + [(x) + 4] = (Known|Imm8), \
>> + [(x) + 5] = (Known|Imm)
>> +
>> + ALU_OPS(0x00) /* ADD */, ALU_OPS(0x08) /* OR */,
>> + ALU_OPS(0x10) /* ADC */, ALU_OPS(0x18) /* SBB */,
>> + ALU_OPS(0x20) /* AND */, ALU_OPS(0x28) /* SUB */,
>> + ALU_OPS(0x30) /* XOR */, ALU_OPS(0x38) /* CMP */,
>> +
>> +#undef ALU_OPS
>> +
>> + [0x50 ... 0x5f] = (Known), /* PUSH/POP %reg */
>> +
>> + [0x62] = 0, /* BOUND, but also EVEX prefix, not implemented. */
>> + [0x63] = (Known|ModRM), /* MOVSxd */
>> +
>> + [0x68] = (Known|Imm), /* PUSH $imm */
>> + [0x69] = (Known|ModRM|Imm), /* IMUL $imm */
>> + [0x6a] = (Known|Imm8), /* PUSH $imm8 */
>> + [0x6b] = (Known|ModRM|Imm8), /* PUSH $imm8 */
>> + [0x6c ... 0x6f] = (Known), /* INS/OUTS */
>> + [0x70 ... 0x7f] = (Known|Branch|Imm8), /* Jcc disp8 */
>> + [0x80] = (Known|ModRM|Imm8), /* Grp1 */
>> + [0x81] = (Known|ModRM|Imm), /* Grp1 */
>> +
>> + [0x83] = (Known|ModRM|Imm8), /* Grp1 */
>> + [0x84 ... 0x8e] = (Known|ModRM), /* TEST/XCHG/MOV/MOV-SREG/LEA */
>> + [0x8f] = 0, /* Grp1A - POP but also XOP prefix, not implemented. */
> POP doesn't look all that unlikely to be used in inline assembly, and
> hence in alternatives. That said, of course using it with a memory
> operand requires quite a bit of care.
We have no alternatives playing with the stack (beyond CALL
instructions), and no alternatives which have any net %rsp delta.
PUSH/POP MEM are rare in general and Xen doesn't have any at all.
> I don't see you excluding the
> PUSH counterpart, though - being consistent for any such pairs would
> seem somewhat desirable.
It would be nice to be handled symmetrically, but this *is* an
odd-instruction-out in the x86 encoding space.
It ought to live in Grp5 where the encoding would be 0xff /7 (and beside
it's matching PUSH), except that's that's a rather important binary
pattern and wants to not be considered a valid instruction.
The fact that the group is split like this shows that the mistake was a
late discovery in the development of the 8086, where it was easier to
move the one opcode than the whole group. (It's likely to have been a
metal-layer fix for the decode PAL, rather than adjusting the
transistors, which is typically an order of magnitude cheaper fix.)
>
>> + [0x90 ... 0x99] = (Known), /* NOP/XCHG %rAX/CLTQ/CQTO */
>> +
>> + [0x9b ... 0x9f] = (Known), /* FWAIT/PUSHF/POPF/SAHF/LAHF */
>> + [0xa0 ... 0xa3] = (Known|Moffs), /* MOVABS */
>> + [0xa4 ... 0xa7] = (Known), /* MOVS/CMPS */
>> + [0xa8] = (Known|Imm8), /* TEST %al */
>> + [0xa9] = (Known|Imm), /* TEST %rAX */
>> + [0xaa ... 0xaf] = (Known), /* STOS/LODS/SCAS */
>> + [0xb0 ... 0xb7] = (Known|Imm8), /* MOV $imm8, %reg */
>> + [0xb8 ... 0xbf] = (Known|Imm), /* MOV $imm{16,32,64}, %reg */
>> + [0xc0 ... 0xc1] = (Known|ModRM|Imm8), /* Grp2 (ROL..SAR $imm8, %reg) */
>> +
>> + [0xc3] = (Known), /* RET */
>> + [0xc4 ... 0xc5] = 0, /* LES/LDS but also VEX prefixes, not implemented. */
> This may bite us sooner or later, due to the VEX-encoded integer insns
> that there are. Of course as long as we don't use this function on
> compiled code, and as long as my "x86: allow Kconfig control over psABI
> level" doesn't come close to going in, that's merely a theoretical
> concern.
>
> Same goes for not supporting the 3-byte opcodes, which also encode
> certain integer insns.
I have no doubt that we're going to need to add support eventually.
But,
a) I don't have time right now
b) We have real bugs/limitations right now needing this functionality to
address (patch 5, and the xsave fixes, and bus lock trap enablement)
c) GitlabCI will reliably notice any new alternative instructions that
this can't decode (patch 3)
d) This function is a fastpath during the alternatives patching critical
region (patch 4)
Option d alone is a good reason not to decode VEX prefixes yet.
>
>> + [0xc6] = (Known|ModRM|Imm8), /* Grp11, Further ModRM decode */
>> + [0xc7] = (Known|ModRM|Imm), /* Grp11, Further ModRM decode */
>> +
>> + [0xcb ... 0xcc] = (Known), /* LRET/INT3 */
>> + [0xcd] = (Known|Imm8), /* INT $imm8 */
>> +
>> + [0xd0 ... 0xd3] = (Known|ModRM), /* Grp2 (ROL..SAR {$1,%cl}, %reg) */
>> +
>> + [0xd6] = (Known), /* UDB */
> I guess you consider XLAT, LOOP*, and J*CXZ as too odd to use in alternatives?
> Decoding-wise they're rather easy to implement.
They are easy, but they also shouldn't appear anywhere in Xen.
I know we've got one J*CXZ in the emulator. I tried quite hard to find
an alternative before deciding it was an acceptable solution given the
constraints, but it's in plain code.
>
>> + [0xe4 ... 0xe7] = (Known|Imm8), /* IN/OUT $imm8 */
>> + [0xe8 ... 0xe9] = (Known|Branch|Imm), /* CALL/JMP disp32 */
>> +
>> + [0xeb] = (Known|Branch|Imm8), /* JMP disp8 */
>> + [0xec ... 0xef] = (Known), /* IN/OUT %dx */
>> +
>> + [0xf1] = (Known), /* ICEBP */
>> +
>> + [0xf4] = (Known), /* HLT */
>> + [0xf5] = (Known), /* CMC */
>> + [0xf6 ... 0xf7] = (Known|ModRM), /* Grp3, Further ModRM decode */
>> + [0xf8 ... 0xfd] = (Known), /* CLC ... STD */
>> + [0xfe ... 0xff] = (Known|ModRM), /* Grp4 */
>> + };
>> + static const uint8_t init_or_livepatch_const twobyte[256] = {
>> + [0x00 ... 0x03] = (Known|ModRM), /* Grp6/Grp7/LAR/LSL */
> Leaving out INVD is surely find, but WBINVD?
Given now expensive WBINVD is, what possible reason can you think for
having it in an alternative ?
>
>> + [0x0b] = (Known), /* UD2 */
>> +
>> + [0x18 ... 0x1f] = (Known|ModRM), /* Grp16 (Hint Nop) */
>> + [0x20 ... 0x23] = (Known|ModRM), /* MOV %cr/%dr */
>> +
>> + [0x30 ... 0x33] = (Known), /* WRMSR/RDTSC/RDMSR/RDPMC */
>> +
>> + [0x40 ... 0x4f] = (Known|ModRM), /* CMOVcc */
>> +
>> + [0x80 ... 0x8f] = (Known|Branch|Imm), /* Jcc disp32 */
>> + [0x90 ... 0x9f] = (Known|ModRM), /* SETcc */
>> +
>> + [0xa0 ... 0xa2] = (Known), /* PUSH/POP %fs/CPUID */
>> + [0xa3] = (Known|ModRM), /* BT */
>> + [0xa4] = (Known|ModRM|Imm8), /* SHLD $imm8 */
>> + [0xa5] = (Known|ModRM), /* SHLD %cl */
>> +
>> + [0xa8 ... 0xa9] = (Known), /* PUSH/POP %gs */
>> +
>> + [0xab] = (Known|ModRM), /* BTS */
>> + [0xac] = (Known|ModRM|Imm8), /* SHRD $imm8 */
>> + [0xad ... 0xaf] = (Known|ModRM), /* SHRD %cl/Grp15/IMUL */
>> +
>> + [0xb0 ... 0xb9] = (Known|ModRM), /* CMPXCHG/LSS/BTR/LFS/LGS/MOVZxx/POPCNT/UD1 */
>> + [0xba] = (Known|ModRM|Imm8), /* Grp8 */
>> + [0xbb ... 0xbf] = (Known|ModRM), /* BTC/BSF/BSR/MOVSX */
>> + [0xc0 ... 0xc1] = (Known|ModRM), /* XADD */
> What about MOVNTI?
I judged that to be on the unlikely side to be needed.
>
>> + [0xc7] = (Known|ModRM), /* Grp9 */
>> + [0xc8 ... 0xcf] = (Known), /* BSWAP */
>> + };
> What about UD0?
UD0 differs between vendors and product lines from Intel.
>
>> + void *start = ip, *rel = NULL;
>> + unsigned int opc, rel_sz = 0;
>> + uint8_t b, d, rex = 0, osize = 4;
>> +
>> +#define OPC_TWOBYTE (1 << 8)
>> +
>> + /* Mutates IP, uses END. */
>> +#define FETCH(ty) \
>> + ({ \
>> + ty _val; \
>> + \
>> + if ( (ip + sizeof(ty)) > end ) \
>> + goto overrun; \
>> + _val = *(ty *)ip; \
>> + ip += sizeof(ty); \
>> + _val; \
>> + })
>> +
>> + for ( ;; ) /* Prefixes */
>> + {
>> + switch ( b = FETCH(uint8_t) )
>> + {
>> + case 0x26: /* ES override */
>> + case 0x2e: /* CS override */
>> + case 0x36: /* DS override */
>> + case 0x3e: /* SS override */
>> + case 0x64: /* FS override */
>> + case 0x65: /* GS override */
>> + case 0xf0: /* LOCK */
>> + case 0xf2: /* REPNE */
>> + case 0xf3: /* REP */
>> + break;
>> +
>> + case 0x66: /* Operand size override */
>> + osize = 2;
>> + break;
>> +
>> + /* case 0x67: Address size override, not implemented */
>> +
>> + case 0x40 ... 0x4f: /* REX */
>> + rex = b;
>> + continue;
>> +
>> + default:
>> + goto prefixes_done;
>> + }
>> + rex = 0; /* REX cancelled by subsequent legacy prefix. */
>> + }
>> + prefixes_done:
>> +
>> + if ( rex & REX_W )
>> + osize = 8;
>> +
>> + /* Fetch the main opcode byte(s) */
>> + if ( b == 0x0f )
>> + {
>> + b = FETCH(uint8_t);
>> + opc = OPC_TWOBYTE | b;
>> +
>> + d = twobyte[b];
>> + }
>> + else
>> + {
>> + opc = b;
>> + d = onebyte[b];
>> + }
>> +
>> + if ( unlikely(!(d & Known)) )
>> + goto unknown;
>> +
>> + if ( d & ModRM )
>> + {
>> + uint8_t modrm = FETCH(uint8_t);
>> + uint8_t mod = modrm >> 6;
>> + uint8_t reg = (modrm >> 3) & 7;
>> + uint8_t rm = modrm & 7;
>> +
>> + /* ModRM/SIB decode */
>> + if ( mod == 0 && rm == 5 ) /* RIP relative */
>> + {
>> + rel = ip;
>> + rel_sz = 4;
>> + FETCH(int32_t);
> FETCH() here but ...
>
>> + }
>> + else if ( mod != 3 && rm == 4 ) /* SIB */
>> + {
>> + uint8_t sib = FETCH(uint8_t);
>> + uint8_t base = sib & 7;
>> +
>> + if ( mod == 0 && base == 5 )
>> + goto disp32;
> ... goto here?
Hmm. That's an artefact of how it developed. Swapping this goto for
FETCH() does drop 20 bytes, but the function is rearranged so much that
it's hard to tell if this is because real logic is getting dropped.
>
>> + }
>> +
>> + if ( mod == 1 ) /* disp8 */
>> + FETCH(int8_t);
>> + else if ( mod == 2 ) /* disp32 */
>> + {
>> + disp32:
>> + FETCH(int32_t);
>> + }
> In several cases the FETCH()ed value isn't used. Compilers as well as Eclair
> (and alike) are happy with that?
Yes. The cover letter has a fully passing pipeline.
> And compilers also manage to eliminate the memory accesses then?
Yes.
>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -835,4 +835,18 @@ static inline void x86_emul_reset_event(struct x86_emulate_ctxt *ctxt)
>> ctxt->event = (struct x86_event){};
>> }
>>
>> +/*
>> + * x86_decode_lite(). Very minimal decoder for managing alternatives.
>> + *
>> + * @len is 0 on error, or nonzero on success. If the instruction has a
>> + * relative field, @rel_sz is nonzero, and @rel points at the field.
>> + */
>> +typedef struct {
>> + uint8_t len;
>> + uint8_t rel_sz; /* bytes: 0, 1 or 4 */
> Perhaps use bitfields in favor of fixed-width integers, seeing what
> ./CODING_STYLE says?
No. That destroys the code generation improvements gained by returning
a pair like this in the first place.
~Andrew
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite()
2026-08-03 16:03 ` Jan Beulich
@ 2026-08-04 19:37 ` Andrew Cooper
2026-08-05 6:45 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2026-08-04 19:37 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Teddy Astie, Xen-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite()
2026-08-04 18:56 ` Andrew Cooper
@ 2026-08-05 6:24 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2026-08-05 6:24 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Teddy Astie, Xen-devel
On 04.08.2026 20:56, Andrew Cooper wrote:
> On 03/08/2026 4:26 pm, Jan Beulich wrote:
>>> + [0x50 ... 0x5f] = (Known), /* PUSH/POP %reg */
>>> +
>>> + [0x62] = 0, /* BOUND, but also EVEX prefix, not implemented. */
>>> + [0x63] = (Known|ModRM), /* MOVSxd */
>>> +
>>> + [0x68] = (Known|Imm), /* PUSH $imm */
>>> + [0x69] = (Known|ModRM|Imm), /* IMUL $imm */
>>> + [0x6a] = (Known|Imm8), /* PUSH $imm8 */
>>> + [0x6b] = (Known|ModRM|Imm8), /* PUSH $imm8 */
>>> + [0x6c ... 0x6f] = (Known), /* INS/OUTS */
>>> + [0x70 ... 0x7f] = (Known|Branch|Imm8), /* Jcc disp8 */
>>> + [0x80] = (Known|ModRM|Imm8), /* Grp1 */
>>> + [0x81] = (Known|ModRM|Imm), /* Grp1 */
>>> +
>>> + [0x83] = (Known|ModRM|Imm8), /* Grp1 */
>>> + [0x84 ... 0x8e] = (Known|ModRM), /* TEST/XCHG/MOV/MOV-SREG/LEA */
>>> + [0x8f] = 0, /* Grp1A - POP but also XOP prefix, not implemented. */
>> POP doesn't look all that unlikely to be used in inline assembly, and
>> hence in alternatives. That said, of course using it with a memory
>> operand requires quite a bit of care.
>
> We have no alternatives playing with the stack (beyond CALL
> instructions), and no alternatives which have any net %rsp delta.
>
> PUSH/POP MEM are rare in general and Xen doesn't have any at all.
Well, okay then. Nevertheless I'd like to mention that the encoding can
also be used for REG forms. If needed for size reasons, that may or may
not be more efficient than adding a NOP or no-op prefix.
>>> + [0x90 ... 0x99] = (Known), /* NOP/XCHG %rAX/CLTQ/CQTO */
>>> +
>>> + [0x9b ... 0x9f] = (Known), /* FWAIT/PUSHF/POPF/SAHF/LAHF */
>>> + [0xa0 ... 0xa3] = (Known|Moffs), /* MOVABS */
>>> + [0xa4 ... 0xa7] = (Known), /* MOVS/CMPS */
>>> + [0xa8] = (Known|Imm8), /* TEST %al */
>>> + [0xa9] = (Known|Imm), /* TEST %rAX */
>>> + [0xaa ... 0xaf] = (Known), /* STOS/LODS/SCAS */
>>> + [0xb0 ... 0xb7] = (Known|Imm8), /* MOV $imm8, %reg */
>>> + [0xb8 ... 0xbf] = (Known|Imm), /* MOV $imm{16,32,64}, %reg */
>>> + [0xc0 ... 0xc1] = (Known|ModRM|Imm8), /* Grp2 (ROL..SAR $imm8, %reg) */
>>> +
>>> + [0xc3] = (Known), /* RET */
>>> + [0xc4 ... 0xc5] = 0, /* LES/LDS but also VEX prefixes, not implemented. */
>> This may bite us sooner or later, due to the VEX-encoded integer insns
>> that there are. Of course as long as we don't use this function on
>> compiled code, and as long as my "x86: allow Kconfig control over psABI
>> level" doesn't come close to going in, that's merely a theoretical
>> concern.
>>
>> Same goes for not supporting the 3-byte opcodes, which also encode
>> certain integer insns.
>
> I have no doubt that we're going to need to add support eventually.
>
> But,
> a) I don't have time right now
> b) We have real bugs/limitations right now needing this functionality to
> address (patch 5, and the xsave fixes, and bus lock trap enablement)
> c) GitlabCI will reliably notice any new alternative instructions that
> this can't decode (patch 3)
> d) This function is a fastpath during the alternatives patching critical
> region (patch 4)
>
> Option d alone is a good reason not to decode VEX prefixes yet.
Personally I think a is most relevant. As said in the later reply, once
we start using MSR-IMM insns, at least VEX3 map 7 will need decoding
anyway.
>>> + [0xe4 ... 0xe7] = (Known|Imm8), /* IN/OUT $imm8 */
>>> + [0xe8 ... 0xe9] = (Known|Branch|Imm), /* CALL/JMP disp32 */
>>> +
>>> + [0xeb] = (Known|Branch|Imm8), /* JMP disp8 */
>>> + [0xec ... 0xef] = (Known), /* IN/OUT %dx */
>>> +
>>> + [0xf1] = (Known), /* ICEBP */
>>> +
>>> + [0xf4] = (Known), /* HLT */
>>> + [0xf5] = (Known), /* CMC */
>>> + [0xf6 ... 0xf7] = (Known|ModRM), /* Grp3, Further ModRM decode */
>>> + [0xf8 ... 0xfd] = (Known), /* CLC ... STD */
>>> + [0xfe ... 0xff] = (Known|ModRM), /* Grp4 */
>>> + };
>>> + static const uint8_t init_or_livepatch_const twobyte[256] = {
>>> + [0x00 ... 0x03] = (Known|ModRM), /* Grp6/Grp7/LAR/LSL */
>> Leaving out INVD is surely find, but WBINVD?
>
> Given now expensive WBINVD is, what possible reason can you think for
> having it in an alternative ?
It's more like e.g. WBNOINVD, which could appear in an alternative in
principle, if its encoding didn't mean WBINVD anyway on older hardware.
>>> + [0x0b] = (Known), /* UD2 */
>>> +
>>> + [0x18 ... 0x1f] = (Known|ModRM), /* Grp16 (Hint Nop) */
>>> + [0x20 ... 0x23] = (Known|ModRM), /* MOV %cr/%dr */
>>> +
>>> + [0x30 ... 0x33] = (Known), /* WRMSR/RDTSC/RDMSR/RDPMC */
>>> +
>>> + [0x40 ... 0x4f] = (Known|ModRM), /* CMOVcc */
>>> +
>>> + [0x80 ... 0x8f] = (Known|Branch|Imm), /* Jcc disp32 */
>>> + [0x90 ... 0x9f] = (Known|ModRM), /* SETcc */
>>> +
>>> + [0xa0 ... 0xa2] = (Known), /* PUSH/POP %fs/CPUID */
>>> + [0xa3] = (Known|ModRM), /* BT */
>>> + [0xa4] = (Known|ModRM|Imm8), /* SHLD $imm8 */
>>> + [0xa5] = (Known|ModRM), /* SHLD %cl */
>>> +
>>> + [0xa8 ... 0xa9] = (Known), /* PUSH/POP %gs */
>>> +
>>> + [0xab] = (Known|ModRM), /* BTS */
>>> + [0xac] = (Known|ModRM|Imm8), /* SHRD $imm8 */
>>> + [0xad ... 0xaf] = (Known|ModRM), /* SHRD %cl/Grp15/IMUL */
>>> +
>>> + [0xb0 ... 0xb9] = (Known|ModRM), /* CMPXCHG/LSS/BTR/LFS/LGS/MOVZxx/POPCNT/UD1 */
>>> + [0xba] = (Known|ModRM|Imm8), /* Grp8 */
>>> + [0xbb ... 0xbf] = (Known|ModRM), /* BTC/BSF/BSR/MOVSX */
>>> + [0xc0 ... 0xc1] = (Known|ModRM), /* XADD */
>> What about MOVNTI?
>
> I judged that to be on the unlikely side to be needed.
Hmm, I'm not going to insist, but I think we'd better have it right away.
>>> + [0xc7] = (Known|ModRM), /* Grp9 */
>>> + [0xc8 ... 0xcf] = (Known), /* BSWAP */
>>> + };
>> What about UD0?
>
> UD0 differs between vendors and product lines from Intel.
Would you mind leaving a commented (to this effect) 0 entry?
>>> + if ( mod == 1 ) /* disp8 */
>>> + FETCH(int8_t);
>>> + else if ( mod == 2 ) /* disp32 */
>>> + {
>>> + disp32:
>>> + FETCH(int32_t);
>>> + }
>> In several cases the FETCH()ed value isn't used. Compilers as well as Eclair
>> (and alike) are happy with that?
>
> Yes. The cover letter has a fully passing pipeline.
Which you know as well as I do, says next to nothing, as we don't even come
close to covering the complete version range. That said, it's likely good
enough. I'm merely surprised no tool has to say anything about these unused
"return" values of the macro.
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>>> @@ -835,4 +835,18 @@ static inline void x86_emul_reset_event(struct x86_emulate_ctxt *ctxt)
>>> ctxt->event = (struct x86_event){};
>>> }
>>>
>>> +/*
>>> + * x86_decode_lite(). Very minimal decoder for managing alternatives.
>>> + *
>>> + * @len is 0 on error, or nonzero on success. If the instruction has a
>>> + * relative field, @rel_sz is nonzero, and @rel points at the field.
>>> + */
>>> +typedef struct {
>>> + uint8_t len;
>>> + uint8_t rel_sz; /* bytes: 0, 1 or 4 */
>> Perhaps use bitfields in favor of fixed-width integers, seeing what
>> ./CODING_STYLE says?
>
> No. That destroys the code generation improvements gained by returning
> a pair like this in the first place.
I was under the pretty clear impression that halfway recent compilers
treat 8-bit bitfields the same as uint8_t ordinary fields. (And no, I
did by no means suggest to shrink the width of the fields.)
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite()
2026-08-04 19:37 ` Andrew Cooper
@ 2026-08-05 6:45 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2026-08-05 6:45 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Teddy Astie, Xen-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-08-05 6:45 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-03 7:20 [PATCH v3 0/5] x86/alternatives: Adjust all insn-relative fields Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 1/5] x86/emul: Introduce x86_decode_lite() Andrew Cooper
2026-08-03 9:14 ` Andrew Cooper
2026-08-03 15:26 ` Jan Beulich
2026-08-04 15:39 ` Jan Beulich
2026-08-04 18:56 ` Andrew Cooper
2026-08-05 6:24 ` Jan Beulich
2026-08-03 7:20 ` [PATCH v3 2/5] tests/x86: Introduce a userspace test harness for x86_decode_lite() Andrew Cooper
2026-08-03 16:03 ` Jan Beulich
2026-08-04 19:37 ` Andrew Cooper
2026-08-05 6:45 ` Jan Beulich
2026-08-03 7:20 ` [PATCH v3 3/5] x86/alternative: Walk all replacements during self tests Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 4/5] x86/alternative: Relocate all insn-relative fields Andrew Cooper
2026-08-03 7:20 ` [PATCH v3 5/5] x86/spec-ctrl: Introduce and use DO_COND_BHB_SEQ Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.