[PATCH] x86 emulator: Add IRET instruction

[PATCH] x86 emulator: Add IRET instruction

[Mohammed Gamal]

Ths patch adds IRET instruction (opcode 0xcf).
Currently, only IRET in real mode is emulated. Protected mode support is to be added later if needed.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>

----

Changes from v1:
- Corrected handling of eflags
---
 arch/x86/kvm/emulate.c |   69 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b38bd8b..e45fd28 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1778,6 +1778,69 @@ static int emulate_popa(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
+static int emulate_iret_real(struct x86_emulate_ctxt *ctxt,
+			     struct x86_emulate_ops *ops)
+{
+	struct decode_cache *c = &ctxt->decode;
+	int rc = X86EMUL_CONTINUE;
+	unsigned long temp_eip = 0;
+	unsigned long temp_eflags = 0;
+	unsigned long mask = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF | EFLG_TF |
+			     EFLG_IF | EFLG_DF | EFLG_OF | EFLG_IOPL | EFLG_NT | EFLG_RF |
+			     EFLG_AC | EFLG_ID | (1 << 1); /* Last one is the reserved bit */
+	unsigned long vm86_mask = EFLG_VM | EFLG_VIF | EFLG_VIP;
+
+	/* TODO: Add stack limit check */
+
+	rc = emulate_pop(ctxt, ops, &temp_eip, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	if (temp_eip & ~0xffff) {
+		emulate_gp(ctxt, 0);
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
+	c->eip = temp_eip;
+
+	rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_CS);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	rc = emulate_pop(ctxt, ops, &temp_eflags, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	if (c->op_bytes == 4) {
+		temp_eflags = ((temp_eflags & mask) | (ctxt->eflags & vm86_mask));
+		ctxt->eflags = temp_eflags;
+	} else if (c->op_bytes == 2) {
+		temp_eflags &= ~0xfffd; /* Do not clear reserved bit 1 */
+		ctxt->eflags |= temp_eflags;
+	}
+
+	return rc;
+}
+
+static inline int emulate_iret(struct x86_emulate_ctxt *ctxt,
+				    struct x86_emulate_ops* ops)
+{
+	switch(ctxt->mode) {
+	case X86EMUL_MODE_REAL:
+		return emulate_iret_real(ctxt, ops);
+	case X86EMUL_MODE_VM86:
+	case X86EMUL_MODE_PROT16:
+	case X86EMUL_MODE_PROT32:
+	case X86EMUL_MODE_PROT64:
+	default:
+		/* iret from protected mode unimplemented yet */
+		return X86EMUL_UNHANDLEABLE;		
+	}
+}
+
 static inline int emulate_grp1a(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {
@@ -2910,6 +2973,12 @@ special_insn:
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
+	case 0xcf:		/* iret */
+		rc = emulate_iret(ctxt, ops);
+
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+		break;
 	case 0xd0 ... 0xd1:	/* Grp2 */
 		c->src.val = 1;
 		emulate_grp2(ctxt);
-- 
1.7.0.4

Re: [PATCH] x86 emulator: Add IRET instruction

[Avi Kivity]

  On 07/28/2010 02:06 AM, Mohammed Gamal wrote:
> Ths patch adds IRET instruction (opcode 0xcf).
> Currently, only IRET in real mode is emulated. Protected mode support is to be added later if needed.
>
> @@ -1778,6 +1778,69 @@ static int emulate_popa(struct x86_emulate_ctxt *ctxt,
>   	return rc;
>   }
>
> +static int emulate_iret_real(struct x86_emulate_ctxt *ctxt,
> +			     struct x86_emulate_ops *ops)
> +{
> +	struct decode_cache *c =&ctxt->decode;
> +	int rc = X86EMUL_CONTINUE;
> +	unsigned long temp_eip = 0;
> +	unsigned long temp_eflags = 0;
> +	unsigned long mask = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF | EFLG_TF |
> +			     EFLG_IF | EFLG_DF | EFLG_OF | EFLG_IOPL | EFLG_NT | EFLG_RF |
> +			     EFLG_AC | EFLG_ID | (1<<  1); /* Last one is the reserved bit */
> +	unsigned long vm86_mask = EFLG_VM | EFLG_VIF | EFLG_VIP;
> +
> +	/* TODO: Add stack limit check */
> +
> +	rc = emulate_pop(ctxt, ops,&temp_eip, c->op_bytes);
> +
> +	if (rc != X86EMUL_CONTINUE)
> +		return rc;
> +
> +	if (temp_eip&  ~0xffff) {
> +		emulate_gp(ctxt, 0);
> +		return X86EMUL_PROPAGATE_FAULT;
> +	}
> +
> +	c->eip = temp_eip;
> +
> +	rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_CS);
> +
> +	if (rc != X86EMUL_CONTINUE)
> +		return rc;
> +
> +	rc = emulate_pop(ctxt, ops,&temp_eflags, c->op_bytes);

If this pop fails, the emulate_pop_sreg() will already have been 
committed and the cs will be corrupted.  That's a preexisting problem, 
however, we can fix it later.

> +
> +	if (rc != X86EMUL_CONTINUE)
> +		return rc;
> +
> +	if (c->op_bytes == 4) {
> +		temp_eflags = ((temp_eflags&  mask) | (ctxt->eflags&  vm86_mask));
> +		ctxt->eflags = temp_eflags;

Can combine to a single assignment instead of two.

> +	} else if (c->op_bytes == 2) {
> +		temp_eflags&= ~0xfffd; /* Do not clear reserved bit 1 */
> +		ctxt->eflags |= temp_eflags;

If temp_eflags has CF clear, but ctxt->eflags has CF set, we end up with 
CF set, incorrectly.  Need to drop the low 16 bits from ctxt->eflags.

> +	}
> +

Need also to ensure reserved bits are set to their required values, 
something like

    ctxt->eflags &= ~EFLG_RESERVED_0_MASK;   (0xffc0802a)
    ctxt->eflags |= EFLG_RESERVERD_1_MASK; (0x2)


> +	return rc;
> +}
> +

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

[PATCH] x86 emulator: Add IRET instruction

[Mohammed Gamal]

Ths patch adds IRET instruction (opcode 0xcf).
Currently, only IRET in real mode is emulated. Protected mode support is to be added later if needed.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>

----
Changes from v2:
- Delay committing cs until all pop operation are ensured to be successful
- Corrected handling for eflags
---
 arch/x86/kvm/emulate.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b38bd8b..620f609 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -392,6 +392,9 @@ static u32 group2_table[] = {
 #define EFLG_PF (1<<2)
 #define EFLG_CF (1<<0)
 
+#define EFLG_RESERVED_ZEROS_MASK 0xffc0802a
+#define EFLG_RESERVED_ONE_MASK 2
+
 /*
  * Instruction emulation:
  * Most instructions are emulated directly via a fragment of inline assembly
@@ -1778,6 +1781,78 @@ static int emulate_popa(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
+static int emulate_iret_real(struct x86_emulate_ctxt *ctxt,
+			     struct x86_emulate_ops *ops)
+{
+	struct decode_cache *c = &ctxt->decode;
+	int rc = X86EMUL_CONTINUE;
+	unsigned long temp_eip = 0;
+	unsigned long temp_eflags = 0;
+	unsigned long cs = 0;
+	unsigned long mask = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF | EFLG_TF |
+			     EFLG_IF | EFLG_DF | EFLG_OF | EFLG_IOPL | EFLG_NT | EFLG_RF |
+			     EFLG_AC | EFLG_ID | (1 << 1); /* Last one is the reserved bit */
+	unsigned long vm86_mask = EFLG_VM | EFLG_VIF | EFLG_VIP;
+
+	/* TODO: Add stack limit check */
+
+	rc = emulate_pop(ctxt, ops, &temp_eip, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	if (temp_eip & ~0xffff) {
+		emulate_gp(ctxt, 0);
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
+	rc = emulate_pop(ctxt, ops, &cs, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	rc = emulate_pop(ctxt, ops, &temp_eflags, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	rc = load_segment_descriptor(ctxt, ops, (u16)cs, VCPU_SREG_CS);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	c->eip = temp_eip;
+
+	
+	if (c->op_bytes == 4)
+		ctxt->eflags = ((temp_eflags & mask) | (ctxt->eflags & vm86_mask));
+	else if (c->op_bytes == 2) {
+		ctxt->eflags &= ~0xffff;
+		ctxt->eflags |= temp_eflags;
+	}
+
+	ctxt->eflags &= ~EFLG_RESERVED_ZEROS_MASK; /* Clear reserved zeros */
+	ctxt->eflags |= EFLG_RESERVED_ONE_MASK;
+
+	return rc;
+}
+
+static inline int emulate_iret(struct x86_emulate_ctxt *ctxt,
+				    struct x86_emulate_ops* ops)
+{
+	switch(ctxt->mode) {
+	case X86EMUL_MODE_REAL:
+		return emulate_iret_real(ctxt, ops);
+	case X86EMUL_MODE_VM86:
+	case X86EMUL_MODE_PROT16:
+	case X86EMUL_MODE_PROT32:
+	case X86EMUL_MODE_PROT64:
+	default:
+		/* iret from protected mode unimplemented yet */
+		return X86EMUL_UNHANDLEABLE;		
+	}
+}
+
 static inline int emulate_grp1a(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {
@@ -2910,6 +2985,12 @@ special_insn:
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
+	case 0xcf:		/* iret */
+		rc = emulate_iret(ctxt, ops);
+
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+		break;
 	case 0xd0 ... 0xd1:	/* Grp2 */
 		c->src.val = 1;
 		emulate_grp2(ctxt);
-- 
1.7.0.4

Re: [PATCH] x86 emulator: Add IRET instruction

[Avi Kivity]

  On 07/28/2010 12:38 PM, Mohammed Gamal wrote:
> Ths patch adds IRET instruction (opcode 0xcf).
> Currently, only IRET in real mode is emulated. Protected mode support is to be added later if needed.

Looks good.

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

Re: [PATCH] x86 emulator: Add IRET instruction

[Paolo Bonzini]

On 07/28/2010 11:38 AM, Mohammed Gamal wrote:
> +	unsigned long mask = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF | EFLG_TF |
> +			     EFLG_IF | EFLG_DF | EFLG_OF | EFLG_IOPL | EFLG_NT | EFLG_RF |
> +			     EFLG_AC | EFLG_ID | (1 << 1); /* Last one is the reserved bit */
> +	unsigned long vm86_mask = EFLG_VM | EFLG_VIF | EFLG_VIP;
> ...
> +	if (c->op_bytes == 4)
> +		ctxt->eflags = ((temp_eflags & mask) | (ctxt->eflags & vm86_mask));
> +	else if (c->op_bytes == 2) {
> +		ctxt->eflags &= ~0xffff;
> +		ctxt->eflags |= temp_eflags;
> +	}

I think that's still not it.  You can set reserved bits for c->op_bytes 
== 2, and you can clear bit 1 for both 16- and 32-bit IRET.

IOW you need something like this:

     mask = ...; /* without (1 << 1); */
     ctxt_mask = (1 << 1) | EFLG_VM | EFLG_VIF | EFLG_VIP;

     if (c->op_bytes == 2) {
         mask &= 0xffff;
         ctxt_mask |= ~0xffff;
     }

     ctxt->eflags = (temp_eflags & mask) | (ctxt->eflags & ctxt_mask);

Paolo

Re: [PATCH] x86 emulator: Add IRET instruction

[Paolo Bonzini]

On 07/28/2010 03:30 PM, Paolo Bonzini wrote:
> On 07/28/2010 11:38 AM, Mohammed Gamal wrote:
>> + unsigned long mask = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF
>> | EFLG_TF |
>> + EFLG_IF | EFLG_DF | EFLG_OF | EFLG_IOPL | EFLG_NT | EFLG_RF |
>> + EFLG_AC | EFLG_ID | (1 << 1); /* Last one is the reserved bit */
>> + unsigned long vm86_mask = EFLG_VM | EFLG_VIF | EFLG_VIP;
>> ...
>> + if (c->op_bytes == 4)
>> + ctxt->eflags = ((temp_eflags & mask) | (ctxt->eflags & vm86_mask));
>> + else if (c->op_bytes == 2) {
>> + ctxt->eflags &= ~0xffff;
>> + ctxt->eflags |= temp_eflags;
>> + }
>
> I think that's still not it. You can set reserved bits for c->op_bytes
> == 2, and you can clear bit 1 for both 16- and 32-bit IRET.
>
> IOW you need something like this:
>
> mask = ...; /* without (1 << 1); */
> ctxt_mask = (1 << 1) | EFLG_VM | EFLG_VIF | EFLG_VIP;
>
> if (c->op_bytes == 2) {
> mask &= 0xffff;
> ctxt_mask |= ~0xffff;
> }
>
> ctxt->eflags = (temp_eflags & mask) | (ctxt->eflags & ctxt_mask);

Sorry, I replied to v3 of the patch while reviewing v2.

Looks good indeed.

Thanks!

Paolo

Re: [PATCH] x86 emulator: Add IRET instruction

[Marcelo Tosatti]

On Wed, Jul 28, 2010 at 12:38:40PM +0300, Mohammed Gamal wrote:
> Ths patch adds IRET instruction (opcode 0xcf).
> Currently, only IRET in real mode is emulated. Protected mode support is to be added later if needed.
> 
> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
> 
> ----
> Changes from v2:
> - Delay committing cs until all pop operation are ensured to be successful
> - Corrected handling for eflags
> ---
>  arch/x86/kvm/emulate.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 81 insertions(+), 0 deletions(-)

Applied, thanks.

[PATCH] x86 emulator: Add IRET instruction

[Mohammed Gamal]

Ths patch adds IRET instruction (opcode 0xcf).
Currently, only IRET in real mode is emulated. Protected mode support is to be added later if needed.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>

[Fixed a typo]
---
 arch/x86/kvm/emulate.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b38bd8b..4f18b9d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1778,6 +1778,61 @@ static int emulate_popa(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
+static int emulate_iret_real(struct x86_emulate_ctxt *ctxt,
+			     struct x86_emulate_ops *ops)
+{
+	struct decode_cache *c = &ctxt->decode;
+	int rc = X86EMUL_CONTINUE;
+	unsigned long temp_eip = 0;
+	unsigned long temp_eflags = 0;
+	/* TODO: Add stack limit check */
+
+	rc = emulate_pop(ctxt, ops, &temp_eip, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	if (temp_eip & ~0xffff) {
+		emulate_gp(ctxt, 0);
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
+	c->eip = temp_eip;
+
+	rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_CS);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	rc = emulate_pop(ctxt, ops, &temp_eflags, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	if (c->op_bytes == 4)
+		temp_eflags = ((temp_eflags & 0x257fd5) | (ctxt->eflags & 0x1a0000));
+	
+	ctxt->eflags = temp_eflags;
+
+	return rc;
+}
+
+static inline int emulate_iret(struct x86_emulate_ctxt *ctxt,
+				    struct x86_emulate_ops* ops)
+{
+	switch(ctxt->mode) {
+	case X86EMUL_MODE_REAL:
+		return emulate_iret_real(ctxt, ops);
+	case X86EMUL_MODE_VM86:
+	case X86EMUL_MODE_PROT16:
+	case X86EMUL_MODE_PROT32:
+	case X86EMUL_MODE_PROT64:
+	default:
+		/* iret from protected mode unimplemented yet */
+		return X86EMUL_UNHANDLEABLE;		
+	}
+}
+
 static inline int emulate_grp1a(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {
@@ -2910,6 +2965,12 @@ special_insn:
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
+	case 0xcf:		/* iret */
+		rc = emulate_iret(ctxt, ops);
+
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+		break;
 	case 0xd0 ... 0xd1:	/* Grp2 */
 		c->src.val = 1;
 		emulate_grp2(ctxt);
-- 
1.7.0.4

Re: [PATCH] x86 emulator: Add IRET instruction

[Paolo Bonzini]

On 07/25/2010 09:20 PM, Mohammed Gamal wrote:
> +	if (c->op_bytes == 4)
> +		temp_eflags = ((temp_eflags&  0x257fd5) | (ctxt->eflags&  0x1a0000));

Should this do also

if (c->op_bytes == 2)
     temp_eflags = ((temp_eflags & 0x7fd5) | (ctxt->eflags & ~0xffffL));

?

Or better, extract a new function computing the mask from emulate_popf, 
which would do something similar to what I wrote above.

Paolo

Re: [PATCH] x86 emulator: Add IRET instruction

[Mohammed Gamal]

On Mon, Jul 26, 2010 at 2:59 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 07/25/2010 09:20 PM, Mohammed Gamal wrote:
>>
>> +       if (c->op_bytes == 4)
>> +               temp_eflags = ((temp_eflags&  0x257fd5) | (ctxt->eflags&
>>  0x1a0000));
>
> Should this do also
>
> if (c->op_bytes == 2)
>    temp_eflags = ((temp_eflags & 0x7fd5) | (ctxt->eflags & ~0xffffL));
>
> ?

I don't think this is needed. The temp_eflags value is assigned
directly to eflags if we're operand size is 16 bits. At least that's
what the Intel manual says!

>
> Or better, extract a new function computing the mask from emulate_popf,
> which would do something similar to what I wrote above.
>
> Paolo
>

Re: [PATCH] x86 emulator: Add IRET instruction

[Wei Yongjun]

> On Mon, Jul 26, 2010 at 2:59 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>   
>> On 07/25/2010 09:20 PM, Mohammed Gamal wrote:
>>     
>>> +       if (c->op_bytes == 4)
>>> +               temp_eflags = ((temp_eflags&  0x257fd5) | (ctxt->eflags&
>>>  0x1a0000));
>>>       
>> Should this do also
>>
>> if (c->op_bytes == 2)
>>    temp_eflags = ((temp_eflags & 0x7fd5) | (ctxt->eflags & ~0xffffL));
>>
>> ?
>>     
> I don't think this is needed. The temp_eflags value is assigned
> directly to eflags if we're operand size is 16 bits. At least that's
> what the Intel manual says!
>   

Intel manual says：

EFLAGS[15:0] ← Pop();


>   
>> Or better, extract a new function computing the mask from emulate_popf,
>> which would do something similar to what I wrote above.
>>
>> Paolo
>>
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>   

Re: [PATCH] x86 emulator: Add IRET instruction

[Paolo Bonzini]

On 07/26/2010 02:07 AM, Mohammed Gamal wrote:
> On Mon, Jul 26, 2010 at 2:59 AM, Paolo Bonzini<pbonzini@redhat.com>  wrote:
>> On 07/25/2010 09:20 PM, Mohammed Gamal wrote:
>>>
>>> +       if (c->op_bytes == 4)
>>> +               temp_eflags = ((temp_eflags & 0x257fd5) | (ctxt->eflags&
>>>   0x1a0000));
>>
>> Should this do also
>>
>> if (c->op_bytes == 2)
>>     temp_eflags = ((temp_eflags & 0x7fd5) | (ctxt->eflags & ~0xffffL));
>>
>> ?
>
> I don't think this is needed. The temp_eflags value is assigned
> directly to eflags if we're operand size is 16 bits. At least that's
> what the Intel manual says!

That's fine, but please make sure that

   mov %sp, %bp
   orw $2, 4(%bp)
   iret

followed at return site by

   pushf
   popw %ax

does not set bit 1 in %ax.  That's the important point (also see how 
emulate_popf avoids magic hex constants).

Paolo

Re: [PATCH] x86 emulator: Add IRET instruction

[Avi Kivity]

  On 07/26/2010 11:47 AM, Paolo Bonzini wrote:
>> I don't think this is needed. The temp_eflags value is assigned
>> directly to eflags if we're operand size is 16 bits. At least that's
>> what the Intel manual says!
>
>
> That's fine, but please make sure that
>
> mov %sp, %bp
> orw $2, 4(%bp)
> iret
>
> followed at return site by
>
> pushf
> popw %ax
>
> does not set bit 1 in %ax. That's the important point (also see how 
> emulate_popf avoids magic hex constants).

Moreover, vmx will fail the next entry if this is not done. 23.3.1.4 says:

> RFLAGS.
> — Reserved bits 63:22 (bits 31:22 on processors that do not support 
> Intel 64
> architecture), bit 15, bit 5 and bit 3 must be 0 in the field, and 
> reserved bit 1
> must be 1.

Looks like a note is missing in the manual. I'll alert the authors.

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

Re: [PATCH] x86 emulator: Add IRET instruction

[Paolo Bonzini]

On 07/26/2010 11:00 AM, Avi Kivity wrote:
> On 07/26/2010 11:47 AM, Paolo Bonzini wrote:
>>> I don't think this is needed. The temp_eflags value is assigned
>>> directly to eflags if we're operand size is 16 bits. At least
>>> that's what the Intel manual says!
>>
>> That's fine, but please make sure that
>>
>> mov %sp, %bp
>>  orw $2, 4(%bp)
>>  iret
>>
>> followed at return site by
>>
>> pushf
>> popw %ax
>>
>> does not set bit 1 in %ax. That's the important point (also see how
>> emulate_popf avoids magic hex constants).
>
> Moreover, vmx will fail the next entry if this is not done. 23.3.1.4
> says:
>
>> RFLAGS. — Reserved bits 63:22 (bits 31:22 on processors that do not
>> support Intel 64 architecture), bit 15, bit 5 and bit 3 must be 0
>> in the field, and reserved bit 1 must be 1.

(I remembered one bit had to be 1, but failed to recall which one.  I 
should have looked up SAHF in the manual).  This means that my code 
actually should be

  mov %sp, %bp
  orw $8, 4(%bp)
  iret

followed by testing bit 3.

The emulate_popf approach that explicitly lists bits taken from the 
stack seems more robust.  For example, Mohammed's "if (c->op_bytes == 
4)" code leaves bit 1 cleared:

     temp_eflags = ((temp_eflags & 0x257fd5)
           | (ctxt->eflags & 0x1a0000));

(But then it is probably never used since only a 32-bit code segment in 
unreal mode would trigger it).

Paolo

Re: [PATCH] x86 emulator: Add IRET instruction

[Paolo Bonzini]

On 07/26/2010 11:00 AM, Avi Kivity wrote:
> On 07/26/2010 11:47 AM, Paolo Bonzini wrote:
>>> I don't think this is needed. The temp_eflags value is assigned
>>> directly to eflags if we're operand size is 16 bits. At least
>>> that's what the Intel manual says!
>>
>> That's fine, but please make sure that
>>
>> mov %sp, %bp
>>  orw $2, 4(%bp)
>>  iret
>>
>> followed at return site by
>>
>> pushf
>> popw %ax
>>
>> does not set bit 1 in %ax. That's the important point (also see how
>> emulate_popf avoids magic hex constants).
>
> Moreover, vmx will fail the next entry if this is not done. 23.3.1.4
> says:
>
>> RFLAGS. — Reserved bits 63:22 (bits 31:22 on processors that do not
>> support Intel 64 architecture), bit 15, bit 5 and bit 3 must be 0
>> in the field, and reserved bit 1 must be 1.

(I remembered one bit had to be 1, but failed to recall which one.  I 
should have looked up SAHF in the manual).  This means that my code 
actually should be

  mov %sp, %bp
  orw $8, 4(%bp)
  iret

followed by testing bit 3.

The emulate_popf approach that explicitly lists bits taken from the 
stack seems more robust.  For example, Mohammed's "if (c->op_bytes == 
4)" code leaves bit 1 cleared:

     temp_eflags = ((temp_eflags & 0x257fd5)
           | (ctxt->eflags & 0x1a0000));

(But then it is probably never used since only a 32-bit code segment in 
unreal mode would trigger it).

Paolo

Re: [PATCH] x86 emulator: Add IRET instruction

[Avi Kivity]

  On 07/25/2010 10:20 PM, Mohammed Gamal wrote:
> Ths patch adds IRET instruction (opcode 0xcf).
> Currently, only IRET in real mode is emulated. Protected mode support is to be added later if needed.
>

Please post a unit test with the next iteration.

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

[PATCH] x86 emulator: Add IRET instruction

[Mohammed Gamal]

Ths patch adds IRET instruction (opcode 0xcf).
Currently, only IRET in real mode is emulated. Protected mode support is to be added later if needed.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b38bd8b..ddc1da3 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1,5 +1,5 @@
 /******************************************************************************
- * emulate.c
+ i emulate.c
  *
  * Generic x86 (32-bit and 64-bit) instruction decoder and emulator.
  *
@@ -1778,6 +1778,61 @@ static int emulate_popa(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
+static int emulate_iret_real(struct x86_emulate_ctxt *ctxt,
+			     struct x86_emulate_ops *ops)
+{
+	struct decode_cache *c = &ctxt->decode;
+	int rc = X86EMUL_CONTINUE;
+	unsigned long temp_eip = 0;
+	unsigned long temp_eflags = 0;
+	/* TODO: Add stack limit check */
+
+	rc = emulate_pop(ctxt, ops, &temp_eip, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	if (temp_eip & ~0xffff) {
+		emulate_gp(ctxt, 0);
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
+	c->eip = temp_eip;
+
+	rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_CS);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	rc = emulate_pop(ctxt, ops, &temp_eflags, c->op_bytes);
+
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	if (c->op_bytes == 4)
+		temp_eflags = ((temp_eflags & 0x257fd5) | (ctxt->eflags & 0x1a0000));
+	
+	ctxt->eflags = temp_eflags;
+
+	return rc;
+}
+
+static inline int emulate_iret(struct x86_emulate_ctxt *ctxt,
+				    struct x86_emulate_ops* ops)
+{
+	switch(ctxt->mode) {
+	case X86EMUL_MODE_REAL:
+		return emulate_iret_real(ctxt, ops);
+	case X86EMUL_MODE_VM86:
+	case X86EMUL_MODE_PROT16:
+	case X86EMUL_MODE_PROT32:
+	case X86EMUL_MODE_PROT64:
+	default:
+		/* iret from protected mode unimplemented yet */
+		return X86EMUL_UNHANDLEABLE;		
+	}
+}
+
 static inline int emulate_grp1a(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {
@@ -2910,6 +2965,12 @@ special_insn:
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
+	case 0xcf:		/* iret */
+		rc = emulate_iret(ctxt, ops);
+
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+		break;
 	case 0xd0 ... 0xd1:	/* Grp2 */
 		c->src.val = 1;
 		emulate_grp2(ctxt);
-- 
1.7.0.4