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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.17 at 17:52, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> On Jo, 2017-09-07 at 09:08 -0600, Jan Beulich wrote:
>> > 
>> > > 
>> > > > 
>> > > > On 07.09.17 at 16:26, <JBeulich@xxxxxxxx> wrote:
>> > After discussing with Andrew I'm willing to agree with the changes
>> > you do here, with one extra requirement: At least on non-debug
>> > builds X86EMUL_UNIMPLEMENTED should always result in #UD being
>> > raised by the final consumer of it. It should never, like would be
>> > the case with the changes you do to vmx/realmode.c, result in
>> > the domain being crashed. Please change that one and check
>> > carefully whether there are any other similar cases.
> 
> Changing the way we handle X86EMUL_UNIMPLEMENTED in some of the
> functions will modify the existing behavior, and I'm a little bit wary
> of making so many changes unrelated to the current patchset'a purpose
> without a thourough way of testing them.

But leaving code you don't directly care about in a state not in
line with the new distinction isn't any better. The groundwork is
to have all existing code honor the new distinction in a sensible
way. Only then comes your intended behavioral change to
VM event handling.

However, thinking about the actually three different scenarios
again, I'm no longer sure either your model or the one I've been
suggesting would be correct, which would get us half way back
to what I've been asking for before. These are the cases to
care about:

- X86EMUL_UNHANDLEABLE: something went wrong while
  processing a recognized instruction (or e.g. while decoding);
  current behavior is fine to retain
- recognized but not implemented: these must not #UD, so
  behavior matching the unhandleable case is what we want
- unrecognized instruction: these should #UD

The problem is that you want to use the same error indicator
for both of the latter two cases. And the "problem" with adding
another new error indicator (e.g. X86EMUL_UNRECOGNIZED) is
that then we need to adjust the emulator when we add support
for new CPUID bits. But that's the long term goal anyway, so I
have to admit that I don't see anything wrong with this, other
than this causing some additional work.

On the grounds that present behavior isn't coming anywhere
close to the outlined target behavior and that you're patch
isn't making the situation worse, I'm now inclined to agree that
the best way forward is to live with the remaining inconsistency
until we've managed to sufficiently fill the gaps. That is, the
patch can, in this respect, stay as it was. I would like to ask you
to extend the comment on the definition though, outlining the
target behavior (perhaps including the other new return value
in that comment, or simply adding

#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED

for the time being).

The alternative of having X86EMUL_UNRECOGNIZED would be to
make the emulator raise #UD itself in such cases, as suggested
earlier.

Andrew, thoughts?

> e.g.: "hvm_descriptor_access_intercept".
> The current behavior is to return false if X86EMUL_UNHANDLEABLE is
> returned by hvm_emulate_one. Up until now, this return code covered
> also the "unimplemented instruction" case.
> If X86EMUL_UNIMPLEMENTED will be handled separately (e.g. by calling
> hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC),
> hvm_emulate_writeback, and finally returning true) some of the
> scenarios where the domain got crashed will result only in an UD being
> injected.

This function doesn't receive any X86EMUL_* values, so I don't
see how it would be relevant here. But with the above this is
likely moot anyway.

Jan


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