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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx> 09/04/17 7:20 PM >>>
>On Vi, 2017-09-01 at 04:33 -0600, Jan Beulich wrote:
>> >
>> > >
>> > > >
>> > > > On 30.08.17 at 20:57, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/hvm/emulate.c
>> > +++ b/xen/arch/x86/hvm/emulate.c
>> > @@ -2044,6 +2044,7 @@ int hvm_emulate_one_mmio(unsigned long mfn,
>> > unsigned long gla)
>> Way earlier in this file there are consumers of X86EMUL_UNHANDLEABLE
>> in hvmemul_do_io() and hvmemul_do_io_buffer(). I'm sure I did
>> point this out before, and I cannot see why you don't adjust those as
>> well, and you also don't say anything in this regard in the
>> description.
>> Similarly there's a consumer in hvm_process_io_intercept() (in a file
>> you don't touch at all). The use in hvm_broadcast_ioreq() is likely
>> fine, but I'd still like you to reason about that in the description.
>
>My mistake. I have added my comments in the cover letter as I thought
>they will be easier to read. I will add them the the patch description
>for the next iteration.

Thanks, that'll make them stay with the changes once committed. Your
further explanations are too long for a commit message though, imo. Aiui
they all boil down to the uses being in functions sitting behind rather than
in front of x86_emulate(). If that's the case, I would suggest you state just
that fact along with the enumeration of places that don't need touching for
that reason. This will make reviewing (and later viewing/understanding)
quite a bit easier.

>> > @@ -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.

>> > @@ -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


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