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

[Jan Beulich <jbeulich@xxxxxxxx>]

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