[PATCH v9 0/2] Notify monitor when emulating an unimplemented instruction

[PATCH v9 0/2] 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)

---

Occurences of X86EMUL_UNHANDLEABLE which were not extended to take into account X86EMUL_UNIMPLEMENTED:

./xen/arch/x86/x86_emulate/x86_emulate.c:898:    rc = (p) ? X86EMUL_UNHANDLEABLE : X86EMUL_OKAY;     \
Used in the fail_if macro. This macro is used to check if certain conditions are met while trying to emulate the instruction (e.g. fail_if(!ops->read_msr); ).
This macro should not be modified as these conditions are not related to the instruction decoding and classification.

./xen/arch/x86/x86_emulate/x86_emulate.c:3429:        rc = X86EMUL_UNHANDLEABLE;
./xen/arch/x86/x86_emulate/x86_emulate.c:3433:            if ( rc != X86EMUL_UNHANDLEABLE )
./xen/arch/x86/x86_emulate/x86_emulate.c:3436:        if ( (nr_reps > 1 || rc == X86EMUL_UNHANDLEABLE) && ops->rep_ins )
./xen/arch/x86/x86_emulate/x86_emulate.c:3439:        if ( nr_reps >= 1 && rc == X86EMUL_UNHANDLEABLE )
./xen/arch/x86/x86_emulate/x86_emulate.c:3469:        rc = X86EMUL_UNHANDLEABLE;
x86_emulate: while emulating ins %dx,%es:%edi the return code is initialized to X86EMUL_UNHANDLEABLE and is used to hold / check the result of various emulation ops (x86_emulate_ops) (read_id, rep_ins).
Should not be changed to X86EMUL_UNIMPLEMENTED as it’s not related to instruction decoding.

./xen/arch/x86/x86_emulate/x86_emulate.c:3474:            if ( rc != X86EMUL_UNHANDLEABLE )
./xen/arch/x86/x86_emulate/x86_emulate.c:3477:        if ( (nr_reps > 1 || rc == X86EMUL_UNHANDLEABLE) && ops->rep_outs )
./xen/arch/x86/x86_emulate/x86_emulate.c:3480:        if ( nr_reps >= 1 && rc == X86EMUL_UNHANDLEABLE )
./xen/arch/x86/x86_emulate/x86_emulate.c:3756:                                  &nr_reps, ctxt)) == X86EMUL_UNHANDLEABLE) )
x86_emulate: while emulatings outs %esi,%dx the return code is initialized to X86EMUL_UNHANDLEABLE and is used to hold / check the result of various emulation ops (x86_emulate_ops) (rep_outs, write_io) and read_ulong.
Should not be changed to X86EMUL_UNIMPLEMENTED as it’s not related to instruction decoding.

./xen/arch/x86/x86_emulate/x86_emulate.c:3802:                                  &nr_reps, ctxt)) == X86EMUL_UNHANDLEABLE) )
x86_emulate: while emulating stos, if rep_stos returns X86EMUL_UNHANDLEABLE, the return value is reset to X86EMUL_OKAY. The emulation callbacks should not return X86EMUL_UNIMPLEMENTED as they are not used by the decoding logic of an instruction.
Should not be changed to X86EMUL_UNIMPLEMENTED as it’s not related to instruction decoding.

./xen/arch/x86/x86_emulate/x86_emulate.c:5082:                else if ( rc != X86EMUL_UNHANDLEABLE )
x86_emulate: while emulating clzero the return value of rep_stos is check against X86EMUL_UNHANDLEABLE. The emulation callbacks should not return X86EMUL_UNIMPLEMENTED as they are not used by the decoding logic of an instruction.
Should not be changed to X86EMUL_UNIMPLEMENTED as it’s not related to instruction decoding.

./xen/arch/x86/hvm/emulate.c:170:            return X86EMUL_UNHANDLEABLE;
./xen/arch/x86/hvm/emulate.c:175:        return X86EMUL_UNHANDLEABLE;
hvmemul_do_io: returns X86EMUL_UNHANDLEABLE if the io_req state is invalid.
Should not be changed to X86EMUL_UNIMPLEMENTED as it’s not related to instruction decoding.

./xen/arch/x86/hvm/emulate.c:202:    case X86EMUL_UNHANDLEABLE:
hvmemul_do_io:  The function checks against X86EMUL_UNHANDLEABLE the return of hvm_io_intercept (which in turn calls hvm_process_io_intercept).
hvm_process_io_intercept does not (and should not ever) return X86EMUL_UNIMPLEMENTED as it's just performs copies to/from the guest phys memory.
Should not be changed to X86EMUL_UNIMPLEMENTED as it's not applicable.

./xen/arch/x86/hvm/emulate.c:318:    if ( rc == X86EMUL_UNHANDLEABLE && dir == IOREQ_READ )
hvmemul_do_io_buffer: Checks against X86EMUL_UNHANDLEABLE the return value of hvmemul_do_io (which cannot return X86EMUL_UNIMPLEMENTED).
Should not be changed to X86EMUL_UNIMPLEMENTED as it's not applicable.

./xen/arch/x86/hvm/emulate.c:1152:           ? X86EMUL_OKAY : X86EMUL_UNHANDLEABLE;
hvmemul_validate: returns X86EMUL_UNHANDLEABLE if the context provided validate function returns false. 
Should not be changed to X86EMUL_UNIMPLEMENTED as the validate function only limits the instructions supported by the emulator to
a specific context. 

./xen/arch/x86/hvm/intercept.c:151:                    return X86EMUL_UNHANDLEABLE;
./xen/arch/x86/hvm/intercept.c:179:                    return X86EMUL_UNHANDLEABLE;
./xen/arch/x86/hvm/intercept.c:199:    else if ( rc == X86EMUL_UNHANDLEABLE )
hvm_process_io_intercept: returns X86EMUL_UNHANDLEABLE if the read/write (including hvm_copy_[from/to]_guest_phys) functions failed.
Should not be changed to X86EMUL_UNIMPLEMENTED as it’s not related to instruction decoding.

./xen/arch/x86/hvm/intercept.c:245:        return X86EMUL_UNHANDLEABLE;
hvm_io_intercept: returns X86EMUL_UNHANDLEABLE if hvm_io_handler is unavailable or hvm_process_io_intercept returns an error;
Should not be changed to X86EMUL_UNIMPLEMENTED as it’s not related to instruction decoding.

---
  x86emul: New return code for unimplemented instruction
  x86/monitor: Notify monitor if an emulation fails.

 tools/libxc/include/xenctrl.h          |  2 ++
 tools/libxc/xc_monitor.c               | 14 ++++++++++++++
 xen/arch/x86/hvm/emulate.c             |  7 +++++++
 xen/arch/x86/hvm/hvm.c                 |  1 +
 xen/arch/x86/hvm/io.c                  |  1 +
 xen/arch/x86/hvm/monitor.c             | 17 +++++++++++++++++
 xen/arch/x86/hvm/vmx/realmode.c        |  2 +-
 xen/arch/x86/mm/shadow/multi.c         |  2 +-
 xen/arch/x86/monitor.c                 | 13 +++++++++++++
 xen/arch/x86/x86_emulate/x86_emulate.c | 35 ++++++++++++++++++----------------
 xen/arch/x86/x86_emulate/x86_emulate.h |  6 ++++++
 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 ++
 16 files changed, 89 insertions(+), 19 deletions(-)

-- 
2.7.4


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

[PATCH v9 1/2] 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 used by the core emulator if it fails to decode
the current instruction, and not by any of the x86_emulate_ops
callbacks.

Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
 xen/arch/x86/hvm/emulate.c             |  3 +++
 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 | 35 ++++++++++++++++++----------------
 xen/arch/x86/x86_emulate/x86_emulate.h |  6 ++++++
 7 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 64454c7..f9f8c25 100644
--- 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)
     switch ( rc )
     {
     case X86EMUL_UNHANDLEABLE:
+    case X86EMUL_UNIMPLEMENTED:
         hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt);
         break;
     case X86EMUL_EXCEPTION:
@@ -2101,6 +2102,8 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
          * consistent with X86EMUL_RETRY.
          */
         return;
+    case X86EMUL_UNIMPLEMENTED:
+        /* fall-through */
     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 c5c0af8..15727a2 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 2201852..242b0af 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2577,7 +2577,7 @@ x86_decode(
                         d = twobyte_table[0x3a].desc;
                         break;
                     default:
-                        rc = X86EMUL_UNHANDLEABLE;
+                        rc = X86EMUL_UNIMPLEMENTED;
                         goto done;
                     }
                 }
@@ -2591,7 +2591,7 @@ x86_decode(
                 }
                 else
                 {
-                    rc = X86EMUL_UNHANDLEABLE;
+                    rc = X86EMUL_UNIMPLEMENTED;
                     goto done;
                 }
 
@@ -2871,7 +2871,7 @@ x86_decode(
 
     default:
         ASSERT_UNREACHABLE();
-        return X86EMUL_UNHANDLEABLE;
+        return X86EMUL_UNIMPLEMENTED;
     }
 
     if ( ea.type == OP_MEM )
@@ -4183,7 +4183,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,
@@ -4194,7 +4194,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);
@@ -4430,7 +4430,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);
@@ -5177,7 +5177,7 @@ x86_emulate(
                 goto done;
             break;
         default:
-            goto cannot_emulate;
+            goto unimplemented_insn;
         }
         break;
     }
@@ -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(
             vcpu_must_have(avx);
             goto stmxcsr;
         }
-        goto cannot_emulate;
+        goto unimplemented_insn;
 
     case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */
         fail_if(modrm_mod != 3);
@@ -6759,7 +6759,7 @@ x86_emulate(
             switch ( modrm_reg & 7 )
             {
             default:
-                goto cannot_emulate;
+                goto unimplemented_insn;
 
 #ifdef HAVE_GAS_RDRAND
             case 6: /* rdrand */
@@ -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;
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 4ddf111..82812ca 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -133,6 +133,12 @@ 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.
+  */
+#define X86EMUL_UNIMPLEMENTED  5
 
 /* 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 v9 2/2] 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        |  4 ++++
 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, 57 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 f9f8c25..c9066bb 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>
@@ -2103,6 +2105,8 @@ 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);
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index a7ccfc4..3551463 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -57,6 +57,23 @@ bool_t 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 d9efb35..0979adf 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 83c7c75..7ecbcde 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1087,6 +1087,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

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

[Jan Beulich]

>>> On 30.08.17 at 20:57, <ppircalabu@bitdefender.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Petre Ovidiu PIRCALABU]

On Vi, 2017-09-01 at 04:33 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 30.08.17 at 20:57, <ppircalabu@bitdefender.com> 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.

My mistake. I have added my comments in the cover letter as I thought
they will be easier to read. I will add them the the patch description
for the next iteration.

In my opinion, hvm_process_io_intercept cannot 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.
In case of the latter, the function crashes the domain and
returns X86EMUL_UNHANDLEABLE if the result is neither HVMCOPY_okay
nor HVMCOPY_bad_gfn_to_mfn.
hvm_op_ops should not return X86EMUL_UNIMPLEMENTED as this return
code's usage should strictly be limited to the core emulator and only
when an instruction opcode is invalid.

hvmemul_do_io - X86EMUL_UNHANDLEABLE is tested against the return code
of hvm_io_intercept which can return X86EMUL_UNHANDLEABLE only if the
IO handler was not found or it was returned by hvm_process_io_intercept
(cannot return X86EMUL_UNIMPLEMENTED).
In this case I think adding the X86EMUL_UNIMPLEMENTED check would mask
a possible misuse of this return code instead of just triggering BUG()
in the early stages of development.

hvm_send_buffered_ioreq - currently returns X86EMUL_UNHANDLEABLE if:
- the buffered iopage is NULL.
- the ioreq size if invalid
- the queue is full.
In none of these cases the function cannot return X86EMUL_UNIMPLEMENTED
because they are not related to the instruction opcode decoding, so
this function cannot return X86EMUL_UNIMPLEMENTED.

hvm_send_ioreq - can return X86EMUL_UNHANDLEABLE only if the current
vcpu in not the hvm_iorec_server's list or the value if returned from
hvm_send_buffered_ioreq, hence this function also cannot return
X86EMUL_UNIMPLEMENTED.

hvm_broadcast_ioreq - calls hvm_send_ioreq and chvemul_do_iohecks the
return code against X86EMUL_UNHANDLEABLE. There is no need to extend
the check to handle X86EMUL_UNIMPLEMENTED because this value cannot be
returned by hvm_send_ioreq.

hvmemul_do_io_buffer - calls hvmemul_do_io and, if the return code is
X86EMUL_UNHANDLEABLE and dir is IOREQ_READ, sets the return buffer to
0xFF.
The return value of hvemul_do_io can be:
- return value of hvm_io_intercept (cannot be X86EMUL_UNIMPLEMENTED),
- return value of hvm_process_io_intercept (cannot be
X86EMUL_UNIMPLEMENTED)
- return value of hvm_send_ioreq(cannot be X86EMUL_UNIMPLEMENTED)
- X86EMUL_RETRY / X86EMUL_OKAY (depending on state)
The condition also should not be extended to take into account
X86EMUL_UNIMPLEMENTED because this value cannot be returned by
hvemul_do_io.

> >
> > @@ -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).
Some of the opcodes are valid but not supported by the emulator. In
this case X86EMUL_UNIMPLEMENTED should be returned to allow the monitor
app to handle this case. Also, in the worst case scenario, when the
opcode doesn't correspond to a valid x86(-64) instruction, if the
monitor app for example tries to single-step it on the real hardware an
UD exception will also be reported.
>
> >
> > @@ -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(
> >      }
> >
Thanks for noticing it. I will change it back to cannot_emulate as
there are no other valid instructions for this opcodes.

> >      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).
I will change it in the new iteration of the patch.

> Jan
>
>
> ________________________
> This email was scanned by Bitdefender

Many thanks for your patience,
//Petre

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

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

[Jan Beulich]

>>> Petre Ovidiu PIRCALABU <ppircalabu@bitdefender.com> 09/04/17 7:20 PM >>>
>On Vi, 2017-09-01 at 04:33 -0600, Jan Beulich wrote:
>> >
>> > >
>> > > >
>> > > > On 30.08.17 at 20:57, <ppircalabu@bitdefender.com> 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.
>
>My mistake. I have added my comments in the cover letter as I thought
>they will be easier to read. I will add them the the patch description
>for the next iteration.

Thanks, that'll make them stay with the changes once committed. Your
further explanations are too long for a commit message though, imo. Aiui
they all boil down to the uses being in functions sitting behind rather than
in front of x86_emulate(). If that's the case, I would suggest you state just
that fact along with the enumeration of places that don't need touching for
that reason. This will make reviewing (and later viewing/understanding)
quite a bit easier.

>> > @@ -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).
>Some of the opcodes are valid but not supported by the emulator. In
>this case X86EMUL_UNIMPLEMENTED should be returned to allow the monitor
>app to handle this case. Also, in the worst case scenario, when the
>opcode doesn't correspond to a valid x86(-64) instruction, if the
>monitor app for example tries to single-step it on the real hardware an
>UD exception will also be reported.

Please be more precise with "some of the opcodes are valid". When I
looked through your change, I don't think I've seen any such case for the
places I meant the comment to apply to. Also, as far as the emulator
changes themselves go, please leave aside considerations of what a
monitor app may or may not do. These changes need to be consistent
all by themselves.

>> > @@ -7716,6 +7716,9 @@ x86_emulate(
>> >      }
>> >
>Thanks for noticing it. I will change it back to cannot_emulate as
>there are no other valid instructions for this opcodes.

I have trouble understanding this comment of yours, not the least
because I don't recall having asked (in particular around here) that
you switch anything back.

Jan


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

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

[Petre Ovidiu PIRCALABU]

On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote:
> > >
> > > >
> > > > @@ -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).
> > Some of the opcodes are valid but not supported by the emulator. In
> > this case X86EMUL_UNIMPLEMENTED should be returned to allow the
> > monitor
> > app to handle this case. Also, in the worst case scenario, when the
> > opcode doesn't correspond to a valid x86(-64) instruction, if the
> > monitor app for example tries to single-step it on the real
> > hardware an
> > UD exception will also be reported.
> Please be more precise with "some of the opcodes are valid". When I
> looked through your change, I don't think I've seen any such case for
> the
> places I meant the comment to apply to. Also, as far as the emulator
> changes themselves go, please leave aside considerations of what a
> monitor app may or may not do. These changes need to be consistent
> all by themselves.
>
Sorry about the poor wording. I was under the impression that you
required the invalid opcodes to be handled differently from the ones
which are valid but unimplemented (directly generate EXC_UD instead of
returning X86EMUL_UNIMPLEMENTED and let the caller handle it).

e.g. for  X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */
the mod R/M possible values are 1,2,3 (source: http://sandpile.org/x86/
opc_grp.htm).
From my perspective I wouldn't differentiate between those 2 cases and
return X86EMUL_UNIMPLEMENTED. The performance penalty is negligible if
the monitor is neither present nor it implements
XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED. The monitor can only offer
a "second-chance" re-execution of the faulty instruction and does not
affect the internal logic of x86_decode/x86_emulate.

> > >
> > > >
> > > > @@ -7716,6 +7716,9 @@ x86_emulate(
> > > >      }
> > > >
> > Thanks for noticing it. I will change it back to cannot_emulate as
> > there are no other valid instructions for this opcodes.
> I have trouble understanding this comment of yours, not the least
> because I don't recall having asked (in particular around here) that
> you switch anything back.
>
> 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 v9 1/2] x86emul: New return code for unimplemented instruction

[Jan Beulich]

>>> On 05.09.17 at 17:23, <ppircalabu@bitdefender.com> wrote:
> On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote:
>> > >
>> > > >
>> > > > @@ -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).
>> > Some of the opcodes are valid but not supported by the emulator. In
>> > this case X86EMUL_UNIMPLEMENTED should be returned to allow the
>> > monitor
>> > app to handle this case. Also, in the worst case scenario, when the
>> > opcode doesn't correspond to a valid x86(-64) instruction, if the
>> > monitor app for example tries to single-step it on the real
>> > hardware an
>> > UD exception will also be reported.
>> Please be more precise with "some of the opcodes are valid". When I
>> looked through your change, I don't think I've seen any such case for
>> the
>> places I meant the comment to apply to. Also, as far as the emulator
>> changes themselves go, please leave aside considerations of what a
>> monitor app may or may not do. These changes need to be consistent
>> all by themselves.
>>
> Sorry about the poor wording. I was under the impression that you
> required the invalid opcodes to be handled differently from the ones
> which are valid but unimplemented (directly generate EXC_UD instead of
> returning X86EMUL_UNIMPLEMENTED and let the caller handle it).

Yes, that's what I think would be best. I admit there is a potential
problem with this, when one or more of them become defined. But
that's something I'd like to also have Andrew's input on.

> e.g. for  X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */
> the mod R/M possible values are 1,2,3 (source: http://sandpile.org/x86/ 
> opc_grp.htm).
> From my perspective I wouldn't differentiate between those 2 cases and
> return X86EMUL_UNIMPLEMENTED.

I must still be missing something: What two cases are you talking
about? The three valid values have an implementation in the
emulator. All others are undefined, i.e. at present would ideally
produce #UD (see above).

Jan


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

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

[Petre Ovidiu PIRCALABU]

On Ma, 2017-09-05 at 09:46 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 05.09.17 at 17:23, <ppircalabu@bitdefender.com> wrote:
> > On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote:
> > >
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > @@ -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).
> > > > Some of the opcodes are valid but not supported by the
> > > > emulator. In
> > > > this case X86EMUL_UNIMPLEMENTED should be returned to allow the
> > > > monitor
> > > > app to handle this case. Also, in the worst case scenario, when
> > > > the
> > > > opcode doesn't correspond to a valid x86(-64) instruction, if
> > > > the
> > > > monitor app for example tries to single-step it on the real
> > > > hardware an
> > > > UD exception will also be reported.
> > > Please be more precise with "some of the opcodes are valid". When
> > > I
> > > looked through your change, I don't think I've seen any such case
> > > for
> > > the
> > > places I meant the comment to apply to. Also, as far as the
> > > emulator
> > > changes themselves go, please leave aside considerations of what
> > > a
> > > monitor app may or may not do. These changes need to be
> > > consistent
> > > all by themselves.
> > >
> > Sorry about the poor wording. I was under the impression that you
> > required the invalid opcodes to be handled differently from the
> > ones
> > which are valid but unimplemented (directly generate EXC_UD instead
> > of
> > returning X86EMUL_UNIMPLEMENTED and let the caller handle it).
> Yes, that's what I think would be best. I admit there is a potential
> problem with this, when one or more of them become defined. But
> that's something I'd like to also have Andrew's input on.
>
> >
> > e.g. for  X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */
> > the mod R/M possible values are 1,2,3 (source: http://sandpile.org/
> > x86/
> > opc_grp.htm).
> > From my perspective I wouldn't differentiate between those 2 cases
> > and
> > return X86EMUL_UNIMPLEMENTED.
> I must still be missing something: What two cases are you talking
> about? The three valid values have an implementation in the
> emulator. All others are undefined, i.e. at present would ideally
> produce #UD (see above).

Probably a better example it the handling of Grp7 (after applying
Andrew's patch).
if modrm is 0xd0 the instruction is xgetbv (valid but unimplemented)
if modrm value is corrupted (the encoding doesn't correspond to a valid
instruction it will also jump to emul_unimplemented.
>
> Jan
>
>
> ________________________
> This email was scanned by Bitdefender

Many thanks,
Petre

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

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

[Jan Beulich]

>>> On 05.09.17 at 18:20, <ppircalabu@bitdefender.com> wrote:
> On Ma, 2017-09-05 at 09:46 -0600, Jan Beulich wrote:
>> >
>> > >
>> > > >
>> > > > On 05.09.17 at 17:23, <ppircalabu@bitdefender.com> wrote:
>> > On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote:
>> > >
>> > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > > @@ -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).
>> > > > Some of the opcodes are valid but not supported by the
>> > > > emulator. In
>> > > > this case X86EMUL_UNIMPLEMENTED should be returned to allow the
>> > > > monitor
>> > > > app to handle this case. Also, in the worst case scenario, when
>> > > > the
>> > > > opcode doesn't correspond to a valid x86(-64) instruction, if
>> > > > the
>> > > > monitor app for example tries to single-step it on the real
>> > > > hardware an
>> > > > UD exception will also be reported.
>> > > Please be more precise with "some of the opcodes are valid". When
>> > > I
>> > > looked through your change, I don't think I've seen any such case
>> > > for
>> > > the
>> > > places I meant the comment to apply to. Also, as far as the
>> > > emulator
>> > > changes themselves go, please leave aside considerations of what
>> > > a
>> > > monitor app may or may not do. These changes need to be
>> > > consistent
>> > > all by themselves.
>> > >
>> > Sorry about the poor wording. I was under the impression that you
>> > required the invalid opcodes to be handled differently from the
>> > ones
>> > which are valid but unimplemented (directly generate EXC_UD instead
>> > of
>> > returning X86EMUL_UNIMPLEMENTED and let the caller handle it).
>> Yes, that's what I think would be best. I admit there is a potential
>> problem with this, when one or more of them become defined. But
>> that's something I'd like to also have Andrew's input on.
>>
>> >
>> > e.g. for  X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */
>> > the mod R/M possible values are 1,2,3 (source: http://sandpile.org/ 
>> > x86/
>> > opc_grp.htm).
>> > From my perspective I wouldn't differentiate between those 2 cases
>> > and
>> > return X86EMUL_UNIMPLEMENTED.
>> I must still be missing something: What two cases are you talking
>> about? The three valid values have an implementation in the
>> emulator. All others are undefined, i.e. at present would ideally
>> produce #UD (see above).
> 
> Probably a better example it the handling of Grp7 (after applying
> Andrew's patch).
> if modrm is 0xd0 the instruction is xgetbv (valid but unimplemented)
> if modrm value is corrupted (the encoding doesn't correspond to a valid
> instruction it will also jump to emul_unimplemented.

In cases when there is a mix of missing and undefined opcodes
I would view it as acceptable to go the unimplemented path
without further splitting. In cases where all unhandled cases
are also undefined, I'd prefer #UD to be raised.

Jan


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