[PATCH v8 0/2] Singlestep unimplemented x86emul instructions

[PATCH v8 0/2] Singlestep unimplemented x86emul instructions

[Petre Pircalabu]

This patchset implements a new way of handling the instructions unimplemented
in x86emul. Instead of inserting a hardware exception the monitor will
be notified and it will to try to single-step the faulty instruction using the
real processor and then resume execution of the normal instruction flow.

---
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.


Petre Pircalabu (2):
  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             |  8 ++++++++
 xen/arch/x86/hvm/io.c                  |  2 ++
 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 |  8 ++++----
 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 ++
 15 files changed, 75 insertions(+), 7 deletions(-)

-- 
2.7.4


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

[PATCH v8 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>
---
 xen/arch/x86/hvm/emulate.c             | 4 ++++
 xen/arch/x86/hvm/io.c                  | 2 ++
 xen/arch/x86/hvm/vmx/realmode.c        | 2 +-
 xen/arch/x86/mm/shadow/multi.c         | 2 +-
 xen/arch/x86/x86_emulate/x86_emulate.c | 8 ++++----
 xen/arch/x86/x86_emulate/x86_emulate.h | 6 ++++++
 6 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 3a8db21..28133c0 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
     switch ( rc )
     {
     case X86EMUL_UNHANDLEABLE:
+        /* fall-through */
+    case X86EMUL_UNIMPLEMENTED:
         hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt);
         break;
     case X86EMUL_EXCEPTION:
@@ -2113,6 +2115,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/io.c b/xen/arch/x86/hvm/io.c
index 214ab30..af4e1dc 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -96,6 +96,8 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr)
     switch ( rc )
     {
     case X86EMUL_UNHANDLEABLE:
+        /* fall-through */
+    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 c9c2252..85fb165 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3486,7 +3486,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..480bad9 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 )
@@ -7717,7 +7717,7 @@ x86_emulate(
 
     default:
     cannot_emulate:
-        rc = X86EMUL_UNHANDLEABLE;
+        rc = X86EMUL_UNIMPLEMENTED;
         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 v8 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>
---
 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 c7710b8..abb9cac 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2027,6 +2027,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 b44ce93..8a76ec1 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -216,6 +216,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 28133c0..64a17f1 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>
@@ -2116,6 +2118,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 c10522b..091447d 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -405,6 +405,7 @@ struct arch_domain
         unsigned int debug_exception_sync        : 1;
         unsigned int cpuid_enabled               : 1;
         unsigned int descriptor_access_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 c5c323b..37255c9 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -77,7 +77,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 0669c31..474aed0 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1083,6 +1083,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 v8 2/2] x86/monitor: Notify monitor if an emulation fails.

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 546 bytes --]

On Tue, Aug 8, 2017 at 12:06 PM, Petre Pircalabu <ppircalabu@bitdefender.com
> wrote:

> 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>

[-- Attachment #1.2: Type: text/html, Size: 998 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails.

[Wei Liu]

On Tue, Aug 08, 2017 at 09:06:38PM +0300, Petre Pircalabu wrote:
> 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>
> ---
>  tools/libxc/include/xenctrl.h     |  2 ++
>  tools/libxc/xc_monitor.c          | 14 ++++++++++++++

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

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

[Paul Durrant]

> -----Original Message-----
> From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]
> Sent: 08 August 2017 19:07
> To: xen-devel@lists.xen.org
> Cc: Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; jbeulich@suse.com; konrad.wilk@oracle.com;
> sstabellini@kernel.org; Tim (Xen.org) <tim@xen.org>; Paul Durrant
> <Paul.Durrant@citrix.com>; rcojocaru@bitdefender.com;
> tamas@tklengyel.com; jun.nakajima@intel.com; Kevin Tian
> <kevin.tian@intel.com>; Petre Pircalabu <ppircalabu@bitdefender.com>
> Subject: [PATCH v8 1/2] x86emul: New return code for unimplemented
> instruction
> 
> Enforce the distinction between an instruction not implemented by the
> emulator and the failure to emulate that instruction by defining a new
> return code, X86EMUL_UNIMPLEMENTED.
> 
> This value should only be 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             | 4 ++++
>  xen/arch/x86/hvm/io.c                  | 2 ++
>  xen/arch/x86/hvm/vmx/realmode.c        | 2 +-
>  xen/arch/x86/mm/shadow/multi.c         | 2 +-
>  xen/arch/x86/x86_emulate/x86_emulate.c | 8 ++++----
>  xen/arch/x86/x86_emulate/x86_emulate.h | 6 ++++++
>  6 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 3a8db21..28133c0 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn,
> unsigned long gla)
>      switch ( rc )
>      {
>      case X86EMUL_UNHANDLEABLE:
> +        /* fall-through */
> +    case X86EMUL_UNIMPLEMENTED:
>          hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG",
> &ctxt);
>          break;
>      case X86EMUL_EXCEPTION:
> @@ -2113,6 +2115,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/io.c b/xen/arch/x86/hvm/io.c
> index 214ab30..af4e1dc 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -96,6 +96,8 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t
> *validate, const char *descr)
>      switch ( rc )
>      {
>      case X86EMUL_UNHANDLEABLE:
> +        /* fall-through */
> +    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 c9c2252..85fb165 100644
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3486,7 +3486,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..480bad9 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 )
> @@ -7717,7 +7717,7 @@ x86_emulate(
> 
>      default:
>      cannot_emulate:
> -        rc = X86EMUL_UNHANDLEABLE;
> +        rc = X86EMUL_UNIMPLEMENTED;
>          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

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

[Jan Beulich]

>>> On 08.08.17 at 20:06, <ppircalabu@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c

What about the use in a switch() statement in hvmemul_do_io()
in this file? And the use in hvmemul_do_io_buffer()?

> @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>      switch ( rc )
>      {
>      case X86EMUL_UNHANDLEABLE:
> +        /* fall-through */
> +    case X86EMUL_UNIMPLEMENTED:

The fall-through comment is pointless in such a case.

hvm/intercept.c has a use in hvm_process_io_intercept() which
looks like it needs dealing with too. And there are more. Any
places you perhaps leave alone intentionally should be reasoned
about in the description.

> @@ -7717,7 +7717,7 @@ x86_emulate(
>  
>      default:
>      cannot_emulate:
> -        rc = X86EMUL_UNHANDLEABLE;
> +        rc = X86EMUL_UNIMPLEMENTED;

There's at least one goto to the label here which can't stay as is
(in invoke_stub()). Did you really audit them all?

Jan


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

Re: [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails.

[Jan Beulich]

>>> On 08.08.17 at 20:06, <ppircalabu@bitdefender.com> wrote:
> 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>

For the parts it is applicable to
Acked-by: Jan Beulich <jbeulich@suse.com>



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

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

[Petre Ovidiu PIRCALABU]

[-- Attachment #1.1: Type: text/plain, Size: 2563 bytes --]

Hi Jan,


The main use-case for the new return code is to have a clear distinction between an instruction not implemented by the emulator (e.g. ?fldenv or fnstenv) and the failure to emulate .


- hvm_process_io_incercept returns X86EMUL_UNHANDLEABLE if one of the hvm_io_ops (read/write) failed or one of the hvm_copy_to(_from)_guest_phys returned an error code which is not handled in their correspondent switch statement. In either cases this is not caused by an unimplemented instruction.

- hvm_do_io / hvm_do_io_buffer call hvm_process_io_incercept which cannot return unimplemented.

- Thank-you very much for pointing out the invoke_stub issue. I have added a new label "unimplemented_insn" and updated the patch.


I will re-send a new patchset with the changes and a more detailed description of the places where the new return value was not handled.


Many thanks,

Petre


________________________________
From: Jan Beulich <JBeulich@suse.com>
Sent: Tuesday, August 22, 2017 11:09 AM
To: Petre Ovidiu PIRCALABU
Cc: rcojocaru@bitdefender.com; andrew.cooper3@citrix.com; paul.durrant@citrix.com; wei.liu2@citrix.com; George.Dunlap@eu.citrix.com; ian.jackson@eu.citrix.com; jun.nakajima@intel.com; kevin.tian@intel.com; sstabellini@kernel.org; xen-devel@lists.xen.org; konrad.wilk@oracle.com; tamas@tklengyel.com; tim@xen.org
Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplemented instruction

>>> On 08.08.17 at 20:06, <ppircalabu@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c

What about the use in a switch() statement in hvmemul_do_io()
in this file? And the use in hvmemul_do_io_buffer()?

> @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>      switch ( rc )
>      {
>      case X86EMUL_UNHANDLEABLE:
> +        /* fall-through */
> +    case X86EMUL_UNIMPLEMENTED:

The fall-through comment is pointless in such a case.

hvm/intercept.c has a use in hvm_process_io_intercept() which
looks like it needs dealing with too. And there are more. Any
places you perhaps leave alone intentionally should be reasoned
about in the description.

> @@ -7717,7 +7717,7 @@ x86_emulate(
>
>      default:
>      cannot_emulate:
> -        rc = X86EMUL_UNHANDLEABLE;
> +        rc = X86EMUL_UNIMPLEMENTED;

There's at least one goto to the label here which can't stay as is
(in invoke_stub()). Did you really audit them all?

Jan


________________________
This email was scanned by Bitdefender

[-- Attachment #1.2: Type: text/html, Size: 4073 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> On 30.08.17 at 19:06, <ppircalabu@bitdefender.com> wrote:

Please don't top-post. It makes it quite hard to see ...

> The main use-case for the new return code is to have a clear distinction 
> between an instruction not implemented by the emulator (e.g. ?fldenv or 
> fnstenv) and the failure to emulate .
> 
> 
> - hvm_process_io_incercept returns X86EMUL_UNHANDLEABLE if one of the 
> hvm_io_ops (read/write) failed or one of the hvm_copy_to(_from)_guest_phys 
> returned an error code which is not handled in their correspondent switch 
> statement. In either cases this is not caused by an unimplemented 
> instruction.
> 
> - hvm_do_io / hvm_do_io_buffer call hvm_process_io_incercept which cannot 
> return unimplemented.
> 
> - Thank-you very much for pointing out the invoke_stub issue. I have added a 
> new label "unimplemented_insn" and updated the patch.

... which of the replies above correspond to which of my earlier
replies.

Jan

> I will re-send a new patchset with the changes and a more detailed 
> description of the places where the new return value was not handled.
> 
> 
> Many thanks,
> 
> Petre
> 
> 
> ________________________________
> From: Jan Beulich <JBeulich@suse.com>
> Sent: Tuesday, August 22, 2017 11:09 AM
> To: Petre Ovidiu PIRCALABU
> Cc: rcojocaru@bitdefender.com; andrew.cooper3@citrix.com; 
> paul.durrant@citrix.com; wei.liu2@citrix.com; George.Dunlap@eu.citrix.com; 
> ian.jackson@eu.citrix.com; jun.nakajima@intel.com; kevin.tian@intel.com; 
> sstabellini@kernel.org; xen-devel@lists.xen.org; konrad.wilk@oracle.com; 
> tamas@tklengyel.com; tim@xen.org 
> Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplemented 
> instruction
> 
>>>> On 08.08.17 at 20:06, <ppircalabu@bitdefender.com> wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
> 
> What about the use in a switch() statement in hvmemul_do_io()
> in this file? And the use in hvmemul_do_io_buffer()?
> 
>> @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned 
> long gla)
>>      switch ( rc )
>>      {
>>      case X86EMUL_UNHANDLEABLE:
>> +        /* fall-through */
>> +    case X86EMUL_UNIMPLEMENTED:
> 
> The fall-through comment is pointless in such a case.
> 
> hvm/intercept.c has a use in hvm_process_io_intercept() which
> looks like it needs dealing with too. And there are more. Any
> places you perhaps leave alone intentionally should be reasoned
> about in the description.
> 
>> @@ -7717,7 +7717,7 @@ x86_emulate(
>>
>>      default:
>>      cannot_emulate:
>> -        rc = X86EMUL_UNHANDLEABLE;
>> +        rc = X86EMUL_UNIMPLEMENTED;
> 
> There's at least one goto to the label here which can't stay as is
> (in invoke_stub()). Did you really audit them all?
> 
> Jan
> 
> 
> ________________________
> This email was scanned by Bitdefender




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