| Message ID | 1502215598-4689-2-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com] > Sent: 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
>>> 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
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
>>> 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
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 {
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(-)