[PATCH v10 0/3] Notify monitor when emulating an unimplemented instruction

[PATCH v10 0/3] 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.

Petre Pircalabu (3):
  gitignore: add local vimrc files
  x86emul: New return code for unimplemented instruction
  x86/monitor: Notify monitor if an emulation fails.

 .gitignore                             |  1 +
 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 | 45 ++++++++++++++++++----------------
 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 ++
 17 files changed, 95 insertions(+), 24 deletions(-)

-- 
2.7.4


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

[PATCH v10 1/3] 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 594ffd9..8af9c02 100644
--- a/.gitignore
+++ b/.gitignore
@@ -27,6 +27,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

Re: [PATCH v10 1/3] gitignore: add local vimrc files

[Wei Liu]

OOI how does this work?

You put a .vimrc under xen.git?

On Wed, Sep 06, 2017 at 04:48:24PM +0300, Petre Pircalabu wrote:
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> ---
>  .gitignore | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/.gitignore b/.gitignore
> index 594ffd9..8af9c02 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -27,6 +27,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

Re: [PATCH v10 1/3] gitignore: add local vimrc files

[Petre Ovidiu PIRCALABU]

On Jo, 2017-09-07 at 15:59 +0100, Wei Liu wrote:
> OOI how does this work?
>
> You put a .vimrc under xen.git?
I haven't added the file to the repository, just to .gitignore in order
to mask it from git. It will help me very much to have it upstream
because right now I have to cherry-pick it each time I create a local
branch.
I'm using neovim and 'MarcWeber/vim-addon-local-vimrc'. My local .vimrc
is quite simple, just sets the alignment, tabs and tabspace according
to the xen coding standard.

//Petre
>
> On Wed, Sep 06, 2017 at 04:48:24PM +0300, Petre Pircalabu wrote:
> >
> > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> > ---
> >  .gitignore | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/.gitignore b/.gitignore
> > index 594ffd9..8af9c02 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -27,6 +27,7 @@ cscope.in.out
> >  cscope.out
> >  cscope.po.out
> >  .config
> > +.vimrc
> >
> >  dist
> >  stubdom/*.tar.gz

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

Re: [PATCH v10 1/3] gitignore: add local vimrc files

[Wei Liu]

On Thu, Sep 07, 2017 at 03:36:00PM +0000, Petre Ovidiu PIRCALABU wrote:
> On Jo, 2017-09-07 at 15:59 +0100, Wei Liu wrote:
> > OOI how does this work?
> >
> > You put a .vimrc under xen.git?
> I haven't added the file to the repository, just to .gitignore in order
> to mask it from git. It will help me very much to have it upstream
> because right now I have to cherry-pick it each time I create a local
> branch.
> I'm using neovim and 'MarcWeber/vim-addon-local-vimrc'. My local .vimrc
> is quite simple, just sets the alignment, tabs and tabspace according
> to the xen coding standard.
> 

Thanks for the pointer.

I have no objection to this patch. Adding a new entry in .gitignore
which makes developers' life easier is a clear win to me.

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

> //Petre
> >
> > On Wed, Sep 06, 2017 at 04:48:24PM +0300, Petre Pircalabu wrote:
> > >
> > > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> > > ---
> > >  .gitignore | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/.gitignore b/.gitignore
> > > index 594ffd9..8af9c02 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -27,6 +27,7 @@ cscope.in.out
> > >  cscope.out
> > >  cscope.po.out
> > >  .config
> > > +.vimrc
> > >
> > >  dist
> > >  stubdom/*.tar.gz
> 
> ________________________
> This email was scanned by Bitdefender
> _______________________________________________
> 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 v10 1/3] gitignore: add local vimrc files

[Ian Jackson]

Petre Ovidiu PIRCALABU writes ("Re: [PATCH v10 1/3] gitignore: add local vimrc files"):
> On Jo, 2017-09-07 at 15:59 +0100, Wei Liu wrote:
> > OOI how does this work?
...
> I haven't added the file to the repository, just to .gitignore in order
> to mask it from git. It will help me very much to have it upstream
> because right now I have to cherry-pick it each time I create a local
> branch.
> I'm using neovim and 'MarcWeber/vim-addon-local-vimrc'. My local .vimrc
> is quite simple, just sets the alignment, tabs and tabspace according
> to the xen coding standard.

Why don't you put the .vimrc in your .. ?

Ian.

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

Re: [PATCH v10 1/3] gitignore: add local vimrc files

[Petre Ovidiu PIRCALABU]

On Jo, 2017-09-07 at 17:12 +0100, Ian Jackson wrote:
> Petre Ovidiu PIRCALABU writes ("Re: [PATCH v10 1/3] gitignore: add
> local vimrc files"):
> > 
> > On Jo, 2017-09-07 at 15:59 +0100, Wei Liu wrote:
> > > 
> > > OOI how does this work?
> ...
> > 
> > I haven't added the file to the repository, just to .gitignore in
> > order
> > to mask it from git. It will help me very much to have it upstream
> > because right now I have to cherry-pick it each time I create a
> > local
> > branch.
> > I'm using neovim and 'MarcWeber/vim-addon-local-vimrc'. My local
> > .vimrc
> > is quite simple, just sets the alignment, tabs and tabspace
> > according
> > to the xen coding standard.
> Why don't you put the .vimrc in your .. ?
> 
> Ian.
> 
My current directory layout is a "work" directory which contains
various projects (with different coding standards). Using "per-
directory" settings in my global .vimrc is possible but personally I
prefer using a local .vimrc which is easier to manage & migrate.

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

[PATCH v10 2/3] 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_incercept 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.
 - 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>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
 xen/arch/x86/hvm/emulate.c             |  2 ++
 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 |  6 +++++
 7 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 64454c7..8a6eb74 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,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 f7efe66..90cfa40 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..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 v10 2/3] x86emul: New return code for unimplemented instruction

[Jan Beulich]

>>> On 06.09.17 at 15:48, <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_incercept 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.
>  - hvmemul_do_io
>  - hvm_send_buffered_ioreq
>  - hvm_send_ioreq
>  - hvm_broadcast_ioreq
>  - hvmemul_do_io_buffer
>  - hvmemul_validate

Granted hvm_io_intercept() is only a relatively thin wrapper
around hvm_process_io_incercept(), but for completeness it
would have been nice to be included here too. Additionally it
would have helped reviewing as well as future development if you
had added ASSERT()s to this effect to all consumers where you
don't add a check for X86EMUL_UNIMPLEMENTED.

> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

The code in v10 changed from v9 or whichever earlier version
Paul had give his R-b on. You should have removed it for that
reason or, if he had given it for a limited range of the patch
which did not change, you should have indicated which range
that was.

Also somewhere here I'm missing a summary of the changes
from v9. Putting it in the overview mail is nice, but for
reviewing purposes it is far more useful to be right in the
affected patch.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c

After discussing with Andrew I'm willing to agree with the changes
you do here, with one extra requirement: At least on non-debug
builds X86EMUL_UNIMPLEMENTED should always result in #UD being
raised by the final consumer of it. It should never, like would be
the case with the changes you do to vmx/realmode.c, result in
the domain being crashed. Please change that one and check
carefully whether there are any other similar cases.

Jan


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

Re: [PATCH v10 2/3] x86emul: New return code for unimplemented instruction

[Jan Beulich]

>>> On 07.09.17 at 16:26, <JBeulich@suse.com> wrote:
> After discussing with Andrew I'm willing to agree with the changes
> you do here, with one extra requirement: At least on non-debug
> builds X86EMUL_UNIMPLEMENTED should always result in #UD being
> raised by the final consumer of it. It should never, like would be
> the case with the changes you do to vmx/realmode.c, result in
> the domain being crashed. Please change that one and check
> carefully whether there are any other similar cases.

Oh, and please make the comment next to the definition of
X86EMUL_UNIMPLEMENTED also say so.

Jan


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

Re: [PATCH v10 2/3] x86emul: New return code for unimplemented instruction

[Petre Ovidiu PIRCALABU]

On Jo, 2017-09-07 at 09:08 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 07.09.17 at 16:26, <JBeulich@suse.com> wrote:
> > After discussing with Andrew I'm willing to agree with the changes
> > you do here, with one extra requirement: At least on non-debug
> > builds X86EMUL_UNIMPLEMENTED should always result in #UD being
> > raised by the final consumer of it. It should never, like would be
> > the case with the changes you do to vmx/realmode.c, result in
> > the domain being crashed. Please change that one and check
> > carefully whether there are any other similar cases.

Hi Jan,

Changing the way we handle X86EMUL_UNIMPLEMENTED in some of the
functions will modify the existing behavior, and I'm a little bit wary
of making so many changes unrelated to the current patchset'a purpose
without a thourough way of testing them.

e.g.: "hvm_descriptor_access_intercept".
The current behavior is to return false if X86EMUL_UNHANDLEABLE is
returned by hvm_emulate_one. Up until now, this return code covered
also the "unimplemented instruction" case.
If X86EMUL_UNIMPLEMENTED will be handled separately (e.g. by calling
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC),
hvm_emulate_writeback, and finally returning true) some of the
scenarios where the domain got crashed will result only in an UD being
injected.

The same reasoning applies also to vmx/realmode. My patch didn't change
the current behavior, the domain crash logic was added by patch
3502a233e0132cb2facfe90c5c4872c823a5cb69.

However, in the end the decision if yours to take. I can add those
changes, but I will require a little help in order to make sure I don't
break anything.

Many thanks,
Petre

> Oh, and please make the comment next to the definition of
> X86EMUL_UNIMPLEMENTED also say so.
> 
> Jan
> 
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v10 2/3] x86emul: New return code for unimplemented instruction

[Jan Beulich]

>>> On 11.09.17 at 17:52, <ppircalabu@bitdefender.com> wrote:
> On Jo, 2017-09-07 at 09:08 -0600, Jan Beulich wrote:
>> > 
>> > > 
>> > > > 
>> > > > On 07.09.17 at 16:26, <JBeulich@suse.com> wrote:
>> > After discussing with Andrew I'm willing to agree with the changes
>> > you do here, with one extra requirement: At least on non-debug
>> > builds X86EMUL_UNIMPLEMENTED should always result in #UD being
>> > raised by the final consumer of it. It should never, like would be
>> > the case with the changes you do to vmx/realmode.c, result in
>> > the domain being crashed. Please change that one and check
>> > carefully whether there are any other similar cases.
> 
> Changing the way we handle X86EMUL_UNIMPLEMENTED in some of the
> functions will modify the existing behavior, and I'm a little bit wary
> of making so many changes unrelated to the current patchset'a purpose
> without a thourough way of testing them.

But leaving code you don't directly care about in a state not in
line with the new distinction isn't any better. The groundwork is
to have all existing code honor the new distinction in a sensible
way. Only then comes your intended behavioral change to
VM event handling.

However, thinking about the actually three different scenarios
again, I'm no longer sure either your model or the one I've been
suggesting would be correct, which would get us half way back
to what I've been asking for before. These are the cases to
care about:

- X86EMUL_UNHANDLEABLE: something went wrong while
  processing a recognized instruction (or e.g. while decoding);
  current behavior is fine to retain
- recognized but not implemented: these must not #UD, so
  behavior matching the unhandleable case is what we want
- unrecognized instruction: these should #UD

The problem is that you want to use the same error indicator
for both of the latter two cases. And the "problem" with adding
another new error indicator (e.g. X86EMUL_UNRECOGNIZED) is
that then we need to adjust the emulator when we add support
for new CPUID bits. But that's the long term goal anyway, so I
have to admit that I don't see anything wrong with this, other
than this causing some additional work.

On the grounds that present behavior isn't coming anywhere
close to the outlined target behavior and that you're patch
isn't making the situation worse, I'm now inclined to agree that
the best way forward is to live with the remaining inconsistency
until we've managed to sufficiently fill the gaps. That is, the
patch can, in this respect, stay as it was. I would like to ask you
to extend the comment on the definition though, outlining the
target behavior (perhaps including the other new return value
in that comment, or simply adding

#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED

for the time being).

The alternative of having X86EMUL_UNRECOGNIZED would be to
make the emulator raise #UD itself in such cases, as suggested
earlier.

Andrew, thoughts?

> e.g.: "hvm_descriptor_access_intercept".
> The current behavior is to return false if X86EMUL_UNHANDLEABLE is
> returned by hvm_emulate_one. Up until now, this return code covered
> also the "unimplemented instruction" case.
> If X86EMUL_UNIMPLEMENTED will be handled separately (e.g. by calling
> hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC),
> hvm_emulate_writeback, and finally returning true) some of the
> scenarios where the domain got crashed will result only in an UD being
> injected.

This function doesn't receive any X86EMUL_* values, so I don't
see how it would be relevant here. But with the above this is
likely moot anyway.

Jan


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

Re: [PATCH v10 2/3] x86emul: New return code for unimplemented instruction

[Andrew Cooper]

On 06/09/17 14:48, Petre Pircalabu wrote:
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 64454c7..8a6eb74 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);

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

Similarly for other diagnostic messages.

~Andrew

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

[PATCH v10 3/3] 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 8a6eb74..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,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);
         hvm_inject_hw_exception(trapnr, errcode);
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 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

Re: [PATCH v10 0/3] Notify monitor when emulating an unimplemented instruction

[Tamas K Lengyel]

On Wed, Sep 6, 2017 at 7:48 AM, Petre Pircalabu
<ppircalabu@bitdefender.com> wrote:
> 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

Hi Petre,
thanks for sharing that! Do you have any plans to upstream that to XTF
so we can have some automated tests for other parts of the monitor
code as well?

Tamas

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

Re: [PATCH v10 0/3] Notify monitor when emulating an unimplemented instruction

[Petre Ovidiu PIRCALABU]

Hi Tamas,

There are still some loose ends to tie up, but a soon as I will fix
then I will try to upstream my patch. 

Best regards,
Petre

On Mi, 2017-09-06 at 09:12 -0600, Tamas K Lengyel wrote:
> On Wed, Sep 6, 2017 at 7:48 AM, Petre Pircalabu
> <ppircalabu@bitdefender.com> wrote:
> > 
> > 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_unim
> > pl
> Hi Petre,
> thanks for sharing that! Do you have any plans to upstream that to
> XTF
> so we can have some automated tests for other parts of the monitor
> code as well?
> 
> Tamas
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel