[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 06/15] x86/emul: Rework emulator event injection
On 24/11/16 17:30, Tim Deegan wrote: > 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? Oh right, yes. Sorry for being dense. As an interim between now and getting a proper audit hook, would a bool permit_traps in x86_emulate_ctxt suffice? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |