Re: [PATCH] x86 emulator: Add IRET instruction

[Avi Kivity <avi@redhat.com>]

  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.