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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> 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.
>*/
>return;
>+    case X86EMUL_UNIMPLEMENTED:
>case X86EMUL_UNHANDLEABLE:
>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(
 >
>default:
>cannot_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. */
>+#define X86EMUL_UNIMPLEMENTED  5

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

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel