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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.16 at 14:12, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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
>>> +         * 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.

I don't understand - what does this have to do with possibly getting
#PF from fetching the insn?

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

At the point of the check we don't know yet whether we're dealing
with a page table, so we need to give other handlers a chance.

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

Certainly not - it can just fall out of the switch statement, reaching
the pre-existing "bail" label. I could see your point if rc was of an
enum type ...

Jan

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