Re: [PATCH v9 1/2] x86emul: New return code for unimplemented instruction

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

On Lu, 2017-09-04 at 23:42 -0600, Jan Beulich wrote:
> > >
> > > >
> > > > @@ -5177,7 +5177,7 @@ x86_emulate(
> > > >                  goto done;
> > > >              break;
> > > >          default:
> > > > -            goto cannot_emulate;
> > > > +            goto unimplemented_insn;
> > > While I can see why you do this change, for many/all of the ones
> > > I'll leave in context below I think you rather want to switch to
> > > generate_exception(EXC_UD).
> > Some of the opcodes are valid but not supported by the emulator. In
> > this case X86EMUL_UNIMPLEMENTED should be returned to allow the
> > monitor
> > app to handle this case. Also, in the worst case scenario, when the
> > opcode doesn't correspond to a valid x86(-64) instruction, if the
> > monitor app for example tries to single-step it on the real
> > hardware an
> > UD exception will also be reported.
> Please be more precise with "some of the opcodes are valid". When I
> looked through your change, I don't think I've seen any such case for
> the
> places I meant the comment to apply to. Also, as far as the emulator
> changes themselves go, please leave aside considerations of what a
> monitor app may or may not do. These changes need to be consistent
> all by themselves.
>
Sorry about the poor wording. I was under the impression that you
required the invalid opcodes to be handled differently from the ones
which are valid but unimplemented (directly generate EXC_UD instead of
returning X86EMUL_UNIMPLEMENTED and let the caller handle it).

e.g. for  X86EMUL_OPC_VEX(0x0f38, 0xf3): /* Grp 17 */
the mod R/M possible values are 1,2,3 (source: http://sandpile.org/x86/
opc_grp.htm).
From my perspective I wouldn't differentiate between those 2 cases and
return X86EMUL_UNIMPLEMENTED. The performance penalty is negligible if
the monitor is neither present nor it implements
XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED. The monitor can only offer
a "second-chance" re-execution of the faulty instruction and does not
affect the internal logic of x86_decode/x86_emulate.

> > >
> > > >
> > > > @@ -7716,6 +7716,9 @@ x86_emulate(
> > > >      }
> > > >
> > Thanks for noticing it. I will change it back to cannot_emulate as
> > there are no other valid instructions for this opcodes.
> I have trouble understanding this comment of yours, not the least
> because I don't recall having asked (in particular around here) that
> you switch anything back.
>
> Jan
>
>
> ________________________
> This email was scanned by Bitdefender

Many thanks for your support,
//Petre

________________________
This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel