Re: [Xen-devel] [PATCH v3 17/24] x86/pv: Avoid raising faults behind the emulators back

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.16 at 14:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -5379,30 +5380,41 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long 
> addr,
>      page_unlock(page);
>      put_page(page);
>  
> -    /*
> -     * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised
> -     * by the emulator itself to become X86EMUL_UNHANDLEABLE.  Such 
> exceptions
> -     * now set event_pending instead.  Exceptions raised behind the back of
> -     * the emulator don't yet set event_pending.
> -     *
> -     * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE path,
> -     * for no functional change from before.  Future patches will fix this
> -     * properly.
> -     */
> -    if ( rc == X86EMUL_EXCEPTION && ptwr_ctxt.ctxt.event_pending )
> -        rc = X86EMUL_UNHANDLEABLE;
> +    /* More strict than x86_emulate_wrapper(), as this is now true for PV. */
> +    ASSERT(ptwr_ctxt.ctxt.event_pending == (rc == X86EMUL_EXCEPTION));
>  
> -    if ( rc == X86EMUL_UNHANDLEABLE )
> -        goto bail;
> +    switch ( rc )
> +    {
> +    case X86EMUL_EXCEPTION:
> +        /*
> +         * This emulation only covers writes to pagetables which marked

It looks to me as if either the "which" wants to be dropped, or "are" /
"have been" be inserted after it.

> +         * read-only by Xen.  We tolerate #PF (from hitting an adjacent 
> page).

Why "adjacent"? Aiui the only legitimate #PF here can be from a
page having got paged out by the guest by the time here, and that
could be either the page table page itself, or the page(s) holding
the instruction (which normally aren't adjacent at all).

> +         * Anything else is an emulation bug, or a guest playing with the
> +         * instruction stream under Xen's feet.
> +         */
> +        if ( ptwr_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
> +             ptwr_ctxt.ctxt.event.vector == TRAP_page_fault )
> +            pv_inject_event(&ptwr_ctxt.ctxt.event);
> +        else
> +            gdprintk(XENLOG_WARNING,
> +                     "Unexpected event (type %u, vector %#x) from 
> emulation\n",
> +                     ptwr_ctxt.ctxt.event.type, ptwr_ctxt.ctxt.event.vector);
> +
> +        /* Fallthrough */
> +    case X86EMUL_OKAY:
>  
> -    if ( ptwr_ctxt.ctxt.retire.singlestep )
> -        pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
> +        if ( ptwr_ctxt.ctxt.retire.singlestep )
> +            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>  
> -    perfc_incr(ptwr_emulations);
> -    return EXCRET_fault_fixed;
> +        /* Fallthrough */
> +    case X86EMUL_RETRY:
> +        perfc_incr(ptwr_emulations);
> +        return EXCRET_fault_fixed;
>  
>   bail:
> -    return 0;
> +    default:
> +        return 0;
> +    }
>  }

I think omitting the default label would not only make the patch
slightly smaller, but also result in the bail label look less misplaced.

With at least the comment aspect above taken care of,
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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