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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.09.17 at 18:20, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> On Ma, 2017-09-05 at 09:46 -0600, Jan Beulich wrote:
>> >
>> > >
>> > > >
>> > > > On 05.09.17 at 17:23, <ppircalabu@xxxxxxxxxxxxxxx> 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.

In cases when there is a mix of missing and undefined opcodes
I would view it as acceptable to go the unimplemented path
without further splitting. In cases where all unhandled cases
are also undefined, I'd prefer #UD to be raised.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel