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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/11/16 12:04, Tim Deegan wrote:
> At 11:13 +0000 on 28 Nov (1480331605), Andrew Cooper wrote:
>> The emulator needs to gain an understanding of interrupts and exceptions
>> generated by its actions.
>>
>> Move hvm_emulate_ctxt.{exn_pending,trap} into struct x86_emulate_ctxt so they
>> are visible to the emulator.  This removes the need for the
>> inject_{hw_exception,sw_interrupt}() hooks, which are dropped and replaced
>> with x86_emul_{hw_exception,software_event,reset_event}() instead.
>>
>> For exceptions raised by x86_emulate() itself (rather than its callbacks), 
>> the
>> shadow pagetable and PV uses of x86_emulate() previously failed with
>> X86EMUL_UNHANDLEABLE due to the lack of inject_*() hooks.
>>
>> This behaviour has changed, and such cases will now return X86EMUL_EXCEPTION
>> with event_pending set.  Until the callers of x86_emulate() have been updated
>> to inject events back into the guest, divert the event_pending case back into
>> the X86EMUL_UNHANDLEABLE path to maintain the same guest-visible behaviour.
>>
>> No overall functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -3374,11 +3374,23 @@ 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);
>> +
> Here I'll grumble about adding this twice in the same function, when
> IMO it ought to be asserted in the emulator instead.  Given that it
> mostly disappears later I'll let it stand if you prefer.

It disappears from different call-sites at different points.  The
PV-only cases are fully resolved in this series, whereas neither the
shadow or HVM cases are fully resolved.

>
>> +    /*
>>       * 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.

We previously hit this path for exceptions raised from within the
emulator, such as singlestepping.  Exceptions raised behind the back of
emulator do not set event_pending yet.

The behaviour of exceptions raised within the emulator is fixed later in
patch 13, but this change does not alter the guest-observed behaviour.

~Andrew

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