From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH 3/5] KVM: Restrict rc values in x86_emulate_insn to
 X86EMUL_* values
Date: Fri, 29 Jan 2010 19:21:37 -0200
Message-ID: <20100129212137.GC18360@amt.cnet>
References: <20100128225114.7a28762c.yoshikawa.takuya@oss.ntt.co.jp>
 <20100128225929.a3915d88.yoshikawa.takuya@oss.ntt.co.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: avi@redhat.com, kvm@vger.kernel.org
To: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:14824 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752804Ab0A2VkR (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 29 Jan 2010 16:40:17 -0500
Content-Disposition: inline
In-Reply-To: <20100128225929.a3915d88.yoshikawa.takuya@oss.ntt.co.jp>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, Jan 28, 2010 at 10:59:29PM +0900, Takuya Yoshikawa wrote:
> This patch differentiate the X86EMUL_* values returned from
> X86EMUL_* type functions.
> 
> Note: During this work, we noticed some buggy return value
>   checks in x86_emulate_insn(). See FIXME in this patch.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
>  arch/x86/kvm/emulate.c |   73 +++++++++++++++++++++++++++++-------------------
>  1 files changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 9953f5b..d49e9de 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
>  
>  	/* Commit shadow register state. */
> @@ -2263,8 +2263,18 @@ twobyte_insn:
>  			if (c->modrm_mod != 3 || c->modrm_rm != 1)
>  				goto cannot_emulate;
>  
> -			rc = kvm_fix_hypercall(ctxt->vcpu);
> -			if (rc)
> +			/* FIXME:
> +			 * kvm_fix_hypercall() calls emulator_write_emulated()
> +			 * and if the return value is not X86EMUL_CONTINUE then
> +			 * returns -EFAULT, otherwise returns X86EMUL_CONTINUE.
> +			 *
> +			 * To handle the former case, original code just did
> +			 * goto done with rc = -EFAULT and passed the
> +			 * if (X86EMUL_UNHANDLEABLE) check.
> +			 * Instead of this, we just set rc to X86EMUL_CONTINUE.
> +			 */
> +			rc = X86EMUL_CONTINUE;
> +			if (kvm_fix_hypercall(ctxt->vcpu))
>  				goto done;

Should fix kvm_fix_hypercall to return X86EMUL_ codes, and send macro
updates separately from logic changes.