From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petre Ovidiu PIRCALABU Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplemented instruction Date: Wed, 30 Aug 2017 17:06:27 +0000 Message-ID: <1504112787319.19120@bitdefender.com> References: <1502215598-4689-1-git-send-email-ppircalabu@bitdefender.com> <1502215598-4689-2-git-send-email-ppircalabu@bitdefender.com>, <599C02CB0200007800171CD3@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4655079626750832188==" Return-path: In-Reply-To: <599C02CB0200007800171CD3@prv-mh.provo.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: "tim@xen.org" , "kevin.tian@intel.com" , "sstabellini@kernel.org" , "wei.liu2@citrix.com" , "rcojocaru@bitdefender.com" , "George.Dunlap@eu.citrix.com" , "andrew.cooper3@citrix.com" , "ian.jackson@eu.citrix.com" , "xen-devel@lists.xen.org" , "paul.durrant@citrix.com" , "tamas@tklengyel.com" , "jun.nakajima@intel.com" List-Id: xen-devel@lists.xenproject.org --===============4655079626750832188== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_150411278731919120bitdefendercom_" --_000_150411278731919120bitdefendercom_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Hi Jan, The main use-case for the new return code is to have a clear distinction be= tween an instruction not implemented by the emulator (e.g. ?fldenv or fnste= nv) and the failure to emulate . - hvm_process_io_incercept returns X86EMUL_UNHANDLEABLE if one of the hvm_i= o_ops (read/write) failed or one of the hvm_copy_to(_from)_guest_phys retur= ned an error code which is not handled in their correspondent switch statem= ent. 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 r= eturn 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 descript= ion 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: rcojocaru@bitdefender.com; andrew.cooper3@citrix.com; paul.durrant@citr= ix.com; wei.liu2@citrix.com; George.Dunlap@eu.citrix.com; ian.jackson@eu.ci= trix.com; jun.nakajima@intel.com; kevin.tian@intel.com; sstabellini@kernel.= org; xen-devel@lists.xen.org; konrad.wilk@oracle.com; tamas@tklengyel.com; = tim@xen.org Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplemented inst= ruction >>> 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, unsigne= d 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 =3D X86EMUL_UNHANDLEABLE; > + rc =3D 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 --_000_150411278731919120bitdefendercom_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable

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. ​f= ldenv 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 correspon= dent 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 w= hich cannot return unimplemented.

- Thank-you very much for pointing out the invoke_stub issue. I have add= ed a new label "unimplemented_insn" and updated the patch.


I will re-send a new patchset with the changes and a more detailed descr= iption of the places where the new return value was not handled.


Many thanks,

Petre


From: Jan Beulich <JBe= ulich@suse.com>
Sent: Tuesday, August 22, 2017 11:09 AM
To: Petre Ovidiu PIRCALABU
Cc: rcojocaru@bitdefender.com; andrew.cooper3@citrix.com; paul.durra= nt@citrix.com; wei.liu2@citrix.com; George.Dunlap@eu.citrix.com; ian.jackso= n@eu.citrix.com; jun.nakajima@intel.com; kevin.tian@intel.com; sstabellini@= kernel.org; xen-devel@lists.xen.org; konrad.wilk@oracle.com; tamas@tklengyel.com; tim@xen.org
Subject: Re: [PATCH v8 1/2] x86emul: New return code for unimplement= ed instruction
 
>>> On 08.08.17 at 20:06, <ppircalabu@= 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 =3D X86EMUL_UNHANDLEABL= E;
> +        rc =3D X86EMUL_UNIMPLE= MENTED;

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
--_000_150411278731919120bitdefendercom_-- --===============4655079626750832188== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============4655079626750832188==--