[PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Mohammed Gamal]

Add call near absolute instruction

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

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 3ac2f14..0630d21 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -286,7 +286,8 @@ static u16 group_table[] = {
 	ByteOp | DstMem | SrcNone | ModRM, ByteOp | DstMem | SrcNone | ModRM,
 	0, 0, 0, 0, 0, 0,
 	[Group5*8] =
-	DstMem | SrcNone | ModRM, DstMem | SrcNone | ModRM, 0, 0,
+	DstMem | SrcNone | ModRM, DstMem | SrcNone | ModRM,
+	SrcMem | ModRM | Stack, 0,
 	SrcMem | ModRM, 0, SrcMem | ModRM | Stack, 0,
 	[Group7*8] =
 	0, 0, ModRM | SrcMem, ModRM | SrcMem,
@@ -1162,6 +1163,14 @@ static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
 	case 1:	/* dec */
 		emulate_1op("dec", c->dst, ctxt->eflags);
 		break;
+	case 2: /* call near abs */ {
+		long int old_eip;
+		old_eip = c->eip;
+		c->eip = c->src.val;
+		c->src.val = old_eip;
+		emulate_push(ctxt);
+		break;
+	}
 	case 4: /* jmp abs */
 		c->eip = c->src.val;
 		break;
-- 
1.5.4.3

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Guillaume Thouvenin]

On Mon, 8 Sep 2008 21:47:19 +0300
Mohammed Gamal <m.gamal005@gmail.com> wrote:


> +	SrcMem | ModRM | Stack, 0,

Are you sure about this? In Xen they use "DstMem | SrcNone | ModRM"
Anyway I tested the patch and the next problem is with the following
instruction:

emulation failed (emulation failure) rip 4423e 3c 25 0f 84

Thus I added the following patch (see the end of this email) and now
another problem:

emulation failed (mmio) rip 44b5b 03 84 c0 75

So it's not the same problem. I'm trying to fix that. It would be cool
to be able to boot a distribution with emulate_invalid_guest_state enabled ;)

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>

---
diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 30b93ee..b65bc94 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -107,7 +107,7 @@ static u16 opcode_table[256] = {
 	/* 0x38 - 0x3F */
 	ByteOp | DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM,
 	ByteOp | DstReg | SrcMem | ModRM, DstReg | SrcMem | ModRM,
-	0, 0, 0, 0,
+	ByteOp | DstReg |SrcImm, DstReg|SrcImm, 0, 0,
 	/* 0x40 - 0x47 */
 	DstReg, DstReg, DstReg, DstReg, DstReg, DstReg, DstReg, DstReg,
 	/* 0x48 - 0x4F */
@@ -1405,10 +1405,22 @@ special_insn:
 	      xor:		/* xor */
 		emulate_2op_SrcV("xor", c->src, c->dst, ctxt->eflags);
 		break;
-	case 0x38 ... 0x3d:
+	case 0x38 ... 0x3b:
 	      cmp:		/* cmp */
 		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
 		break;
+	case 0x3c ... 0x3d:     /* cmp al imm8 or ax imm16 or eax imm32 */
+		c->dst.type = OP_REG;
+		c->dst.bytes = c->op_bytes;
+		c->dst.ptr = &c->regs[VCPU_REGS_RAX];
+		if (c->op_bytes == 1)
+			c->dst.val = *(u8 *)c->dst.ptr;
+		else if (c->op_bytes == 2)
+			c->dst.val = *(u16 *)c->dst.ptr;
+		else
+			c->dst.val = *(u32 *)c->dst.ptr;
+		c->dst.orig_val = c->dst.val;
+		goto cmp;
 	case 0x40 ... 0x47: /* inc r16/r32 */
 		emulate_1op("inc", c->dst, ctxt->eflags);
 		break;

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Mohammed Gamal]

On Tue, Sep 9, 2008 at 10:16 AM, Guillaume Thouvenin
<guillaume.thouvenin@ext.bull.net> wrote:
> On Mon, 8 Sep 2008 21:47:19 +0300
> Mohammed Gamal <m.gamal005@gmail.com> wrote:
>
>
>> +     SrcMem | ModRM | Stack, 0,
>
> Are you sure about this? In Xen they use "DstMem | SrcNone | ModRM"
> Anyway I tested the patch and the next problem is with the following
> instruction:
>
I'll look at the Xen code, however this is working well with me. I
added some logging to report all instructions in addition to a test
case in the real mode test harness and the call instruction jumps to
the correct address.

> emulation failed (emulation failure) rip 4423e 3c 25 0f 84
>
> Thus I added the following patch (see the end of this email) and now
> another problem:
>
> emulation failed (mmio) rip 44b5b 03 84 c0 75
>
We don't handle mmio emulation yet. It'd be great to see it handled.

> So it's not the same problem. I'm trying to fix that. It would be cool
> to be able to boot a distribution with emulate_invalid_guest_state enabled ;)
>
> Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
>
> ---
> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
> index 30b93ee..b65bc94 100644
> --- a/arch/x86/kvm/x86_emulate.c
> +++ b/arch/x86/kvm/x86_emulate.c
> @@ -107,7 +107,7 @@ static u16 opcode_table[256] = {
>        /* 0x38 - 0x3F */
>        ByteOp | DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM,
>        ByteOp | DstReg | SrcMem | ModRM, DstReg | SrcMem | ModRM,
> -       0, 0, 0, 0,
> +       ByteOp | DstReg |SrcImm, DstReg|SrcImm, 0, 0,
>        /* 0x40 - 0x47 */
>        DstReg, DstReg, DstReg, DstReg, DstReg, DstReg, DstReg, DstReg,
>        /* 0x48 - 0x4F */
> @@ -1405,10 +1405,22 @@ special_insn:
>              xor:              /* xor */
>                emulate_2op_SrcV("xor", c->src, c->dst, ctxt->eflags);
>                break;
> -       case 0x38 ... 0x3d:
> +       case 0x38 ... 0x3b:
>              cmp:              /* cmp */
>                emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
>                break;
> +       case 0x3c ... 0x3d:     /* cmp al imm8 or ax imm16 or eax imm32 */
> +               c->dst.type = OP_REG;
> +               c->dst.bytes = c->op_bytes;
> +               c->dst.ptr = &c->regs[VCPU_REGS_RAX];
> +               if (c->op_bytes == 1)
> +                       c->dst.val = *(u8 *)c->dst.ptr;
> +               else if (c->op_bytes == 2)
> +                       c->dst.val = *(u16 *)c->dst.ptr;
> +               else
> +                       c->dst.val = *(u32 *)c->dst.ptr;
> +               c->dst.orig_val = c->dst.val;
> +               goto cmp;
>        case 0x40 ... 0x47: /* inc r16/r32 */
>                emulate_1op("inc", c->dst, ctxt->eflags);
>                break;
>

It'd be great to add a test case for this instruction in the to ensure
it's not broken. You can find the test harness in the userspace tree
at user/test/x86/realmode.c

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Avi Kivity]

Guillaume Thouvenin wrote:
> -	case 0x38 ... 0x3d:
> +	case 0x38 ... 0x3b:
>  	      cmp:		/* cmp */
>  		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
>  		break;
> +	case 0x3c ... 0x3d:     /* cmp al imm8 or ax imm16 or eax imm32 */
> +		c->dst.type = OP_REG;
> +		c->dst.bytes = c->op_bytes;
> +		c->dst.ptr = &c->regs[VCPU_REGS_RAX];
> +		if (c->op_bytes == 1)
> +			c->dst.val = *(u8 *)c->dst.ptr;
> +		else if (c->op_bytes == 2)
> +			c->dst.val = *(u16 *)c->dst.ptr;
> +		else
> +			c->dst.val = *(u32 *)c->dst.ptr;
> +		c->dst.orig_val = c->dst.val;
> +		goto cmp;
>   

SrcAcc would remove the need for this change.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Mohammed Gamal]

On Tue, Sep 9, 2008 at 5:51 PM, Avi Kivity <avi@qumranet.com> wrote:
> Guillaume Thouvenin wrote:
>>
>> -       case 0x38 ... 0x3d:
>> +       case 0x38 ... 0x3b:
>>              cmp:              /* cmp */
>>                emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
>>                break;
>> +       case 0x3c ... 0x3d:     /* cmp al imm8 or ax imm16 or eax imm32 */
>> +               c->dst.type = OP_REG;
>> +               c->dst.bytes = c->op_bytes;
>> +               c->dst.ptr = &c->regs[VCPU_REGS_RAX];
>> +               if (c->op_bytes == 1)
>> +                       c->dst.val = *(u8 *)c->dst.ptr;
>> +               else if (c->op_bytes == 2)
>> +                       c->dst.val = *(u16 *)c->dst.ptr;
>> +               else
>> +                       c->dst.val = *(u32 *)c->dst.ptr;
>> +               c->dst.orig_val = c->dst.val;
>> +               goto cmp;
>>
>
> SrcAcc would remove the need for this change.
>

Stupid question: What does Acc stand for? :)

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Avi Kivity]

Mohammed Gamal wrote:
>>
>> SrcAcc would remove the need for this change.
>>
>>     
>
> Stupid question: What does Acc stand for? :)
>   

Accumulator (al/ax/eax/rax).  In the good old days cpus would have only 
one register that was able to fully participate in arithmetic 
operations, typically called A for Accumulator.  The x86 retains this 
tradition by having special, shorter encodings for the A register (like 
the cmp opcode), and even some instructions that only operate on A (like 
mul).

SrcAcc and DstAcc would accommodate these instructions by decoding A 
into the corresponding 'struct operand'.

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

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Guillaume Thouvenin]

On Tue, 9 Sep 2008 15:49:35 +0300
"Mohammed Gamal" <m.gamal005@gmail.com> wrote:

> > Thus I added the following patch (see the end of this email) and now
> > another problem:
> >
> > emulation failed (mmio) rip 44b5b 03 84 c0 75
> >
> We don't handle mmio emulation yet. It'd be great to see it handled.

If we have an EMULATE_DO_MMIO error when emulating instruction in
handle_invalid_guest_state() we should forward the problem to kvm-qemu
no?

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Avi Kivity]

Mohammed Gamal wrote:
> Add call near absolute instruction
>   

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Guillaume Thouvenin]

On Tue, 09 Sep 2008 17:51:03 +0300
Avi Kivity <avi@qumranet.com> wrote:

> Guillaume Thouvenin wrote:
> > -	case 0x38 ... 0x3d:
> > +	case 0x38 ... 0x3b:
> >  	      cmp:		/* cmp */
> >  		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> >  		break;
> > +	case 0x3c ... 0x3d:     /* cmp al imm8 or ax imm16 or eax imm32 */
> > +		c->dst.type = OP_REG;
> > +		c->dst.bytes = c->op_bytes;
> > +		c->dst.ptr = &c->regs[VCPU_REGS_RAX];
> > +		if (c->op_bytes == 1)
> > +			c->dst.val = *(u8 *)c->dst.ptr;
> > +		else if (c->op_bytes == 2)
> > +			c->dst.val = *(u16 *)c->dst.ptr;
> > +		else
> > +			c->dst.val = *(u32 *)c->dst.ptr;
> > +		c->dst.orig_val = c->dst.val;
> > +		goto cmp;
> >   
> 
> SrcAcc would remove the need for this change.

As I don't see SrcAcc in the source it means that you suggest to add a
new type of source operand right?

Guillaume 

Re: [PATCH] x86 emulator: Add call near absolute instruction (group5: opcode 0xff mod r/m 2)

[Mohammed Gamal]

On Fri, Sep 12, 2008 at 10:40 AM, Guillaume Thouvenin
<guillaume.thouvenin@ext.bull.net> wrote:
> On Tue, 09 Sep 2008 17:51:03 +0300
> Avi Kivity <avi@qumranet.com> wrote:
>
>> Guillaume Thouvenin wrote:
>> > -   case 0x38 ... 0x3d:
>> > +   case 0x38 ... 0x3b:
>> >           cmp:              /* cmp */
>> >             emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
>> >             break;
>> > +   case 0x3c ... 0x3d:     /* cmp al imm8 or ax imm16 or eax imm32 */
>> > +           c->dst.type = OP_REG;
>> > +           c->dst.bytes = c->op_bytes;
>> > +           c->dst.ptr = &c->regs[VCPU_REGS_RAX];
>> > +           if (c->op_bytes == 1)
>> > +                   c->dst.val = *(u8 *)c->dst.ptr;
>> > +           else if (c->op_bytes == 2)
>> > +                   c->dst.val = *(u16 *)c->dst.ptr;
>> > +           else
>> > +                   c->dst.val = *(u32 *)c->dst.ptr;
>> > +           c->dst.orig_val = c->dst.val;
>> > +           goto cmp;
>> >
>>
>> SrcAcc would remove the need for this change.
>
> As I don't see SrcAcc in the source it means that you suggest to add a
> new type of source operand right?
>
> Guillaume
>

Yes