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

[Tim Deegan <tim@xxxxxxx>]

At 17:19 +0000 on 24 Nov (1480007992), Andrew Cooper wrote:
> 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.

So is that not the case I was worried about, where the emulator
updates register state but we then drop the expected event on the
floor?

Tim.

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