* [PATCH v8 0/2] Singlestep unimplemented x86emul instructions @ 2017-08-08 18:06 Petre Pircalabu 2017-08-08 18:06 ` [PATCH v8 1/2] x86emul: New return code for unimplemented instruction Petre Pircalabu 2017-08-08 18:06 ` [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu 0 siblings, 2 replies; 10+ messages in thread From: Petre Pircalabu @ 2017-08-08 18:06 UTC (permalink / raw) To: xen-devel Cc: Petre Pircalabu, kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, tim, paul.durrant, tamas, jbeulich This patchset implements a new way of handling the instructions unimplemented in x86emul. Instead of inserting a hardware exception the monitor will be notified and it will to try to single-step the faulty instruction using the real processor and then resume execution of the normal instruction flow. --- Changed since v1: * Removed the emulation kind check when calling hvm_inject_hw_exception Changed since v2: * Removed a file added by mistake Changed since v3: * Removed extra stray line * Added the _enabled suffix to the emul_unhandleable monitor option Changed since v4 * Fixed return expression of hvm_monitor_emul_unhandleable handle monitor_traps failures. * Removed stray parantheses. Changed since v5: * Removed unnecessary "else" when calling hvm_monitor_emul_unhandleable. * Added extra line in arch_monitor_domctl_event. Changed since v6: * add the distinction between unimplemented instructions and emulation failures. * changed "emul_unhandleable" event name to "emul_unimplemented" Changed since v7: * Add "fall-through" comments to the switch statements (coverity) * Added X86EMUL_UNIMPLEMENTED to X86EMUL_UNHANDLEABLE checks the in functions referencing x86_emulate. * Improved comment describing X86EMUL_UNIMPLEMENTED. Petre Pircalabu (2): x86emul: New return code for unimplemented instruction x86/monitor: Notify monitor if an emulation fails. tools/libxc/include/xenctrl.h | 2 ++ tools/libxc/xc_monitor.c | 14 ++++++++++++++ xen/arch/x86/hvm/emulate.c | 8 ++++++++ xen/arch/x86/hvm/io.c | 2 ++ xen/arch/x86/hvm/monitor.c | 17 +++++++++++++++++ xen/arch/x86/hvm/vmx/realmode.c | 2 +- xen/arch/x86/mm/shadow/multi.c | 2 +- xen/arch/x86/monitor.c | 13 +++++++++++++ xen/arch/x86/x86_emulate/x86_emulate.c | 8 ++++---- xen/arch/x86/x86_emulate/x86_emulate.h | 6 ++++++ xen/include/asm-x86/domain.h | 1 + xen/include/asm-x86/hvm/monitor.h | 1 + xen/include/asm-x86/monitor.h | 3 ++- xen/include/public/domctl.h | 1 + xen/include/public/vm_event.h | 2 ++ 15 files changed, 75 insertions(+), 7 deletions(-) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v8 1/2] x86emul: New return code for unimplemented instruction 2017-08-08 18:06 [PATCH v8 0/2] Singlestep unimplemented x86emul instructions Petre Pircalabu @ 2017-08-08 18:06 ` Petre Pircalabu 2017-08-09 8:11 ` Paul Durrant 2017-08-22 8:09 ` Jan Beulich 2017-08-08 18:06 ` [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu 1 sibling, 2 replies; 10+ messages in thread From: Petre Pircalabu @ 2017-08-08 18:06 UTC (permalink / raw) To: xen-devel Cc: Petre Pircalabu, kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, tim, paul.durrant, tamas, jbeulich 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 used by the core emulator if it fails to decode the current instruction, and not by any of the x86_emulate_ops callbacks. Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> --- xen/arch/x86/hvm/emulate.c | 4 ++++ xen/arch/x86/hvm/io.c | 2 ++ xen/arch/x86/hvm/vmx/realmode.c | 2 +- xen/arch/x86/mm/shadow/multi.c | 2 +- xen/arch/x86/x86_emulate/x86_emulate.c | 8 ++++---- xen/arch/x86/x86_emulate/x86_emulate.h | 6 ++++++ 6 files changed, 18 insertions(+), 6 deletions(-) diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 3a8db21..28133c0 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) switch ( rc ) { case X86EMUL_UNHANDLEABLE: + /* fall-through */ + case X86EMUL_UNIMPLEMENTED: hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt); break; case X86EMUL_EXCEPTION: @@ -2113,6 +2115,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/io.c b/xen/arch/x86/hvm/io.c index 214ab30..af4e1dc 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -96,6 +96,8 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr) switch ( rc ) { case X86EMUL_UNHANDLEABLE: + /* fall-through */ + 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 c9c2252..85fb165 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3486,7 +3486,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..480bad9 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 ) @@ -7717,7 +7717,7 @@ x86_emulate( default: cannot_emulate: - rc = X86EMUL_UNHANDLEABLE; + rc = X86EMUL_UNIMPLEMENTED; 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 { -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v8 1/2] x86emul: New return code for unimplemented instruction 2017-08-08 18:06 ` [PATCH v8 1/2] x86emul: New return code for unimplemented instruction Petre Pircalabu @ 2017-08-09 8:11 ` Paul Durrant 2017-08-22 8:09 ` Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Paul Durrant @ 2017-08-09 8:11 UTC (permalink / raw) To: 'Petre Pircalabu', xen-devel@lists.xen.org Cc: Kevin Tian, sstabellini@kernel.org, Wei Liu, jun.nakajima@intel.com, rcojocaru@bitdefender.com, Andrew Cooper, Tim (Xen.org), George Dunlap, tamas@tklengyel.com, jbeulich@suse.com, Ian Jackson > -----Original Message----- > From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com] > Sent: 08 August 2017 19:07 > 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 v8 1/2] 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 used by the core emulator if it fails to decode > the current instruction, and not by any of the x86_emulate_ops > callbacks. > > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > --- > xen/arch/x86/hvm/emulate.c | 4 ++++ > xen/arch/x86/hvm/io.c | 2 ++ > xen/arch/x86/hvm/vmx/realmode.c | 2 +- > xen/arch/x86/mm/shadow/multi.c | 2 +- > xen/arch/x86/x86_emulate/x86_emulate.c | 8 ++++---- > xen/arch/x86/x86_emulate/x86_emulate.h | 6 ++++++ > 6 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 3a8db21..28133c0 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, > unsigned long gla) > switch ( rc ) > { > case X86EMUL_UNHANDLEABLE: > + /* fall-through */ > + case X86EMUL_UNIMPLEMENTED: > hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", > &ctxt); > break; > case X86EMUL_EXCEPTION: > @@ -2113,6 +2115,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/io.c b/xen/arch/x86/hvm/io.c > index 214ab30..af4e1dc 100644 > --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -96,6 +96,8 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t > *validate, const char *descr) > switch ( rc ) > { > case X86EMUL_UNHANDLEABLE: > + /* fall-through */ > + 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 c9c2252..85fb165 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3486,7 +3486,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..480bad9 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 ) > @@ -7717,7 +7717,7 @@ x86_emulate( > > default: > cannot_emulate: > - rc = X86EMUL_UNHANDLEABLE; > + rc = X86EMUL_UNIMPLEMENTED; > 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 { > -- > 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v8 1/2] x86emul: New return code for unimplemented instruction 2017-08-08 18:06 ` [PATCH v8 1/2] x86emul: New return code for unimplemented instruction Petre Pircalabu 2017-08-09 8:11 ` Paul Durrant @ 2017-08-22 8:09 ` Jan Beulich 2017-08-30 17:06 ` Petre Ovidiu PIRCALABU 1 sibling, 1 reply; 10+ messages in thread From: Jan Beulich @ 2017-08-22 8:09 UTC (permalink / raw) To: Petre Pircalabu Cc: tim, kevin.tian, sstabellini, wei.liu2, rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, xen-devel, paul.durrant, tamas, jun.nakajima >>> On 08.08.17 at 20:06, <ppircalabu@bitdefender.com> wrote: > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c What about the use in a switch() statement in hvmemul_do_io() in this file? And the use in hvmemul_do_io_buffer()? > @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) > switch ( rc ) > { > case X86EMUL_UNHANDLEABLE: > + /* fall-through */ > + case X86EMUL_UNIMPLEMENTED: The fall-through comment is pointless in such a case. hvm/intercept.c has a use in hvm_process_io_intercept() which looks like it needs dealing with too. And there are more. Any places you perhaps leave alone intentionally should be reasoned about in the description. > @@ -7717,7 +7717,7 @@ x86_emulate( > > default: > cannot_emulate: > - rc = X86EMUL_UNHANDLEABLE; > + rc = X86EMUL_UNIMPLEMENTED; There's at least one goto to the label here which can't stay as is (in invoke_stub()). Did you really audit them all? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v8 1/2] x86emul: New return code for unimplemented instruction 2017-08-22 8:09 ` Jan Beulich @ 2017-08-30 17:06 ` Petre Ovidiu PIRCALABU 2017-08-31 7:36 ` Jan Beulich 0 siblings, 1 reply; 10+ messages in thread From: Petre Ovidiu PIRCALABU @ 2017-08-30 17:06 UTC (permalink / raw) To: Jan Beulich Cc: tim@xen.org, kevin.tian@intel.com, sstabellini@kernel.org, wei.liu2@citrix.com, rcojocaru@bitdefender.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, paul.durrant@citrix.com, tamas@tklengyel.com, jun.nakajima@intel.com [-- Attachment #1.1: Type: text/plain, Size: 2563 bytes --] Hi Jan, The main use-case for the new return code is to have a clear distinction between an instruction not implemented by the emulator (e.g. ?fldenv or fnstenv) and the failure to emulate . - hvm_process_io_incercept returns X86EMUL_UNHANDLEABLE if one of the hvm_io_ops (read/write) failed or one of the hvm_copy_to(_from)_guest_phys returned an error code which is not handled in their correspondent switch statement. In either cases this is not caused by an unimplemented instruction. - hvm_do_io / hvm_do_io_buffer call hvm_process_io_incercept which cannot return unimplemented. - Thank-you very much for pointing out the invoke_stub issue. I have added a new label "unimplemented_insn" and updated the patch. I will re-send a new patchset with the changes and a more detailed description of the places where the new return value was not handled. Many thanks, Petre ________________________________ From: Jan Beulich <JBeulich@suse.com> Sent: Tuesday, August 22, 2017 11:09 AM To: Petre Ovidiu PIRCALABU Cc: rcojocaru@bitdefender.com; andrew.cooper3@citrix.com; paul.durrant@citrix.com; wei.liu2@citrix.com; George.Dunlap@eu.citrix.com; ian.jackson@eu.citrix.com; jun.nakajima@intel.com; kevin.tian@intel.com; sstabellini@kernel.org; xen-devel@lists.xen.org; konrad.wilk@oracle.com; tamas@tklengyel.com; tim@xen.org Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplemented instruction >>> On 08.08.17 at 20:06, <ppircalabu@bitdefender.com> wrote: > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c What about the use in a switch() statement in hvmemul_do_io() in this file? And the use in hvmemul_do_io_buffer()? > @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) > switch ( rc ) > { > case X86EMUL_UNHANDLEABLE: > + /* fall-through */ > + case X86EMUL_UNIMPLEMENTED: The fall-through comment is pointless in such a case. hvm/intercept.c has a use in hvm_process_io_intercept() which looks like it needs dealing with too. And there are more. Any places you perhaps leave alone intentionally should be reasoned about in the description. > @@ -7717,7 +7717,7 @@ x86_emulate( > > default: > cannot_emulate: > - rc = X86EMUL_UNHANDLEABLE; > + rc = X86EMUL_UNIMPLEMENTED; There's at least one goto to the label here which can't stay as is (in invoke_stub()). Did you really audit them all? Jan ________________________ This email was scanned by Bitdefender [-- Attachment #1.2: Type: text/html, Size: 4073 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v8 1/2] x86emul: New return code for unimplemented instruction 2017-08-30 17:06 ` Petre Ovidiu PIRCALABU @ 2017-08-31 7:36 ` Jan Beulich 0 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2017-08-31 7:36 UTC (permalink / raw) To: Petre Ovidiu PIRCALABU Cc: tim@xen.org, kevin.tian@intel.com, sstabellini@kernel.org, wei.liu2@citrix.com, rcojocaru@bitdefender.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, paul.durrant@citrix.com, tamas@tklengyel.com, jun.nakajima@intel.com >>> On 30.08.17 at 19:06, <ppircalabu@bitdefender.com> wrote: Please don't top-post. It makes it quite hard to see ... > The main use-case for the new return code is to have a clear distinction > between an instruction not implemented by the emulator (e.g. ?fldenv or > fnstenv) and the failure to emulate . > > > - hvm_process_io_incercept returns X86EMUL_UNHANDLEABLE if one of the > hvm_io_ops (read/write) failed or one of the hvm_copy_to(_from)_guest_phys > returned an error code which is not handled in their correspondent switch > statement. In either cases this is not caused by an unimplemented > instruction. > > - hvm_do_io / hvm_do_io_buffer call hvm_process_io_incercept which cannot > return unimplemented. > > - Thank-you very much for pointing out the invoke_stub issue. I have added a > new label "unimplemented_insn" and updated the patch. ... which of the replies above correspond to which of my earlier replies. Jan > I will re-send a new patchset with the changes and a more detailed > description of the places where the new return value was not handled. > > > Many thanks, > > Petre > > > ________________________________ > From: Jan Beulich <JBeulich@suse.com> > Sent: Tuesday, August 22, 2017 11:09 AM > To: Petre Ovidiu PIRCALABU > Cc: rcojocaru@bitdefender.com; andrew.cooper3@citrix.com; > paul.durrant@citrix.com; wei.liu2@citrix.com; George.Dunlap@eu.citrix.com; > ian.jackson@eu.citrix.com; jun.nakajima@intel.com; kevin.tian@intel.com; > sstabellini@kernel.org; xen-devel@lists.xen.org; konrad.wilk@oracle.com; > tamas@tklengyel.com; tim@xen.org > Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplemented > instruction > >>>> On 08.08.17 at 20:06, <ppircalabu@bitdefender.com> wrote: >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c > > What about the use in a switch() statement in hvmemul_do_io() > in this file? And the use in hvmemul_do_io_buffer()? > >> @@ -2044,6 +2044,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned > long gla) >> switch ( rc ) >> { >> case X86EMUL_UNHANDLEABLE: >> + /* fall-through */ >> + case X86EMUL_UNIMPLEMENTED: > > The fall-through comment is pointless in such a case. > > hvm/intercept.c has a use in hvm_process_io_intercept() which > looks like it needs dealing with too. And there are more. Any > places you perhaps leave alone intentionally should be reasoned > about in the description. > >> @@ -7717,7 +7717,7 @@ x86_emulate( >> >> default: >> cannot_emulate: >> - rc = X86EMUL_UNHANDLEABLE; >> + rc = X86EMUL_UNIMPLEMENTED; > > There's at least one goto to the label here which can't stay as is > (in invoke_stub()). Did you really audit them all? > > Jan > > > ________________________ > This email was scanned by Bitdefender _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails. 2017-08-08 18:06 [PATCH v8 0/2] Singlestep unimplemented x86emul instructions Petre Pircalabu 2017-08-08 18:06 ` [PATCH v8 1/2] x86emul: New return code for unimplemented instruction Petre Pircalabu @ 2017-08-08 18:06 ` Petre Pircalabu 2017-08-08 18:25 ` Tamas K Lengyel ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Petre Pircalabu @ 2017-08-08 18:06 UTC (permalink / raw) To: xen-devel Cc: Petre Pircalabu, kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, tim, paul.durrant, tamas, jbeulich If case of a vm_event with the emulate_flags set, if the instruction is not implemented by the emulator, the monitor should be notified instead of directly injecting a hw exception. This behavior can be used to re-execute an instruction not supported by the emulator using the real processor (e.g. altp2m) instead of just crashing. Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> --- tools/libxc/include/xenctrl.h | 2 ++ tools/libxc/xc_monitor.c | 14 ++++++++++++++ xen/arch/x86/hvm/emulate.c | 4 ++++ xen/arch/x86/hvm/monitor.c | 17 +++++++++++++++++ xen/arch/x86/monitor.c | 13 +++++++++++++ xen/include/asm-x86/domain.h | 1 + xen/include/asm-x86/hvm/monitor.h | 1 + xen/include/asm-x86/monitor.h | 3 ++- xen/include/public/domctl.h | 1 + xen/include/public/vm_event.h | 2 ++ 10 files changed, 57 insertions(+), 1 deletion(-) diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index c7710b8..abb9cac 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2027,6 +2027,8 @@ int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id, int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable); int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id, bool enable); +int xc_monitor_emul_unimplemented(xc_interface *xch, domid_t domain_id, + bool enable); /** * This function enables / disables emulation for each REP for a * REP-compatible instruction. diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c index b44ce93..8a76ec1 100644 --- a/tools/libxc/xc_monitor.c +++ b/tools/libxc/xc_monitor.c @@ -216,6 +216,20 @@ int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id, return do_domctl(xch, &domctl); } +int xc_monitor_emul_unimplemented(xc_interface *xch, domid_t domain_id, + bool enable) +{ + DECLARE_DOMCTL; + + domctl.cmd = XEN_DOMCTL_monitor_op; + domctl.domain = domain_id; + domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE + : XEN_DOMCTL_MONITOR_OP_DISABLE; + domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED; + + return do_domctl(xch, &domctl); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 28133c0..64a17f1 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -14,12 +14,14 @@ #include <xen/sched.h> #include <xen/paging.h> #include <xen/trace.h> +#include <xen/vm_event.h> #include <asm/event.h> #include <asm/i387.h> #include <asm/xstate.h> #include <asm/hvm/emulate.h> #include <asm/hvm/hvm.h> #include <asm/hvm/ioreq.h> +#include <asm/hvm/monitor.h> #include <asm/hvm/trace.h> #include <asm/hvm/support.h> #include <asm/hvm/svm/svm.h> @@ -2116,6 +2118,8 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, */ return; case X86EMUL_UNIMPLEMENTED: + if ( hvm_monitor_emul_unimplemented() ) + return; /* fall-through */ case X86EMUL_UNHANDLEABLE: hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx); diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index a7ccfc4..3551463 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -57,6 +57,23 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old return 0; } +bool hvm_monitor_emul_unimplemented(void) +{ + struct vcpu *curr = current; + + /* + * Send a vm_event to the monitor to signal that the current + * instruction couldn't be emulated. + */ + vm_event_request_t req = { + .reason = VM_EVENT_REASON_EMUL_UNIMPLEMENTED, + .vcpu_id = curr->vcpu_id, + }; + + return curr->domain->arch.monitor.emul_unimplemented_enabled && + monitor_traps(curr, true, &req) == 1; +} + void hvm_monitor_msr(unsigned int msr, uint64_t value) { struct vcpu *curr = current; diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 706454f..e59f1f5 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -283,6 +283,19 @@ int arch_monitor_domctl_event(struct domain *d, break; } + case XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED: + { + bool old_status = ad->monitor.emul_unimplemented_enabled; + + if ( unlikely(old_status == requested_status) ) + return -EEXIST; + + domain_pause(d); + ad->monitor.emul_unimplemented_enabled = requested_status; + domain_unpause(d); + break; + } + default: /* * Should not be reached unless arch_monitor_get_capabilities() is diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index c10522b..091447d 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -405,6 +405,7 @@ struct arch_domain unsigned int debug_exception_sync : 1; unsigned int cpuid_enabled : 1; unsigned int descriptor_access_enabled : 1; + unsigned int emul_unimplemented_enabled : 1; struct monitor_msr_bitmap *msr_bitmap; uint64_t write_ctrlreg_mask[4]; } monitor; diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h index d9efb35..0979adf 100644 --- a/xen/include/asm-x86/hvm/monitor.h +++ b/xen/include/asm-x86/hvm/monitor.h @@ -47,6 +47,7 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, unsigned int subleaf); void hvm_monitor_interrupt(unsigned int vector, unsigned int type, unsigned int err, uint64_t cr2); +bool hvm_monitor_emul_unimplemented(void); #endif /* __ASM_X86_HVM_MONITOR_H__ */ diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index c5c323b..37255c9 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -77,7 +77,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d) (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) | (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) | (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) | - (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT); + (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) | + (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED); /* Since we know this is on VMX, we can just call the hvm func */ if ( hvm_is_singlestep_supported() ) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 0669c31..474aed0 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1083,6 +1083,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t); #define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL 7 #define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT 8 #define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS 9 +#define XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED 10 struct xen_domctl_monitor_op { uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */ diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index f01e471..b531f71 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -148,6 +148,8 @@ #define VM_EVENT_REASON_INTERRUPT 12 /* A descriptor table register was accessed. */ #define VM_EVENT_REASON_DESCRIPTOR_ACCESS 13 +/* Current instruction is not implemented by the emulator */ +#define VM_EVENT_REASON_EMUL_UNIMPLEMENTED 14 /* Supported values for the vm_event_write_ctrlreg index. */ #define VM_EVENT_X86_CR0 0 -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails. 2017-08-08 18:06 ` [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu @ 2017-08-08 18:25 ` Tamas K Lengyel 2017-08-09 7:03 ` Wei Liu 2017-08-22 8:10 ` Jan Beulich 2 siblings, 0 replies; 10+ messages in thread From: Tamas K Lengyel @ 2017-08-08 18:25 UTC (permalink / raw) To: Petre Pircalabu Cc: Tim Deegan, Kevin Tian, Stefano Stabellini, wei.liu2@citrix.com, Jun Nakajima, Razvan Cojocaru, George Dunlap, Andrew Cooper, Ian Jackson, Xen-devel, Paul Durrant, Jan Beulich [-- Attachment #1.1: Type: text/plain, Size: 546 bytes --] On Tue, Aug 8, 2017 at 12:06 PM, Petre Pircalabu <ppircalabu@bitdefender.com > wrote: > If case of a vm_event with the emulate_flags set, if the instruction > is not implemented by the emulator, the monitor should be notified instead > of directly injecting a hw exception. > This behavior can be used to re-execute an instruction not supported by > the emulator using the real processor (e.g. altp2m) instead of just > crashing. > > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> [-- Attachment #1.2: Type: text/html, Size: 998 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails. 2017-08-08 18:06 ` [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu 2017-08-08 18:25 ` Tamas K Lengyel @ 2017-08-09 7:03 ` Wei Liu 2017-08-22 8:10 ` Jan Beulich 2 siblings, 0 replies; 10+ messages in thread From: Wei Liu @ 2017-08-09 7:03 UTC (permalink / raw) To: Petre Pircalabu Cc: tim, kevin.tian, sstabellini, wei.liu2, jun.nakajima, rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, xen-devel, paul.durrant, tamas, jbeulich On Tue, Aug 08, 2017 at 09:06:38PM +0300, Petre Pircalabu wrote: > If case of a vm_event with the emulate_flags set, if the instruction > is not implemented by the emulator, the monitor should be notified instead > of directly injecting a hw exception. > This behavior can be used to re-execute an instruction not supported by > the emulator using the real processor (e.g. altp2m) instead of just > crashing. > > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> > --- > tools/libxc/include/xenctrl.h | 2 ++ > tools/libxc/xc_monitor.c | 14 ++++++++++++++ Acked-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails. 2017-08-08 18:06 ` [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu 2017-08-08 18:25 ` Tamas K Lengyel 2017-08-09 7:03 ` Wei Liu @ 2017-08-22 8:10 ` Jan Beulich 2 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2017-08-22 8:10 UTC (permalink / raw) To: Petre Pircalabu Cc: tim, kevin.tian, sstabellini, wei.liu2, rcojocaru, George.Dunlap, andrew.cooper3, ian.jackson, xen-devel, paul.durrant, tamas, jun.nakajima >>> On 08.08.17 at 20:06, <ppircalabu@bitdefender.com> wrote: > If case of a vm_event with the emulate_flags set, if the instruction > is not implemented by the emulator, the monitor should be notified instead > of directly injecting a hw exception. > This behavior can be used to re-execute an instruction not supported by > the emulator using the real processor (e.g. altp2m) instead of just > crashing. > > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> For the parts it is applicable to Acked-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-08-31 7:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-08 18:06 [PATCH v8 0/2] Singlestep unimplemented x86emul instructions Petre Pircalabu 2017-08-08 18:06 ` [PATCH v8 1/2] x86emul: New return code for unimplemented instruction Petre Pircalabu 2017-08-09 8:11 ` Paul Durrant 2017-08-22 8:09 ` Jan Beulich 2017-08-30 17:06 ` Petre Ovidiu PIRCALABU 2017-08-31 7:36 ` Jan Beulich 2017-08-08 18:06 ` [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails Petre Pircalabu 2017-08-08 18:25 ` Tamas K Lengyel 2017-08-09 7:03 ` Wei Liu 2017-08-22 8:10 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).