|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v11 2/5] x86emul: New return code for unimplemented instruction
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@xxxxxxxxxxxxxxx>
>
> ---
> 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@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |