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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.17 at 15:48, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> 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 returned by the core emulator only if it fails to
> properly decode the current instruction's opcode, and not by any of other
> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks.
> 
> e.g. hvm_process_io_incercept should not 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.
> 
> Similary, none of this functions should not return X86EMUL_UNIMPLEMENTED.
>  - hvmemul_do_io
>  - hvm_send_buffered_ioreq
>  - hvm_send_ioreq
>  - hvm_broadcast_ioreq
>  - hvmemul_do_io_buffer
>  - hvmemul_validate

Granted hvm_io_intercept() is only a relatively thin wrapper
around hvm_process_io_incercept(), but for completeness it
would have been nice to be included here too. Additionally it
would have helped reviewing as well as future development if you
had added ASSERT()s to this effect to all consumers where you
don't add a check for X86EMUL_UNIMPLEMENTED.

> Signed-off-by: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

The code in v10 changed from v9 or whichever earlier version
Paul had give his R-b on. You should have removed it for that
reason or, if he had given it for a limited range of the patch
which did not change, you should have indicated which range
that was.

Also somewhere here I'm missing a summary of the changes
from v9. Putting it in the overview mail is nice, but for
reviewing purposes it is far more useful to be right in the
affected patch.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c

After discussing with Andrew I'm willing to agree with the changes
you do here, with one extra requirement: At least on non-debug
builds X86EMUL_UNIMPLEMENTED should always result in #UD being
raised by the final consumer of it. It should never, like would be
the case with the changes you do to vmx/realmode.c, result in
the domain being crashed. Please change that one and check
carefully whether there are any other similar cases.

Jan


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