* [PATCH v13 0/3] Notify monitor when emulating an unimplemented instruction
@ 2017-09-25 12:03 Petre Pircalabu
2017-09-25 12:03 ` [PATCH v13 1/3] x86emul: New return code for " Petre Pircalabu
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Petre Pircalabu @ 2017-09-25 12:03 UTC (permalink / raw)
To: xen-devel
Cc: Petre Pircalabu, kevin.tian, sstabellini, wei.liu2, jun.nakajima,
rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
This patchset implements a mechanism which allows XEN to send first an event
if the emulator encountered an unsupported instruction.
The monitor application can choose to mitigate the error, for example to singlestep
the instruction using the real processor and then resume execution of the normal
instruction flow.
This feature was tested using a modified version of XTF:
https://github.com/petrepircalabu/xen-test-framework/tree/emul_unimpl
---
Changed since v1:
* Removed the emulation kind check when calling hvm_inject_hw_exception
Changed since v2:
* Removed a file added by mistake
Changed since v3:
* Removed extra stray line
* Added the _enabled suffix to the emul_unhandleable monitor option
Changed since v4
* Fixed return expression of hvm_monitor_emul_unhandleable handle
monitor_traps failures.
* Removed stray parantheses.
Changed since v5:
* Removed unnecessary "else" when calling hvm_monitor_emul_unhandleable.
* Added extra line in arch_monitor_domctl_event.
Changed since v6:
* add the distinction between unimplemented instructions and emulation failures.
* changed "emul_unhandleable" event name to "emul_unimplemented"
Changed since v7:
* Add "fall-through" comments to the switch statements (coverity)
* Added X86EMUL_UNIMPLEMENTED to X86EMUL_UNHANDLEABLE checks the in functions
referencing x86_emulate.
* Improved comment describing X86EMUL_UNIMPLEMENTED.
Changed since v8:
* Removed unnecessary "fall-through" comments.
* Added check for X86EMUL_UNIMPLEMENTED in hvm_ud_intercept.
* add a new label 'unimplemented_insn' to accomodate the existing jumps to
'cannot_emulate' (e.g. invoke_stub)
Changed since v9:
* Added detailed description in the patch comment regarding the usage (and lack of it)
of the new X86EMUL_UNIMPLEMENTED return code.
* removed 'cannot_emulate' label.
* added local vimrc files to the gitignore list.
Changed since v10:
* Added asserts to make sure the return code cannot be X86EMUL_UNIMPLEMENTED.
* Added new return code (X86EMUL_UNRECOGNIZED) to be used when trying
to emulate an instruction with an invalid opcode.
* Added emulation return code information to error messages.
* Raise #UD when emulating an unimplemented instruction instead of just crash the domain
Changed since v11:
* Fixed double negative comment.
* Move assertion into the switch and use ASSERT_UNREACHABLE() when
applicable.
* Changed the description of X86EMUL_UNIMPLEMENTED / X86EMUL_UNRECOGNIZED
to reflect the differences between those 2 return codes.
* Changed the returned value to X86EMUL_UNRECOGNIZED in some cases (a detailed list is
provided in the patch description)
* Removed "rc=" from the error message.
* Check for X86EMUL_UNRECOGNIZED instead of X86EMUL_UNIMPLEMENTED when generating an
Invalid Opcode trap
Changed since v12:
* return X86EMUL_UNIMPLEMENTED if HAVE_GAS_RDRAND is not defined
* create unrecognized_insn label
* return X86EMUL_UNRECOGNIZED in case of failure to decode Group #13
instructions.
* add a "TODO:" comment to the description of X86EMUL_UNRECOGNIZED
stating that for now it can be used interchangeably with
X86EMUL_UNIMPLEMENTED.
Petre Pircalabu (3):
x86emul: New return code for unimplemented instruction
x86emul: Add return code information to error messages
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 | 30 ++++++++++++++++----
xen/arch/x86/hvm/hvm.c | 1 +
xen/arch/x86/hvm/io.c | 7 ++++-
xen/arch/x86/hvm/monitor.c | 17 ++++++++++++
xen/arch/x86/hvm/vmx/realmode.c | 11 +++++++-
xen/arch/x86/mm/shadow/multi.c | 6 ++--
xen/arch/x86/monitor.c | 13 +++++++++
xen/arch/x86/x86_emulate/x86_emulate.c | 51 +++++++++++++++++++---------------
xen/arch/x86/x86_emulate/x86_emulate.h | 17 ++++++++++++
xen/include/asm-x86/domain.h | 1 +
xen/include/asm-x86/hvm/emulate.h | 2 +-
xen/include/asm-x86/hvm/monitor.h | 1 +
xen/include/asm-x86/monitor.h | 3 +-
xen/include/public/domctl.h | 1 +
xen/include/public/vm_event.h | 2 ++
17 files changed, 144 insertions(+), 35 deletions(-)
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-09-25 12:03 [PATCH v13 0/3] Notify monitor when emulating an unimplemented instruction Petre Pircalabu
@ 2017-09-25 12:03 ` Petre Pircalabu
2017-09-25 12:15 ` Paul Durrant
` (3 more replies)
2017-09-25 12:03 ` [PATCH v13 2/3] x86emul: Add return code information to error messages Petre Pircalabu
2017-09-25 12:03 ` [PATCH v13 3/3] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu
2 siblings, 4 replies; 17+ messages in thread
From: Petre Pircalabu @ 2017-09-25 12:03 UTC (permalink / raw)
To: xen-devel
Cc: Petre Pircalabu, kevin.tian, sstabellini, wei.liu2, jun.nakajima,
rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
Enforce the distinction between an instruction not implemented by the
emulator and the failure to emulate that instruction by defining a new
return code, X86EMUL_UNIMPLEMENTED.
This value should only be returned by the core emulator only if it fails to
properly decode the current instruction's opcode, and not by any of other
functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
e.g. hvm_process_io_intercept should not return X86EMUL_UNIMPLEMENTED.
The return value of this function depends on either the return code of
one of the hvm_io_ops handlers (read/write) or the value returned by
hvm_copy_guest_from_phys / hvm_copy_to_guest_phys.
Similary, none of this functions should return X86EMUL_UNIMPLEMENTED.
- hvm_io_intercept
- hvmemul_do_io
- hvm_send_buffered_ioreq
- hvm_send_ioreq
- hvm_broadcast_ioreq
- hvmemul_do_io_buffer
- hvmemul_validate
Also the behavior of hvm_emulate_one_insn and vmx_realmode_emulate_one
was modified to generate an Invalid Opcode trap when X86EMUL_UNRECOGNIZED
is returned by the emulator instead of just crash the domain.
Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
---
Changed since v10:
* Added asserts to make sure the return code cannot be X86EMUL_UNIMPLEMENTED.
* Add new return code (X86EMUL_UNRECOGNIZED) to be used when trying
to emulate an instruction with an invalid opcode.
Changed since v11:
* Fixed double negative in the patch description.
* Move assertion into the switch and use ASSERT_UNREACHABLE() when
applicable.
* Changed the description of X86EMUL_UNIMPLEMENTED / X86EMUL_UNRECOGNIZED
to reflect the differences between those 2 return codes.
* Changed the returned value to X86EMUL_UNRECOGNIZED in the
following cases:
X86EMUL_OPC(0x0f, 0x73): /* Group 14 */
X86EMUL_OPC_66(0x0f, 0x73):
X86EMUL_OPC_VEX_66(0x0f, 0x73):
- All valid opcodes are defined, so it should return
X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
X86EMUL_OPC(0x0f, 0xc7) /* Group 9 */
- For register type instructions all possible opcodes are
defined, so it should return X86EMUL_UNRECOGNIZED if
mod R/M bits are not matched.
X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Group 17 */
- All valid opcodes are defined, so it should return
X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
X86EMUL_OPC_XOP(09, 0x01): /* XOP Grp1 */
X86EMUL_OPC_XOP(09, 0x02): /* XOP Grp2 */
- All valid opcodes are defined, so it should return
X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
X86EMUL_OPC(0x0f, 0x01): /* Grp7 */
- Not all valid opcodes are defined so it should return
X86EMUL_UNIMPLEMENTED if mod R/M bits are not matched.
(e.g. XGETBV)
X86EMUL_OPC_66(0x0f, 0x78):
- All valid opcodes are defined, so it should return
X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
X86EMUL_OPC(0x0f, 0xae):
X86EMUL_OPC_66(0x0f, 0xae): /* Grp15 */
- Not all valid opcodes are defined so it should return
X86EMUL_UNIMPLEMENTED if mod R/M bits are not matched.
(e.g. FXSAVE/FXRSTOR )
Changed since v12:
* return X86EMUL_UNIMPLEMENTED if HAVE_GAS_RDRAND is not defined
* create unrecognized_insn label
* return X86EMUL_UNRECOGNIZED in case of failure to decode Group #13
instructions.
* add a "TODO:" comment to the description of X86EMUL_UNRECOGNIZED
stating that for now it can be used interchangeably with
X86EMUL_UNIMPLEMENTED.
---
xen/arch/x86/hvm/emulate.c | 12 ++++++++
xen/arch/x86/hvm/hvm.c | 1 +
xen/arch/x86/hvm/io.c | 5 ++++
xen/arch/x86/hvm/vmx/realmode.c | 9 ++++++
xen/arch/x86/mm/shadow/multi.c | 2 +-
xen/arch/x86/x86_emulate/x86_emulate.c | 51 +++++++++++++++++++---------------
xen/arch/x86/x86_emulate/x86_emulate.h | 17 ++++++++++++
7 files changed, 74 insertions(+), 23 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index cc874ce..385fe1e 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -284,10 +284,15 @@ static int hvmemul_do_io(
}
break;
}
+ case X86EMUL_UNIMPLEMENTED:
+ ASSERT_UNREACHABLE();
+ /* Fall-through */
default:
BUG();
}
+ ASSERT(rc != X86EMUL_UNIMPLEMENTED);
+
if ( rc != X86EMUL_OKAY )
return rc;
@@ -313,6 +318,9 @@ static int hvmemul_do_io_buffer(
rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 0,
(uintptr_t)buffer);
+
+ ASSERT(rc != X86EMUL_UNIMPLEMENTED);
+
if ( rc == X86EMUL_UNHANDLEABLE && dir == IOREQ_READ )
memset(buffer, 0xff, size);
@@ -405,6 +413,8 @@ static int hvmemul_do_io_addr(
rc = hvmemul_do_io(is_mmio, addr, &count, size, dir, df, 1,
ram_gpa);
+ ASSERT(rc != X86EMUL_UNIMPLEMENTED);
+
if ( rc == X86EMUL_OKAY )
v->arch.hvm_vcpu.hvm_io.mmio_retry = (count < *reps);
@@ -2045,6 +2055,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
switch ( rc )
{
case X86EMUL_UNHANDLEABLE:
+ case X86EMUL_UNIMPLEMENTED:
hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt);
break;
case X86EMUL_EXCEPTION:
@@ -2102,6 +2113,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
* consistent with X86EMUL_RETRY.
*/
return;
+ case X86EMUL_UNIMPLEMENTED:
case X86EMUL_UNHANDLEABLE:
hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx);
hvm_inject_hw_exception(trapnr, errcode);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d99f4b4..57f3f76 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3735,6 +3735,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..b8c0ae7 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -99,6 +99,11 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr)
hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
return false;
+ case X86EMUL_UNRECOGNIZED:
+ hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
+ hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+ break;
+
case X86EMUL_EXCEPTION:
hvm_inject_event(&ctxt.ctxt.event);
break;
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index 12d43ad..b73fc80 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -112,6 +112,15 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
goto fail;
}
+ if ( rc == X86EMUL_UNRECOGNIZED )
+ {
+ gdprintk(XENLOG_ERR, "Unrecognized insn.\n");
+ if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE )
+ goto fail;
+
+ realmode_deliver_exception(TRAP_invalid_op, 0, hvmemul_ctxt);
+ }
+
if ( rc == X86EMUL_EXCEPTION )
{
if ( unlikely(curr->domain->debugger_attached) &&
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 8d4f244..2557e21 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3488,7 +3488,7 @@ static int sh_page_fault(struct vcpu *v,
* would be a good unshadow hint. If we *do* decide to unshadow-on-fault
* then it must be 'failable': we cannot require the unshadow to succeed.
*/
- if ( r == X86EMUL_UNHANDLEABLE )
+ if ( r == X86EMUL_UNHANDLEABLE || r == X86EMUL_UNIMPLEMENTED )
{
perfc_incr(shadow_fault_emulate_failed);
#if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index c1e2300..5be33d8 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -848,7 +848,8 @@ do{ asm volatile ( \
stub.func); \
generate_exception_if(res_.fields.trapnr == EXC_UD, EXC_UD); \
domain_crash(current->domain); \
- goto cannot_emulate; \
+ rc = X86EMUL_UNHANDLEABLE; \
+ goto done; \
} \
} while (0)
#else
@@ -2585,7 +2586,7 @@ x86_decode(
d = twobyte_table[0x3a].desc;
break;
default:
- rc = X86EMUL_UNHANDLEABLE;
+ rc = X86EMUL_UNRECOGNIZED;
goto done;
}
}
@@ -2599,7 +2600,7 @@ x86_decode(
}
else
{
- rc = X86EMUL_UNHANDLEABLE;
+ rc = X86EMUL_UNRECOGNIZED;
goto done;
}
@@ -2879,7 +2880,7 @@ x86_decode(
default:
ASSERT_UNREACHABLE();
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_UNIMPLEMENTED;
}
if ( ea.type == OP_MEM )
@@ -4191,7 +4192,7 @@ x86_emulate(
break;
case 4: /* fldenv - TODO */
state->fpu_ctrl = true;
- goto cannot_emulate;
+ goto unimplemented_insn;
case 5: /* fldcw m2byte */
state->fpu_ctrl = true;
if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val,
@@ -4202,7 +4203,7 @@ x86_emulate(
break;
case 6: /* fnstenv - TODO */
state->fpu_ctrl = true;
- goto cannot_emulate;
+ goto unimplemented_insn;
case 7: /* fnstcw m2byte */
state->fpu_ctrl = true;
emulate_fpu_insn_memdst("fnstcw", dst.val);
@@ -4438,7 +4439,7 @@ x86_emulate(
case 4: /* frstor - TODO */
case 6: /* fnsave - TODO */
state->fpu_ctrl = true;
- goto cannot_emulate;
+ goto unimplemented_insn;
case 7: /* fnstsw m2byte */
state->fpu_ctrl = true;
emulate_fpu_insn_memdst("fnstsw", dst.val);
@@ -5197,7 +5198,7 @@ x86_emulate(
#undef _GRP7
default:
- goto cannot_emulate;
+ goto unimplemented_insn;
}
break;
}
@@ -6195,7 +6196,7 @@ x86_emulate(
/* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */
break;
default:
- goto cannot_emulate;
+ goto unrecognized_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 unrecognized_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 unrecognized_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 unrecognized_insn;
}
/* fall through */
case X86EMUL_OPC_F2(0x0f, 0x78): /* insertq $imm8,$imm8,xmm,xmm */
@@ -6518,7 +6519,7 @@ x86_emulate(
goto done;
break;
default:
- goto cannot_emulate;
+ goto unimplemented_insn;
}
break;
@@ -6534,7 +6535,7 @@ x86_emulate(
vcpu_must_have(avx);
goto stmxcsr;
}
- goto cannot_emulate;
+ goto unrecognized_insn;
case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */
fail_if(modrm_mod != 3);
@@ -6777,10 +6778,10 @@ x86_emulate(
switch ( modrm_reg & 7 )
{
default:
- goto cannot_emulate;
+ goto unrecognized_insn;
-#ifdef HAVE_GAS_RDRAND
case 6: /* rdrand */
+#ifdef HAVE_GAS_RDRAND
generate_exception_if(rep_prefix(), EXC_UD);
host_and_vcpu_must_have(rdrand);
dst = ea;
@@ -6805,6 +6806,8 @@ x86_emulate(
if ( carry )
_regs.eflags |= X86_EFLAGS_CF;
break;
+#else
+ goto unimplemented_insn;
#endif
case 7: /* rdseed / rdpid */
@@ -7359,7 +7362,7 @@ x86_emulate(
host_and_vcpu_must_have(bmi1);
break;
default:
- goto cannot_emulate;
+ goto unrecognized_insn;
}
generate_exception_if(vex.l, EXC_UD);
@@ -7670,7 +7673,7 @@ x86_emulate(
host_and_vcpu_must_have(tbm);
break;
default:
- goto cannot_emulate;
+ goto unrecognized_insn;
}
xop_09_rm_rv:
@@ -7704,7 +7707,7 @@ x86_emulate(
host_and_vcpu_must_have(tbm);
goto xop_09_rm_rv;
}
- goto cannot_emulate;
+ goto unrecognized_insn;
case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */
{
@@ -7736,8 +7739,11 @@ x86_emulate(
}
default:
- cannot_emulate:
- rc = X86EMUL_UNHANDLEABLE;
+ unimplemented_insn:
+ rc = X86EMUL_UNIMPLEMENTED;
+ goto done;
+ unrecognized_insn:
+ rc = X86EMUL_UNRECOGNIZED;
goto done;
}
@@ -7789,7 +7795,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..0c8c80a 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -133,6 +133,23 @@ struct x86_emul_fpu_aux {
* Undefined behavior when used anywhere else.
*/
#define X86EMUL_DONE 4
+ /*
+ * Current instruction is not implemented by the emulator.
+ * This value should only be returned by the core emulator when a valid
+ * opcode is found but the execution logic for that instruction is missing.
+ * It should NOT be returned by any of the x86_emulate_ops callbacks.
+ */
+#define X86EMUL_UNIMPLEMENTED 5
+ /*
+ * The current instruction's opcode is not valid.
+ * If this error code is returned by a function, an #UD trap should be
+ * raised by the final consumer of it.
+ *
+ * TODO: For the moment X86EMUL_UNRECOGNIZED and X86EMUL_UNIMPLEMENTED
+ * can be used interchangeably therefore raising an #UD trap is not
+ * strictly expected for now.
+ */
+#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
/* FPU sub-types which may be requested via ->get_fpu(). */
enum x86_emulate_fpu_type {
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v13 2/3] x86emul: Add return code information to error messages
2017-09-25 12:03 [PATCH v13 0/3] Notify monitor when emulating an unimplemented instruction Petre Pircalabu
2017-09-25 12:03 ` [PATCH v13 1/3] x86emul: New return code for " Petre Pircalabu
@ 2017-09-25 12:03 ` Petre Pircalabu
2017-09-25 12:11 ` Paul Durrant
2017-10-02 13:41 ` George Dunlap
2017-09-25 12:03 ` [PATCH v13 3/3] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu
2 siblings, 2 replies; 17+ messages in thread
From: Petre Pircalabu @ 2017-09-25 12:03 UTC (permalink / raw)
To: xen-devel
Cc: Petre Pircalabu, kevin.tian, sstabellini, wei.liu2, jun.nakajima,
rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
- print the return code of the last failed emulator operation
in hvm_dump_emulation_state.
- print the return code in sh_page_fault (SHADOW_PRINTK) to make the
distiction between X86EMUL_UNHANDLEABLE and X86EMUL_UNIMPLEMENTED.
Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changed since v11:
* Removed "rc=" from the error message.
---
xen/arch/x86/hvm/emulate.c | 13 +++++++------
xen/arch/x86/hvm/io.c | 4 ++--
xen/arch/x86/hvm/vmx/realmode.c | 2 +-
xen/arch/x86/mm/shadow/multi.c | 4 ++--
xen/include/asm-x86/hvm/emulate.h | 2 +-
5 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 385fe1e..4fe61b4 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2056,7 +2056,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
{
case X86EMUL_UNHANDLEABLE:
case X86EMUL_UNIMPLEMENTED:
- hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt);
+ hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt, rc);
break;
case X86EMUL_EXCEPTION:
hvm_inject_event(&ctxt.ctxt.event);
@@ -2115,7 +2115,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
return;
case X86EMUL_UNIMPLEMENTED:
case X86EMUL_UNHANDLEABLE:
- hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx);
+ hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx, rc);
hvm_inject_hw_exception(trapnr, errcode);
break;
case X86EMUL_EXCEPTION:
@@ -2243,16 +2243,17 @@ static const char *guest_x86_mode_to_str(int mode)
}
void hvm_dump_emulation_state(const char *loglvl, const char *prefix,
- struct hvm_emulate_ctxt *hvmemul_ctxt)
+ struct hvm_emulate_ctxt *hvmemul_ctxt, int rc)
{
struct vcpu *curr = current;
const char *mode_str = guest_x86_mode_to_str(hvm_guest_x86_mode(curr));
const struct segment_register *cs =
hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
- printk("%s%s emulation failed: %pv %s @ %04x:%08lx -> %*ph\n",
- loglvl, prefix, curr, mode_str, cs->sel, hvmemul_ctxt->insn_buf_eip,
- hvmemul_ctxt->insn_buf_bytes, hvmemul_ctxt->insn_buf);
+ printk("%s%s emulation failed (%d): %pv %s @ %04x:%08lx -> %*ph\n",
+ loglvl, prefix, rc, curr, mode_str, cs->sel,
+ hvmemul_ctxt->insn_buf_eip, hvmemul_ctxt->insn_buf_bytes,
+ hvmemul_ctxt->insn_buf);
}
/*
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index b8c0ae7..c7b1c53 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -96,11 +96,11 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr)
switch ( rc )
{
case X86EMUL_UNHANDLEABLE:
- hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
+ hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
return false;
case X86EMUL_UNRECOGNIZED:
- hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
+ hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
break;
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index b73fc80..03dea6c 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -147,7 +147,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
return;
fail:
- hvm_dump_emulation_state(XENLOG_G_ERR, "Real-mode", hvmemul_ctxt);
+ hvm_dump_emulation_state(XENLOG_G_ERR, "Real-mode", hvmemul_ctxt, rc);
domain_crash(curr->domain);
}
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 2557e21..28030ac 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3498,8 +3498,8 @@ static int sh_page_fault(struct vcpu *v,
v->arch.paging.last_write_emul_ok = 0;
}
#endif
- SHADOW_PRINTK("emulator failure, unshadowing mfn %#lx\n",
- mfn_x(gmfn));
+ SHADOW_PRINTK("emulator failure (rc=%d), unshadowing mfn %#lx\n",
+ r, mfn_x(gmfn));
/* If this is actually a page table, then we have a bug, and need
* to support more operations in the emulator. More likely,
* though, this is a hint that this page should not be shadowed. */
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index 8864775..58d17c4 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -92,7 +92,7 @@ int hvmemul_do_pio_buffer(uint16_t port,
void *buffer);
void hvm_dump_emulation_state(const char *loglvl, const char *prefix,
- struct hvm_emulate_ctxt *hvmemul_ctxt);
+ struct hvm_emulate_ctxt *hvmemul_ctxt, int rc);
#endif /* __ASM_X86_HVM_EMULATE_H__ */
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v13 3/3] x86/monitor: Notify monitor if an emulation fails.
2017-09-25 12:03 [PATCH v13 0/3] Notify monitor when emulating an unimplemented instruction Petre Pircalabu
2017-09-25 12:03 ` [PATCH v13 1/3] x86emul: New return code for " Petre Pircalabu
2017-09-25 12:03 ` [PATCH v13 2/3] x86emul: Add return code information to error messages Petre Pircalabu
@ 2017-09-25 12:03 ` Petre Pircalabu
2017-09-25 12:13 ` Paul Durrant
2 siblings, 1 reply; 17+ messages in thread
From: Petre Pircalabu @ 2017-09-25 12:03 UTC (permalink / raw)
To: xen-devel
Cc: Petre Pircalabu, kevin.tian, sstabellini, wei.liu2, jun.nakajima,
rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
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 073fbc9..5ea8b4c 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 4fe61b4..19e7035 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>
@@ -2114,6 +2116,9 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
*/
return;
case X86EMUL_UNIMPLEMENTED:
+ if ( hvm_monitor_emul_unimplemented() )
+ return;
+ /* fall-through */
case X86EMUL_UNHANDLEABLE:
hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx, rc);
hvm_inject_hw_exception(trapnr, errcode);
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 2787dfa..4ce778c 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -57,6 +57,23 @@ bool hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old)
return 0;
}
+bool hvm_monitor_emul_unimplemented(void)
+{
+ struct vcpu *curr = current;
+
+ /*
+ * Send a vm_event to the monitor to signal that the current
+ * instruction couldn't be emulated.
+ */
+ vm_event_request_t req = {
+ .reason = VM_EVENT_REASON_EMUL_UNIMPLEMENTED,
+ .vcpu_id = curr->vcpu_id,
+ };
+
+ return curr->domain->arch.monitor.emul_unimplemented_enabled &&
+ monitor_traps(curr, true, &req) == 1;
+}
+
void hvm_monitor_msr(unsigned int msr, uint64_t value)
{
struct vcpu *curr = current;
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 706454f..e59f1f5 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -283,6 +283,19 @@ int arch_monitor_domctl_event(struct domain *d,
break;
}
+ case XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED:
+ {
+ bool old_status = ad->monitor.emul_unimplemented_enabled;
+
+ if ( unlikely(old_status == requested_status) )
+ return -EEXIST;
+
+ domain_pause(d);
+ ad->monitor.emul_unimplemented_enabled = requested_status;
+ domain_unpause(d);
+ break;
+ }
+
default:
/*
* Should not be reached unless arch_monitor_get_capabilities() is
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 57da0fc..ee3f42d 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -407,6 +407,7 @@ struct arch_domain
unsigned int cpuid_enabled : 1;
unsigned int descriptor_access_enabled : 1;
unsigned int guest_request_userspace_enabled : 1;
+ unsigned int emul_unimplemented_enabled : 1;
struct monitor_msr_bitmap *msr_bitmap;
uint64_t write_ctrlreg_mask[4];
} monitor;
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index cfd6661..6e22091 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -47,6 +47,7 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf,
unsigned int subleaf);
void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
unsigned int err, uint64_t cr2);
+bool hvm_monitor_emul_unimplemented(void);
#endif /* __ASM_X86_HVM_MONITOR_H__ */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 765d0b4..0ada970 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -83,7 +83,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
(1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
(1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
(1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
- (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT);
+ (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
+ (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED);
/* Since we know this is on VMX, we can just call the hvm func */
if ( hvm_is_singlestep_supported() )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index e2fd685..da7e4f8 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1007,6 +1007,7 @@ struct xen_domctl_psr_cmt_op {
#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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v13 2/3] x86emul: Add return code information to error messages
2017-09-25 12:03 ` [PATCH v13 2/3] x86emul: Add return code information to error messages Petre Pircalabu
@ 2017-09-25 12:11 ` Paul Durrant
2017-10-02 13:41 ` George Dunlap
1 sibling, 0 replies; 17+ messages in thread
From: Paul Durrant @ 2017-09-25 12:11 UTC (permalink / raw)
To: 'Petre Pircalabu', xen-devel
Cc: Kevin Tian, sstabellini, Wei Liu, jun.nakajima, rcojocaru,
Andrew Cooper, Tim (Xen.org),
George Dunlap, tamas, jbeulich, Ian Jackson
> -----Original Message-----
> From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]
> Sent: 25 September 2017 13:03
> 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 v13 2/3] x86emul: Add return code information to error
> messages
>
> - print the return code of the last failed emulator operation
> in hvm_dump_emulation_state.
> - print the return code in sh_page_fault (SHADOW_PRINTK) to make the
> distiction between X86EMUL_UNHANDLEABLE and
> X86EMUL_UNIMPLEMENTED.
>
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
>
> ---
> Changed since v11:
> * Removed "rc=" from the error message.
> ---
> xen/arch/x86/hvm/emulate.c | 13 +++++++------
> xen/arch/x86/hvm/io.c | 4 ++--
> xen/arch/x86/hvm/vmx/realmode.c | 2 +-
> xen/arch/x86/mm/shadow/multi.c | 4 ++--
> xen/include/asm-x86/hvm/emulate.h | 2 +-
> 5 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 385fe1e..4fe61b4 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2056,7 +2056,7 @@ int hvm_emulate_one_mmio(unsigned long mfn,
> unsigned long gla)
> {
> case X86EMUL_UNHANDLEABLE:
> case X86EMUL_UNIMPLEMENTED:
> - hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG",
> &ctxt);
> + hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG",
> &ctxt, rc);
> break;
> case X86EMUL_EXCEPTION:
> hvm_inject_event(&ctxt.ctxt.event);
> @@ -2115,7 +2115,7 @@ void hvm_emulate_one_vm_event(enum
> emul_kind kind, unsigned int trapnr,
> return;
> case X86EMUL_UNIMPLEMENTED:
> case X86EMUL_UNHANDLEABLE:
> - hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx);
> + hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx,
> rc);
> hvm_inject_hw_exception(trapnr, errcode);
> break;
> case X86EMUL_EXCEPTION:
> @@ -2243,16 +2243,17 @@ static const char *guest_x86_mode_to_str(int
> mode)
> }
>
> void hvm_dump_emulation_state(const char *loglvl, const char *prefix,
> - struct hvm_emulate_ctxt *hvmemul_ctxt)
> + struct hvm_emulate_ctxt *hvmemul_ctxt, int rc)
> {
> struct vcpu *curr = current;
> const char *mode_str =
> guest_x86_mode_to_str(hvm_guest_x86_mode(curr));
> const struct segment_register *cs =
> hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
>
> - printk("%s%s emulation failed: %pv %s @ %04x:%08lx -> %*ph\n",
> - loglvl, prefix, curr, mode_str, cs->sel, hvmemul_ctxt->insn_buf_eip,
> - hvmemul_ctxt->insn_buf_bytes, hvmemul_ctxt->insn_buf);
> + printk("%s%s emulation failed (%d): %pv %s @ %04x:%08lx -> %*ph\n",
> + loglvl, prefix, rc, curr, mode_str, cs->sel,
> + hvmemul_ctxt->insn_buf_eip, hvmemul_ctxt->insn_buf_bytes,
> + hvmemul_ctxt->insn_buf);
> }
>
> /*
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index b8c0ae7..c7b1c53 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -96,11 +96,11 @@ bool
> hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char
> *descr)
> switch ( rc )
> {
> case X86EMUL_UNHANDLEABLE:
> - hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
> + hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
> return false;
>
> case X86EMUL_UNRECOGNIZED:
> - hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
> + hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
> hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> break;
>
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c
> b/xen/arch/x86/hvm/vmx/realmode.c
> index b73fc80..03dea6c 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -147,7 +147,7 @@ void vmx_realmode_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt)
> return;
>
> fail:
> - hvm_dump_emulation_state(XENLOG_G_ERR, "Real-mode",
> hvmemul_ctxt);
> + hvm_dump_emulation_state(XENLOG_G_ERR, "Real-mode",
> hvmemul_ctxt, rc);
> domain_crash(curr->domain);
> }
>
> diff --git a/xen/arch/x86/mm/shadow/multi.c
> b/xen/arch/x86/mm/shadow/multi.c
> index 2557e21..28030ac 100644
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3498,8 +3498,8 @@ static int sh_page_fault(struct vcpu *v,
> v->arch.paging.last_write_emul_ok = 0;
> }
> #endif
> - SHADOW_PRINTK("emulator failure, unshadowing mfn %#lx\n",
> - mfn_x(gmfn));
> + SHADOW_PRINTK("emulator failure (rc=%d), unshadowing mfn
> %#lx\n",
> + r, mfn_x(gmfn));
> /* If this is actually a page table, then we have a bug, and need
> * to support more operations in the emulator. More likely,
> * though, this is a hint that this page should not be shadowed. */
> diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-
> x86/hvm/emulate.h
> index 8864775..58d17c4 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -92,7 +92,7 @@ int hvmemul_do_pio_buffer(uint16_t port,
> void *buffer);
>
> void hvm_dump_emulation_state(const char *loglvl, const char *prefix,
> - struct hvm_emulate_ctxt *hvmemul_ctxt);
> + struct hvm_emulate_ctxt *hvmemul_ctxt, int rc);
>
> #endif /* __ASM_X86_HVM_EMULATE_H__ */
>
> --
> 2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 3/3] x86/monitor: Notify monitor if an emulation fails.
2017-09-25 12:03 ` [PATCH v13 3/3] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu
@ 2017-09-25 12:13 ` Paul Durrant
0 siblings, 0 replies; 17+ messages in thread
From: Paul Durrant @ 2017-09-25 12:13 UTC (permalink / raw)
To: 'Petre Pircalabu', xen-devel
Cc: Kevin Tian, sstabellini, Wei Liu, jun.nakajima, rcojocaru,
Andrew Cooper, Tim (Xen.org),
George Dunlap, tamas, jbeulich, Ian Jackson
> -----Original Message-----
> From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]
> Sent: 25 September 2017 13:03
> 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 v13 3/3] x86/monitor: Notify monitor if an emulation fails.
>
> 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>
Reviewed-by: Paul Durrant <paul.durrant@citrix.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 073fbc9..5ea8b4c 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 4fe61b4..19e7035 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>
> @@ -2114,6 +2116,9 @@ void hvm_emulate_one_vm_event(enum
> emul_kind kind, unsigned int trapnr,
> */
> return;
> case X86EMUL_UNIMPLEMENTED:
> + if ( hvm_monitor_emul_unimplemented() )
> + return;
> + /* fall-through */
> case X86EMUL_UNHANDLEABLE:
> hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx,
> rc);
> hvm_inject_hw_exception(trapnr, errcode);
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index 2787dfa..4ce778c 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -57,6 +57,23 @@ bool hvm_monitor_cr(unsigned int index, unsigned
> long value, unsigned long old)
> return 0;
> }
>
> +bool hvm_monitor_emul_unimplemented(void)
> +{
> + struct vcpu *curr = current;
> +
> + /*
> + * Send a vm_event to the monitor to signal that the current
> + * instruction couldn't be emulated.
> + */
> + vm_event_request_t req = {
> + .reason = VM_EVENT_REASON_EMUL_UNIMPLEMENTED,
> + .vcpu_id = curr->vcpu_id,
> + };
> +
> + return curr->domain->arch.monitor.emul_unimplemented_enabled &&
> + monitor_traps(curr, true, &req) == 1;
> +}
> +
> void hvm_monitor_msr(unsigned int msr, uint64_t value)
> {
> struct vcpu *curr = current;
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 706454f..e59f1f5 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -283,6 +283,19 @@ int arch_monitor_domctl_event(struct domain *d,
> break;
> }
>
> + case XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED:
> + {
> + bool old_status = ad->monitor.emul_unimplemented_enabled;
> +
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
> +
> + domain_pause(d);
> + ad->monitor.emul_unimplemented_enabled = requested_status;
> + domain_unpause(d);
> + break;
> + }
> +
> default:
> /*
> * Should not be reached unless arch_monitor_get_capabilities() is
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 57da0fc..ee3f42d 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -407,6 +407,7 @@ struct arch_domain
> unsigned int cpuid_enabled : 1;
> unsigned int descriptor_access_enabled : 1;
> unsigned int guest_request_userspace_enabled : 1;
> + unsigned int emul_unimplemented_enabled : 1;
> struct monitor_msr_bitmap *msr_bitmap;
> uint64_t write_ctrlreg_mask[4];
> } monitor;
> diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-
> x86/hvm/monitor.h
> index cfd6661..6e22091 100644
> --- a/xen/include/asm-x86/hvm/monitor.h
> +++ b/xen/include/asm-x86/hvm/monitor.h
> @@ -47,6 +47,7 @@ int hvm_monitor_cpuid(unsigned long insn_length,
> unsigned int leaf,
> unsigned int subleaf);
> void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
> unsigned int err, uint64_t cr2);
> +bool hvm_monitor_emul_unimplemented(void);
>
> #endif /* __ASM_X86_HVM_MONITOR_H__ */
>
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-
> x86/monitor.h
> index 765d0b4..0ada970 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -83,7 +83,8 @@ static inline uint32_t
> arch_monitor_get_capabilities(struct domain *d)
> (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
> (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
> (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
> - (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT);
> + (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
> + (1U <<
> XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED);
>
> /* Since we know this is on VMX, we can just call the hvm func */
> if ( hvm_is_singlestep_supported() )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index e2fd685..da7e4f8 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1007,6 +1007,7 @@ struct xen_domctl_psr_cmt_op {
> #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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-09-25 12:03 ` [PATCH v13 1/3] x86emul: New return code for " Petre Pircalabu
@ 2017-09-25 12:15 ` Paul Durrant
2017-09-26 13:53 ` Jan Beulich
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Paul Durrant @ 2017-09-25 12:15 UTC (permalink / raw)
To: 'Petre Pircalabu', xen-devel
Cc: Kevin Tian, sstabellini, Wei Liu, jun.nakajima, rcojocaru,
Andrew Cooper, Tim (Xen.org),
George Dunlap, tamas, jbeulich, Ian Jackson
> -----Original Message-----
> From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]
> Sent: 25 September 2017 13:03
> 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 v13 1/3] x86emul: New return code for unimplemented
> instruction
>
> Enforce the distinction between an instruction not implemented by the
> emulator and the failure to emulate that instruction by defining a new
> return code, X86EMUL_UNIMPLEMENTED.
>
> This value should only be returned by the core emulator only if it fails to
> properly decode the current instruction's opcode, and not by any of other
> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
>
> e.g. hvm_process_io_intercept should not return
> X86EMUL_UNIMPLEMENTED.
> The return value of this function depends on either the return code of
> one of the hvm_io_ops handlers (read/write) or the value returned by
> hvm_copy_guest_from_phys / hvm_copy_to_guest_phys.
>
> Similary, none of this functions should return X86EMUL_UNIMPLEMENTED.
> - hvm_io_intercept
> - hvmemul_do_io
> - hvm_send_buffered_ioreq
> - hvm_send_ioreq
> - hvm_broadcast_ioreq
> - hvmemul_do_io_buffer
> - hvmemul_validate
>
> Also the behavior of hvm_emulate_one_insn and
> vmx_realmode_emulate_one
> was modified to generate an Invalid Opcode trap when
> X86EMUL_UNRECOGNIZED
> is returned by the emulator instead of just crash the domain.
>
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Changed since v10:
> * Added asserts to make sure the return code cannot be
> X86EMUL_UNIMPLEMENTED.
> * Add new return code (X86EMUL_UNRECOGNIZED) to be used when
> trying
> to emulate an instruction with an invalid opcode.
>
> Changed since v11:
> * Fixed double negative in the patch description.
> * Move assertion into the switch and use ASSERT_UNREACHABLE() when
> applicable.
> * Changed the description of X86EMUL_UNIMPLEMENTED /
> X86EMUL_UNRECOGNIZED
> to reflect the differences between those 2 return codes.
> * Changed the returned value to X86EMUL_UNRECOGNIZED in the
> following cases:
> X86EMUL_OPC(0x0f, 0x73): /* Group 14 */
> X86EMUL_OPC_66(0x0f, 0x73):
> X86EMUL_OPC_VEX_66(0x0f, 0x73):
> - All valid opcodes are defined, so it should return
> X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
>
> X86EMUL_OPC(0x0f, 0xc7) /* Group 9 */
> - For register type instructions all possible opcodes are
> defined, so it should return X86EMUL_UNRECOGNIZED if
> mod R/M bits are not matched.
>
> X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Group 17 */
> - All valid opcodes are defined, so it should return
> X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
>
> X86EMUL_OPC_XOP(09, 0x01): /* XOP Grp1 */
> X86EMUL_OPC_XOP(09, 0x02): /* XOP Grp2 */
> - All valid opcodes are defined, so it should return
> X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
>
> X86EMUL_OPC(0x0f, 0x01): /* Grp7 */
> - Not all valid opcodes are defined so it should return
> X86EMUL_UNIMPLEMENTED if mod R/M bits are not matched.
> (e.g. XGETBV)
>
> X86EMUL_OPC_66(0x0f, 0x78):
> - All valid opcodes are defined, so it should return
> X86EMUL_UNRECOGNIZED if mod R/M bits are not matched.
>
> X86EMUL_OPC(0x0f, 0xae):
> X86EMUL_OPC_66(0x0f, 0xae): /* Grp15 */
> - Not all valid opcodes are defined so it should return
> X86EMUL_UNIMPLEMENTED if mod R/M bits are not matched.
> (e.g. FXSAVE/FXRSTOR )
>
> Changed since v12:
> * return X86EMUL_UNIMPLEMENTED if HAVE_GAS_RDRAND is not
> defined
> * create unrecognized_insn label
> * return X86EMUL_UNRECOGNIZED in case of failure to decode Group #13
> instructions.
> * add a "TODO:" comment to the description of X86EMUL_UNRECOGNIZED
> stating that for now it can be used interchangeably with
> X86EMUL_UNIMPLEMENTED.
> ---
> xen/arch/x86/hvm/emulate.c | 12 ++++++++
> xen/arch/x86/hvm/hvm.c | 1 +
> xen/arch/x86/hvm/io.c | 5 ++++
> xen/arch/x86/hvm/vmx/realmode.c | 9 ++++++
> xen/arch/x86/mm/shadow/multi.c | 2 +-
> xen/arch/x86/x86_emulate/x86_emulate.c | 51 +++++++++++++++++++----
> -----------
> xen/arch/x86/x86_emulate/x86_emulate.h | 17 ++++++++++++
> 7 files changed, 74 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index cc874ce..385fe1e 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -284,10 +284,15 @@ static int hvmemul_do_io(
> }
> break;
> }
> + case X86EMUL_UNIMPLEMENTED:
> + ASSERT_UNREACHABLE();
> + /* Fall-through */
> default:
> BUG();
> }
>
> + ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +
> if ( rc != X86EMUL_OKAY )
> return rc;
>
> @@ -313,6 +318,9 @@ static int hvmemul_do_io_buffer(
>
> rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 0,
> (uintptr_t)buffer);
> +
> + ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +
> if ( rc == X86EMUL_UNHANDLEABLE && dir == IOREQ_READ )
> memset(buffer, 0xff, size);
>
> @@ -405,6 +413,8 @@ static int hvmemul_do_io_addr(
> rc = hvmemul_do_io(is_mmio, addr, &count, size, dir, df, 1,
> ram_gpa);
>
> + ASSERT(rc != X86EMUL_UNIMPLEMENTED);
> +
> if ( rc == X86EMUL_OKAY )
> v->arch.hvm_vcpu.hvm_io.mmio_retry = (count < *reps);
>
> @@ -2045,6 +2055,7 @@ int hvm_emulate_one_mmio(unsigned long mfn,
> unsigned long gla)
> switch ( rc )
> {
> case X86EMUL_UNHANDLEABLE:
> + case X86EMUL_UNIMPLEMENTED:
> hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG",
> &ctxt);
> break;
> case X86EMUL_EXCEPTION:
> @@ -2102,6 +2113,7 @@ void hvm_emulate_one_vm_event(enum
> emul_kind kind, unsigned int trapnr,
> * consistent with X86EMUL_RETRY.
> */
> return;
> + case X86EMUL_UNIMPLEMENTED:
> case X86EMUL_UNHANDLEABLE:
> hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx);
> hvm_inject_hw_exception(trapnr, errcode);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d99f4b4..57f3f76 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3735,6 +3735,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..b8c0ae7 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -99,6 +99,11 @@ bool
> hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char
> *descr)
> hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
> return false;
>
> + case X86EMUL_UNRECOGNIZED:
> + hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt);
> + hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> + break;
> +
> case X86EMUL_EXCEPTION:
> hvm_inject_event(&ctxt.ctxt.event);
> break;
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c
> b/xen/arch/x86/hvm/vmx/realmode.c
> index 12d43ad..b73fc80 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -112,6 +112,15 @@ void vmx_realmode_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt)
> goto fail;
> }
>
> + if ( rc == X86EMUL_UNRECOGNIZED )
> + {
> + gdprintk(XENLOG_ERR, "Unrecognized insn.\n");
> + if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE )
> + goto fail;
> +
> + realmode_deliver_exception(TRAP_invalid_op, 0, hvmemul_ctxt);
> + }
> +
> if ( rc == X86EMUL_EXCEPTION )
> {
> if ( unlikely(curr->domain->debugger_attached) &&
> diff --git a/xen/arch/x86/mm/shadow/multi.c
> b/xen/arch/x86/mm/shadow/multi.c
> index 8d4f244..2557e21 100644
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3488,7 +3488,7 @@ static int sh_page_fault(struct vcpu *v,
> * would be a good unshadow hint. If we *do* decide to unshadow-on-
> fault
> * then it must be 'failable': we cannot require the unshadow to succeed.
> */
> - if ( r == X86EMUL_UNHANDLEABLE )
> + if ( r == X86EMUL_UNHANDLEABLE || r == X86EMUL_UNIMPLEMENTED )
> {
> perfc_incr(shadow_fault_emulate_failed);
> #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index c1e2300..5be33d8 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -848,7 +848,8 @@ do{ asm volatile ( \
> stub.func); \
> generate_exception_if(res_.fields.trapnr == EXC_UD, EXC_UD); \
> domain_crash(current->domain); \
> - goto cannot_emulate; \
> + rc = X86EMUL_UNHANDLEABLE; \
> + goto done; \
> } \
> } while (0)
> #else
> @@ -2585,7 +2586,7 @@ x86_decode(
> d = twobyte_table[0x3a].desc;
> break;
> default:
> - rc = X86EMUL_UNHANDLEABLE;
> + rc = X86EMUL_UNRECOGNIZED;
> goto done;
> }
> }
> @@ -2599,7 +2600,7 @@ x86_decode(
> }
> else
> {
> - rc = X86EMUL_UNHANDLEABLE;
> + rc = X86EMUL_UNRECOGNIZED;
> goto done;
> }
>
> @@ -2879,7 +2880,7 @@ x86_decode(
>
> default:
> ASSERT_UNREACHABLE();
> - return X86EMUL_UNHANDLEABLE;
> + return X86EMUL_UNIMPLEMENTED;
> }
>
> if ( ea.type == OP_MEM )
> @@ -4191,7 +4192,7 @@ x86_emulate(
> break;
> case 4: /* fldenv - TODO */
> state->fpu_ctrl = true;
> - goto cannot_emulate;
> + goto unimplemented_insn;
> case 5: /* fldcw m2byte */
> state->fpu_ctrl = true;
> if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val,
> @@ -4202,7 +4203,7 @@ x86_emulate(
> break;
> case 6: /* fnstenv - TODO */
> state->fpu_ctrl = true;
> - goto cannot_emulate;
> + goto unimplemented_insn;
> case 7: /* fnstcw m2byte */
> state->fpu_ctrl = true;
> emulate_fpu_insn_memdst("fnstcw", dst.val);
> @@ -4438,7 +4439,7 @@ x86_emulate(
> case 4: /* frstor - TODO */
> case 6: /* fnsave - TODO */
> state->fpu_ctrl = true;
> - goto cannot_emulate;
> + goto unimplemented_insn;
> case 7: /* fnstsw m2byte */
> state->fpu_ctrl = true;
> emulate_fpu_insn_memdst("fnstsw", dst.val);
> @@ -5197,7 +5198,7 @@ x86_emulate(
> #undef _GRP7
>
> default:
> - goto cannot_emulate;
> + goto unimplemented_insn;
> }
> break;
> }
> @@ -6195,7 +6196,7 @@ x86_emulate(
> /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */
> break;
> default:
> - goto cannot_emulate;
> + goto unrecognized_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 unrecognized_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 unrecognized_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 unrecognized_insn;
> }
> /* fall through */
> case X86EMUL_OPC_F2(0x0f, 0x78): /* insertq $imm8,$imm8,xmm,xmm
> */
> @@ -6518,7 +6519,7 @@ x86_emulate(
> goto done;
> break;
> default:
> - goto cannot_emulate;
> + goto unimplemented_insn;
> }
> break;
>
> @@ -6534,7 +6535,7 @@ x86_emulate(
> vcpu_must_have(avx);
> goto stmxcsr;
> }
> - goto cannot_emulate;
> + goto unrecognized_insn;
>
> case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */
> fail_if(modrm_mod != 3);
> @@ -6777,10 +6778,10 @@ x86_emulate(
> switch ( modrm_reg & 7 )
> {
> default:
> - goto cannot_emulate;
> + goto unrecognized_insn;
>
> -#ifdef HAVE_GAS_RDRAND
> case 6: /* rdrand */
> +#ifdef HAVE_GAS_RDRAND
> generate_exception_if(rep_prefix(), EXC_UD);
> host_and_vcpu_must_have(rdrand);
> dst = ea;
> @@ -6805,6 +6806,8 @@ x86_emulate(
> if ( carry )
> _regs.eflags |= X86_EFLAGS_CF;
> break;
> +#else
> + goto unimplemented_insn;
> #endif
>
> case 7: /* rdseed / rdpid */
> @@ -7359,7 +7362,7 @@ x86_emulate(
> host_and_vcpu_must_have(bmi1);
> break;
> default:
> - goto cannot_emulate;
> + goto unrecognized_insn;
> }
>
> generate_exception_if(vex.l, EXC_UD);
> @@ -7670,7 +7673,7 @@ x86_emulate(
> host_and_vcpu_must_have(tbm);
> break;
> default:
> - goto cannot_emulate;
> + goto unrecognized_insn;
> }
>
> xop_09_rm_rv:
> @@ -7704,7 +7707,7 @@ x86_emulate(
> host_and_vcpu_must_have(tbm);
> goto xop_09_rm_rv;
> }
> - goto cannot_emulate;
> + goto unrecognized_insn;
>
> case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */
> {
> @@ -7736,8 +7739,11 @@ x86_emulate(
> }
>
> default:
> - cannot_emulate:
> - rc = X86EMUL_UNHANDLEABLE;
> + unimplemented_insn:
> + rc = X86EMUL_UNIMPLEMENTED;
> + goto done;
> + unrecognized_insn:
> + rc = X86EMUL_UNRECOGNIZED;
> goto done;
> }
>
> @@ -7789,7 +7795,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..0c8c80a 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -133,6 +133,23 @@ struct x86_emul_fpu_aux {
> * Undefined behavior when used anywhere else.
> */
> #define X86EMUL_DONE 4
> + /*
> + * Current instruction is not implemented by the emulator.
> + * This value should only be returned by the core emulator when a valid
> + * opcode is found but the execution logic for that instruction is missing.
> + * It should NOT be returned by any of the x86_emulate_ops callbacks.
> + */
> +#define X86EMUL_UNIMPLEMENTED 5
> + /*
> + * The current instruction's opcode is not valid.
> + * If this error code is returned by a function, an #UD trap should be
> + * raised by the final consumer of it.
> + *
> + * TODO: For the moment X86EMUL_UNRECOGNIZED and
> X86EMUL_UNIMPLEMENTED
> + * can be used interchangeably therefore raising an #UD trap is not
> + * strictly expected for now.
> + */
> +#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
>
> /* FPU sub-types which may be requested via ->get_fpu(). */
> enum x86_emulate_fpu_type {
> --
> 2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-09-25 12:03 ` [PATCH v13 1/3] x86emul: New return code for " Petre Pircalabu
2017-09-25 12:15 ` Paul Durrant
@ 2017-09-26 13:53 ` Jan Beulich
2017-10-02 13:39 ` George Dunlap
2017-10-02 13:43 ` George Dunlap
3 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2017-09-26 13:53 UTC (permalink / raw)
To: Petre Pircalabu
Cc: tim, kevin.tian, sstabellini, wei.liu2, rcojocaru, George.Dunlap,
andrew.cooper3, ian.jackson, xen-devel, paul.durrant, tamas,
jun.nakajima
>>> On 25.09.17 at 14:03, <ppircalabu@bitdefender.com> wrote:
> Enforce the distinction between an instruction not implemented by the
> emulator and the failure to emulate that instruction by defining a new
> return code, X86EMUL_UNIMPLEMENTED.
>
> This value should only be returned by the core emulator only if it fails to
> properly decode the current instruction's opcode, and not by any of other
> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
>
> e.g. hvm_process_io_intercept should not return X86EMUL_UNIMPLEMENTED.
> The return value of this function depends on either the return code of
> one of the hvm_io_ops handlers (read/write) or the value returned by
> hvm_copy_guest_from_phys / hvm_copy_to_guest_phys.
>
> Similary, none of this functions should return X86EMUL_UNIMPLEMENTED.
> - hvm_io_intercept
> - hvmemul_do_io
> - hvm_send_buffered_ioreq
> - hvm_send_ioreq
> - hvm_broadcast_ioreq
> - hvmemul_do_io_buffer
> - hvmemul_validate
>
> Also the behavior of hvm_emulate_one_insn and vmx_realmode_emulate_one
> was modified to generate an Invalid Opcode trap when X86EMUL_UNRECOGNIZED
> is returned by the emulator instead of just crash the domain.
>
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-09-25 12:03 ` [PATCH v13 1/3] x86emul: New return code for " Petre Pircalabu
2017-09-25 12:15 ` Paul Durrant
2017-09-26 13:53 ` Jan Beulich
@ 2017-10-02 13:39 ` George Dunlap
2017-10-02 13:43 ` George Dunlap
3 siblings, 0 replies; 17+ messages in thread
From: George Dunlap @ 2017-10-02 13:39 UTC (permalink / raw)
To: Petre Pircalabu, xen-devel
Cc: kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru,
konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
> Enforce the distinction between an instruction not implemented by the
> emulator and the failure to emulate that instruction by defining a new
> return code, X86EMUL_UNIMPLEMENTED.
>
> This value should only be returned by the core emulator only if it fails to
> properly decode the current instruction's opcode, and not by any of other
> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
>
> e.g. hvm_process_io_intercept should not return X86EMUL_UNIMPLEMENTED.
> The return value of this function depends on either the return code of
> one of the hvm_io_ops handlers (read/write) or the value returned by
> hvm_copy_guest_from_phys / hvm_copy_to_guest_phys.
>
> Similary, none of this functions should return X86EMUL_UNIMPLEMENTED.
> - hvm_io_intercept
> - hvmemul_do_io
> - hvm_send_buffered_ioreq
> - hvm_send_ioreq
> - hvm_broadcast_ioreq
> - hvmemul_do_io_buffer
> - hvmemul_validate
>
> Also the behavior of hvm_emulate_one_insn and vmx_realmode_emulate_one
> was modified to generate an Invalid Opcode trap when X86EMUL_UNRECOGNIZED
> is returned by the emulator instead of just crash the domain.
>
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
Looks good, thanks:
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 2/3] x86emul: Add return code information to error messages
2017-09-25 12:03 ` [PATCH v13 2/3] x86emul: Add return code information to error messages Petre Pircalabu
2017-09-25 12:11 ` Paul Durrant
@ 2017-10-02 13:41 ` George Dunlap
1 sibling, 0 replies; 17+ messages in thread
From: George Dunlap @ 2017-10-02 13:41 UTC (permalink / raw)
To: Petre Pircalabu, xen-devel
Cc: kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru,
konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
> - print the return code of the last failed emulator operation
> in hvm_dump_emulation_state.
> - print the return code in sh_page_fault (SHADOW_PRINTK) to make the
> distiction between X86EMUL_UNHANDLEABLE and X86EMUL_UNIMPLEMENTED.
>
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-09-25 12:03 ` [PATCH v13 1/3] x86emul: New return code for " Petre Pircalabu
` (2 preceding siblings ...)
2017-10-02 13:39 ` George Dunlap
@ 2017-10-02 13:43 ` George Dunlap
2017-10-02 14:09 ` George Dunlap
3 siblings, 1 reply; 17+ messages in thread
From: George Dunlap @ 2017-10-02 13:43 UTC (permalink / raw)
To: Petre Pircalabu, xen-devel
Cc: kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru,
konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
> Enforce the distinction between an instruction not implemented by the
> emulator and the failure to emulate that instruction by defining a new
> return code, X86EMUL_UNIMPLEMENTED.
>
> This value should only be returned by the core emulator only if it fails to
> properly decode the current instruction's opcode, and not by any of other
> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
Oh, minor comment: Should this paragraph be changed to match the
comment in the patch itself? I.e.:
"This value should only be returned by the core emulator when a valid
opcode is found but the execution logic for that instruction is missing.
It should NOT be returned by any of the x86_emulate_ops callbacks."
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-10-02 13:43 ` George Dunlap
@ 2017-10-02 14:09 ` George Dunlap
2017-10-02 14:42 ` Petre Ovidiu PIRCALABU
2017-10-04 11:11 ` Jan Beulich
0 siblings, 2 replies; 17+ messages in thread
From: George Dunlap @ 2017-10-02 14:09 UTC (permalink / raw)
To: Petre Pircalabu, xen-devel
Cc: kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru,
konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
On 10/02/2017 02:43 PM, George Dunlap wrote:
> On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
>> Enforce the distinction between an instruction not implemented by the
>> emulator and the failure to emulate that instruction by defining a new
>> return code, X86EMUL_UNIMPLEMENTED.
>>
>> This value should only be returned by the core emulator only if it fails to
>> properly decode the current instruction's opcode, and not by any of other
>> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
>
> Oh, minor comment: Should this paragraph be changed to match the
> comment in the patch itself? I.e.:
>
> "This value should only be returned by the core emulator when a valid
> opcode is found but the execution logic for that instruction is missing.
> It should NOT be returned by any of the x86_emulate_ops callbacks."
I'll do this on check-in if I don't hear any objections by tomorrow.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-10-02 14:09 ` George Dunlap
@ 2017-10-02 14:42 ` Petre Ovidiu PIRCALABU
2017-10-02 15:00 ` George Dunlap
2017-10-04 11:11 ` Jan Beulich
1 sibling, 1 reply; 17+ messages in thread
From: Petre Ovidiu PIRCALABU @ 2017-10-02 14:42 UTC (permalink / raw)
To: george.dunlap, xen-devel
Cc: kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru,
konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
On Lu, 2017-10-02 at 15:09 +0100, George Dunlap wrote:
> On 10/02/2017 02:43 PM, George Dunlap wrote:
> >
> > On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
> > >
> > > Enforce the distinction between an instruction not implemented by
> > > the
> > > emulator and the failure to emulate that instruction by defining
> > > a new
> > > return code, X86EMUL_UNIMPLEMENTED.
> > >
> > > This value should only be returned by the core emulator only if
> > > it fails to
> > > properly decode the current instruction's opcode, and not by any
> > > of other
> > > functions, such as the x86_emulate_ops or the hvm_io_ops
> > > callbacks.
> > Oh, minor comment: Should this paragraph be changed to match the
> > comment in the patch itself? I.e.:
> >
> > "This value should only be returned by the core emulator when a
> > valid
> > opcode is found but the execution logic for that instruction is
> > missing.
> > It should NOT be returned by any of the x86_emulate_ops callbacks."
I've changed the comment from the definition multiple times and I
forgot to update the patch comment. Indeed, the newest description
better reflects the current usage of X86EMUL_UNIMPLEMENTED.
> I'll do this on check-in if I don't hear any objections by tomorrow.
>
> -George
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-10-02 14:42 ` Petre Ovidiu PIRCALABU
@ 2017-10-02 15:00 ` George Dunlap
0 siblings, 0 replies; 17+ messages in thread
From: George Dunlap @ 2017-10-02 15:00 UTC (permalink / raw)
To: Petre Ovidiu PIRCALABU, xen-devel
Cc: kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru,
konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson, tim,
paul.durrant, tamas, jbeulich
On 10/02/2017 03:42 PM, Petre Ovidiu PIRCALABU wrote:
> On Lu, 2017-10-02 at 15:09 +0100, George Dunlap wrote:
>> On 10/02/2017 02:43 PM, George Dunlap wrote:
>>>
>>> On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
>>>>
>>>> Enforce the distinction between an instruction not implemented by
>>>> the
>>>> emulator and the failure to emulate that instruction by defining
>>>> a new
>>>> return code, X86EMUL_UNIMPLEMENTED.
>>>>
>>>> This value should only be returned by the core emulator only if
>>>> it fails to
>>>> properly decode the current instruction's opcode, and not by any
>>>> of other
>>>> functions, such as the x86_emulate_ops or the hvm_io_ops
>>>> callbacks.
>>> Oh, minor comment: Should this paragraph be changed to match the
>>> comment in the patch itself? I.e.:
>>>
>>> "This value should only be returned by the core emulator when a
>>> valid
>>> opcode is found but the execution logic for that instruction is
>>> missing.
>>> It should NOT be returned by any of the x86_emulate_ops callbacks."
> I've changed the comment from the definition multiple times and I
> forgot to update the patch comment. Indeed, the newest description
> better reflects the current usage of X86EMUL_UNIMPLEMENTED.
No worries, it happens. :-) Thanks again for all your effort with this,
and sorry for the late review.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-10-02 14:09 ` George Dunlap
2017-10-02 14:42 ` Petre Ovidiu PIRCALABU
@ 2017-10-04 11:11 ` Jan Beulich
2017-10-04 11:14 ` George Dunlap
1 sibling, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2017-10-04 11:11 UTC (permalink / raw)
To: George Dunlap
Cc: Petre Pircalabu, tim, kevin.tian, sstabellini, wei.liu2,
rcojocaru, konrad.wilk, George.Dunlap, andrew.cooper3,
ian.jackson, xen-devel, paul.durrant, tamas, jun.nakajima
>>> On 02.10.17 at 16:09, <george.dunlap@citrix.com> wrote:
> On 10/02/2017 02:43 PM, George Dunlap wrote:
>> On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
>>> Enforce the distinction between an instruction not implemented by the
>>> emulator and the failure to emulate that instruction by defining a new
>>> return code, X86EMUL_UNIMPLEMENTED.
>>>
>>> This value should only be returned by the core emulator only if it fails to
>>> properly decode the current instruction's opcode, and not by any of other
>>> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
>>
>> Oh, minor comment: Should this paragraph be changed to match the
>> comment in the patch itself? I.e.:
>>
>> "This value should only be returned by the core emulator when a valid
>> opcode is found but the execution logic for that instruction is missing.
>> It should NOT be returned by any of the x86_emulate_ops callbacks."
>
> I'll do this on check-in if I don't hear any objections by tomorrow.
This shouldn't really have gone in without a VMX maintainer ack.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-10-04 11:11 ` Jan Beulich
@ 2017-10-04 11:14 ` George Dunlap
2017-10-10 5:23 ` Tian, Kevin
0 siblings, 1 reply; 17+ messages in thread
From: George Dunlap @ 2017-10-04 11:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Petre Pircalabu, tim, kevin.tian, sstabellini, wei.liu2,
rcojocaru, konrad.wilk, George.Dunlap, andrew.cooper3,
ian.jackson, xen-devel, paul.durrant, tamas, jun.nakajima
On 10/04/2017 12:11 PM, Jan Beulich wrote:
>>>> On 02.10.17 at 16:09, <george.dunlap@citrix.com> wrote:
>> On 10/02/2017 02:43 PM, George Dunlap wrote:
>>> On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
>>>> Enforce the distinction between an instruction not implemented by the
>>>> emulator and the failure to emulate that instruction by defining a new
>>>> return code, X86EMUL_UNIMPLEMENTED.
>>>>
>>>> This value should only be returned by the core emulator only if it fails to
>>>> properly decode the current instruction's opcode, and not by any of other
>>>> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
>>>
>>> Oh, minor comment: Should this paragraph be changed to match the
>>> comment in the patch itself? I.e.:
>>>
>>> "This value should only be returned by the core emulator when a valid
>>> opcode is found but the execution logic for that instruction is missing.
>>> It should NOT be returned by any of the x86_emulate_ops callbacks."
>>
>> I'll do this on check-in if I don't hear any objections by tomorrow.
>
> This shouldn't really have gone in without a VMX maintainer ack.
Indeed -- sorry I missed that.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
2017-10-04 11:14 ` George Dunlap
@ 2017-10-10 5:23 ` Tian, Kevin
0 siblings, 0 replies; 17+ messages in thread
From: Tian, Kevin @ 2017-10-10 5:23 UTC (permalink / raw)
To: George Dunlap, Jan Beulich
Cc: Petre Pircalabu, tim, sstabellini, wei.liu2, rcojocaru,
konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson,
xen-devel, paul.durrant, tamas, Nakajima, Jun
> From: George Dunlap [mailto:george.dunlap@citrix.com]
> Sent: Wednesday, October 4, 2017 7:14 PM
>
> On 10/04/2017 12:11 PM, Jan Beulich wrote:
> >>>> On 02.10.17 at 16:09, <george.dunlap@citrix.com> wrote:
> >> On 10/02/2017 02:43 PM, George Dunlap wrote:
> >>> On 09/25/2017 01:03 PM, Petre Pircalabu wrote:
> >>>> Enforce the distinction between an instruction not implemented by
> the
> >>>> emulator and the failure to emulate that instruction by defining a new
> >>>> return code, X86EMUL_UNIMPLEMENTED.
> >>>>
> >>>> This value should only be returned by the core emulator only if it fails
> to
> >>>> properly decode the current instruction's opcode, and not by any of
> other
> >>>> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
> >>>
> >>> Oh, minor comment: Should this paragraph be changed to match the
> >>> comment in the patch itself? I.e.:
> >>>
> >>> "This value should only be returned by the core emulator when a valid
> >>> opcode is found but the execution logic for that instruction is missing.
> >>> It should NOT be returned by any of the x86_emulate_ops callbacks."
> >>
> >> I'll do this on check-in if I don't hear any objections by tomorrow.
> >
> > This shouldn't really have gone in without a VMX maintainer ack.
>
> Indeed -- sorry I missed that.
>
here comes:
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-10-10 5:23 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-25 12:03 [PATCH v13 0/3] Notify monitor when emulating an unimplemented instruction Petre Pircalabu
2017-09-25 12:03 ` [PATCH v13 1/3] x86emul: New return code for " Petre Pircalabu
2017-09-25 12:15 ` Paul Durrant
2017-09-26 13:53 ` Jan Beulich
2017-10-02 13:39 ` George Dunlap
2017-10-02 13:43 ` George Dunlap
2017-10-02 14:09 ` George Dunlap
2017-10-02 14:42 ` Petre Ovidiu PIRCALABU
2017-10-02 15:00 ` George Dunlap
2017-10-04 11:11 ` Jan Beulich
2017-10-04 11:14 ` George Dunlap
2017-10-10 5:23 ` Tian, Kevin
2017-09-25 12:03 ` [PATCH v13 2/3] x86emul: Add return code information to error messages Petre Pircalabu
2017-09-25 12:11 ` Paul Durrant
2017-10-02 13:41 ` George Dunlap
2017-09-25 12:03 ` [PATCH v13 3/3] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu
2017-09-25 12:13 ` Paul Durrant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.