| Message ID | 1505226727-5029-3-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Minor nit in commit message: On Tue, Sep 12, 2017 at 05:32:04PM +0300, Petre Pircalabu wrote: > Enforce the distinction between an instruction not implemented by the > emulator and the failure to emulate that instruction by defining a new > return code, X86EMUL_UNIMPLEMENTED. > > This value should only be returned by the core emulator only if it fails to > properly decode the current instruction's opcode, and not by any of other > functions, such as the x86_emulate_ops or the hvm_io_ops callbacks. > > e.g. hvm_process_io_intercept should not return X86EMUL_UNIMPLEMENTED. > The return value of this function depends on either the return code of > one of the hvm_io_ops handlers (read/write) or the value returned by > hvm_copy_guest_from_phys / hvm_copy_to_guest_phys. > > Similary, none of this functions should not return X86EMUL_UNIMPLEMENTED. "none of this... should not return..." is a double negative. > - hvm_io_intercept > - hvmemul_do_io > - hvm_send_buffered_ioreq > - hvm_send_ioreq > - hvm_broadcast_ioreq > - hvmemul_do_io_buffer > - hvmemul_validate > > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> > > --- > Changed since v10: > * Added asserts to make sure the return code cannot be X86EMUL_UNIMPLEMENTED. > * Add new return code (X86EMUL_UNRECOGNIZED) to be used when trying > to emulate an instruction with an invalid opcode. > --- > xen/arch/x86/hvm/emulate.c | 11 +++++++++ > xen/arch/x86/hvm/hvm.c | 1 + > xen/arch/x86/hvm/io.c | 1 + > xen/arch/x86/hvm/vmx/realmode.c | 2 +- > xen/arch/x86/mm/shadow/multi.c | 2 +- > xen/arch/x86/x86_emulate/x86_emulate.c | 45 ++++++++++++++++++---------------- > xen/arch/x86/x86_emulate/x86_emulate.h | 12 +++++++++ > 7 files changed, 51 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 54811c1..bf12593 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -192,6 +192,8 @@ static int hvmemul_do_io( > ASSERT(p.count <= *reps); > *reps = vio->io_req.count = p.count; > > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > + > switch ( rc ) > { > case X86EMUL_OKAY: > @@ -288,6 +290,8 @@ static int hvmemul_do_io( > BUG(); > } > > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > + > if ( rc != X86EMUL_OKAY ) > return rc; > > @@ -313,6 +317,9 @@ static int hvmemul_do_io_buffer( > > rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 0, > (uintptr_t)buffer); > + > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > + > if ( rc == X86EMUL_UNHANDLEABLE && dir == IOREQ_READ ) > memset(buffer, 0xff, size); > > @@ -405,6 +412,8 @@ static int hvmemul_do_io_addr( > rc = hvmemul_do_io(is_mmio, addr, &count, size, dir, df, 1, > ram_gpa); > > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > + > if ( rc == X86EMUL_OKAY ) > v->arch.hvm_vcpu.hvm_io.mmio_retry = (count < *reps); > > @@ -2045,6 +2054,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) > switch ( rc ) > { > case X86EMUL_UNHANDLEABLE: > + case X86EMUL_UNIMPLEMENTED: > hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt); > break; > case X86EMUL_EXCEPTION: > @@ -2102,6 +2112,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, > * consistent with X86EMUL_RETRY. > */ > return; > + case X86EMUL_UNIMPLEMENTED: > case X86EMUL_UNHANDLEABLE: > hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx); > hvm_inject_hw_exception(trapnr, errcode); > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 6cb903d..ea2812c 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3695,6 +3695,7 @@ void hvm_ud_intercept(struct cpu_user_regs *regs) > switch ( hvm_emulate_one(&ctxt) ) > { > case X86EMUL_UNHANDLEABLE: > + case X86EMUL_UNIMPLEMENTED: > hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC); > break; > case X86EMUL_EXCEPTION: > diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c > index bf41954..984db21 100644 > --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -96,6 +96,7 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr) > switch ( rc ) > { > case X86EMUL_UNHANDLEABLE: > + case X86EMUL_UNIMPLEMENTED: > hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt); > return false; > > diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c > index 11bde58..fdbbee2 100644 > --- a/xen/arch/x86/hvm/vmx/realmode.c > +++ b/xen/arch/x86/hvm/vmx/realmode.c > @@ -106,7 +106,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt) > if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry ) > vio->io_completion = HVMIO_realmode_completion; > > - if ( rc == X86EMUL_UNHANDLEABLE ) > + if ( rc == X86EMUL_UNHANDLEABLE || rc == X86EMUL_UNIMPLEMENTED ) > { > gdprintk(XENLOG_ERR, "Failed to emulate insn.\n"); > goto fail; > diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c > index 8d4f244..2557e21 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3488,7 +3488,7 @@ static int sh_page_fault(struct vcpu *v, > * would be a good unshadow hint. If we *do* decide to unshadow-on-fault > * then it must be 'failable': we cannot require the unshadow to succeed. > */ > - if ( r == X86EMUL_UNHANDLEABLE ) > + if ( r == X86EMUL_UNHANDLEABLE || r == X86EMUL_UNIMPLEMENTED ) > { > perfc_incr(shadow_fault_emulate_failed); > #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c > index c1e2300..ad97d93 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -848,7 +848,8 @@ do{ asm volatile ( \ > stub.func); \ > generate_exception_if(res_.fields.trapnr == EXC_UD, EXC_UD); \ > domain_crash(current->domain); \ > - goto cannot_emulate; \ > + rc = X86EMUL_UNHANDLEABLE; \ > + goto done; \ > } \ > } while (0) > #else > @@ -2585,7 +2586,7 @@ x86_decode( > d = twobyte_table[0x3a].desc; > break; > default: > - rc = X86EMUL_UNHANDLEABLE; > + rc = X86EMUL_UNIMPLEMENTED; > goto done; > } > } > @@ -2599,7 +2600,7 @@ x86_decode( > } > else > { > - rc = X86EMUL_UNHANDLEABLE; > + rc = X86EMUL_UNIMPLEMENTED; > goto done; > } > > @@ -2879,7 +2880,7 @@ x86_decode( > > default: > ASSERT_UNREACHABLE(); > - return X86EMUL_UNHANDLEABLE; > + return X86EMUL_UNIMPLEMENTED; > } > > if ( ea.type == OP_MEM ) > @@ -4191,7 +4192,7 @@ x86_emulate( > break; > case 4: /* fldenv - TODO */ > state->fpu_ctrl = true; > - goto cannot_emulate; > + goto unimplemented_insn; > case 5: /* fldcw m2byte */ > state->fpu_ctrl = true; > if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val, > @@ -4202,7 +4203,7 @@ x86_emulate( > break; > case 6: /* fnstenv - TODO */ > state->fpu_ctrl = true; > - goto cannot_emulate; > + goto unimplemented_insn; > case 7: /* fnstcw m2byte */ > state->fpu_ctrl = true; > emulate_fpu_insn_memdst("fnstcw", dst.val); > @@ -4438,7 +4439,7 @@ x86_emulate( > case 4: /* frstor - TODO */ > case 6: /* fnsave - TODO */ > state->fpu_ctrl = true; > - goto cannot_emulate; > + goto unimplemented_insn; > case 7: /* fnstsw m2byte */ > state->fpu_ctrl = true; > emulate_fpu_insn_memdst("fnstsw", dst.val); > @@ -5197,7 +5198,7 @@ x86_emulate( > #undef _GRP7 > > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } > break; > } > @@ -6195,7 +6196,7 @@ x86_emulate( > /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */ > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } > simd_0f_shift_imm: > generate_exception_if(ea.type != OP_REG, EXC_UD); > @@ -6243,7 +6244,7 @@ x86_emulate( > case 6: /* psllq $imm8,mm */ > goto simd_0f_shift_imm; > } > - goto cannot_emulate; > + goto unimplemented_insn; > > case X86EMUL_OPC_66(0x0f, 0x73): > case X86EMUL_OPC_VEX_66(0x0f, 0x73): > @@ -6259,7 +6260,7 @@ x86_emulate( > /* vpslldq $imm8,{x,y}mm,{x,y}mm */ > goto simd_0f_shift_imm; > } > - goto cannot_emulate; > + goto unimplemented_insn; > > case X86EMUL_OPC(0x0f, 0x77): /* emms */ > case X86EMUL_OPC_VEX(0x0f, 0x77): /* vzero{all,upper} */ > @@ -6323,7 +6324,7 @@ x86_emulate( > case 0: /* extrq $imm8,$imm8,xmm */ > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } > /* fall through */ > case X86EMUL_OPC_F2(0x0f, 0x78): /* insertq $imm8,$imm8,xmm,xmm */ > @@ -6518,7 +6519,8 @@ x86_emulate( > goto done; > break; > default: > - goto cannot_emulate; > + rc = X86EMUL_UNHANDLEABLE; > + goto done; > } > break; > > @@ -6534,7 +6536,7 @@ x86_emulate( > vcpu_must_have(avx); > goto stmxcsr; > } > - goto cannot_emulate; > + goto unimplemented_insn; > > case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ > fail_if(modrm_mod != 3); > @@ -6777,7 +6779,7 @@ x86_emulate( > switch ( modrm_reg & 7 ) > { > default: > - goto cannot_emulate; > + goto unimplemented_insn; > > #ifdef HAVE_GAS_RDRAND > case 6: /* rdrand */ > @@ -7359,7 +7361,7 @@ x86_emulate( > host_and_vcpu_must_have(bmi1); > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } > > generate_exception_if(vex.l, EXC_UD); > @@ -7670,7 +7672,7 @@ x86_emulate( > host_and_vcpu_must_have(tbm); > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } > > xop_09_rm_rv: > @@ -7704,7 +7706,7 @@ x86_emulate( > host_and_vcpu_must_have(tbm); > goto xop_09_rm_rv; > } > - goto cannot_emulate; > + goto unimplemented_insn; > > case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */ > { > @@ -7736,8 +7738,8 @@ x86_emulate( > } > > default: > - cannot_emulate: > - rc = X86EMUL_UNHANDLEABLE; > + unimplemented_insn: > + rc = X86EMUL_UNIMPLEMENTED; > goto done; > } > > @@ -7789,7 +7791,8 @@ x86_emulate( > if ( (d & DstMask) != DstMem ) > { > ASSERT_UNREACHABLE(); > - goto cannot_emulate; > + rc = X86EMUL_UNHANDLEABLE; > + goto done; > } > break; > } > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h > index 4ddf111..2fc19e0 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -133,6 +133,18 @@ struct x86_emul_fpu_aux { > * Undefined behavior when used anywhere else. > */ > #define X86EMUL_DONE 4 > + /* > + * Current instruction is not implemented by the emulator. > + * This value should only be returned by the core emulator if decode fails > + * and not by any of the x86_emulate_ops callbacks. > + * If this error code is returned by a function, an #UD trap should be > + * raised by the final consumer of it. > + */ > +#define X86EMUL_UNIMPLEMENTED 5 > +/* > + * The current instruction's opcode is not valid. > + */ > +#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED > > /* FPU sub-types which may be requested via ->get_fpu(). */ > enum x86_emulate_fpu_type { > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote: > Enforce the distinction between an instruction not implemented by the > emulator and the failure to emulate that instruction by defining a new > return code, X86EMUL_UNIMPLEMENTED. > > This value should only be returned by the core emulator only if it fails to > properly decode the current instruction's opcode, and not by any of other > functions, such as the x86_emulate_ops or the hvm_io_ops callbacks. > > e.g. hvm_process_io_intercept should not return X86EMUL_UNIMPLEMENTED. > The return value of this function depends on either the return code of > one of the hvm_io_ops handlers (read/write) or the value returned by > hvm_copy_guest_from_phys / hvm_copy_to_guest_phys. > > Similary, none of this functions should not return X86EMUL_UNIMPLEMENTED. I think someone had already pointed out the strange double negation here. > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -192,6 +192,8 @@ static int hvmemul_do_io( > ASSERT(p.count <= *reps); > *reps = vio->io_req.count = p.count; > > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > + > switch ( rc ) > { > case X86EMUL_OKAY: The assertion want to move into the switch(), making use of ASSERT_UNREACHABLE(). > @@ -2045,6 +2054,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) > switch ( rc ) > { > case X86EMUL_UNHANDLEABLE: > + case X86EMUL_UNIMPLEMENTED: > hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt); > break; I would have preferred if, just like you do here, ... > @@ -2102,6 +2112,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, > * consistent with X86EMUL_RETRY. > */ > return; > + case X86EMUL_UNIMPLEMENTED: > case X86EMUL_UNHANDLEABLE: > hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx); ... you had added the new case label below existing ones uniformly. But anyway. > @@ -2585,7 +2586,7 @@ x86_decode( > d = twobyte_table[0x3a].desc; > break; > default: > - rc = X86EMUL_UNHANDLEABLE; > + rc = X86EMUL_UNIMPLEMENTED; > goto done; > } > } > @@ -2599,7 +2600,7 @@ x86_decode( > } > else > { > - rc = X86EMUL_UNHANDLEABLE; > + rc = X86EMUL_UNIMPLEMENTED; > goto done; At least these two should be "unrecognized" now. > @@ -2879,7 +2880,7 @@ x86_decode( > > default: > ASSERT_UNREACHABLE(); > - return X86EMUL_UNHANDLEABLE; > + return X86EMUL_UNIMPLEMENTED; > } This one, otoh, is probably fine this way for now. > @@ -6195,7 +6196,7 @@ x86_emulate( > /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */ > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } This again wants to be "unrecognized". > @@ -6243,7 +6244,7 @@ x86_emulate( > case 6: /* psllq $imm8,mm */ > goto simd_0f_shift_imm; > } > - goto cannot_emulate; > + goto unimplemented_insn; And this too. Together with previous discussion I think you should now see the pattern for everything further down from here. > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -133,6 +133,18 @@ struct x86_emul_fpu_aux { > * Undefined behavior when used anywhere else. > */ > #define X86EMUL_DONE 4 > + /* > + * Current instruction is not implemented by the emulator. > + * This value should only be returned by the core emulator if decode fails Why "if decode fails"? In that case it's more "unrecognized" than "unimplemented"; the latter can only ever arise (long term, i.e. once we have proper distinction of the two) if we successfully decoded an insn, but have no code to actually handle it. > + * and not by any of the x86_emulate_ops callbacks. > + * If this error code is returned by a function, an #UD trap should be > + * raised by the final consumer of it. This last sentence would now really belong to X86EMUL_UNRECOGNIZED. As explained earlier, raising #UD for unimplemented is precisely the wrong choice architecturally, we merely tolerate doing so for the time being. Jan
On Ma, 2017-09-19 at 09:19 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote: > > Enforce the distinction between an instruction not implemented by > > the > > emulator and the failure to emulate that instruction by defining a > > new > > return code, X86EMUL_UNIMPLEMENTED. > > > > This value should only be returned by the core emulator only if it > > fails to > > properly decode the current instruction's opcode, and not by any of > > other > > functions, such as the x86_emulate_ops or the hvm_io_ops callbacks. > > > > e.g. hvm_process_io_intercept should not return > > X86EMUL_UNIMPLEMENTED. > > The return value of this function depends on either the return code > > of > > one of the hvm_io_ops handlers (read/write) or the value returned > > by > > hvm_copy_guest_from_phys / hvm_copy_to_guest_phys. > > > > Similary, none of this functions should not return > > X86EMUL_UNIMPLEMENTED. > I think someone had already pointed out the strange double > negation here. Will be fixed in the next patchset iteration. > > > > > --- a/xen/arch/x86/hvm/emulate.c > > +++ b/xen/arch/x86/hvm/emulate.c > > @@ -192,6 +192,8 @@ static int hvmemul_do_io( > > ASSERT(p.count <= *reps); > > *reps = vio->io_req.count = p.count; > > > > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > > + > > switch ( rc ) > > { > > case X86EMUL_OKAY: > The assertion want to move into the switch(), making use > of ASSERT_UNREACHABLE(). > Will be fixed in the next patchset iteration. > > > > @@ -2045,6 +2054,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, > > unsigned long gla) > > switch ( rc ) > > { > > case X86EMUL_UNHANDLEABLE: > > + case X86EMUL_UNIMPLEMENTED: > > hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", > > &ctxt); > > break; > I would have preferred if, just like you do here, ... > > > > > @@ -2102,6 +2112,7 @@ void hvm_emulate_one_vm_event(enum emul_kind > > kind, unsigned int trapnr, > > * consistent with X86EMUL_RETRY. > > */ > > return; > > + case X86EMUL_UNIMPLEMENTED: > > case X86EMUL_UNHANDLEABLE: > > hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", > > &ctx); > ... you had added the new case label below existing ones uniformly. > But anyway. The order is reversed in this case because the patch 4/5 from this series adds the hvm_monitor_emul_unimplemented call and the "Fall- through" in case of failure. If I change the default order here, I will have to reverse it when adding the monitor notification handling. > > > > > @@ -2585,7 +2586,7 @@ x86_decode( > > d = twobyte_table[0x3a].desc; > > break; > > default: > > - rc = X86EMUL_UNHANDLEABLE; > > + rc = X86EMUL_UNIMPLEMENTED; > > goto done; > > } > > } > > @@ -2599,7 +2600,7 @@ x86_decode( > > } > > else > > { > > - rc = X86EMUL_UNHANDLEABLE; > > + rc = X86EMUL_UNIMPLEMENTED; > > goto done; > At least these two should be "unrecognized" now. Will be fixed in the next patchset iteration. > > > > > @@ -2879,7 +2880,7 @@ x86_decode( > > > > default: > > ASSERT_UNREACHABLE(); > > - return X86EMUL_UNHANDLEABLE; > > + return X86EMUL_UNIMPLEMENTED; > > } > This one, otoh, is probably fine this way for now. > > > > > @@ -6195,7 +6196,7 @@ x86_emulate( > > /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */ > > break; > > default: > > - goto cannot_emulate; > > + goto unimplemented_insn; > > } > This again wants to be "unrecognized". In this case "unrecognized" cannot be returned (yet) as instructions VPRORD and VPROLD are not implemented. http://sandpile.org/x86/opc_grp.htm (group #13 (EVEX 66h) (0Fh,72h) ) > > > > > @@ -6243,7 +6244,7 @@ x86_emulate( > > case 6: /* psllq $imm8,mm */ > > goto simd_0f_shift_imm; > > } > > - goto cannot_emulate; > > + goto unimplemented_insn; > And this too. Together with previous discussion I think you should > now see the pattern for everything further down from here. Will be fixed in the next patchset iteration. > > > > > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > > @@ -133,6 +133,18 @@ struct x86_emul_fpu_aux { > > * Undefined behavior when used anywhere else. > > */ > > #define X86EMUL_DONE 4 > > + /* > > + * Current instruction is not implemented by the emulator. > > + * This value should only be returned by the core emulator if > > decode fails > Why "if decode fails"? In that case it's more "unrecognized" than > "unimplemented"; the latter can only ever arise (long term, i.e. > once we have proper distinction of the two) if we successfully > decoded an insn, but have no code to actually handle it. > > > > > + * and not by any of the x86_emulate_ops callbacks. > > + * If this error code is returned by a function, an #UD trap > > should be > > + * raised by the final consumer of it. > This last sentence would now really belong to > X86EMUL_UNRECOGNIZED. As explained earlier, raising #UD > for unimplemented is precisely the wrong choice architecturally, > we merely tolerate doing so for the time being. > Will be fixed in the next patchset iteration. > Jan > > ________________________ > This email was scanned by Bitdefender Many thanks for your support, //Petre ________________________ This email was scanned by Bitdefender
>>> On 20.09.17 at 23:47, <ppircalabu@bitdefender.com> wrote: > On Ma, 2017-09-19 at 09:19 -0600, Jan Beulich wrote: >> > > > On 12.09.17 at 16:32, <ppircalabu@bitdefender.com> wrote: >> > @@ -2102,6 +2112,7 @@ void hvm_emulate_one_vm_event(enum emul_kind >> > kind, unsigned int trapnr, >> > * consistent with X86EMUL_RETRY. >> > */ >> > return; >> > + case X86EMUL_UNIMPLEMENTED: >> > case X86EMUL_UNHANDLEABLE: >> > hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", >> > &ctx); >> ... you had added the new case label below existing ones uniformly. >> But anyway. > The order is reversed in this case because the patch 4/5 from this > series adds the hvm_monitor_emul_unimplemented call and the "Fall- > through" in case of failure. If I change the default order here, I will > have to reverse it when adding the monitor notification handling. Ah, okay, that's fine then. >> > @@ -6195,7 +6196,7 @@ x86_emulate( >> > /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */ >> > break; >> > default: >> > - goto cannot_emulate; >> > + goto unimplemented_insn; >> > } >> This again wants to be "unrecognized". > In this case "unrecognized" cannot be returned (yet) as instructions > VPRORD and VPROLD are not implemented. > http://sandpile.org/x86/opc_grp.htm (group #13 (EVEX 66h) (0Fh,72h) ) As you say yourself - these are EVEX-prefixed. We don't support any AVX-512 instructions so far. As you're certainly aware even the completion of AVX, AVX2, etc is pending review (and now unlikely to make it into 4.10). Hence the "unimplemented" status is supposed to be coming from a much earlier (decode stage) place for these insns. Jan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 54811c1..bf12593 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -192,6 +192,8 @@ static int hvmemul_do_io( ASSERT(p.count <= *reps); *reps = vio->io_req.count = p.count; + ASSERT(rc != X86EMUL_UNIMPLEMENTED); + switch ( rc ) { case X86EMUL_OKAY: @@ -288,6 +290,8 @@ static int hvmemul_do_io( BUG(); } + ASSERT(rc != X86EMUL_UNIMPLEMENTED); + if ( rc != X86EMUL_OKAY ) return rc; @@ -313,6 +317,9 @@ static int hvmemul_do_io_buffer( rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 0, (uintptr_t)buffer); + + ASSERT(rc != X86EMUL_UNIMPLEMENTED); + if ( rc == X86EMUL_UNHANDLEABLE && dir == IOREQ_READ ) memset(buffer, 0xff, size); @@ -405,6 +412,8 @@ static int hvmemul_do_io_addr( rc = hvmemul_do_io(is_mmio, addr, &count, size, dir, df, 1, ram_gpa); + ASSERT(rc != X86EMUL_UNIMPLEMENTED); + if ( rc == X86EMUL_OKAY ) v->arch.hvm_vcpu.hvm_io.mmio_retry = (count < *reps); @@ -2045,6 +2054,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) switch ( rc ) { case X86EMUL_UNHANDLEABLE: + case X86EMUL_UNIMPLEMENTED: hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt); break; case X86EMUL_EXCEPTION: @@ -2102,6 +2112,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, * consistent with X86EMUL_RETRY. */ return; + case X86EMUL_UNIMPLEMENTED: case X86EMUL_UNHANDLEABLE: hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx); hvm_inject_hw_exception(trapnr, errcode); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 6cb903d..ea2812c 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3695,6 +3695,7 @@ void hvm_ud_intercept(struct cpu_user_regs *regs) switch ( hvm_emulate_one(&ctxt) ) { case X86EMUL_UNHANDLEABLE: + case X86EMUL_UNIMPLEMENTED: hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC); break; case X86EMUL_EXCEPTION: diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index bf41954..984db21 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -96,6 +96,7 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr) switch ( rc ) { case X86EMUL_UNHANDLEABLE: + case X86EMUL_UNIMPLEMENTED: hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt); return false; diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c index 11bde58..fdbbee2 100644 --- a/xen/arch/x86/hvm/vmx/realmode.c +++ b/xen/arch/x86/hvm/vmx/realmode.c @@ -106,7 +106,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt) if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry ) vio->io_completion = HVMIO_realmode_completion; - if ( rc == X86EMUL_UNHANDLEABLE ) + if ( rc == X86EMUL_UNHANDLEABLE || rc == X86EMUL_UNIMPLEMENTED ) { gdprintk(XENLOG_ERR, "Failed to emulate insn.\n"); goto fail; diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index 8d4f244..2557e21 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3488,7 +3488,7 @@ static int sh_page_fault(struct vcpu *v, * would be a good unshadow hint. If we *do* decide to unshadow-on-fault * then it must be 'failable': we cannot require the unshadow to succeed. */ - if ( r == X86EMUL_UNHANDLEABLE ) + if ( r == X86EMUL_UNHANDLEABLE || r == X86EMUL_UNIMPLEMENTED ) { perfc_incr(shadow_fault_emulate_failed); #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index c1e2300..ad97d93 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -848,7 +848,8 @@ do{ asm volatile ( \ stub.func); \ generate_exception_if(res_.fields.trapnr == EXC_UD, EXC_UD); \ domain_crash(current->domain); \ - goto cannot_emulate; \ + rc = X86EMUL_UNHANDLEABLE; \ + goto done; \ } \ } while (0) #else @@ -2585,7 +2586,7 @@ x86_decode( d = twobyte_table[0x3a].desc; break; default: - rc = X86EMUL_UNHANDLEABLE; + rc = X86EMUL_UNIMPLEMENTED; goto done; } } @@ -2599,7 +2600,7 @@ x86_decode( } else { - rc = X86EMUL_UNHANDLEABLE; + rc = X86EMUL_UNIMPLEMENTED; goto done; } @@ -2879,7 +2880,7 @@ x86_decode( default: ASSERT_UNREACHABLE(); - return X86EMUL_UNHANDLEABLE; + return X86EMUL_UNIMPLEMENTED; } if ( ea.type == OP_MEM ) @@ -4191,7 +4192,7 @@ x86_emulate( break; case 4: /* fldenv - TODO */ state->fpu_ctrl = true; - goto cannot_emulate; + goto unimplemented_insn; case 5: /* fldcw m2byte */ state->fpu_ctrl = true; if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val, @@ -4202,7 +4203,7 @@ x86_emulate( break; case 6: /* fnstenv - TODO */ state->fpu_ctrl = true; - goto cannot_emulate; + goto unimplemented_insn; case 7: /* fnstcw m2byte */ state->fpu_ctrl = true; emulate_fpu_insn_memdst("fnstcw", dst.val); @@ -4438,7 +4439,7 @@ x86_emulate( case 4: /* frstor - TODO */ case 6: /* fnsave - TODO */ state->fpu_ctrl = true; - goto cannot_emulate; + goto unimplemented_insn; case 7: /* fnstsw m2byte */ state->fpu_ctrl = true; emulate_fpu_insn_memdst("fnstsw", dst.val); @@ -5197,7 +5198,7 @@ x86_emulate( #undef _GRP7 default: - goto cannot_emulate; + goto unimplemented_insn; } break; } @@ -6195,7 +6196,7 @@ x86_emulate( /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */ break; default: - goto cannot_emulate; + goto unimplemented_insn; } simd_0f_shift_imm: generate_exception_if(ea.type != OP_REG, EXC_UD); @@ -6243,7 +6244,7 @@ x86_emulate( case 6: /* psllq $imm8,mm */ goto simd_0f_shift_imm; } - goto cannot_emulate; + goto unimplemented_insn; case X86EMUL_OPC_66(0x0f, 0x73): case X86EMUL_OPC_VEX_66(0x0f, 0x73): @@ -6259,7 +6260,7 @@ x86_emulate( /* vpslldq $imm8,{x,y}mm,{x,y}mm */ goto simd_0f_shift_imm; } - goto cannot_emulate; + goto unimplemented_insn; case X86EMUL_OPC(0x0f, 0x77): /* emms */ case X86EMUL_OPC_VEX(0x0f, 0x77): /* vzero{all,upper} */ @@ -6323,7 +6324,7 @@ x86_emulate( case 0: /* extrq $imm8,$imm8,xmm */ break; default: - goto cannot_emulate; + goto unimplemented_insn; } /* fall through */ case X86EMUL_OPC_F2(0x0f, 0x78): /* insertq $imm8,$imm8,xmm,xmm */ @@ -6518,7 +6519,8 @@ x86_emulate( goto done; break; default: - goto cannot_emulate; + rc = X86EMUL_UNHANDLEABLE; + goto done; } break; @@ -6534,7 +6536,7 @@ x86_emulate( vcpu_must_have(avx); goto stmxcsr; } - goto cannot_emulate; + goto unimplemented_insn; case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ fail_if(modrm_mod != 3); @@ -6777,7 +6779,7 @@ x86_emulate( switch ( modrm_reg & 7 ) { default: - goto cannot_emulate; + goto unimplemented_insn; #ifdef HAVE_GAS_RDRAND case 6: /* rdrand */ @@ -7359,7 +7361,7 @@ x86_emulate( host_and_vcpu_must_have(bmi1); break; default: - goto cannot_emulate; + goto unimplemented_insn; } generate_exception_if(vex.l, EXC_UD); @@ -7670,7 +7672,7 @@ x86_emulate( host_and_vcpu_must_have(tbm); break; default: - goto cannot_emulate; + goto unimplemented_insn; } xop_09_rm_rv: @@ -7704,7 +7706,7 @@ x86_emulate( host_and_vcpu_must_have(tbm); goto xop_09_rm_rv; } - goto cannot_emulate; + goto unimplemented_insn; case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */ { @@ -7736,8 +7738,8 @@ x86_emulate( } default: - cannot_emulate: - rc = X86EMUL_UNHANDLEABLE; + unimplemented_insn: + rc = X86EMUL_UNIMPLEMENTED; goto done; } @@ -7789,7 +7791,8 @@ x86_emulate( if ( (d & DstMask) != DstMem ) { ASSERT_UNREACHABLE(); - goto cannot_emulate; + rc = X86EMUL_UNHANDLEABLE; + goto done; } break; } diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index 4ddf111..2fc19e0 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -133,6 +133,18 @@ struct x86_emul_fpu_aux { * Undefined behavior when used anywhere else. */ #define X86EMUL_DONE 4 + /* + * Current instruction is not implemented by the emulator. + * This value should only be returned by the core emulator if decode fails + * and not by any of the x86_emulate_ops callbacks. + * If this error code is returned by a function, an #UD trap should be + * raised by the final consumer of it. + */ +#define X86EMUL_UNIMPLEMENTED 5 +/* + * The current instruction's opcode is not valid. + */ +#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED /* FPU sub-types which may be requested via ->get_fpu(). */ enum x86_emulate_fpu_type {
Enforce the distinction between an instruction not implemented by the emulator and the failure to emulate that instruction by defining a new return code, X86EMUL_UNIMPLEMENTED. This value should only be returned by the core emulator only if it fails to properly decode the current instruction's opcode, and not by any of other functions, such as the x86_emulate_ops or the hvm_io_ops callbacks. e.g. hvm_process_io_intercept should not return X86EMUL_UNIMPLEMENTED. The return value of this function depends on either the return code of one of the hvm_io_ops handlers (read/write) or the value returned by hvm_copy_guest_from_phys / hvm_copy_to_guest_phys. Similary, none of this functions should not return X86EMUL_UNIMPLEMENTED. - hvm_io_intercept - hvmemul_do_io - hvm_send_buffered_ioreq - hvm_send_ioreq - hvm_broadcast_ioreq - hvmemul_do_io_buffer - hvmemul_validate Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> --- Changed since v10: * Added asserts to make sure the return code cannot be X86EMUL_UNIMPLEMENTED. * Add new return code (X86EMUL_UNRECOGNIZED) to be used when trying to emulate an instruction with an invalid opcode. --- xen/arch/x86/hvm/emulate.c | 11 +++++++++ xen/arch/x86/hvm/hvm.c | 1 + xen/arch/x86/hvm/io.c | 1 + xen/arch/x86/hvm/vmx/realmode.c | 2 +- xen/arch/x86/mm/shadow/multi.c | 2 +- xen/arch/x86/x86_emulate/x86_emulate.c | 45 ++++++++++++++++++---------------- xen/arch/x86/x86_emulate/x86_emulate.h | 12 +++++++++ 7 files changed, 51 insertions(+), 23 deletions(-)