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 <jbeul...@suse.com>
Sent: Tuesday, August 22, 2017 11:09 AM
To: Petre Ovidiu PIRCALABU
Cc: rcojoc...@bitdefender.com; andrew.coop...@citrix.com;
paul.durr...@citrix.com; wei.l...@citrix.com; george.dun...@eu.citrix.com;
ian.jack...@eu.citrix.com; jun.nakaj...@intel.com; kevin.t...@intel.com;
sstabell...@kernel.org; xen-devel@lists.xen.org; konrad.w...@oracle.com;
ta...@tklengyel.com; t...@xen.org
Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplemented
instruction
>>> On 08.08.17 at 20:06, <ppircal...@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
