Re: [Xen-devel] [PATCH v2 13/19] x86/shadow: Avoid raising faults behind the emulators back

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/11/16 17:21, Tim Deegan wrote:
>>> I think it would be better to run this check before the 
>>> X86EMUL_UNHANDLEABLE one
>>> and convert injections that we choose not to handle into
>>> X86EMUL_UNHANDLEABLE.
>>>
>>> Which I guess brings us back full circle to the behaviour we had
>>> before, and perhaps the right change to the earlier patch is to
>>> start down this road, with an explanatory comment.
>>>
>>> e.g. start with something like this, immediately after the first call
>>> to x86_emulate():
>>>
>>>     /*
>>>      * Events raised within the emulator itself used to return
>>>      * X86EMUL_UNHANDLEABLE because we didn't supply injection
>>>      * callbacks.  Now the emulator supplies those to us via
>>>      * ctxt.event_pending instead.  Preserve the old behaviour
>>>      * for now.
>>>      */
>>>      if (emul_ctxt.ctxt.event_pending)
>>>          r = X86EMUL_UNHANDLEABLE;
>>>
>>> And in this patch, replace it with the hunk you have above, but
>>> setting r = X86EMUL_UNHANDLEABLE instead of injecting #GP.
>>>
>>> Does that make sense?
>> It does make sense, but that goes against the comment of not unshadowing
>> on exception.
> Yep, that comment would need to be updated.  The final behaviour is
> something like what we had before: some kinds of event (#PF in
> particular) get injected, and for the others we unshadow and retry.
> You've expanded the set of events that we'll inject, and the mechanism has
> moved around.

Ok.  I will drop the #GP's and cause this to fall into the unhandleable
path.

>
>> A lot of this revolves around how likely we are to hit the #GP[0] case. 
>> I assert that we shouldn't be able to hit it unless the guest is playing
>> games, or we have a bug in emulation.
>>
>> If we don't go throwing a #GP back, we should at least leave something
>> obvious in the log.
>>
>>> Also, I'm a little confused after all this as to whether the emulator
>>> can still return with X86EMUL_OKAY and the event_pending set.
>> I have now determined this not to be the case.  Apologies for my
>> misinformation in v1.
> Phew!

Yes.  Despite being the last person to fix this several times, it is
very opaque code.

>
>>> This looks like code duplication, but rather than trying to merge the
>>> two cases, I think we can drop this one entirely.  This emulation is
>>> optimistically trying to find the second half of a PAE PTE write -
>>> it's OK just to stop emulating if we hit anything this exciting.
>>> So we can lose the whole hunk.
>> At the very least we should retain the singlestep #DB injection, as it
>> still has trap semantics.
> Argh!  Meaning it returns X86EMUL_EXCEPTION but has already updated
> register state?

Yes

> So yeah, we have to inject that.  But it can go away
> when you change everything to have fault semantics, right?

No.  Singlestep comes out as a hardware exception, not a software interrupt.

Implemented in this way, it is always going to be a special case; we
must manually raise the singlestep event if necessary, as hardware won't
do it automatically on re-entry to the guest.

~Andrew

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