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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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