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

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

On Ma, 2017-09-05 at 09:46 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 05.09.17 at 17:23, <ppircalabu@bitdefender.com> wrote:
> > 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).
> Yes, that's what I think would be best. I admit there is a potential
> problem with this, when one or more of them become defined. But
> that's something I'd like to also have Andrew's input on.
>
> >
> > 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.
> I must still be missing something: What two cases are you talking
> about? The three valid values have an implementation in the
> emulator. All others are undefined, i.e. at present would ideally
> produce #UD (see above).

Probably a better example it the handling of Grp7 (after applying
Andrew's patch).
if modrm is 0xd0 the instruction is xgetbv (valid but unimplemented)
if modrm value is corrupted (the encoding doesn't correspond to a valid
instruction it will also jump to emul_unimplemented.
>
> Jan
>
>
> ________________________
> This email was scanned by Bitdefender

Many thanks,
Petre

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