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

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