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

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,


From: Jan Beulich <JBeulich@xxxxxxxx>
Sent: Tuesday, August 22, 2017 11:09 AM
To: Petre Ovidiu PIRCALABU
Cc: rcojocaru@xxxxxxxxxxxxxxx; andrew.cooper3@xxxxxxxxxx; paul.durrant@xxxxxxxxxx; wei.liu2@xxxxxxxxxx; George.Dunlap@xxxxxxxxxxxxx; ian.jackson@xxxxxxxxxxxxx; jun.nakajima@xxxxxxxxx; kevin.tian@xxxxxxxxx; sstabellini@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; konrad.wilk@xxxxxxxxxx; tamas@xxxxxxxxxxxxx; tim@xxxxxxx
Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplemented instruction
>>> On 08.08.17 at 20:06, <ppircalabu@xxxxxxxxxxxxxxx> 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 )
>      {
> +        /* fall-through */

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?


This email was scanned by Bitdefender
Xen-devel mailing list



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