Re: [PATCH 3/5] KVM: Restrict rc values in x86_emulate_insn to X86EMUL_* values

[Marcelo Tosatti <mtosatti@redhat.com>]

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.