[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

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

Cheers,

Tim.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.