Re: [PATCH v4 1/5] x86/emul: Introduce x86_decode_lite()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2026 23:05, Andrew Cooper wrote:
> 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,
> as this will be a fasthpath during patching.

Just that patching isn't really something where performance matters very much.
It's done during boot (or livepatch load), and then not anymore. (I still
agree that reduction of complexity is a worthwhile goal, or course.)

> --- /dev/null
> +++ b/xen/arch/x86/x86_emulate/decode-lite.c
> @@ -0,0 +1,331 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifdef __XEN__
> +# include <xen/init.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 code 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, const 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. */

Given the purpose, BOUND isn't relevant here at all: We don't mean to use
this decoder on 32- or 16-bit code. Hence I'd suggest to put emphasis on
EVEX, and name BOUND secondarily. See how ...

> +        [0x63]          = (Known|ModRM),       /* MOVSxd */

... here you don't mention ARPL at all. (Also - why xd in lower case?)

> +        [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. */

I continue thinking that the asymmetry between PUSH r/m and POP r/m being
deliberate wants at least mentioning in the description.

> +        [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 */

Here and for RET above: I think it's fine to entirely omit entries for
opcodes not defined in 64-bit mode (e.g. 9A, EA), and for SIMD ones. At
the same time I think we'd be better off having explicit (zero) entries
for (integer) opcodes we deliberately mean to not support (RET/LRET with
imm16, ENTER, LEAVE; more further down).

> +        [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 */

With it being MOVSX here, perhaps MOVZX above? Or the other way around,
MOVSxx here?

> +        [0xc0 ... 0xc1] = (Known|ModRM),       /* XADD */

Is there a reason to permit XADD but not CMPXCHG?

Preferably with the above addressed (but explicitly also with some / all
left as is, yet then with the expectation that I may send an adjustment
patch on top, which I'd then expect to not be outright refused/ignored):
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan