Re: [Xen-devel] [PATCH 06/15] x86/emul: Rework emulator event injection

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/11/16 17:08, Jan Beulich wrote:
>>>> On 24.11.16 at 18:00, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 24/11/16 14:53, Jan Beulich wrote:
>>>>>> On 23.11.16 at 16:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -5377,7 +5377,7 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long 
>>>> addr,
>>>>      page_unlock(page);
>>>>      put_page(page);
>>>>  
>>>> -    if ( rc == X86EMUL_UNHANDLEABLE )
>>>> +    if ( rc == X86EMUL_UNHANDLEABLE || ptwr_ctxt.ctxt.event_pending )
>>>>          goto bail;
>>>>  
>>>>      perfc_incr(ptwr_emulations);
>>>> @@ -5501,7 +5501,8 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned 
>>>> long addr,
>>>>      else
>>>>          rc = x86_emulate(&ctxt, &mmio_ro_emulate_ops);
>>>>  
>>>> -    return rc != X86EMUL_UNHANDLEABLE ? EXCRET_fault_fixed : 0;
>>>> +    return ((rc != X86EMUL_UNHANDLEABLE && !ctxt.event_pending)
>>>> +            ? EXCRET_fault_fixed : 0);
>>>>  }
>>> Wouldn't these two better be adjusted to check for OKAY and RETRY,
>>> the more that iirc we had settled on it not (yet) being guaranteed to
>>> see event_pending set whenever getting back EXCEPTION?
>> In this patch, the key point I am guarding against is that, without the
>> ->inject_*() hooks, some actions which previously took a fail_if() path
>> now succeed and latch an event.
>>
>> From that point of view, it doesn't matter how the event became pending,
>> but the fact that one is means that it was a codepath which would
>> previously have returned UNHANDLEABLE.
>>
>>
>> Later patches, which stop raising faults behind the back of emulator,
>> are the ones where new consideration is needed towards the handling of
>> EXCEPTION/event_pending.  Following Tim's feedback, I have more work to
>> do in patch 9, as propagate_page_fault() raises #PF behind the back of
>> the emulator for PV guests.
>>
>> In other words, I think this patch wants to stay like this, and a later
>> one change to be better accommodating.
> Okay.
>
>>>> @@ -3433,7 +3433,7 @@ static int sh_page_fault(struct vcpu *v,
>>>>              shadow_continue_emulation(&emul_ctxt, regs);
>>>>              v->arch.paging.last_write_was_pt = 0;
>>>>              r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
>>>> -            if ( r == X86EMUL_OKAY )
>>>> +            if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.event_pending )
>>> Aiui you need this for the swint case.
>> Why?  software interrupts were never previously tolerated in shadow
>> emulation.
> Then why would you expect OKAY together with event_pending set?
> I'm not saying swint handling needs to succeed here, but I can't see
> anything else to cause that particular state to occur.

Before this patch, a VM playing race conditions with the emulator could
cause this case to emulate 0xcc, which would fail because of the lack of
->inject_sw_interrupt() hook, and return X86_UNHANDLEABLE.

The changes in this patch now mean that the same case would properly
latch #BP, returning OKAY because its a trap not an exception.

By not explicitly failing the OKAY case with an event pending, we are
suddenly opening up rather more functionality than previously existed.

>
>>> But wouldn't you then need to add similar checks in OKAY paths elsewhere?
>> I don't see why I would.  Does my explanation above resolve your concern?
> I'm afraid not: On the same basis as above, code not expecting to
> handle swint may now see OKAY together with event_pending set,
> and would need to indicate failure to their callers just like you do in
> sh_page_fault().

That is my intent with the current code.  I have double checked it, and
it still looks correct.

~Andrew

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