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

> --- /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