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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/12/16 12:57, Jan Beulich wrote:
>>>> 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.

I meant "which are".

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

This is less clear-cut than the subsequent case, as non-aligned accesses
are disallowed, so simply misaligning a write across the page boundary
won't result in the emulator raising #PF.

One issue I have decided to defer is the behaviour of UNHANDLEABLE in
this case.  If the #PF we are servicing is caused by a misaligned write
to a l1e, instead of explicitly re-injecting #PF, we let the logic
continue searching for all other cases which may have caused a #PF.

It would be better to explicitly disallow the access by re-raising #PF
than returning UNHANDLEABLE, as it skips the rest of the pagefault handler.

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

The default label is needed to cover the UNHANDLEABLE case.

~Andrew

>
> 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