| Message ID | 1504119449-7324-2-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 30.08.17 at 20:57, <ppircalabu@bitdefender.com> wrote: > --- 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) Way earlier in this file there are consumers of X86EMUL_UNHANDLEABLE in hvmemul_do_io() and hvmemul_do_io_buffer(). I'm sure I did point this out before, and I cannot see why you don't adjust those as well, and you also don't say anything in this regard in the description. Similarly there's a consumer in hvm_process_io_intercept() (in a file you don't touch at all). The use in hvm_broadcast_ioreq() is likely fine, but I'd still like you to reason about that in the description. > @@ -5177,7 +5177,7 @@ x86_emulate( > goto done; > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; While I can see why you do this change, for many/all of the ones I'll leave in context below I think you rather want to switch to generate_exception(EXC_UD). > @@ -6176,7 +6176,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); > @@ -6224,7 +6224,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): > @@ -6240,7 +6240,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} */ > @@ -6304,7 +6304,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 */ > @@ -6515,7 +6515,7 @@ x86_emulate( A few lines up from here is another instance you appear to have missed. > @@ -7341,7 +7341,7 @@ x86_emulate( > host_and_vcpu_must_have(bmi1); > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } > > generate_exception_if(vex.l, EXC_UD); > @@ -7650,7 +7650,7 @@ x86_emulate( > host_and_vcpu_must_have(tbm); > break; > default: > - goto cannot_emulate; > + goto unimplemented_insn; > } > > xop_09_rm_rv: > @@ -7684,7 +7684,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 */ > { > @@ -7716,6 +7716,9 @@ x86_emulate( > } > > default: > + unimplemented_insn: > + rc = X86EMUL_UNIMPLEMENTED; > + goto done; > cannot_emulate: > rc = X86EMUL_UNHANDLEABLE; > goto done; I guess the cannot_emulate label would then better be moved elsewhere (either out of the switch or to a place where one of the goto-s to it currently sits). Jan
On Vi, 2017-09-01 at 04:33 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 30.08.17 at 20:57, <ppircalabu@bitdefender.com> wrote: > > --- 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) > Way earlier in this file there are consumers of X86EMUL_UNHANDLEABLE > in hvmemul_do_io() and hvmemul_do_io_buffer(). I'm sure I did > point this out before, and I cannot see why you don't adjust those as > well, and you also don't say anything in this regard in the > description. > Similarly there's a consumer in hvm_process_io_intercept() (in a file > you don't touch at all). The use in hvm_broadcast_ioreq() is likely > fine, but I'd still like you to reason about that in the description. My mistake. I have added my comments in the cover letter as I thought they will be easier to read. I will add them the the patch description for the next iteration. In my opinion, hvm_process_io_intercept cannot 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. In case of the latter, the function crashes the domain and returns X86EMUL_UNHANDLEABLE if the result is neither HVMCOPY_okay nor HVMCOPY_bad_gfn_to_mfn. hvm_op_ops should not return X86EMUL_UNIMPLEMENTED as this return code's usage should strictly be limited to the core emulator and only when an instruction opcode is invalid. hvmemul_do_io - X86EMUL_UNHANDLEABLE is tested against the return code of hvm_io_intercept which can return X86EMUL_UNHANDLEABLE only if the IO handler was not found or it was returned by hvm_process_io_intercept (cannot return X86EMUL_UNIMPLEMENTED). In this case I think adding the X86EMUL_UNIMPLEMENTED check would mask a possible misuse of this return code instead of just triggering BUG() in the early stages of development. hvm_send_buffered_ioreq - currently returns X86EMUL_UNHANDLEABLE if: - the buffered iopage is NULL. - the ioreq size if invalid - the queue is full. In none of these cases the function cannot return X86EMUL_UNIMPLEMENTED because they are not related to the instruction opcode decoding, so this function cannot return X86EMUL_UNIMPLEMENTED. hvm_send_ioreq - can return X86EMUL_UNHANDLEABLE only if the current vcpu in not the hvm_iorec_server's list or the value if returned from hvm_send_buffered_ioreq, hence this function also cannot return X86EMUL_UNIMPLEMENTED. hvm_broadcast_ioreq - calls hvm_send_ioreq and chvemul_do_iohecks the return code against X86EMUL_UNHANDLEABLE. There is no need to extend the check to handle X86EMUL_UNIMPLEMENTED because this value cannot be returned by hvm_send_ioreq. hvmemul_do_io_buffer - calls hvmemul_do_io and, if the return code is X86EMUL_UNHANDLEABLE and dir is IOREQ_READ, sets the return buffer to 0xFF. The return value of hvemul_do_io can be: - return value of hvm_io_intercept (cannot be X86EMUL_UNIMPLEMENTED), - return value of hvm_process_io_intercept (cannot be X86EMUL_UNIMPLEMENTED) - return value of hvm_send_ioreq(cannot be X86EMUL_UNIMPLEMENTED) - X86EMUL_RETRY / X86EMUL_OKAY (depending on state) The condition also should not be extended to take into account X86EMUL_UNIMPLEMENTED because this value cannot be returned by hvemul_do_io. > > > > @@ -5177,7 +5177,7 @@ x86_emulate( > > goto done; > > break; > > default: > > - goto cannot_emulate; > > + goto unimplemented_insn; > While I can see why you do this change, for many/all of the ones > I'll leave in context below I think you rather want to switch to > generate_exception(EXC_UD). Some of the opcodes are valid but not supported by the emulator. In this case X86EMUL_UNIMPLEMENTED should be returned to allow the monitor app to handle this case. Also, in the worst case scenario, when the opcode doesn't correspond to a valid x86(-64) instruction, if the monitor app for example tries to single-step it on the real hardware an UD exception will also be reported. > > > > > @@ -6176,7 +6176,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); > > @@ -6224,7 +6224,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): > > @@ -6240,7 +6240,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} */ > > @@ -6304,7 +6304,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 */ > > @@ -6515,7 +6515,7 @@ x86_emulate( > A few lines up from here is another instance you appear to have > missed. > > > > > @@ -7341,7 +7341,7 @@ x86_emulate( > > host_and_vcpu_must_have(bmi1); > > break; > > default: > > - goto cannot_emulate; > > + goto unimplemented_insn; > > } > > > > generate_exception_if(vex.l, EXC_UD); > > @@ -7650,7 +7650,7 @@ x86_emulate( > > host_and_vcpu_must_have(tbm); > > break; > > default: > > - goto cannot_emulate; > > + goto unimplemented_insn; > > } > > > > xop_09_rm_rv: > > @@ -7684,7 +7684,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 */ > > { > > @@ -7716,6 +7716,9 @@ x86_emulate( > > } > > Thanks for noticing it. I will change it back to cannot_emulate as there are no other valid instructions for this opcodes. > > default: > > + unimplemented_insn: > > + rc = X86EMUL_UNIMPLEMENTED; > > + goto done; > > cannot_emulate: > > rc = X86EMUL_UNHANDLEABLE; > > goto done; > I guess the cannot_emulate label would then better be moved > elsewhere (either out of the switch or to a place where one of > the goto-s to it currently sits). I will change it in the new iteration of the patch. > Jan > > > ________________________ > This email was scanned by Bitdefender Many thanks for your patience, //Petre ________________________ This email was scanned by Bitdefender
>>> Petre Ovidiu PIRCALABU <ppircalabu@bitdefender.com> 09/04/17 7:20 PM >>> >On Vi, 2017-09-01 at 04:33 -0600, Jan Beulich wrote: >> > >> > > >> > > > >> > > > On 30.08.17 at 20:57, <ppircalabu@bitdefender.com> wrote: >> > --- 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) >> Way earlier in this file there are consumers of X86EMUL_UNHANDLEABLE >> in hvmemul_do_io() and hvmemul_do_io_buffer(). I'm sure I did >> point this out before, and I cannot see why you don't adjust those as >> well, and you also don't say anything in this regard in the >> description. >> Similarly there's a consumer in hvm_process_io_intercept() (in a file >> you don't touch at all). The use in hvm_broadcast_ioreq() is likely >> fine, but I'd still like you to reason about that in the description. > >My mistake. I have added my comments in the cover letter as I thought >they will be easier to read. I will add them the the patch description >for the next iteration. Thanks, that'll make them stay with the changes once committed. Your further explanations are too long for a commit message though, imo. Aiui they all boil down to the uses being in functions sitting behind rather than in front of x86_emulate(). If that's the case, I would suggest you state just that fact along with the enumeration of places that don't need touching for that reason. This will make reviewing (and later viewing/understanding) quite a bit easier. >> > @@ -5177,7 +5177,7 @@ x86_emulate( >> > goto done; >> > break; >> > default: >> > - goto cannot_emulate; >> > + goto unimplemented_insn; >> While I can see why you do this change, for many/all of the ones >> I'll leave in context below I think you rather want to switch to >> generate_exception(EXC_UD). >Some of the opcodes are valid but not supported by the emulator. In >this case X86EMUL_UNIMPLEMENTED should be returned to allow the monitor >app to handle this case. Also, in the worst case scenario, when the >opcode doesn't correspond to a valid x86(-64) instruction, if the >monitor app for example tries to single-step it on the real hardware an >UD exception will also be reported. Please be more precise with "some of the opcodes are valid". When I looked through your change, I don't think I've seen any such case for the places I meant the comment to apply to. Also, as far as the emulator changes themselves go, please leave aside considerations of what a monitor app may or may not do. These changes need to be consistent all by themselves. >> > @@ -7716,6 +7716,9 @@ x86_emulate( >> > } >> > >Thanks for noticing it. I will change it back to cannot_emulate as >there are no other valid instructions for this opcodes. I have trouble understanding this comment of yours, not the least because I don't recall having asked (in particular around here) that you switch anything back. Jan
On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote: > > > > > > > > > > > @@ -5177,7 +5177,7 @@ x86_emulate( > > > > goto done; > > > > break; > > > > default: > > > > - goto cannot_emulate; > > > > + goto unimplemented_insn; > > > While I can see why you do this change, for many/all of the ones > > > I'll leave in context below I think you rather want to switch to > > > generate_exception(EXC_UD). > > Some of the opcodes are valid but not supported by the emulator. In > > this case X86EMUL_UNIMPLEMENTED should be returned to allow the > > monitor > > app to handle this case. Also, in the worst case scenario, when the > > opcode doesn't correspond to a valid x86(-64) instruction, if the > > monitor app for example tries to single-step it on the real > > hardware an > > UD exception will also be reported. > Please be more precise with "some of the opcodes are valid". When I > looked through your change, I don't think I've seen any such case for > the > places I meant the comment to apply to. Also, as far as the emulator > changes themselves go, please leave aside considerations of what a > monitor app may or may not do. These changes need to be consistent > all by themselves. > Sorry about the poor wording. I was under the impression that you required the invalid opcodes to be handled differently from the ones which are valid but unimplemented (directly generate EXC_UD instead of returning X86EMUL_UNIMPLEMENTED and let the caller handle it). e.g. for X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */ the mod R/M possible values are 1,2,3 (source: http://sandpile.org/x86/ opc_grp.htm). From my perspective I wouldn't differentiate between those 2 cases and return X86EMUL_UNIMPLEMENTED. The performance penalty is negligible if the monitor is neither present nor it implements XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED. The monitor can only offer a "second-chance" re-execution of the faulty instruction and does not affect the internal logic of x86_decode/x86_emulate. > > > > > > > > > > > @@ -7716,6 +7716,9 @@ x86_emulate( > > > > } > > > > > > Thanks for noticing it. I will change it back to cannot_emulate as > > there are no other valid instructions for this opcodes. > I have trouble understanding this comment of yours, not the least > because I don't recall having asked (in particular around here) that > you switch anything back. > > Jan > > > ________________________ > This email was scanned by Bitdefender Many thanks for your support, //Petre ________________________ This email was scanned by Bitdefender
>>> On 05.09.17 at 17:23, <ppircalabu@bitdefender.com> wrote: > On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote: >> > > >> > > > >> > > > @@ -5177,7 +5177,7 @@ x86_emulate( >> > > > goto done; >> > > > break; >> > > > default: >> > > > - goto cannot_emulate; >> > > > + goto unimplemented_insn; >> > > While I can see why you do this change, for many/all of the ones >> > > I'll leave in context below I think you rather want to switch to >> > > generate_exception(EXC_UD). >> > Some of the opcodes are valid but not supported by the emulator. In >> > this case X86EMUL_UNIMPLEMENTED should be returned to allow the >> > monitor >> > app to handle this case. Also, in the worst case scenario, when the >> > opcode doesn't correspond to a valid x86(-64) instruction, if the >> > monitor app for example tries to single-step it on the real >> > hardware an >> > UD exception will also be reported. >> Please be more precise with "some of the opcodes are valid". When I >> looked through your change, I don't think I've seen any such case for >> the >> places I meant the comment to apply to. Also, as far as the emulator >> changes themselves go, please leave aside considerations of what a >> monitor app may or may not do. These changes need to be consistent >> all by themselves. >> > Sorry about the poor wording. I was under the impression that you > required the invalid opcodes to be handled differently from the ones > which are valid but unimplemented (directly generate EXC_UD instead of > returning X86EMUL_UNIMPLEMENTED and let the caller handle it). Yes, that's what I think would be best. I admit there is a potential problem with this, when one or more of them become defined. But that's something I'd like to also have Andrew's input on. > e.g. for X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */ > the mod R/M possible values are 1,2,3 (source: http://sandpile.org/x86/ > opc_grp.htm). > From my perspective I wouldn't differentiate between those 2 cases and > return X86EMUL_UNIMPLEMENTED. I must still be missing something: What two cases are you talking about? The three valid values have an implementation in the emulator. All others are undefined, i.e. at present would ideally produce #UD (see above). Jan
On Ma, 2017-09-05 at 09:46 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 05.09.17 at 17:23, <ppircalabu@bitdefender.com> wrote: > > On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -5177,7 +5177,7 @@ x86_emulate( > > > > > > goto done; > > > > > > break; > > > > > > default: > > > > > > - goto cannot_emulate; > > > > > > + goto unimplemented_insn; > > > > > While I can see why you do this change, for many/all of the > > > > > ones > > > > > I'll leave in context below I think you rather want to switch > > > > > to > > > > > generate_exception(EXC_UD). > > > > Some of the opcodes are valid but not supported by the > > > > emulator. In > > > > this case X86EMUL_UNIMPLEMENTED should be returned to allow the > > > > monitor > > > > app to handle this case. Also, in the worst case scenario, when > > > > the > > > > opcode doesn't correspond to a valid x86(-64) instruction, if > > > > the > > > > monitor app for example tries to single-step it on the real > > > > hardware an > > > > UD exception will also be reported. > > > Please be more precise with "some of the opcodes are valid". When > > > I > > > looked through your change, I don't think I've seen any such case > > > for > > > the > > > places I meant the comment to apply to. Also, as far as the > > > emulator > > > changes themselves go, please leave aside considerations of what > > > a > > > monitor app may or may not do. These changes need to be > > > consistent > > > all by themselves. > > > > > Sorry about the poor wording. I was under the impression that you > > required the invalid opcodes to be handled differently from the > > ones > > which are valid but unimplemented (directly generate EXC_UD instead > > of > > returning X86EMUL_UNIMPLEMENTED and let the caller handle it). > Yes, that's what I think would be best. I admit there is a potential > problem with this, when one or more of them become defined. But > that's something I'd like to also have Andrew's input on. > > > > > e.g. for X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */ > > the mod R/M possible values are 1,2,3 (source: http://sandpile.org/ > > x86/ > > opc_grp.htm). > > From my perspective I wouldn't differentiate between those 2 cases > > and > > return X86EMUL_UNIMPLEMENTED. > I must still be missing something: What two cases are you talking > about? The three valid values have an implementation in the > emulator. All others are undefined, i.e. at present would ideally > produce #UD (see above). Probably a better example it the handling of Grp7 (after applying Andrew's patch). if modrm is 0xd0 the instruction is xgetbv (valid but unimplemented) if modrm value is corrupted (the encoding doesn't correspond to a valid instruction it will also jump to emul_unimplemented. > > Jan > > > ________________________ > This email was scanned by Bitdefender Many thanks, Petre ________________________ This email was scanned by Bitdefender
>>> On 05.09.17 at 18:20, <ppircalabu@bitdefender.com> wrote: > On Ma, 2017-09-05 at 09:46 -0600, Jan Beulich wrote: >> > >> > > >> > > > >> > > > On 05.09.17 at 17:23, <ppircalabu@bitdefender.com> wrote: >> > On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote: >> > > >> > > > >> > > > > >> > > > > >> > > > > > >> > > > > > >> > > > > > @@ -5177,7 +5177,7 @@ x86_emulate( >> > > > > > goto done; >> > > > > > break; >> > > > > > default: >> > > > > > - goto cannot_emulate; >> > > > > > + goto unimplemented_insn; >> > > > > While I can see why you do this change, for many/all of the >> > > > > ones >> > > > > I'll leave in context below I think you rather want to switch >> > > > > to >> > > > > generate_exception(EXC_UD). >> > > > Some of the opcodes are valid but not supported by the >> > > > emulator. In >> > > > this case X86EMUL_UNIMPLEMENTED should be returned to allow the >> > > > monitor >> > > > app to handle this case. Also, in the worst case scenario, when >> > > > the >> > > > opcode doesn't correspond to a valid x86(-64) instruction, if >> > > > the >> > > > monitor app for example tries to single-step it on the real >> > > > hardware an >> > > > UD exception will also be reported. >> > > Please be more precise with "some of the opcodes are valid". When >> > > I >> > > looked through your change, I don't think I've seen any such case >> > > for >> > > the >> > > places I meant the comment to apply to. Also, as far as the >> > > emulator >> > > changes themselves go, please leave aside considerations of what >> > > a >> > > monitor app may or may not do. These changes need to be >> > > consistent >> > > all by themselves. >> > > >> > Sorry about the poor wording. I was under the impression that you >> > required the invalid opcodes to be handled differently from the >> > ones >> > which are valid but unimplemented (directly generate EXC_UD instead >> > of >> > returning X86EMUL_UNIMPLEMENTED and let the caller handle it). >> Yes, that's what I think would be best. I admit there is a potential >> problem with this, when one or more of them become defined. But >> that's something I'd like to also have Andrew's input on. >> >> > >> > e.g. for X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */ >> > the mod R/M possible values are 1,2,3 (source: http://sandpile.org/ >> > x86/ >> > opc_grp.htm). >> > From my perspective I wouldn't differentiate between those 2 cases >> > and >> > return X86EMUL_UNIMPLEMENTED. >> I must still be missing something: What two cases are you talking >> about? The three valid values have an implementation in the >> emulator. All others are undefined, i.e. at present would ideally >> produce #UD (see above). > > Probably a better example it the handling of Grp7 (after applying > Andrew's patch). > if modrm is 0xd0 the instruction is xgetbv (valid but unimplemented) > if modrm value is corrupted (the encoding doesn't correspond to a valid > instruction it will also jump to emul_unimplemented. In cases when there is a mix of missing and undefined opcodes I would view it as acceptable to go the unimplemented path without further splitting. In cases where all unhandled cases are also undefined, I'd prefer #UD to be raised. Jan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 64454c7..f9f8c25 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,8 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, * consistent with X86EMUL_RETRY. */ return; + case X86EMUL_UNIMPLEMENTED: + /* fall-through */ 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 c5c0af8..15727a2 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 2201852..242b0af 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2577,7 +2577,7 @@ x86_decode( d = twobyte_table[0x3a].desc; break; default: - rc = X86EMUL_UNHANDLEABLE; + rc = X86EMUL_UNIMPLEMENTED; goto done; } } @@ -2591,7 +2591,7 @@ x86_decode( } else { - rc = X86EMUL_UNHANDLEABLE; + rc = X86EMUL_UNIMPLEMENTED; goto done; } @@ -2871,7 +2871,7 @@ x86_decode( default: ASSERT_UNREACHABLE(); - return X86EMUL_UNHANDLEABLE; + return X86EMUL_UNIMPLEMENTED; } if ( ea.type == OP_MEM ) @@ -4183,7 +4183,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, @@ -4194,7 +4194,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); @@ -4430,7 +4430,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); @@ -5177,7 +5177,7 @@ x86_emulate( goto done; break; default: - goto cannot_emulate; + goto unimplemented_insn; } break; } @@ -6176,7 +6176,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); @@ -6224,7 +6224,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): @@ -6240,7 +6240,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} */ @@ -6304,7 +6304,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 */ @@ -6515,7 +6515,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); @@ -6759,7 +6759,7 @@ x86_emulate( switch ( modrm_reg & 7 ) { default: - goto cannot_emulate; + goto unimplemented_insn; #ifdef HAVE_GAS_RDRAND case 6: /* rdrand */ @@ -7341,7 +7341,7 @@ x86_emulate( host_and_vcpu_must_have(bmi1); break; default: - goto cannot_emulate; + goto unimplemented_insn; } generate_exception_if(vex.l, EXC_UD); @@ -7650,7 +7650,7 @@ x86_emulate( host_and_vcpu_must_have(tbm); break; default: - goto cannot_emulate; + goto unimplemented_insn; } xop_09_rm_rv: @@ -7684,7 +7684,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 */ { @@ -7716,6 +7716,9 @@ x86_emulate( } default: + unimplemented_insn: + rc = X86EMUL_UNIMPLEMENTED; + goto done; cannot_emulate: rc = X86EMUL_UNHANDLEABLE; goto done; 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 {