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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Petre Pircalabu [mailto:ppircalabu@xxxxxxxxxxxxxxx]
> Sent: 21 September 2017 06:12
> To: xen-devel@xxxxxxxxxxxxx
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; jbeulich@xxxxxxxx; konrad.wilk@xxxxxxxxxx;
> sstabellini@xxxxxxxxxx; Tim (Xen.org) <tim@xxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; rcojocaru@xxxxxxxxxxxxxxx;
> tamas@xxxxxxxxxxxxx; jun.nakajima@xxxxxxxxx; Kevin Tian
> <kevin.tian@xxxxxxxxx>; Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
> Subject: [PATCH v12 1/4] x86emul: New return code for unimplemented
> instruction
> 
> Enforce the distinction between an instruction not implemented by the
> emulator and the failure to emulate that instruction by defining a new
> return code, X86EMUL_UNIMPLEMENTED.
> 
> This value should only be returned by the core emulator only if it fails to
> properly decode the current instruction's opcode, and not by any of other
> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
> 
> e.g. hvm_process_io_intercept should not return
> X86EMUL_UNIMPLEMENTED.
> The return value of this function depends on either the return code of
> one of the hvm_io_ops handlers (read/write) or the value returned by
> hvm_copy_guest_from_phys / hvm_copy_to_guest_phys.
> 
> Similary, none of this functions should return X86EMUL_UNIMPLEMENTED.
>  - hvm_io_intercept
>  - hvmemul_do_io
>  - hvm_send_buffered_ioreq
>  - hvm_send_ioreq
>  - hvm_broadcast_ioreq
>  - hvmemul_do_io_buffer
>  - hvmemul_validate
> 
> Signed-off-by: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
> 
> ---
> Changed since v10:
>     * Added asserts to make sure the return code cannot be
> X86EMUL_UNIMPLEMENTED.
>     * Add new return code (X86EMUL_UNRECOGNIZED) to be used when
> trying
>     to emulate an instruction with an invalid opcode.
> 
> Changed since v11:
>     * Fixed double negative in the patch description.
>     * Move assertion into the switch and use ASSERT_UNREACHABLE() when
>     applicable.
>     * Changed the description of X86EMUL_UNIMPLEMENTED /
> X86EMUL_UNRECOGNIZED
>     to reflect the differences between those 2 return codes.
>     * Changed the returned value to X86EMUL_UNRECOGNIZED in the
>     following cases:
>         X86EMUL_OPC(0x0f, 0x73): /* Group 14 */
>         X86EMUL_OPC_66(0x0f, 0x73):
>         X86EMUL_OPC_VEX_66(0x0f, 0x73):
>                 - All valid opcodes are defined, so it should return
>                 X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
> 
>         X86EMUL_OPC(0x0f, 0xc7) /* Group 9 */
>                 - For register type instructions all possible opcodes are
>                 defined, so it should return X86EMUL_UNRECOGNIZED if
>                 mod R/M bits are not matched.
> 
>         X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Group 17 */
>                 - All valid opcodes are defined, so it should return
>                 X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
> 
>         X86EMUL_OPC_XOP(09, 0x01): /* XOP Grp1 */
>         X86EMUL_OPC_XOP(09, 0x02): /* XOP Grp2 */
>                 - All valid opcodes are defined, so it should return
>                 X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
> 
>         X86EMUL_OPC(0x0f, 0x01): /* Grp7 */
>                 - Not all valid opcodes are defined so it should return
>                 X86EMUL_UNIMPLEMENTED if mod R/M bits are not matched.
>                 (e.g. XGETBV)
> 
>         X86EMUL_OPC_66(0x0f, 0x78):
>                 - All valid opcodes are defined, so it should return
>                 X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
> 
>         X86EMUL_OPC(0x0f, 0xae):
>         X86EMUL_OPC_66(0x0f, 0xae): /* Grp15 */
>                 - Not all valid opcodes are defined so it should return
>                 X86EMUL_UNIMPLEMENTED if mod R/M bits are not matched.
>                 (e.g. FXSAVE/FXRSTOR )
> ---
>  xen/arch/x86/hvm/emulate.c             | 12 ++++++++
>  xen/arch/x86/hvm/hvm.c                 |  1 +
>  xen/arch/x86/hvm/io.c                  |  1 +
>  xen/arch/x86/hvm/vmx/realmode.c        |  2 +-
>  xen/arch/x86/mm/shadow/multi.c         |  2 +-
>  xen/arch/x86/x86_emulate/x86_emulate.c | 52 ++++++++++++++++++++--
> ------------
>  xen/arch/x86/x86_emulate/x86_emulate.h | 13 +++++++++
>  7 files changed, 60 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index cc874ce..385fe1e 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -284,10 +284,15 @@ static int hvmemul_do_io(
>          }
>          break;
>      }
> +    case X86EMUL_UNIMPLEMENTED:
> +        ASSERT_UNREACHABLE();
> +        /* Fall-through */

Kind of surprised you need the fall-through if you assert the code is 
unreachable... but analysis tools can be a bit temperamental so ok.

>      default:
>          BUG();
>      }
> 
> +    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +

Isn't this assertion redundant given the ASSERT_UNREACHABLE() above?

  Paul

>      if ( rc != X86EMUL_OKAY )
>          return rc;
> 
> @@ -313,6 +318,9 @@ static int hvmemul_do_io_buffer(
> 
>      rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 0,
>                         (uintptr_t)buffer);
> +
> +    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +
>      if ( rc == X86EMUL_UNHANDLEABLE && dir == IOREQ_READ )
>          memset(buffer, 0xff, size);
> 
> @@ -405,6 +413,8 @@ static int hvmemul_do_io_addr(
>      rc = hvmemul_do_io(is_mmio, addr, &count, size, dir, df, 1,
>                         ram_gpa);
> 
> +    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +
>      if ( rc == X86EMUL_OKAY )
>          v->arch.hvm_vcpu.hvm_io.mmio_retry = (count < *reps);
> 
> @@ -2045,6 +2055,7 @@ int hvm_emulate_one_mmio(unsigned long mfn,
> unsigned long gla)
>      switch ( rc )
>      {
>      case X86EMUL_UNHANDLEABLE:
> +    case X86EMUL_UNIMPLEMENTED:
>          hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG",
> &ctxt);
>          break;
>      case X86EMUL_EXCEPTION:
> @@ -2102,6 +2113,7 @@ void hvm_emulate_one_vm_event(enum
> emul_kind kind, unsigned int trapnr,
>           * consistent with X86EMUL_RETRY.
>           */
>          return;
> +    case X86EMUL_UNIMPLEMENTED:
>      case X86EMUL_UNHANDLEABLE:
>          hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx);
>          hvm_inject_hw_exception(trapnr, errcode);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 93394c1..43ff5aa 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3723,6 +3723,7 @@ void hvm_ud_intercept(struct cpu_user_regs
> *regs)
>      switch ( hvm_emulate_one(&ctxt) )
>      {
>      case X86EMUL_UNHANDLEABLE:
> +    case X86EMUL_UNIMPLEMENTED:
>          hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
>          break;
>      case X86EMUL_EXCEPTION:
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index bf41954..984db21 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -96,6 +96,7 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t
> *validate, const char *descr)
>      switch ( rc )
>      {
>      case X86EMUL_UNHANDLEABLE:
> +    case X86EMUL_UNIMPLEMENTED:
>          hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
>          return false;
> 
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c
> b/xen/arch/x86/hvm/vmx/realmode.c
> index 12d43ad..cf48139 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -106,7 +106,7 @@ void vmx_realmode_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt)
>      if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry )
>          vio->io_completion = HVMIO_realmode_completion;
> 
> -    if ( rc == X86EMUL_UNHANDLEABLE )
> +    if ( rc == X86EMUL_UNHANDLEABLE || rc == X86EMUL_UNIMPLEMENTED
> )
>      {
>          gdprintk(XENLOG_ERR, "Failed to emulate insn.\n");
>          goto fail;
> diff --git a/xen/arch/x86/mm/shadow/multi.c
> b/xen/arch/x86/mm/shadow/multi.c
> index 8d4f244..2557e21 100644
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3488,7 +3488,7 @@ static int sh_page_fault(struct vcpu *v,
>       * would be a good unshadow hint. If we *do* decide to unshadow-on-
> fault
>       * then it must be 'failable': we cannot require the unshadow to succeed.
>       */
> -    if ( r == X86EMUL_UNHANDLEABLE )
> +    if ( r == X86EMUL_UNHANDLEABLE || r == X86EMUL_UNIMPLEMENTED )
>      {
>          perfc_incr(shadow_fault_emulate_failed);
>  #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index c1e2300..f334de9 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -848,7 +848,8 @@ do{ asm volatile (                                        
>               \
>                  stub.func);                                             \
>          generate_exception_if(res_.fields.trapnr == EXC_UD, EXC_UD);    \
>          domain_crash(current->domain);                                  \
> -        goto cannot_emulate;                                            \
> +        rc = X86EMUL_UNHANDLEABLE;                                      \
> +        goto done;                                                      \
>      }                                                                   \
>  } while (0)
>  #else
> @@ -2585,7 +2586,7 @@ x86_decode(
>                          d = twobyte_table[0x3a].desc;
>                          break;
>                      default:
> -                        rc = X86EMUL_UNHANDLEABLE;
> +                        rc = X86EMUL_UNRECOGNIZED;
>                          goto done;
>                      }
>                  }
> @@ -2599,7 +2600,7 @@ x86_decode(
>                  }
>                  else
>                  {
> -                    rc = X86EMUL_UNHANDLEABLE;
> +                    rc = X86EMUL_UNRECOGNIZED;
>                      goto done;
>                  }
> 
> @@ -2879,7 +2880,7 @@ x86_decode(
> 
>      default:
>          ASSERT_UNREACHABLE();
> -        return X86EMUL_UNHANDLEABLE;
> +        return X86EMUL_UNIMPLEMENTED;
>      }
> 
>      if ( ea.type == OP_MEM )
> @@ -4191,7 +4192,7 @@ x86_emulate(
>                  break;
>              case 4: /* fldenv - TODO */
>                  state->fpu_ctrl = true;
> -                goto cannot_emulate;
> +                goto unimplemented_insn;
>              case 5: /* fldcw m2byte */
>                  state->fpu_ctrl = true;
>                  if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val,
> @@ -4202,7 +4203,7 @@ x86_emulate(
>                  break;
>              case 6: /* fnstenv - TODO */
>                  state->fpu_ctrl = true;
> -                goto cannot_emulate;
> +                goto unimplemented_insn;
>              case 7: /* fnstcw m2byte */
>                  state->fpu_ctrl = true;
>                  emulate_fpu_insn_memdst("fnstcw", dst.val);
> @@ -4438,7 +4439,7 @@ x86_emulate(
>              case 4: /* frstor - TODO */
>              case 6: /* fnsave - TODO */
>                  state->fpu_ctrl = true;
> -                goto cannot_emulate;
> +                goto unimplemented_insn;
>              case 7: /* fnstsw m2byte */
>                  state->fpu_ctrl = true;
>                  emulate_fpu_insn_memdst("fnstsw", dst.val);
> @@ -5197,7 +5198,7 @@ x86_emulate(
>  #undef _GRP7
> 
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }
>          break;
>      }
> @@ -6195,7 +6196,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);
> @@ -6243,7 +6244,8 @@ x86_emulate(
>          case 6: /* psllq $imm8,mm */
>              goto simd_0f_shift_imm;
>          }
> -        goto cannot_emulate;
> +        rc = X86EMUL_UNRECOGNIZED;
> +        goto done;
> 
>      case X86EMUL_OPC_66(0x0f, 0x73):
>      case X86EMUL_OPC_VEX_66(0x0f, 0x73):
> @@ -6259,7 +6261,8 @@ x86_emulate(
>                  /* vpslldq $imm8,{x,y}mm,{x,y}mm */
>              goto simd_0f_shift_imm;
>          }
> -        goto cannot_emulate;
> +        rc = X86EMUL_UNRECOGNIZED;
> +        goto done;
> 
>      case X86EMUL_OPC(0x0f, 0x77):        /* emms */
>      case X86EMUL_OPC_VEX(0x0f, 0x77):    /* vzero{all,upper} */
> @@ -6323,7 +6326,8 @@ x86_emulate(
>          case 0: /* extrq $imm8,$imm8,xmm */
>              break;
>          default:
> -            goto cannot_emulate;
> +            rc = X86EMUL_UNRECOGNIZED;
> +            goto done;
>          }
>          /* fall through */
>      case X86EMUL_OPC_F2(0x0f, 0x78):     /* insertq $imm8,$imm8,xmm,xmm
> */
> @@ -6518,7 +6522,7 @@ x86_emulate(
>                  goto done;
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }
>          break;
> 
> @@ -6534,7 +6538,8 @@ x86_emulate(
>              vcpu_must_have(avx);
>              goto stmxcsr;
>          }
> -        goto cannot_emulate;
> +        rc = X86EMUL_UNRECOGNIZED;
> +        goto done;
> 
>      case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */
>          fail_if(modrm_mod != 3);
> @@ -6777,7 +6782,8 @@ x86_emulate(
>              switch ( modrm_reg & 7 )
>              {
>              default:
> -                goto cannot_emulate;
> +                rc = X86EMUL_UNRECOGNIZED;
> +                goto done;
> 
>  #ifdef HAVE_GAS_RDRAND
>              case 6: /* rdrand */
> @@ -7359,7 +7365,8 @@ x86_emulate(
>              host_and_vcpu_must_have(bmi1);
>              break;
>          default:
> -            goto cannot_emulate;
> +            rc = X86EMUL_UNRECOGNIZED;
> +            goto done;
>          }
> 
>          generate_exception_if(vex.l, EXC_UD);
> @@ -7670,7 +7677,8 @@ x86_emulate(
>              host_and_vcpu_must_have(tbm);
>              break;
>          default:
> -            goto cannot_emulate;
> +            rc = X86EMUL_UNRECOGNIZED;
> +            goto done;
>          }
> 
>      xop_09_rm_rv:
> @@ -7704,7 +7712,8 @@ x86_emulate(
>              host_and_vcpu_must_have(tbm);
>              goto xop_09_rm_rv;
>          }
> -        goto cannot_emulate;
> +        rc = X86EMUL_UNRECOGNIZED;
> +        goto done;
> 
>      case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */
>      {
> @@ -7736,8 +7745,8 @@ x86_emulate(
>      }
> 
>      default:
> -    cannot_emulate:
> -        rc = X86EMUL_UNHANDLEABLE;
> +    unimplemented_insn:
> +        rc = X86EMUL_UNIMPLEMENTED;
>          goto done;
>      }
> 
> @@ -7789,7 +7798,8 @@ x86_emulate(
>                  if ( (d & DstMask) != DstMem )
>                  {
>                      ASSERT_UNREACHABLE();
> -                    goto cannot_emulate;
> +                    rc = X86EMUL_UNHANDLEABLE;
> +                    goto done;
>                  }
>                  break;
>              }
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h
> b/xen/arch/x86/x86_emulate/x86_emulate.h
> index 4ddf111..1fb74c0 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -133,6 +133,19 @@ struct x86_emul_fpu_aux {
>    * Undefined behavior when used anywhere else.
>    */
>  #define X86EMUL_DONE           4
> + /*
> +  * Current instruction is not implemented by the emulator.
> +  * This value should only be returned by the core emulator when a valid
> +  * opcode is found but the execution logic for that instruction is missing.
> +  * It should NOT be returned by any of the x86_emulate_ops callbacks.
> +  */
> +#define X86EMUL_UNIMPLEMENTED  5
> + /*
> +  * The current instruction's opcode is not valid.
> +  * If this error code is returned by a function, an #UD trap should be
> +  * raised by the final consumer of it.
> + */
> +#define X86EMUL_UNRECOGNIZED   X86EMUL_UNIMPLEMENTED
> 
>  /* FPU sub-types which may be requested via ->get_fpu(). */
>  enum x86_emulate_fpu_type {
> --
> 2.7.4


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