Re: [Xen-devel] [PATCH v8 1/2] x86emul: New return code for unimplemented instruction
>>> On 30.08.17 at 19:06, 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
> 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, 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
Re: [Xen-devel] [PATCH v8 1/2] x86emul: New return code for unimplemented instruction
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
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, 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
Re: [Xen-devel] [PATCH v8 1/2] x86emul: New return code for unimplemented instruction
>>> On 08.08.17 at 20:06, 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 1/2] x86emul: New return code for unimplemented instruction
> -Original Message-
> From: Petre Pircalabu [mailto:ppircal...@bitdefender.com]
> Sent: 08 August 2017 19:07
> To: xen-devel@lists.xen.org
> Cc: Ian Jackson ; Wei Liu ;
> Andrew Cooper ; George Dunlap
> ; jbeul...@suse.com; konrad.w...@oracle.com;
> sstabell...@kernel.org; Tim (Xen.org) ; Paul Durrant
> ; rcojoc...@bitdefender.com;
> ta...@tklengyel.com; jun.nakaj...@intel.com; Kevin Tian
> ; Petre Pircalabu
> 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
Reviewed-by: Paul Durrant
> ---
> 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
