| Message ID | 1504705706-4859-3-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.09.17 at 15:48, <ppircalabu@bitdefender.com> wrote: > Enforce the distinction between an instruction not implemented by the > emulator and the failure to emulate that instruction by defining a new > return code, X86EMUL_UNIMPLEMENTED. > > This value should only be returned by the core emulator only if it fails to > properly decode the current instruction's opcode, and not by any of other > functions, such as the x86_emulate_ops or the hvm_io_ops callbacks. > > e.g. hvm_process_io_incercept should not return X86EMUL_UNIMPLEMENTED. > The return value of this function depends on either the return code of > one of the hvm_io_ops handlers (read/write) or the value returned by > hvm_copy_guest_from_phys / hvm_copy_to_guest_phys. > > Similary, none of this functions should not return X86EMUL_UNIMPLEMENTED. > - hvmemul_do_io > - hvm_send_buffered_ioreq > - hvm_send_ioreq > - hvm_broadcast_ioreq > - hvmemul_do_io_buffer > - hvmemul_validate Granted hvm_io_intercept() is only a relatively thin wrapper around hvm_process_io_incercept(), but for completeness it would have been nice to be included here too. Additionally it would have helped reviewing as well as future development if you had added ASSERT()s to this effect to all consumers where you don't add a check for X86EMUL_UNIMPLEMENTED. > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com> The code in v10 changed from v9 or whichever earlier version Paul had give his R-b on. You should have removed it for that reason or, if he had given it for a limited range of the patch which did not change, you should have indicated which range that was. Also somewhere here I'm missing a summary of the changes from v9. Putting it in the overview mail is nice, but for reviewing purposes it is far more useful to be right in the affected patch. > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c After discussing with Andrew I'm willing to agree with the changes you do here, with one extra requirement: At least on non-debug builds X86EMUL_UNIMPLEMENTED should always result in #UD being raised by the final consumer of it. It should never, like would be the case with the changes you do to vmx/realmode.c, result in the domain being crashed. Please change that one and check carefully whether there are any other similar cases. Jan
On 06/09/17 14:48, Petre Pircalabu wrote: > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 64454c7..8a6eb74 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -2044,6 +2044,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) > switch ( rc ) > { > case X86EMUL_UNHANDLEABLE: > + case X86EMUL_UNIMPLEMENTED: > hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt); Please modify hvm_dump_emulation_state to pass rc in, and distinguish UNHANDLEABLE vs UNIMPLEMENTED in the printed message. Similarly for other diagnostic messages. ~Andrew
>>> On 07.09.17 at 16:26, <JBeulich@suse.com> wrote: > After discussing with Andrew I'm willing to agree with the changes > you do here, with one extra requirement: At least on non-debug > builds X86EMUL_UNIMPLEMENTED should always result in #UD being > raised by the final consumer of it. It should never, like would be > the case with the changes you do to vmx/realmode.c, result in > the domain being crashed. Please change that one and check > carefully whether there are any other similar cases. Oh, and please make the comment next to the definition of X86EMUL_UNIMPLEMENTED also say so. Jan
On Jo, 2017-09-07 at 09:08 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 07.09.17 at 16:26, <JBeulich@suse.com> wrote: > > After discussing with Andrew I'm willing to agree with the changes > > you do here, with one extra requirement: At least on non-debug > > builds X86EMUL_UNIMPLEMENTED should always result in #UD being > > raised by the final consumer of it. It should never, like would be > > the case with the changes you do to vmx/realmode.c, result in > > the domain being crashed. Please change that one and check > > carefully whether there are any other similar cases. Hi Jan, Changing the way we handle X86EMUL_UNIMPLEMENTED in some of the functions will modify the existing behavior, and I'm a little bit wary of making so many changes unrelated to the current patchset'a purpose without a thourough way of testing them. e.g.: "hvm_descriptor_access_intercept". The current behavior is to return false if X86EMUL_UNHANDLEABLE is returned by hvm_emulate_one. Up until now, this return code covered also the "unimplemented instruction" case. If X86EMUL_UNIMPLEMENTED will be handled separately (e.g. by calling hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC), hvm_emulate_writeback, and finally returning true) some of the scenarios where the domain got crashed will result only in an UD being injected. The same reasoning applies also to vmx/realmode. My patch didn't change the current behavior, the domain crash logic was added by patch 3502a233e0132cb2facfe90c5c4872c823a5cb69. However, in the end the decision if yours to take. I can add those changes, but I will require a little help in order to make sure I don't break anything. Many thanks, Petre > Oh, and please make the comment next to the definition of > X86EMUL_UNIMPLEMENTED also say so. > > Jan > > > ________________________ > This email was scanned by Bitdefender
>>> On 11.09.17 at 17:52, <ppircalabu@bitdefender.com> wrote: > On Jo, 2017-09-07 at 09:08 -0600, Jan Beulich wrote: >> > >> > > >> > > > >> > > > On 07.09.17 at 16:26, <JBeulich@suse.com> wrote: >> > After discussing with Andrew I'm willing to agree with the changes >> > you do here, with one extra requirement: At least on non-debug >> > builds X86EMUL_UNIMPLEMENTED should always result in #UD being >> > raised by the final consumer of it. It should never, like would be >> > the case with the changes you do to vmx/realmode.c, result in >> > the domain being crashed. Please change that one and check >> > carefully whether there are any other similar cases. > > Changing the way we handle X86EMUL_UNIMPLEMENTED in some of the > functions will modify the existing behavior, and I'm a little bit wary > of making so many changes unrelated to the current patchset'a purpose > without a thourough way of testing them. But leaving code you don't directly care about in a state not in line with the new distinction isn't any better. The groundwork is to have all existing code honor the new distinction in a sensible way. Only then comes your intended behavioral change to VM event handling. However, thinking about the actually three different scenarios again, I'm no longer sure either your model or the one I've been suggesting would be correct, which would get us half way back to what I've been asking for before. These are the cases to care about: - X86EMUL_UNHANDLEABLE: something went wrong while processing a recognized instruction (or e.g. while decoding); current behavior is fine to retain - recognized but not implemented: these must not #UD, so behavior matching the unhandleable case is what we want - unrecognized instruction: these should #UD The problem is that you want to use the same error indicator for both of the latter two cases. And the "problem" with adding another new error indicator (e.g. X86EMUL_UNRECOGNIZED) is that then we need to adjust the emulator when we add support for new CPUID bits. But that's the long term goal anyway, so I have to admit that I don't see anything wrong with this, other than this causing some additional work. On the grounds that present behavior isn't coming anywhere close to the outlined target behavior and that you're patch isn't making the situation worse, I'm now inclined to agree that the best way forward is to live with the remaining inconsistency until we've managed to sufficiently fill the gaps. That is, the patch can, in this respect, stay as it was. I would like to ask you to extend the comment on the definition though, outlining the target behavior (perhaps including the other new return value in that comment, or simply adding #define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED for the time being). The alternative of having X86EMUL_UNRECOGNIZED would be to make the emulator raise #UD itself in such cases, as suggested earlier. Andrew, thoughts? > e.g.: "hvm_descriptor_access_intercept". > The current behavior is to return false if X86EMUL_UNHANDLEABLE is > returned by hvm_emulate_one. Up until now, this return code covered > also the "unimplemented instruction" case. > If X86EMUL_UNIMPLEMENTED will be handled separately (e.g. by calling > hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC), > hvm_emulate_writeback, and finally returning true) some of the > scenarios where the domain got crashed will result only in an UD being > injected. This function doesn't receive any X86EMUL_* values, so I don't see how it would be relevant here. But with the above this is likely moot anyway. Jan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 64454c7..8a6eb74 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -2044,6 +2044,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) switch ( rc ) { case X86EMUL_UNHANDLEABLE: + case X86EMUL_UNIMPLEMENTED: hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt); break; case X86EMUL_EXCEPTION: @@ -2101,6 +2102,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, * consistent with X86EMUL_RETRY. */ return; + case X86EMUL_UNIMPLEMENTED: case X86EMUL_UNHANDLEABLE: hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx); hvm_inject_hw_exception(trapnr, errcode); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 6cb903d..ea2812c 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3695,6 +3695,7 @@ void hvm_ud_intercept(struct cpu_user_regs *regs) switch ( hvm_emulate_one(&ctxt) ) { case X86EMUL_UNHANDLEABLE: + case X86EMUL_UNIMPLEMENTED: hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC); break; case X86EMUL_EXCEPTION: diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index bf41954..984db21 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -96,6 +96,7 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr) switch ( rc ) { case X86EMUL_UNHANDLEABLE: + case X86EMUL_UNIMPLEMENTED: hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt); return false; diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c index 11bde58..fdbbee2 100644 --- a/xen/arch/x86/hvm/vmx/realmode.c +++ b/xen/arch/x86/hvm/vmx/realmode.c @@ -106,7 +106,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt) if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry ) vio->io_completion = HVMIO_realmode_completion; - if ( rc == X86EMUL_UNHANDLEABLE ) + if ( rc == X86EMUL_UNHANDLEABLE || rc == X86EMUL_UNIMPLEMENTED ) { gdprintk(XENLOG_ERR, "Failed to emulate insn.\n"); goto fail; diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index f7efe66..90cfa40 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3488,7 +3488,7 @@ static int sh_page_fault(struct vcpu *v, * would be a good unshadow hint. If we *do* decide to unshadow-on-fault * then it must be 'failable': we cannot require the unshadow to succeed. */ - if ( r == X86EMUL_UNHANDLEABLE ) + if ( r == X86EMUL_UNHANDLEABLE || r == X86EMUL_UNIMPLEMENTED ) { perfc_incr(shadow_fault_emulate_failed); #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index c1e2300..ad97d93 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -848,7 +848,8 @@ do{ asm volatile ( \ stub.func); \ generate_exception_if(res_.fields.trapnr == EXC_UD, EXC_UD); \ domain_crash(current->domain); \ - goto cannot_emulate; \ + rc = X86EMUL_UNHANDLEABLE; \ + goto done; \ } \ } while (0) #else @@ -2585,7 +2586,7 @@ x86_decode( d = twobyte_table[0x3a].desc; break; default: - rc = X86EMUL_UNHANDLEABLE; + rc = X86EMUL_UNIMPLEMENTED; goto done; } } @@ -2599,7 +2600,7 @@ x86_decode( } else { - rc = X86EMUL_UNHANDLEABLE; + rc = X86EMUL_UNIMPLEMENTED; goto done; } @@ -2879,7 +2880,7 @@ x86_decode( default: ASSERT_UNREACHABLE(); - return X86EMUL_UNHANDLEABLE; + return X86EMUL_UNIMPLEMENTED; } if ( ea.type == OP_MEM ) @@ -4191,7 +4192,7 @@ x86_emulate( break; case 4: /* fldenv - TODO */ state->fpu_ctrl = true; - goto cannot_emulate; + goto unimplemented_insn; case 5: /* fldcw m2byte */ state->fpu_ctrl = true; if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val, @@ -4202,7 +4203,7 @@ x86_emulate( break; case 6: /* fnstenv - TODO */ state->fpu_ctrl = true; - goto cannot_emulate; + goto unimplemented_insn; case 7: /* fnstcw m2byte */ state->fpu_ctrl = true; emulate_fpu_insn_memdst("fnstcw", dst.val); @@ -4438,7 +4439,7 @@ x86_emulate( case 4: /* frstor - TODO */ case 6: /* fnsave - TODO */ state->fpu_ctrl = true; - goto cannot_emulate; + goto unimplemented_insn; case 7: /* fnstsw m2byte */ state->fpu_ctrl = true; emulate_fpu_insn_memdst("fnstsw", dst.val); @@ -5197,7 +5198,7 @@ x86_emulate( #undef _GRP7 default: - goto cannot_emulate; + goto unimplemented_insn; } break; } @@ -6195,7 +6196,7 @@ x86_emulate( /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */ break; default: - goto cannot_emulate; + goto unimplemented_insn; } simd_0f_shift_imm: generate_exception_if(ea.type != OP_REG, EXC_UD); @@ -6243,7 +6244,7 @@ x86_emulate( case 6: /* psllq $imm8,mm */ goto simd_0f_shift_imm; } - goto cannot_emulate; + goto unimplemented_insn; case X86EMUL_OPC_66(0x0f, 0x73): case X86EMUL_OPC_VEX_66(0x0f, 0x73): @@ -6259,7 +6260,7 @@ x86_emulate( /* vpslldq $imm8,{x,y}mm,{x,y}mm */ goto simd_0f_shift_imm; } - goto cannot_emulate; + goto unimplemented_insn; case X86EMUL_OPC(0x0f, 0x77): /* emms */ case X86EMUL_OPC_VEX(0x0f, 0x77): /* vzero{all,upper} */ @@ -6323,7 +6324,7 @@ x86_emulate( case 0: /* extrq $imm8,$imm8,xmm */ break; default: - goto cannot_emulate; + goto unimplemented_insn; } /* fall through */ case X86EMUL_OPC_F2(0x0f, 0x78): /* insertq $imm8,$imm8,xmm,xmm */ @@ -6518,7 +6519,8 @@ x86_emulate( goto done; break; default: - goto cannot_emulate; + rc = X86EMUL_UNHANDLEABLE; + goto done; } break; @@ -6534,7 +6536,7 @@ x86_emulate( vcpu_must_have(avx); goto stmxcsr; } - goto cannot_emulate; + goto unimplemented_insn; case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ fail_if(modrm_mod != 3); @@ -6777,7 +6779,7 @@ x86_emulate( switch ( modrm_reg & 7 ) { default: - goto cannot_emulate; + goto unimplemented_insn; #ifdef HAVE_GAS_RDRAND case 6: /* rdrand */ @@ -7359,7 +7361,7 @@ x86_emulate( host_and_vcpu_must_have(bmi1); break; default: - goto cannot_emulate; + goto unimplemented_insn; } generate_exception_if(vex.l, EXC_UD); @@ -7670,7 +7672,7 @@ x86_emulate( host_and_vcpu_must_have(tbm); break; default: - goto cannot_emulate; + goto unimplemented_insn; } xop_09_rm_rv: @@ -7704,7 +7706,7 @@ x86_emulate( host_and_vcpu_must_have(tbm); goto xop_09_rm_rv; } - goto cannot_emulate; + goto unimplemented_insn; case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */ { @@ -7736,8 +7738,8 @@ x86_emulate( } default: - cannot_emulate: - rc = X86EMUL_UNHANDLEABLE; + unimplemented_insn: + rc = X86EMUL_UNIMPLEMENTED; goto done; } @@ -7789,7 +7791,8 @@ x86_emulate( if ( (d & DstMask) != DstMem ) { ASSERT_UNREACHABLE(); - goto cannot_emulate; + rc = X86EMUL_UNHANDLEABLE; + goto done; } break; } diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index 4ddf111..82812ca 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -133,6 +133,12 @@ struct x86_emul_fpu_aux { * Undefined behavior when used anywhere else. */ #define X86EMUL_DONE 4 + /* + * Current instruction is not implemented by the emulator. + * This value should only be returned by the core emulator if decode fails + * and not by any of the x86_emulate_ops callbacks. + */ +#define X86EMUL_UNIMPLEMENTED 5 /* FPU sub-types which may be requested via ->get_fpu(). */ enum x86_emulate_fpu_type {