[PATCH] x86_emulate: Emulate nop (0x90) instruction

[PATCH] x86_emulate: Emulate nop (0x90) instruction

[Mohammed Gamal]

While testing FreeDOS under KVM I encountered a vmentry failure
because of a nop instruction.
This patch adds the instruction to the x86 emulator.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/x86_emulate.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index b90857c..ef1b7d7 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -141,7 +141,7 @@ static u16 opcode_table[256] = {
 	DstMem | SrcReg | ModRM | Mov, ModRM | DstReg,
 	DstReg | SrcMem | ModRM | Mov, Group | Group1A,
 	/* 0x90 - 0x9F */
-	0, 0, 0, 0, 0, 0, 0, 0,
+	ImplicitOps, 0, 0, 0, 0, 0, 0, 0,
 	0, 0, 0, 0, ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
 	/* 0xA0 - 0xA7 */
 	ByteOp | DstReg | SrcMem | Mov | MemAbs, DstReg | SrcMem | Mov | MemAbs,
@@ -1560,6 +1560,9 @@ special_insn:
 		if (rc != 0)
 			goto done;
 		break;
+	case 0x90: /* nop */
+		if(! (c->rex_prefix & 1) )
+			break;
 	case 0x9c: /* pushf */
 		c->src.val =  (unsigned long) ctxt->eflags;
 		emulate_push(ctxt);

Re: [PATCH] x86_emulate: Emulate nop (0x90) instruction

[Andi Kleen]

"Mohammed Gamal" <m.gamal005@gmail.com> writes:
>  	ByteOp | DstReg | SrcMem | Mov | MemAbs, DstReg | SrcMem | Mov | MemAbs,
> @@ -1560,6 +1560,9 @@ special_insn:
>  		if (rc != 0)
>  			goto done;
>  		break;
> +	case 0x90: /* nop */
> +		if(! (c->rex_prefix & 1) )
> +			break;
>  	case 0x9c: /* pushf */
>  		c->src.val =  (unsigned long) ctxt->eflags;
>  		emulate_push(ctxt);

Is falling through to pushf really correct?  And not sure what the if checks.

iirc it should be just

     case 0x90: /* nop */
          break;

BTW there are lots more nop encodings.

-Andi

Re: [PATCH] x86_emulate: Emulate nop (0x90) instruction

[Mohammed Gamal]

On Sat, Jun 14, 2008 at 4:26 PM, Andi Kleen <andi@firstfloor.org> wrote:
> "Mohammed Gamal" <m.gamal005@gmail.com> writes:
>>       ByteOp | DstReg | SrcMem | Mov | MemAbs, DstReg | SrcMem | Mov | MemAbs,
>> @@ -1560,6 +1560,9 @@ special_insn:
>>               if (rc != 0)
>>                       goto done;
>>               break;
>> +     case 0x90: /* nop */
>> +             if(! (c->rex_prefix & 1) )
>> +                     break;
>>       case 0x9c: /* pushf */
>>               c->src.val =  (unsigned long) ctxt->eflags;
>>               emulate_push(ctxt);
>
> Is falling through to pushf really correct?  And not sure what the if checks.
>
> iirc it should be just
>
>     case 0x90: /* nop */
>          break;
>
> BTW there are lots more nop encodings.
>
> -Andi
>

Thanks for pointing this out. 0x90 is also the opcode for xchg r8,rAX
instruction. The code should rather fall to the yet-to-be added xchg
instructions. A patch will be underway ;)

Re: [PATCH] x86_emulate: Emulate nop (0x90) instruction

[Andi Kleen]

> Thanks for pointing this out. 0x90 is also the opcode for xchg r8,rAX
> instruction.

But nop doesn't actually need to execute exchange.

-Andi

Re: [PATCH] x86_emulate: Emulate nop (0x90) instruction

[Avi Kivity]

Andi Kleen wrote:
>> Thanks for pointing this out. 0x90 is also the opcode for xchg r8,rAX
>> instruction.
>>     
>
> But nop doesn't actually need to execute exchange.
>   

The sequence 0x41 0x90 uses the 0x90 opcode but is not a nop.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.