[PATCH v11 0/5] Notify monitor when emulating an unimplemented instruction

[PATCH v11 0/5] Notify monitor when emulating an unimplemented instruction

[Petre Pircalabu]

This patchset implements a mechanism which allows XEN to send first an event
if the emulator encountered an unsupported instruction.
The monitor application can choose to mitigate the error, for example to singlestep
the instruction using the real processor and then resume execution of the normal
instruction flow.

This feature was tested using a modified version of XTF:
https://github.com/petrepircalabu/xen-test-framework/tree/emul_unimpl

---
Changed since v1:
  * Removed the emulation kind check when calling hvm_inject_hw_exception

Changed since v2:
  * Removed a file added by mistake

Changed since v3:
  * Removed extra stray line
  * Added the _enabled suffix to the emul_unhandleable monitor option

Changed since v4
  * Fixed return expression of hvm_monitor_emul_unhandleable handle
  monitor_traps failures.
  * Removed stray parantheses.

Changed since v5:
  * Removed unnecessary "else" when calling hvm_monitor_emul_unhandleable.
  * Added extra line in arch_monitor_domctl_event.

Changed since v6:
  * add the distinction between unimplemented instructions and emulation failures.
  * changed "emul_unhandleable" event name to "emul_unimplemented"

Changed since v7:
  * Add "fall-through" comments to the switch statements (coverity)
  * Added X86EMUL_UNIMPLEMENTED to X86EMUL_UNHANDLEABLE checks the in functions
  referencing x86_emulate.
  * Improved comment describing X86EMUL_UNIMPLEMENTED.

Changed since v8:
  * Removed unnecessary "fall-through" comments.
  * Added check for X86EMUL_UNIMPLEMENTED in hvm_ud_intercept.
  * add a new label 'unimplemented_insn' to accomodate the existing jumps to
  'cannot_emulate' (e.g. invoke_stub)

Changed since v9:
  * Added detailed description in the patch comment regarding the usage (and lack of it) 
  of the new X86EMUL_UNIMPLEMENTED return code.
  * removed 'cannot_emulate' label.
  * added local vimrc files to the gitignore list.

Changed since v10:
  * Added asserts to make sure the return code cannot be X86EMUL_UNIMPLEMENTED.
  * Added new return code (X86EMUL_UNRECOGNIZED) to be used when trying
  to emulate an instruction with an invalid opcode.
  * Added emulation return code information to error messages.
  * Raise #UD when emulating an unimplemented instruction instead of just crash the domain

Petre Pircalabu (5):
  gitignore: add local vimrc files
  x86emul: New return code for unimplemented instruction
  x86emul: Add return code information to error messages
  x86/monitor: Notify monitor if an emulation fails.
  x86emul: Raise #UD when emulating an unimplemented instruction.

 .gitignore                             |  1 +
 tools/libxc/include/xenctrl.h          |  2 ++
 tools/libxc/xc_monitor.c               | 14 +++++++++++
 xen/arch/x86/hvm/emulate.c             | 29 +++++++++++++++++-----
 xen/arch/x86/hvm/hvm.c                 |  1 +
 xen/arch/x86/hvm/io.c                  |  7 +++++-
 xen/arch/x86/hvm/monitor.c             | 17 +++++++++++++
 xen/arch/x86/hvm/vmx/realmode.c        | 11 ++++++++-
 xen/arch/x86/mm/shadow/multi.c         |  6 ++---
 xen/arch/x86/monitor.c                 | 13 ++++++++++
 xen/arch/x86/x86_emulate/x86_emulate.c | 45 ++++++++++++++++++----------------
 xen/arch/x86/x86_emulate/x86_emulate.h | 12 +++++++++
 xen/include/asm-x86/domain.h           |  1 +
 xen/include/asm-x86/hvm/emulate.h      |  2 +-
 xen/include/asm-x86/hvm/monitor.h      |  1 +
 xen/include/asm-x86/monitor.h          |  3 ++-
 xen/include/public/domctl.h            |  1 +
 xen/include/public/vm_event.h          |  2 ++
 18 files changed, 134 insertions(+), 34 deletions(-)

-- 
2.7.4


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

[PATCH v11 1/5] gitignore: add local vimrc files

[Petre Pircalabu]

Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index ecb198f..cc16649 100644
--- a/.gitignore
+++ b/.gitignore
@@ -29,6 +29,7 @@ cscope.in.out
 cscope.out
 cscope.po.out
 .config
+.vimrc
 
 dist
 stubdom/*.tar.gz
-- 
2.7.4


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

[PATCH v11 2/5] x86emul: New return code for unimplemented instruction

[Petre Pircalabu]

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 not 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@bitdefender.com>

---
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.
---
 xen/arch/x86/hvm/emulate.c             | 11 +++++++++
 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 | 45 ++++++++++++++++++----------------
 xen/arch/x86/x86_emulate/x86_emulate.h | 12 +++++++++
 7 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 54811c1..bf12593 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -192,6 +192,8 @@ static int hvmemul_do_io(
     ASSERT(p.count <= *reps);
     *reps = vio->io_req.count = p.count;
 
+    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
+
     switch ( rc )
     {
     case X86EMUL_OKAY:
@@ -288,6 +290,8 @@ static int hvmemul_do_io(
         BUG();
     }
 
+    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
+
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -313,6 +317,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 +412,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 +2054,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 +2112,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 6cb903d..ea2812c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3695,6 +3695,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 11bde58..fdbbee2 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..ad97d93 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_UNIMPLEMENTED;
                         goto done;
                     }
                 }
@@ -2599,7 +2600,7 @@ x86_decode(
                 }
                 else
                 {
-                    rc = X86EMUL_UNHANDLEABLE;
+                    rc = X86EMUL_UNIMPLEMENTED;
                     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,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):
@@ -6259,7 +6260,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} */
@@ -6323,7 +6324,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 */
@@ -6518,7 +6519,8 @@ x86_emulate(
                 goto done;
             break;
         default:
-            goto cannot_emulate;
+            rc = X86EMUL_UNHANDLEABLE;
+            goto done;
         }
         break;
 
@@ -6534,7 +6536,7 @@ x86_emulate(
             vcpu_must_have(avx);
             goto stmxcsr;
         }
-        goto cannot_emulate;
+        goto unimplemented_insn;
 
     case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */
         fail_if(modrm_mod != 3);
@@ -6777,7 +6779,7 @@ x86_emulate(
             switch ( modrm_reg & 7 )
             {
             default:
-                goto cannot_emulate;
+                goto unimplemented_insn;
 
 #ifdef HAVE_GAS_RDRAND
             case 6: /* rdrand */
@@ -7359,7 +7361,7 @@ x86_emulate(
             host_and_vcpu_must_have(bmi1);
             break;
         default:
-            goto cannot_emulate;
+            goto unimplemented_insn;
         }
 
         generate_exception_if(vex.l, EXC_UD);
@@ -7670,7 +7672,7 @@ x86_emulate(
             host_and_vcpu_must_have(tbm);
             break;
         default:
-            goto cannot_emulate;
+            goto unimplemented_insn;
         }
 
     xop_09_rm_rv:
@@ -7704,7 +7706,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 */
     {
@@ -7736,8 +7738,8 @@ x86_emulate(
     }
 
     default:
-    cannot_emulate:
-        rc = X86EMUL_UNHANDLEABLE;
+    unimplemented_insn:
+        rc = X86EMUL_UNIMPLEMENTED;
         goto done;
     }
 
@@ -7789,7 +7791,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..2fc19e0 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -133,6 +133,18 @@ 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 if decode fails
+  * and not by any of the x86_emulate_ops callbacks.
+  * If this error code is returned by a function, an #UD trap should be
+  * raised by the final consumer of it.
+  */
+#define X86EMUL_UNIMPLEMENTED  5
+/*
+ * The current instruction's opcode is not valid.
+ */
+#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@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v11 3/5] x86emul: Add return code information to error messages

[Petre Pircalabu]

- print the return code of the last failed emulator operation
in hvm_dump_emulation_state.
- print the return code in sh_page_fault (SHADOW_PRINTK) to make the
distiction between X86EMUL_UNHANDLEABLE and X86EMUL_UNIMPLEMENTED.

Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
---
 xen/arch/x86/hvm/emulate.c        | 13 +++++++------
 xen/arch/x86/hvm/io.c             |  2 +-
 xen/arch/x86/hvm/vmx/realmode.c   |  2 +-
 xen/arch/x86/mm/shadow/multi.c    |  4 ++--
 xen/include/asm-x86/hvm/emulate.h |  2 +-
 5 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index bf12593..05b0453 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2055,7 +2055,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
     {
     case X86EMUL_UNHANDLEABLE:
     case X86EMUL_UNIMPLEMENTED:
-        hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt);
+        hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt, rc);
         break;
     case X86EMUL_EXCEPTION:
         hvm_inject_event(&ctxt.ctxt.event);
@@ -2114,7 +2114,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
         return;
     case X86EMUL_UNIMPLEMENTED:
     case X86EMUL_UNHANDLEABLE:
-        hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx);
+        hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx, rc);
         hvm_inject_hw_exception(trapnr, errcode);
         break;
     case X86EMUL_EXCEPTION:
@@ -2242,16 +2242,17 @@ static const char *guest_x86_mode_to_str(int mode)
 }
 
 void hvm_dump_emulation_state(const char *loglvl, const char *prefix,
-                              struct hvm_emulate_ctxt *hvmemul_ctxt)
+                              struct hvm_emulate_ctxt *hvmemul_ctxt, int rc)
 {
     struct vcpu *curr = current;
     const char *mode_str = guest_x86_mode_to_str(hvm_guest_x86_mode(curr));
     const struct segment_register *cs =
         hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
 
-    printk("%s%s emulation failed: %pv %s @ %04x:%08lx -> %*ph\n",
-           loglvl, prefix, curr, mode_str, cs->sel, hvmemul_ctxt->insn_buf_eip,
-           hvmemul_ctxt->insn_buf_bytes, hvmemul_ctxt->insn_buf);
+    printk("%s%s emulation failed (rc=%d): %pv %s @ %04x:%08lx -> %*ph\n",
+           loglvl, prefix, rc, curr, mode_str, cs->sel,
+           hvmemul_ctxt->insn_buf_eip, hvmemul_ctxt->insn_buf_bytes,
+           hvmemul_ctxt->insn_buf);
 }
 
 /*
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 984db21..7152c28 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -97,7 +97,7 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr)
     {
     case X86EMUL_UNHANDLEABLE:
     case X86EMUL_UNIMPLEMENTED:
-        hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
+        hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
         return false;
 
     case X86EMUL_EXCEPTION:
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index fdbbee2..c4815ed 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -138,7 +138,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
     return;
 
  fail:
-    hvm_dump_emulation_state(XENLOG_G_ERR, "Real-mode", hvmemul_ctxt);
+    hvm_dump_emulation_state(XENLOG_G_ERR, "Real-mode", hvmemul_ctxt, rc);
     domain_crash(curr->domain);
 }
 
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 2557e21..28030ac 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3498,8 +3498,8 @@ static int sh_page_fault(struct vcpu *v,
             v->arch.paging.last_write_emul_ok = 0;
         }
 #endif
-        SHADOW_PRINTK("emulator failure, unshadowing mfn %#lx\n",
-                       mfn_x(gmfn));
+        SHADOW_PRINTK("emulator failure (rc=%d), unshadowing mfn %#lx\n",
+                       r, mfn_x(gmfn));
         /* If this is actually a page table, then we have a bug, and need
          * to support more operations in the emulator.  More likely,
          * though, this is a hint that this page should not be shadowed. */
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index 8864775..58d17c4 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -92,7 +92,7 @@ int hvmemul_do_pio_buffer(uint16_t port,
                           void *buffer);
 
 void hvm_dump_emulation_state(const char *loglvl, const char *prefix,
-                              struct hvm_emulate_ctxt *hvmemul_ctxt);
+                              struct hvm_emulate_ctxt *hvmemul_ctxt, int rc);
 
 #endif /* __ASM_X86_HVM_EMULATE_H__ */
 
-- 
2.7.4


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

[PATCH v11 4/5] x86/monitor: Notify monitor if an emulation fails.

[Petre Pircalabu]

If case of a vm_event with the emulate_flags set, if the instruction
is not implemented by the emulator, the monitor should be notified instead
of directly injecting a hw exception.
This behavior can be used to re-execute an instruction not supported by
the emulator using the real processor (e.g. altp2m) instead of just
crashing.

Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 tools/libxc/include/xenctrl.h     |  2 ++
 tools/libxc/xc_monitor.c          | 14 ++++++++++++++
 xen/arch/x86/hvm/emulate.c        |  5 +++++
 xen/arch/x86/hvm/monitor.c        | 17 +++++++++++++++++
 xen/arch/x86/monitor.c            | 13 +++++++++++++
 xen/include/asm-x86/domain.h      |  1 +
 xen/include/asm-x86/hvm/monitor.h |  1 +
 xen/include/asm-x86/monitor.h     |  3 ++-
 xen/include/public/domctl.h       |  1 +
 xen/include/public/vm_event.h     |  2 ++
 10 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 43151cb..1a179d9 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2028,6 +2028,8 @@ int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
 int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
 int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
                                bool enable);
+int xc_monitor_emul_unimplemented(xc_interface *xch, domid_t domain_id,
+                                  bool enable);
 /**
  * This function enables / disables emulation for each REP for a
  * REP-compatible instruction.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index a677820..6046680 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -217,6 +217,20 @@ int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
     return do_domctl(xch, &domctl);
 }
 
+int xc_monitor_emul_unimplemented(xc_interface *xch, domid_t domain_id,
+                                  bool enable)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
+    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED;
+
+    return do_domctl(xch, &domctl);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 05b0453..2c62313 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -14,12 +14,14 @@
 #include <xen/sched.h>
 #include <xen/paging.h>
 #include <xen/trace.h>
+#include <xen/vm_event.h>
 #include <asm/event.h>
 #include <asm/i387.h>
 #include <asm/xstate.h>
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/ioreq.h>
+#include <asm/hvm/monitor.h>
 #include <asm/hvm/trace.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/svm/svm.h>
@@ -2113,6 +2115,9 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
          */
         return;
     case X86EMUL_UNIMPLEMENTED:
+        if ( hvm_monitor_emul_unimplemented() )
+            return;
+        /* fall-through */
     case X86EMUL_UNHANDLEABLE:
         hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx, rc);
         hvm_inject_hw_exception(trapnr, errcode);
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 2787dfa..4ce778c 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -57,6 +57,23 @@ bool hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old)
     return 0;
 }
 
+bool hvm_monitor_emul_unimplemented(void)
+{
+    struct vcpu *curr = current;
+
+    /*
+     * Send a vm_event to the monitor to signal that the current
+     * instruction couldn't be emulated.
+     */
+    vm_event_request_t req = {
+        .reason = VM_EVENT_REASON_EMUL_UNIMPLEMENTED,
+        .vcpu_id  = curr->vcpu_id,
+    };
+
+    return curr->domain->arch.monitor.emul_unimplemented_enabled &&
+        monitor_traps(curr, true, &req) == 1;
+}
+
 void hvm_monitor_msr(unsigned int msr, uint64_t value)
 {
     struct vcpu *curr = current;
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 706454f..e59f1f5 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -283,6 +283,19 @@ int arch_monitor_domctl_event(struct domain *d,
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED:
+    {
+        bool old_status = ad->monitor.emul_unimplemented_enabled;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        ad->monitor.emul_unimplemented_enabled = requested_status;
+        domain_unpause(d);
+        break;
+    }
+
     default:
         /*
          * Should not be reached unless arch_monitor_get_capabilities() is
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index fb8bf17..fcab8f8 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -406,6 +406,7 @@ struct arch_domain
         unsigned int cpuid_enabled                                         : 1;
         unsigned int descriptor_access_enabled                             : 1;
         unsigned int guest_request_userspace_enabled                       : 1;
+        unsigned int emul_unimplemented_enabled                            : 1;
         struct monitor_msr_bitmap *msr_bitmap;
         uint64_t write_ctrlreg_mask[4];
     } monitor;
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index cfd6661..6e22091 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -47,6 +47,7 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf,
                       unsigned int subleaf);
 void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
                            unsigned int err, uint64_t cr2);
+bool hvm_monitor_emul_unimplemented(void);
 
 #endif /* __ASM_X86_HVM_MONITOR_H__ */
 
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 765d0b4..0ada970 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -83,7 +83,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
                    (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
                    (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
                    (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT);
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED);
 
     /* Since we know this is on VMX, we can just call the hvm func */
     if ( hvm_is_singlestep_supported() )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 50ff58f..fce6557 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1094,6 +1094,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL       7
 #define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
 #define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS           9
+#define XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED    10
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index f01e471..b531f71 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -148,6 +148,8 @@
 #define VM_EVENT_REASON_INTERRUPT               12
 /* A descriptor table register was accessed. */
 #define VM_EVENT_REASON_DESCRIPTOR_ACCESS       13
+/* Current instruction is not implemented by the emulator */
+#define VM_EVENT_REASON_EMUL_UNIMPLEMENTED      14
 
 /* Supported values for the vm_event_write_ctrlreg index. */
 #define VM_EVENT_X86_CR0    0
-- 
2.7.4


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

[PATCH v11 5/5] x86emul: Raise #UD when emulating an unimplemented instruction.

[Petre Pircalabu]

Modified the behavior of hvm_emulate_one_insn and
vmx_realmode_emulate_one to generate an Invalid Opcode trap when
X86EMUL_UNIMPLEMENTED is returned by the emulator instead of just
crashing the domain.

Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
---
 xen/arch/x86/hvm/io.c           |  6 +++++-
 xen/arch/x86/hvm/vmx/realmode.c | 11 ++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 7152c28..a78ab05 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -96,10 +96,14 @@ 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, rc);
         return false;
 
+    case X86EMUL_UNIMPLEMENTED:
+        hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
+        hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+        break;
+
     case X86EMUL_EXCEPTION:
         hvm_inject_event(&ctxt.ctxt.event);
         break;
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index c4815ed..dfd5833 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -106,12 +106,21 @@ 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 || rc == X86EMUL_UNIMPLEMENTED )
+    if ( rc == X86EMUL_UNHANDLEABLE )
     {
         gdprintk(XENLOG_ERR, "Failed to emulate insn.\n");
         goto fail;
     }
 
+    if ( rc == X86EMUL_UNIMPLEMENTED )
+    {
+        gdprintk(XENLOG_ERR, "Unimplemented insn.\n");
+        if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE )
+            goto fail;
+
+        realmode_deliver_exception(TRAP_invalid_op, 0, hvmemul_ctxt);
+    }
+
     if ( rc == X86EMUL_EXCEPTION )
     {
         if ( unlikely(curr->domain->debugger_attached) &&
-- 
2.7.4


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

Re: [PATCH v11 1/5] gitignore: add local vimrc files

[Wei Liu]

On Tue, Sep 12, 2017 at 05:32:03PM +0300, Petre Pircalabu wrote:
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>

Applied this one patch.

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

Re: [PATCH v11 2/5] x86emul: New return code for unimplemented instruction

[Kent R. Spillner]

Minor nit in commit message:

On Tue, Sep 12, 2017 at 05:32:04PM +0300, Petre Pircalabu wrote:
> 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 not return X86EMUL_UNIMPLEMENTED.

"none of this... should not return..." is a double negative.

>  - 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@bitdefender.com>
> 
> ---
> 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.
> ---
>  xen/arch/x86/hvm/emulate.c             | 11 +++++++++
>  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 | 45 ++++++++++++++++++----------------
>  xen/arch/x86/x86_emulate/x86_emulate.h | 12 +++++++++
>  7 files changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 54811c1..bf12593 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -192,6 +192,8 @@ static int hvmemul_do_io(
>      ASSERT(p.count <= *reps);
>      *reps = vio->io_req.count = p.count;
>  
> +    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +
>      switch ( rc )
>      {
>      case X86EMUL_OKAY:
> @@ -288,6 +290,8 @@ static int hvmemul_do_io(
>          BUG();
>      }
>  
> +    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +
>      if ( rc != X86EMUL_OKAY )
>          return rc;
>  
> @@ -313,6 +317,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 +412,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 +2054,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 +2112,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 6cb903d..ea2812c 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3695,6 +3695,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 11bde58..fdbbee2 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..ad97d93 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_UNIMPLEMENTED;
>                          goto done;
>                      }
>                  }
> @@ -2599,7 +2600,7 @@ x86_decode(
>                  }
>                  else
>                  {
> -                    rc = X86EMUL_UNHANDLEABLE;
> +                    rc = X86EMUL_UNIMPLEMENTED;
>                      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,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):
> @@ -6259,7 +6260,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} */
> @@ -6323,7 +6324,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 */
> @@ -6518,7 +6519,8 @@ x86_emulate(
>                  goto done;
>              break;
>          default:
> -            goto cannot_emulate;
> +            rc = X86EMUL_UNHANDLEABLE;
> +            goto done;
>          }
>          break;
>  
> @@ -6534,7 +6536,7 @@ x86_emulate(
>              vcpu_must_have(avx);
>              goto stmxcsr;
>          }
> -        goto cannot_emulate;
> +        goto unimplemented_insn;
>  
>      case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */
>          fail_if(modrm_mod != 3);
> @@ -6777,7 +6779,7 @@ x86_emulate(
>              switch ( modrm_reg & 7 )
>              {
>              default:
> -                goto cannot_emulate;
> +                goto unimplemented_insn;
>  
>  #ifdef HAVE_GAS_RDRAND
>              case 6: /* rdrand */
> @@ -7359,7 +7361,7 @@ x86_emulate(
>              host_and_vcpu_must_have(bmi1);
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }
>  
>          generate_exception_if(vex.l, EXC_UD);
> @@ -7670,7 +7672,7 @@ x86_emulate(
>              host_and_vcpu_must_have(tbm);
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }
>  
>      xop_09_rm_rv:
> @@ -7704,7 +7706,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 */
>      {
> @@ -7736,8 +7738,8 @@ x86_emulate(
>      }
>  
>      default:
> -    cannot_emulate:
> -        rc = X86EMUL_UNHANDLEABLE;
> +    unimplemented_insn:
> +        rc = X86EMUL_UNIMPLEMENTED;
>          goto done;
>      }
>  
> @@ -7789,7 +7791,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..2fc19e0 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -133,6 +133,18 @@ 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 if decode fails
> +  * and not by any of the x86_emulate_ops callbacks.
> +  * If this error code is returned by a function, an #UD trap should be
> +  * raised by the final consumer of it.
> +  */
> +#define X86EMUL_UNIMPLEMENTED  5
> +/*
> + * The current instruction's opcode is not valid.
> + */
> +#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@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH v11 3/5] x86emul: Add return code information to error messages

[Tian, Kevin]

> From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]
> Sent: Tuesday, September 12, 2017 10:32 PM
> 
> - print the return code of the last failed emulator operation
> in hvm_dump_emulation_state.
> - print the return code in sh_page_fault (SHADOW_PRINTK) to make the
> distiction between X86EMUL_UNHANDLEABLE and
> X86EMUL_UNIMPLEMENTED.
> 
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v11 5/5] x86emul: Raise #UD when emulating an unimplemented instruction.

[Tian, Kevin]

> From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]
> Sent: Tuesday, September 12, 2017 10:32 PM
> 
> Modified the behavior of hvm_emulate_one_insn and
> vmx_realmode_emulate_one to generate an Invalid Opcode trap when
> X86EMUL_UNIMPLEMENTED is returned by the emulator instead of just
> crashing the domain.
> 
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v11 2/5] x86emul: New return code for unimplemented instruction

[Jan Beulich]

>>> On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote:
> 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 not return X86EMUL_UNIMPLEMENTED.

I think someone had already pointed out the strange double
negation here.

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -192,6 +192,8 @@ static int hvmemul_do_io(
>      ASSERT(p.count <= *reps);
>      *reps = vio->io_req.count = p.count;
>  
> +    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +
>      switch ( rc )
>      {
>      case X86EMUL_OKAY:

The assertion want to move into the switch(), making use
of ASSERT_UNREACHABLE().

> @@ -2045,6 +2054,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;

I would have preferred if, just like you do here, ...

> @@ -2102,6 +2112,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);

... you had added the new case label below existing ones uniformly.
But anyway.

> @@ -2585,7 +2586,7 @@ x86_decode(
>                          d = twobyte_table[0x3a].desc;
>                          break;
>                      default:
> -                        rc = X86EMUL_UNHANDLEABLE;
> +                        rc = X86EMUL_UNIMPLEMENTED;
>                          goto done;
>                      }
>                  }
> @@ -2599,7 +2600,7 @@ x86_decode(
>                  }
>                  else
>                  {
> -                    rc = X86EMUL_UNHANDLEABLE;
> +                    rc = X86EMUL_UNIMPLEMENTED;
>                      goto done;

At least these two should be "unrecognized" now.

> @@ -2879,7 +2880,7 @@ x86_decode(
>  
>      default:
>          ASSERT_UNREACHABLE();
> -        return X86EMUL_UNHANDLEABLE;
> +        return X86EMUL_UNIMPLEMENTED;
>      }

This one, otoh, is probably fine this way for now.

> @@ -6195,7 +6196,7 @@ x86_emulate(
>                  /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */
>              break;
>          default:
> -            goto cannot_emulate;
> +            goto unimplemented_insn;
>          }

This again wants to be "unrecognized".

> @@ -6243,7 +6244,7 @@ x86_emulate(
>          case 6: /* psllq $imm8,mm */
>              goto simd_0f_shift_imm;
>          }
> -        goto cannot_emulate;
> +        goto unimplemented_insn;

And this too. Together with previous discussion I think you should
now see the pattern for everything further down from here.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -133,6 +133,18 @@ 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 if decode fails

Why "if decode fails"? In that case it's more "unrecognized" than
"unimplemented"; the latter can only ever arise (long term, i.e.
once we have proper distinction of the two) if we successfully
decoded an insn, but have no code to actually handle it.

> +  * and not by any of the x86_emulate_ops callbacks.
> +  * If this error code is returned by a function, an #UD trap should be
> +  * raised by the final consumer of it.

This last sentence would now really belong to
X86EMUL_UNRECOGNIZED. As explained earlier, raising #UD
for unimplemented is precisely the wrong choice architecturally,
we merely tolerate doing so for the time being.

Jan

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

Re: [PATCH v11 3/5] x86emul: Add return code information to error messages

[Jan Beulich]

>>> On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2055,7 +2055,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>      {
>      case X86EMUL_UNHANDLEABLE:
>      case X86EMUL_UNIMPLEMENTED:
> -        hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt);
> +        hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt, rc);
>          break;

At the example of this one I think it is pretty clear that the order
of patches would be the other way around. But I won't insist.

> @@ -2242,16 +2242,17 @@ static const char *guest_x86_mode_to_str(int mode)
>  }
>  
>  void hvm_dump_emulation_state(const char *loglvl, const char *prefix,
> -                              struct hvm_emulate_ctxt *hvmemul_ctxt)
> +                              struct hvm_emulate_ctxt *hvmemul_ctxt, int rc)
>  {
>      struct vcpu *curr = current;
>      const char *mode_str = guest_x86_mode_to_str(hvm_guest_x86_mode(curr));
>      const struct segment_register *cs =
>          hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
>  
> -    printk("%s%s emulation failed: %pv %s @ %04x:%08lx -> %*ph\n",
> -           loglvl, prefix, curr, mode_str, cs->sel, hvmemul_ctxt->insn_buf_eip,
> -           hvmemul_ctxt->insn_buf_bytes, hvmemul_ctxt->insn_buf);
> +    printk("%s%s emulation failed (rc=%d): %pv %s @ %04x:%08lx -> %*ph\n",

Please try to keep log messages short (but without losing relevant
information). In the case here the "rc=" is unnecessary. With it
dropped
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v11 5/5] x86emul: Raise #UD when emulating an unimplemented instruction.

[Jan Beulich]

>>> On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote:
> Modified the behavior of hvm_emulate_one_insn and
> vmx_realmode_emulate_one to generate an Invalid Opcode trap when
> X86EMUL_UNIMPLEMENTED is returned by the emulator instead of just
> crashing the domain.

Along the lines of my comments on the earlier patch, I think you really
mean X86EMUL_UNRECOGNIZED here as well as in the changes you
make.

Jan


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

Re: [PATCH v11 3/5] x86emul: Add return code information to error messages

[Petre Ovidiu PIRCALABU]

On Ma, 2017-09-19 at 09:22 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote:
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -2055,7 +2055,7 @@ int hvm_emulate_one_mmio(unsigned long mfn,
> > unsigned long gla)
> >      {
> >      case X86EMUL_UNHANDLEABLE:
> >      case X86EMUL_UNIMPLEMENTED:
> > -        hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG",
> > &ctxt);
> > +        hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt,
> > rc);
> >          break;
> At the example of this one I think it is pretty clear that the order
> of patches would be the other way around. But I won't insist.
> 
> > 
> > @@ -2242,16 +2242,17 @@ static const char
> > *guest_x86_mode_to_str(int mode)
> >  }
> >  
> >  void hvm_dump_emulation_state(const char *loglvl, const char
> > *prefix,
> > -                              struct hvm_emulate_ctxt
> > *hvmemul_ctxt)
> > +                              struct hvm_emulate_ctxt
> > *hvmemul_ctxt, int rc)
> >  {
> >      struct vcpu *curr = current;
> >      const char *mode_str =
> > guest_x86_mode_to_str(hvm_guest_x86_mode(curr));
> >      const struct segment_register *cs =
> >          hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
> >  
> > -    printk("%s%s emulation failed: %pv %s @ %04x:%08lx -> %*ph\n",
> > -           loglvl, prefix, curr, mode_str, cs->sel, hvmemul_ctxt-
> > >insn_buf_eip,
> > -           hvmemul_ctxt->insn_buf_bytes, hvmemul_ctxt->insn_buf);
> > +    printk("%s%s emulation failed (rc=%d): %pv %s @ %04x:%08lx ->
> > %*ph\n",
> Please try to keep log messages short (but without losing relevant
> information). In the case here the "rc=" is unnecessary. With it
> dropped
I added the "rc=" to mark the distinction between "unimplemented" and
"unhandleable", as requested by Andrew Cooper for v10 

"Please modify hvm_dump_emulation_state to pass rc in, and distinguish
UNHANDLEABLE vs UNIMPLEMENTED in the printed message."


> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Jan
> 
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v11 3/5] x86emul: Add return code information to error messages

[Jan Beulich]

>>> On 20.09.17 at 14:54, <ppircalabu@bitdefender.com> wrote:
> On Ma, 2017-09-19 at 09:22 -0600, Jan Beulich wrote:
>> > > > On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote:
>> > @@ -2242,16 +2242,17 @@ static const char
>> > *guest_x86_mode_to_str(int mode)
>> >  }
>> >  
>> >  void hvm_dump_emulation_state(const char *loglvl, const char
>> > *prefix,
>> > -                              struct hvm_emulate_ctxt
>> > *hvmemul_ctxt)
>> > +                              struct hvm_emulate_ctxt
>> > *hvmemul_ctxt, int rc)
>> >  {
>> >      struct vcpu *curr = current;
>> >      const char *mode_str =
>> > guest_x86_mode_to_str(hvm_guest_x86_mode(curr));
>> >      const struct segment_register *cs =
>> >          hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
>> >  
>> > -    printk("%s%s emulation failed: %pv %s @ %04x:%08lx -> %*ph\n",
>> > -           loglvl, prefix, curr, mode_str, cs->sel, hvmemul_ctxt-
>> > >insn_buf_eip,
>> > -           hvmemul_ctxt->insn_buf_bytes, hvmemul_ctxt->insn_buf);
>> > +    printk("%s%s emulation failed (rc=%d): %pv %s @ %04x:%08lx ->
>> > %*ph\n",
>> Please try to keep log messages short (but without losing relevant
>> information). In the case here the "rc=" is unnecessary. With it
>> dropped
> I added the "rc=" to mark the distinction between "unimplemented" and
> "unhandleable", as requested by Andrew Cooper for v10 
> 
> "Please modify hvm_dump_emulation_state to pass rc in, and distinguish
> UNHANDLEABLE vs UNIMPLEMENTED in the printed message."

You don't need to print "rc=%d" to meet that requirement, just
"%d" will do.

Jan


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

Re: [PATCH v11 2/5] x86emul: New return code for unimplemented instruction

[Petre Ovidiu PIRCALABU]

On Ma, 2017-09-19 at 09:19 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote:
> > 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 not return
> > X86EMUL_UNIMPLEMENTED.
> I think someone had already pointed out the strange double
> negation here.
Will be fixed in the next patchset iteration.
>
> >
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -192,6 +192,8 @@ static int hvmemul_do_io(
> >      ASSERT(p.count <= *reps);
> >      *reps = vio->io_req.count = p.count;
> >
> > +    ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> > +
> >      switch ( rc )
> >      {
> >      case X86EMUL_OKAY:
> The assertion want to move into the switch(), making use
> of ASSERT_UNREACHABLE().
>
Will be fixed in the next patchset iteration.
> >
> > @@ -2045,6 +2054,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;
> I would have preferred if, just like you do here, ...
>
> >
> > @@ -2102,6 +2112,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);
> ... you had added the new case label below existing ones uniformly.
> But anyway.
The order is reversed in this case because the patch 4/5 from this
series adds the hvm_monitor_emul_unimplemented call and the "Fall-
through" in case of failure. If I change the default order here, I will
have to reverse it when adding the monitor notification handling.
>
> >
> > @@ -2585,7 +2586,7 @@ x86_decode(
> >                          d = twobyte_table[0x3a].desc;
> >                          break;
> >                      default:
> > -                        rc = X86EMUL_UNHANDLEABLE;
> > +                        rc = X86EMUL_UNIMPLEMENTED;
> >                          goto done;
> >                      }
> >                  }
> > @@ -2599,7 +2600,7 @@ x86_decode(
> >                  }
> >                  else
> >                  {
> > -                    rc = X86EMUL_UNHANDLEABLE;
> > +                    rc = X86EMUL_UNIMPLEMENTED;
> >                      goto done;
> At least these two should be "unrecognized" now.
Will be fixed in the next patchset iteration.
>
> >
> > @@ -2879,7 +2880,7 @@ x86_decode(
> >
> >      default:
> >          ASSERT_UNREACHABLE();
> > -        return X86EMUL_UNHANDLEABLE;
> > +        return X86EMUL_UNIMPLEMENTED;
> >      }
> This one, otoh, is probably fine this way for now.
>
> >
> > @@ -6195,7 +6196,7 @@ x86_emulate(
> >                  /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */
> >              break;
> >          default:
> > -            goto cannot_emulate;
> > +            goto unimplemented_insn;
> >          }
> This again wants to be "unrecognized".
In this case "unrecognized" cannot be returned (yet) as instructions
VPRORD and VPROLD are not implemented.
http://sandpile.org/x86/opc_grp.htm (group #13 (EVEX 66h) (0Fh,72h) )

>
> >
> > @@ -6243,7 +6244,7 @@ x86_emulate(
> >          case 6: /* psllq $imm8,mm */
> >              goto simd_0f_shift_imm;
> >          }
> > -        goto cannot_emulate;
> > +        goto unimplemented_insn;
> And this too. Together with previous discussion I think you should
> now see the pattern for everything further down from here.
Will be fixed in the next patchset iteration.
>
> >
> > --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> > @@ -133,6 +133,18 @@ 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 if
> > decode fails
> Why "if decode fails"? In that case it's more "unrecognized" than
> "unimplemented"; the latter can only ever arise (long term, i.e.
> once we have proper distinction of the two) if we successfully
> decoded an insn, but have no code to actually handle it.
>
> >
> > +  * and not by any of the x86_emulate_ops callbacks.
> > +  * If this error code is returned by a function, an #UD trap
> > should be
> > +  * raised by the final consumer of it.
> This last sentence would now really belong to
> X86EMUL_UNRECOGNIZED. As explained earlier, raising #UD
> for unimplemented is precisely the wrong choice architecturally,
> we merely tolerate doing so for the time being.
>
Will be fixed in the next patchset iteration.
> Jan
>
> ________________________
> This email was scanned by Bitdefender
Many thanks for your support,
//Petre

________________________
This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v11 2/5] x86emul: New return code for unimplemented instruction

[Jan Beulich]

>>> On 20.09.17 at 23:47, <ppircalabu@bitdefender.com> wrote:
> On Ma, 2017-09-19 at 09:19 -0600, Jan Beulich wrote:
>> > > > On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote:
>> > @@ -2102,6 +2112,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);
>> ... you had added the new case label below existing ones uniformly.
>> But anyway.
> The order is reversed in this case because the patch 4/5 from this
> series adds the hvm_monitor_emul_unimplemented call and the "Fall-
> through" in case of failure. If I change the default order here, I will
> have to reverse it when adding the monitor notification handling.

Ah, okay, that's fine then.

>> > @@ -6195,7 +6196,7 @@ x86_emulate(
>> >                  /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */
>> >              break;
>> >          default:
>> > -            goto cannot_emulate;
>> > +            goto unimplemented_insn;
>> >          }
>> This again wants to be "unrecognized".
> In this case "unrecognized" cannot be returned (yet) as instructions
> VPRORD and VPROLD are not implemented.
> http://sandpile.org/x86/opc_grp.htm (group #13 (EVEX 66h) (0Fh,72h) )

As you say yourself - these are EVEX-prefixed. We don't support
any AVX-512 instructions so far. As you're certainly aware even
the completion of AVX, AVX2, etc is pending review (and now
unlikely to make it into 4.10). Hence the "unimplemented" status
is supposed to be coming from a much earlier (decode stage)
place for these insns.

Jan


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