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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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