Re: [PATCH RFC v1 16/20] KVM: x86: Decode REX2 prefix in the emulator

["Chang S. Bae" <chang.seok.bae@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2564 bytes --]

On 11/11/2025 9:55 AM, Paolo Bonzini wrote:
> On 11/10/25 19:01, Chang S. Bae wrote:
>>
>>           case 0x40 ... 0x4f: /* REX */
>>               if (mode != X86EMUL_MODE_PROT64)
>>                   goto done_prefixes;
>> +            if (ctxt->rex_prefix == REX2_PREFIX)
>> +                break;
>>               ctxt->rex_prefix = REX_PREFIX;
>>               ctxt->rex.raw    = 0x0f & ctxt->b;
>>               continue;
>> +        case 0xd5: /* REX2 */
>> +            if (mode != X86EMUL_MODE_PROT64)
>> +                goto done_prefixes;
> Here you should also check
> 
>      if (ctxt->rex_prefix == REX_PREFIX) {
>          ctxt->rex_prefix = REX2_INVALID;
>          goto done_prefixes;
>      }

You're right. Section 3.1.2.1 states:
| A REX prefix (0x4*) immediately preceding the REX2 prefix is not
| allowed and triggers #UD.

Now I think REX2_INVALID would just add another condition to handle
later. Instead, for such invalid case, it might be simpler to mark the
opcode as undefined and jump all the way after the lookup. See the diff
-- please let me know if you dislike it.

>> +            if (ctxt->rex_prefix == REX2_PREFIX &&
>> +                ctxt->rex.bits.m0 == 0)
>> +                break;
>> +            ctxt->rex_prefix = REX2_PREFIX;
>> +            ctxt->rex.raw    = insn_fetch(u8, ctxt);
>> +            continue;
> After REX2 always comes the main opcode byte, so you can "goto 
> done_prefixes" here.  Or even jump here already; in pseudocode:
> 
>      ctxt->b = insn_fetch(u8, ctxt);
>      if (rex2 & REX_M)
>          goto decode_twobyte;
>      else
>          goto decode_onebyte;

Yes, agreed. I think this makes the control flow more explicit.

>> +        if (ctxt->rex_prefix == REX2_PREFIX) {
>> +            /*
>> +             * A legacy or REX prefix following a REX2 prefix
>> +             * forms an invalid byte sequences. Likewise,
>> +             * a second REX2 prefix following a REX2 prefix
>> +             * with M0=0 is invalid.
>> +             */
>> +            ctxt->rex_prefix = REX2_INVALID;
>> +            goto done_prefixes;
>> +        }
> 
> ... and this is not needed.

I really like that this can go away.

[-- Attachment #2: PATCH16.diff --]
[-- Type: text/plain, Size: 2699 bytes --]

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b8a946cbd587..c62d21de14cb 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4479,6 +4479,8 @@ static const struct opcode opcode_map_0f_38[256] = {
 	N, N, X4(N), X8(N)
 };
 
+static const struct opcode undefined = D(Undefined);
+
 #undef D
 #undef N
 #undef G
@@ -4765,6 +4767,11 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
 	return rc;
 }
 
+static inline bool emul_egpr_enabled(struct x86_emulate_ctxt *ctxt __maybe_unused)
+{
+	return false;
+}
+
 int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int emulation_type)
 {
 	int rc = X86EMUL_CONTINUE;
@@ -4817,7 +4824,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int
 	ctxt->op_bytes = def_op_bytes;
 	ctxt->ad_bytes = def_ad_bytes;
 
-	/* Legacy prefixes. */
+	/* Legacy and REX/REX2 prefixes. */
 	for (;;) {
 		switch (ctxt->b = insn_fetch(u8, ctxt)) {
 		case 0x66:	/* operand-size override */
@@ -4860,9 +4867,29 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int
 		case 0x40 ... 0x4f: /* REX */
 			if (mode != X86EMUL_MODE_PROT64)
 				goto done_prefixes;
+			if (ctxt->rex_prefix == REX2_PREFIX) {
+				opcode = undefined;
+				goto decode_done;
+			}
 			ctxt->rex_prefix = REX_PREFIX;
 			ctxt->rex        = 0x0f & ctxt->b;
 			continue;
+		case 0xd5: /* REX2 */
+			if (mode != X86EMUL_MODE_PROT64)
+				goto done_prefixes;
+			if ((ctxt->rex_prefix == REX2_PREFIX && (ctxt->rex & REX_M) == 0) ||
+			    (ctxt->rex_prefix == REX_PREFIX) ||
+			    (!emul_egpr_enabled(ctxt))) {
+				opcode = undefined;
+				goto decode_done;
+			}
+			ctxt->rex_prefix = REX2_PREFIX;
+			ctxt->rex = insn_fetch(u8, ctxt);
+			ctxt->b   = insn_fetch(u8, ctxt);
+			if (ctxt->rex & REX_M)
+				goto decode_twobytes;
+			else
+				goto decode_onebyte;
 		case 0xf0:	/* LOCK */
 			ctxt->lock_prefix = 1;
 			break;
@@ -4889,6 +4916,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int
 	if (ctxt->b == 0x0f) {
 		/* Escape byte: start two-byte opcode sequence */
 		ctxt->b = insn_fetch(u8, ctxt);
+decode_twobytes:
 		if (ctxt->b == 0x38 && ctxt->rex_prefix != REX2_PREFIX) {
 			/* Three-byte opcode */
 			ctxt->opcode_len = 3;
@@ -4900,10 +4928,12 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int
 			opcode = twobyte_table[ctxt->b];
 		}
 	} else {
+decode_onebyte:
 		/* Single-byte opcode */
 		ctxt->opcode_len = 1;
 		opcode = opcode_table[ctxt->b];
 	}
+decode_done:
 	ctxt->d = opcode.flags;
 
 	if (ctxt->d & NoRex2 && ctxt->rex_prefix == REX2_PREFIX)