[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v7 1/2] x86emul: New return code for unimplemented instruction

>>> Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx> 08/04/17 8:36 PM >>>
>--- a/xen/arch/x86/hvm/emulate.c
>+++ b/xen/arch/x86/hvm/emulate.c
>@@ -2113,6 +2113,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, 
>unsigned int trapnr,
>* consistent with X86EMUL_RETRY.
>hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx);
>hvm_inject_hw_exception(trapnr, errcode);

I'm afraid there are more similar changes to be made. Any consumer of
UNHANDLEABLE needs to also check for UNIMPLEMENTED now (or an
explanation be given in the commit message why certain ones don't need
adjustment). Also I'd prefer if you put the new case below the existing one.

>--- a/xen/arch/x86/x86_emulate/x86_emulate.c
>+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>@@ -7717,7 +7717,7 @@ x86_emulate(
>-        rc = X86EMUL_UNHANDLEABLE;
>+        rc = X86EMUL_UNIMPLEMENTED;
>goto done;

Along the same lines, this is too little of an adjustment as well. For example,
there's "switch ( ext )" in the VEX decoding, which should be changed. A few
lines down from there a similar change for XOP decoding would be needed.
Right now it looks to me as if these two are the only ones you've missed.

>--- a/xen/arch/x86/x86_emulate/x86_emulate.h
>+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>@@ -133,6 +133,8 @@ struct x86_emul_fpu_aux {
>* Undefined behavior when used anywhere else.
>#define X86EMUL_DONE           4
>+ /* The instruction is not implemented by the emulator. */

Please extend the comment to state that only the core emulator is allowed
to return this (callbacks must not return it).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.