| Message ID | 1505970726-5671-2-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com] > Sent: 21 September 2017 06:12 > 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 v12 1/4] 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 > > 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 ) > --- > xen/arch/x86/hvm/emulate.c | 12 ++++++++ > 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 | 52 ++++++++++++++++++++-- > ------------ > xen/arch/x86/x86_emulate/x86_emulate.h | 13 +++++++++ > 7 files changed, 60 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 */ Kind of surprised you need the fall-through if you assert the code is unreachable... but analysis tools can be a bit temperamental so ok. > default: > BUG(); > } > > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > + Isn't this assertion redundant given the ASSERT_UNREACHABLE() above? Paul > 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 93394c1..43ff5aa 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3723,6 +3723,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 12d43ad..cf48139 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..f334de9 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 unimplemented_insn; > } > simd_0f_shift_imm: > generate_exception_if(ea.type != OP_REG, EXC_UD); > @@ -6243,7 +6244,8 @@ x86_emulate( > case 6: /* psllq $imm8,mm */ > goto simd_0f_shift_imm; > } > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > > case X86EMUL_OPC_66(0x0f, 0x73): > case X86EMUL_OPC_VEX_66(0x0f, 0x73): > @@ -6259,7 +6261,8 @@ x86_emulate( > /* vpslldq $imm8,{x,y}mm,{x,y}mm */ > goto simd_0f_shift_imm; > } > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > > case X86EMUL_OPC(0x0f, 0x77): /* emms */ > case X86EMUL_OPC_VEX(0x0f, 0x77): /* vzero{all,upper} */ > @@ -6323,7 +6326,8 @@ x86_emulate( > case 0: /* extrq $imm8,$imm8,xmm */ > break; > default: > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > } > /* fall through */ > case X86EMUL_OPC_F2(0x0f, 0x78): /* insertq $imm8,$imm8,xmm,xmm > */ > @@ -6518,7 +6522,7 @@ x86_emulate( > goto done; > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } > break; > > @@ -6534,7 +6538,8 @@ x86_emulate( > vcpu_must_have(avx); > goto stmxcsr; > } > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > > case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ > fail_if(modrm_mod != 3); > @@ -6777,7 +6782,8 @@ x86_emulate( > switch ( modrm_reg & 7 ) > { > default: > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > > #ifdef HAVE_GAS_RDRAND > case 6: /* rdrand */ > @@ -7359,7 +7365,8 @@ x86_emulate( > host_and_vcpu_must_have(bmi1); > break; > default: > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > } > > generate_exception_if(vex.l, EXC_UD); > @@ -7670,7 +7677,8 @@ x86_emulate( > host_and_vcpu_must_have(tbm); > break; > default: > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > } > > xop_09_rm_rv: > @@ -7704,7 +7712,8 @@ x86_emulate( > host_and_vcpu_must_have(tbm); > goto xop_09_rm_rv; > } > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > > case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */ > { > @@ -7736,8 +7745,8 @@ x86_emulate( > } > > default: > - cannot_emulate: > - rc = X86EMUL_UNHANDLEABLE; > + unimplemented_insn: > + rc = X86EMUL_UNIMPLEMENTED; > goto done; > } > > @@ -7789,7 +7798,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..1fb74c0 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -133,6 +133,19 @@ 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. > + */ > +#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED > > /* FPU sub-types which may be requested via ->get_fpu(). */ > enum x86_emulate_fpu_type { > -- > 2.7.4
>>> On 21.09.17 at 07:12, <ppircalabu@bitdefender.com> wrote: > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -6195,7 +6196,7 @@ x86_emulate( > /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */ > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; I would really appreciate if you were a little more patient and waited for replies to earlier review threads before sending a new version. As said on the v11 thread, this ought to be "unrecognized". > @@ -6243,7 +6244,8 @@ x86_emulate( > case 6: /* psllq $imm8,mm */ > goto simd_0f_shift_imm; > } > - goto cannot_emulate; > + rc = X86EMUL_UNRECOGNIZED; > + goto done; I think it would read better if we had an "unrecognized_insn" label just like now we gain an "unimplemented_insn" one. Whether the _insn suffixes are really useful is another question. > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -133,6 +133,19 @@ 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. > + */ > +#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED But with this aliasing of values the comment still is somewhat off. Jan
>>> On 21.09.17 at 07:12, <ppircalabu@bitdefender.com> wrote: > 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. This is not entirely correct, btw (and hence the code change isn't either): The code there has a dependency on gas features, so hypervisor builds may not include support for rdrand and/or rdseed. You will want to add "#else" cases producing "unimplemented". Jan
On Thu, 2017-09-21 at 08:53 +0000, Paul Durrant wrote: > > } > > + case X86EMUL_UNIMPLEMENTED: > > + ASSERT_UNREACHABLE(); > > + /* Fall-through */ > > Kind of surprised you need the fall-through if you assert the code is > unreachable... but analysis tools can be a bit temperamental so ok. > > > default: > > BUG(); > > } > > > > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > > + > > Isn't this assertion redundant given the ASSERT_UNREACHABLE() above? > > > > Paul The second ASSERT statement is used to make sure the return value of hvm_process_io_intercept or hvm_send_ioreq (called from the "case X86EMUL_UNHANDLEABLE:" branch of the switch statement above) cannot be X86EMUL_UNIMPLEMENTED. > hvm_process_io_intercept > > if ( rc != X86EMUL_OKAY ) > > return rc; > > > > //Petre ________________________ This email was scanned by Bitdefender
> -----Original Message----- > From: Petre Ovidiu PIRCALABU [mailto:ppircalabu@bitdefender.com] > Sent: 23 September 2017 19:57 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xen.org > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu > <wei.liu2@citrix.com>; sstabellini@kernel.org; Ian Jackson > <Ian.Jackson@citrix.com>; rcojocaru@bitdefender.com; > konrad.wilk@oracle.com; George Dunlap <George.Dunlap@citrix.com>; > Kevin Tian <kevin.tian@intel.com>; tamas@tklengyel.com; > jbeulich@suse.com; jun.nakajima@intel.com; Tim (Xen.org) <tim@xen.org> > Subject: Re: [PATCH v12 1/4] x86emul: New return code for unimplemented > instruction > > On Thu, 2017-09-21 at 08:53 +0000, Paul Durrant wrote: > > > } > > > + case X86EMUL_UNIMPLEMENTED: > > > + ASSERT_UNREACHABLE(); > > > + /* Fall-through */ > > > > Kind of surprised you need the fall-through if you assert the code is > > unreachable... but analysis tools can be a bit temperamental so ok. > > > > > default: > > > BUG(); > > > } > > > > > > + ASSERT(rc != X86EMUL_UNIMPLEMENTED); > > > + > > > > Isn't this assertion redundant given the ASSERT_UNREACHABLE() above? > > > > > > > Paul > > The second ASSERT statement is used to make sure the return value of > hvm_process_io_intercept or hvm_send_ioreq (called from the "case > X86EMUL_UNHANDLEABLE:" branch of the switch statement above) cannot > be X86EMUL_UNIMPLEMENTED. Ah, ok. Just out of context in the patch. Paul > > > hvm_process_io_intercept > > > if ( rc != X86EMUL_OKAY ) > > > return rc; > > > > > > > //Petre > > ________________________ > This email was scanned by Bitdefender
On Jo, 2017-09-21 at 06:42 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 21.09.17 at 07:12, <ppircalabu@bitdefender.com> wrote: > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > > @@ -6195,7 +6196,7 @@ x86_emulate( > > /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */ > > break; > > default: > > - goto cannot_emulate; > > + goto unimplemented_insn; > I would really appreciate if you were a little more patient and > waited > for replies to earlier review threads before sending a new version. > As said on the v11 thread, this ought to be "unrecognized". Thank-you very much for clearing this out. I will change the return value to "unrecognized" in the next patchset iteration. > > > > > @@ -6243,7 +6244,8 @@ x86_emulate( > > case 6: /* psllq $imm8,mm */ > > goto simd_0f_shift_imm; > > } > > - goto cannot_emulate; > > + rc = X86EMUL_UNRECOGNIZED; > > + goto done; > I think it would read better if we had an "unrecognized_insn" > label just like now we gain an "unimplemented_insn" one. Whether > the _insn suffixes are really useful is another question. The best place to add this label is, in my opinion, at the end of the "default" label of the "switch ( ctxt->opcode )" statement in x86_emulate. This will make sure the current instruction flow will not be altered. e.g.: case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */ { @@ -7750,6 +7742,9 @@ x86_emulate( unimplemented_insn: rc = X86EMUL_UNIMPLEMENTED; goto done; + unrecognized_insn: + rc = X86EMUL_UNRECOGNIZED; + goto done; } Do you find this approach OK? > > > > > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > > @@ -133,6 +133,19 @@ 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. > > + */ > > +#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED > But with this aliasing of values the comment still is somewhat off. Do you think adding a "TODO:" comment can make things more clear? e.g.: * The current instruction's opcode is not valid. * If this error code is returned by a function, an #UD trap should e * raised by the final consumer of it. + * + * TODO: For the moment X86EMUL_UNRECOGNIZED and 86EMUL_UNIMPLEMENTED + * can be used interchangeably. */ Many thanks for your support, //Petre > > Jan > > > ________________________ > This email was scanned by Bitdefender
>>> On 25.09.17 at 11:16, <ppircalabu@bitdefender.com> wrote: > @@ -7750,6 +7742,9 @@ x86_emulate( > unimplemented_insn: > rc = X86EMUL_UNIMPLEMENTED; > goto done; > + unrecognized_insn: > + rc = X86EMUL_UNRECOGNIZED; > + goto done; > } > Do you find this approach OK? Yes, that's reasonable I think. > Do you think adding a "TODO:" comment can make things more clear? > e.g.: > * The current instruction's opcode is not valid. > * If this error code is returned by a function, an #UD trap should e > * raised by the final consumer of it. > + * > + * TODO: For the moment X86EMUL_UNRECOGNIZED and 86EMUL_UNIMPLEMENTED > + * can be used interchangeably. > */ Something along these lines, yes, plus an indication that the #UD raising therefore also isn't strictly being expected for now. Jan
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 93394c1..43ff5aa 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3723,6 +3723,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 12d43ad..cf48139 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..f334de9 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 unimplemented_insn; } simd_0f_shift_imm: generate_exception_if(ea.type != OP_REG, EXC_UD); @@ -6243,7 +6244,8 @@ x86_emulate( case 6: /* psllq $imm8,mm */ goto simd_0f_shift_imm; } - goto cannot_emulate; + rc = X86EMUL_UNRECOGNIZED; + goto done; case X86EMUL_OPC_66(0x0f, 0x73): case X86EMUL_OPC_VEX_66(0x0f, 0x73): @@ -6259,7 +6261,8 @@ x86_emulate( /* vpslldq $imm8,{x,y}mm,{x,y}mm */ goto simd_0f_shift_imm; } - goto cannot_emulate; + rc = X86EMUL_UNRECOGNIZED; + goto done; case X86EMUL_OPC(0x0f, 0x77): /* emms */ case X86EMUL_OPC_VEX(0x0f, 0x77): /* vzero{all,upper} */ @@ -6323,7 +6326,8 @@ x86_emulate( case 0: /* extrq $imm8,$imm8,xmm */ break; default: - goto cannot_emulate; + rc = X86EMUL_UNRECOGNIZED; + goto done; } /* fall through */ case X86EMUL_OPC_F2(0x0f, 0x78): /* insertq $imm8,$imm8,xmm,xmm */ @@ -6518,7 +6522,7 @@ x86_emulate( goto done; break; default: - goto cannot_emulate; + goto unimplemented_insn; } break; @@ -6534,7 +6538,8 @@ x86_emulate( vcpu_must_have(avx); goto stmxcsr; } - goto cannot_emulate; + rc = X86EMUL_UNRECOGNIZED; + goto done; case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ fail_if(modrm_mod != 3); @@ -6777,7 +6782,8 @@ x86_emulate( switch ( modrm_reg & 7 ) { default: - goto cannot_emulate; + rc = X86EMUL_UNRECOGNIZED; + goto done; #ifdef HAVE_GAS_RDRAND case 6: /* rdrand */ @@ -7359,7 +7365,8 @@ x86_emulate( host_and_vcpu_must_have(bmi1); break; default: - goto cannot_emulate; + rc = X86EMUL_UNRECOGNIZED; + goto done; } generate_exception_if(vex.l, EXC_UD); @@ -7670,7 +7677,8 @@ x86_emulate( host_and_vcpu_must_have(tbm); break; default: - goto cannot_emulate; + rc = X86EMUL_UNRECOGNIZED; + goto done; } xop_09_rm_rv: @@ -7704,7 +7712,8 @@ x86_emulate( host_and_vcpu_must_have(tbm); goto xop_09_rm_rv; } - goto cannot_emulate; + rc = X86EMUL_UNRECOGNIZED; + goto done; case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */ { @@ -7736,8 +7745,8 @@ x86_emulate( } default: - cannot_emulate: - rc = X86EMUL_UNHANDLEABLE; + unimplemented_insn: + rc = X86EMUL_UNIMPLEMENTED; goto done; } @@ -7789,7 +7798,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..1fb74c0 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -133,6 +133,19 @@ 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. + */ +#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 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. 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 ) --- xen/arch/x86/hvm/emulate.c | 12 ++++++++ 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 | 52 ++++++++++++++++++++-------------- xen/arch/x86/x86_emulate/x86_emulate.h | 13 +++++++++ 7 files changed, 60 insertions(+), 23 deletions(-)