Re: [Xen-devel] [PATCH v2 08/19] x86/emul: Rework emulator event injection

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/11/16 14:24, Tim Deegan wrote:
> At 12:48 +0000 on 28 Nov (1480337304), Andrew Cooper wrote:
>> On 28/11/16 12:04, Tim Deegan wrote:
>>> At 11:13 +0000 on 28 Nov (1480331605), Andrew Cooper wrote:
>>>> +    /*
>>>>       * NB. We do not unshadow on X86EMUL_EXCEPTION. It's not clear that it
>>>>       * would be a good unshadow hint. If we *do* decide to 
>>>> unshadow-on-fault
>>>>       * then it must be 'failable': we cannot require the unshadow to 
>>>> succeed.
>>>>       */
>>>> -    if ( r == X86EMUL_UNHANDLEABLE )
>>>> +    if ( r == X86EMUL_UNHANDLEABLE || emul_ctxt.ctxt.event_pending )
>>> No thank you.  The comment there explains why we don't want to
>>> unshadow for an injection; please let it stand.  Or if the new
>>> semantics have changed, update the comment.
>> This addition is no functional behavioural change from before, which is
>> the point I was trying to get across.
> I can understand why you want to do it that way but it makes the
> series (and this code!) read quite oddly.  If you keep this, then
> please add a second comment above this line that explains why the code
> temporarily disagrees with the first comment. :)  You can delete that
> comment again in patch #13.
>
> With that, Acked-by: Tim Deegan <tim@xxxxxxx>

How about this?

~Andrew

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3374,11 +3374,33 @@ static int sh_page_fault(struct vcpu *v,
     r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
 
     /*
+     * TODO: Make this true:
+     *
+    ASSERT(emul_ctxt.ctxt.event_pending == (rc == X86EMUL_EXCEPTION));
+     *
+     * Some codepaths still raise exceptions behind the back of the
+     * emulator. (i.e. return X86EMUL_EXCEPTION but without event_pending
+     * being set).  In the meantime, use a slightly relaxed check...
+     */
+    if ( emul_ctxt.ctxt.event_pending )
+        ASSERT(r == X86EMUL_EXCEPTION);
+
+    /*
      * NB. We do not unshadow on X86EMUL_EXCEPTION. It's not clear that it
      * would be a good unshadow hint. If we *do* decide to
unshadow-on-fault
      * then it must be 'failable': we cannot require the unshadow to
succeed.
+     *
+     * Note: Despite the above comment, this path has actually been handing
+     * exception circumstances raised by the emulator itself (e.g.
singlestep)
+     * because of the lack of the inject_hw_exception() hook.
+     *
+     * With this change, exceptions raised behind the back of the emulator
+     * still return without setting event_pending, but exceptions raised by
+     * the emulator do.  Force these exceptions back onto the UNHANDLEABLE
+     * path for now, so they are similarly ignored.  A future change
will fix
+     * this properly.
      */
-    if ( r == X86EMUL_UNHANDLEABLE )
+    if ( r == X86EMUL_UNHANDLEABLE || emul_ctxt.ctxt.event_pending )
     {
         perfc_incr(shadow_fault_emulate_failed);



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