From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH] x86 emulator: Add IRET instruction
Date: Wed, 28 Jul 2010 07:20:45 +0300
Message-ID: <4C4FB01D.50503@redhat.com>
References: <1280272001-1415-1-git-send-email-m.gamal005@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org, mtosatti@redhat.com
To: Mohammed Gamal <m.gamal005@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:57605 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750736Ab0G1EUu (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 28 Jul 2010 00:20:50 -0400
In-Reply-To: <1280272001-1415-1-git-send-email-m.gamal005@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

  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.