Re: [Xen-devel] [PATCH v9 1/2] x86emul: New return code for unimplemented instruction

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.08.17 at 20:57, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2044,6 +2044,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned 
> long gla)

Way earlier in this file there are consumers of X86EMUL_UNHANDLEABLE
in hvmemul_do_io() and hvmemul_do_io_buffer(). I'm sure I did
point this out before, and I cannot see why you don't adjust those as
well, and you also don't say anything in this regard in the description.
Similarly there's a consumer in hvm_process_io_intercept() (in a file
you don't touch at all). The use in hvm_broadcast_ioreq() is likely
fine, but I'd still like you to reason about that in the description.

> @@ -5177,7 +5177,7 @@ x86_emulate(
>                  goto done;
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;

While I can see why you do this change, for many/all of the ones
I'll leave in context below I think you rather want to switch to
generate_exception(EXC_UD).

> @@ -6176,7 +6176,7 @@ x86_emulate(
>                  /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }
>      simd_0f_shift_imm:
>          generate_exception_if(ea.type != OP_REG, EXC_UD);
> @@ -6224,7 +6224,7 @@ x86_emulate(
>          case 6: /* psllq $imm8,mm */
>              goto simd_0f_shift_imm;
>          }
> -        goto cannot_emulate;
> +        goto unimplemented_insn;
>  
>      case X86EMUL_OPC_66(0x0f, 0x73):
>      case X86EMUL_OPC_VEX_66(0x0f, 0x73):
> @@ -6240,7 +6240,7 @@ x86_emulate(
>                  /* vpslldq $imm8,{x,y}mm,{x,y}mm */
>              goto simd_0f_shift_imm;
>          }
> -        goto cannot_emulate;
> +        goto unimplemented_insn;
>  
>      case X86EMUL_OPC(0x0f, 0x77):        /* emms */
>      case X86EMUL_OPC_VEX(0x0f, 0x77):    /* vzero{all,upper} */
> @@ -6304,7 +6304,7 @@ x86_emulate(
>          case 0: /* extrq $imm8,$imm8,xmm */
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }
>          /* fall through */
>      case X86EMUL_OPC_F2(0x0f, 0x78):     /* insertq $imm8,$imm8,xmm,xmm */
> @@ -6515,7 +6515,7 @@ x86_emulate(

A few lines up from here is another instance you appear to have
missed.

> @@ -7341,7 +7341,7 @@ x86_emulate(
>              host_and_vcpu_must_have(bmi1);
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }
>  
>          generate_exception_if(vex.l, EXC_UD);
> @@ -7650,7 +7650,7 @@ x86_emulate(
>              host_and_vcpu_must_have(tbm);
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }
>  
>      xop_09_rm_rv:
> @@ -7684,7 +7684,7 @@ x86_emulate(
>              host_and_vcpu_must_have(tbm);
>              goto xop_09_rm_rv;
>          }
> -        goto cannot_emulate;
> +        goto unimplemented_insn;
>  
>      case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */
>      {
> @@ -7716,6 +7716,9 @@ x86_emulate(
>      }
>  
>      default:
> +    unimplemented_insn:
> +        rc = X86EMUL_UNIMPLEMENTED;
> +        goto done;
>      cannot_emulate:
>          rc = X86EMUL_UNHANDLEABLE;
>          goto done;

I guess the cannot_emulate label would then better be moved
elsewhere (either out of the switch or to a place where one of
the goto-s to it currently sits).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel