Re: [PATCH v12 1/4] x86emul: New return code for unimplemented instruction

[Petre Ovidiu PIRCALABU <ppircalabu@bitdefender.com>]

On Jo, 2017-09-21 at 06:42 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 21.09.17 at 07:12, <ppircalabu@bitdefender.com> wrote:
> > --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> > @@ -6195,7 +6196,7 @@ x86_emulate(
> >                  /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */
> >              break;
> >          default:
> > -            goto cannot_emulate;
> > +            goto unimplemented_insn;
> I would really appreciate if you were a little more patient and
> waited
> for replies to earlier review threads before sending a new version.
> As said on the v11 thread, this ought to be "unrecognized".
Thank-you very much for clearing this out. I will change the return
value to "unrecognized" in the next patchset iteration.

> 
> > 
> > @@ -6243,7 +6244,8 @@ x86_emulate(
> >          case 6: /* psllq $imm8,mm */
> >              goto simd_0f_shift_imm;
> >          }
> > -        goto cannot_emulate;
> > +        rc = X86EMUL_UNRECOGNIZED;
> > +        goto done;
> I think it would read better if we had an "unrecognized_insn"
> label just like now we gain an "unimplemented_insn" one. Whether
> the _insn suffixes are really useful is another question.

The best place to add this label is, in my opinion, at the end of the
"default" label of the "switch ( ctxt->opcode )" statement in
x86_emulate. This will make sure the current instruction flow will not
be altered.
e.g.:
     case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */
     {
@@ -7750,6 +7742,9 @@ x86_emulate(
     unimplemented_insn:
         rc = X86EMUL_UNIMPLEMENTED;
         goto done;
+    unrecognized_insn:
+        rc = X86EMUL_UNRECOGNIZED;
+        goto done;
     }
Do you find this approach OK?
> 
> > 
> > --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> > @@ -133,6 +133,19 @@ struct x86_emul_fpu_aux {
> >    * Undefined behavior when used anywhere else.
> >    */
> >  #define X86EMUL_DONE           4
> > + /*
> > +  * Current instruction is not implemented by the emulator.
> > +  * This value should only be returned by the core emulator when a
> > valid
> > +  * opcode is found but the execution logic for that instruction
> > is missing.
> > +  * It should NOT be returned by any of the x86_emulate_ops
> > callbacks.
> > +  */
> > +#define X86EMUL_UNIMPLEMENTED  5
> > + /*
> > +  * The current instruction's opcode is not valid.
> > +  * If this error code is returned by a function, an #UD trap
> > should be
> > +  * raised by the final consumer of it.
> > + */
> > +#define X86EMUL_UNRECOGNIZED   X86EMUL_UNIMPLEMENTED
> But with this aliasing of values the comment still is somewhat off.

Do you think adding a "TODO:" comment can make things more clear?
e.g.:
   * The current instruction's opcode is not valid.
   * If this error code is returned by a function, an #UD trap should e
   * raised by the final consumer of it.
+  *
+  * TODO: For the moment X86EMUL_UNRECOGNIZED and 86EMUL_UNIMPLEMENTED
+  * can be used interchangeably.
  */

Many thanks for your support,
//Petre
> 
> Jan
> 
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel