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

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 11:13 +0000 on 28 Nov (1480331610), Andrew Cooper wrote:
> Use x86_emul_{hw_exception,pagefault}() rather than
> {pv,hvm}_inject_page_fault() and hvm_inject_hw_exception() to cause raised
> faults to be known to the emulator.  This requires altering the callers of
> x86_emulate() to properly re-inject the event.
> 
> While fixing this, fix the singlestep behaviour.  Previously, an otherwise
> successful emulation would fail if singlestepping was active, as the emulator
> couldn't raise #DB.  This is unreasonable from the point of view of the guest.
> 
> We therefore tolerate #PF/#GP/SS and #DB being raised by the emulator, but
> reject anything else as unexpected.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

> +    if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending )
> +    {
> +        /*
> +         * This emulation covers writes to shadow pagetables.  We tolerate 
> #PF
> +         * (from hitting adjacent pages), #GP/#SS (from segmentation errors),
> +         * and #DB (from singlestepping).  Anything else is an emulation bug,
> +         * or a guest playing with the instruction stream under Xen's feet.
> +         */
> +        if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
> +             (emul_ctxt.ctxt.event.vector < 32) &&
> +             ((1u << emul_ctxt.ctxt.event.vector) &
> +              ((1u << TRAP_debug) | (1u << TRAP_stack_error) |
> +               (1u << TRAP_gp_fault) | (1u << TRAP_page_fault))) )
> +        {
> +            if ( is_hvm_vcpu(v) )
> +                hvm_inject_event(&emul_ctxt.ctxt.event);
> +            else
> +                pv_inject_event(&emul_ctxt.ctxt.event);
> +        }
> +        else
> +        {
> +            if ( is_hvm_vcpu(v) )
> +                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +            else
> +                pv_inject_hw_exception(TRAP_gp_fault, 0);
> +        }

I don't think it's OK to lob #GP into a HVM guest here -- we can hit
this path even if the guest isn't behaving strangely.  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?

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.  If it
can do that then we need to inject the event come what may, because
any other side-effects will have been committted already. 

> @@ -3475,6 +3503,37 @@ static int sh_page_fault(struct vcpu *v,
>              {
>                  perfc_incr(shadow_em_ex_fail);
>                  TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED);
> +
> +                if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending )
> +                {
> +                    /*
> +                     * This emulation covers writes to shadow pagetables.  We
> +                     * tolerate #PF (from hitting adjacent pages), #GP/#SS
> +                     * (from segmentation errors), and #DB (from
> +                     * singlestepping).  Anything else is an emulation bug, 
> or
> +                     * a guest playing with the instruction stream under 
> Xen's
> +                     * feet.
> +                     */
> +                    if ( emul_ctxt.ctxt.event.type == 
> X86_EVENTTYPE_HW_EXCEPTION &&
> +                         (emul_ctxt.ctxt.event.vector < 32) &&
> +                         ((1u << emul_ctxt.ctxt.event.vector) &
> +                          ((1u << TRAP_debug) | (1u << TRAP_stack_error) |
> +                           (1u << TRAP_gp_fault) | (1u << TRAP_page_fault))) 
> )
> +                    {
> +                        if ( is_hvm_vcpu(v) )
> +                            hvm_inject_event(&emul_ctxt.ctxt.event);
> +                        else
> +                            pv_inject_event(&emul_ctxt.ctxt.event);
> +                    }
> +                    else
> +                    {
> +                        if ( is_hvm_vcpu(v) )
> +                            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +                        else
> +                            pv_inject_hw_exception(TRAP_gp_fault, 0);
> +                    }
> +                }
> +

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.

(Again, unless we need to handle X86EMUL_OKAY and event_pending).

Cheers,

Tim.

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