| Message ID | 1480513841-7565-19-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At 13:50 +0000 on 30 Nov (1480513835), Andrew Cooper wrote: > Use x86_emul_{hw_exception,pagefault}() rather than > {pv,hvm}_inject_page_fault() and hvm_inject_hw_exception() to cause raised > faults to be known to the emulator. This requires altering the callers of > x86_emulate() to properly re-inject the event. > > While fixing this, fix the singlestep behaviour. Previously, an otherwise > successful emulation would fail if singlestepping was active, as the emulator > couldn't raise #DB. This is unreasonable from the point of view of the guest. > > We therefore tolerate #PF/#GP/SS and #DB being raised by the emulator, but > reject anything else as unexpected. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Please update the patch description to remove the bits about singlestepping and #DB. With that, Acked-by: Tim Deegan <tim@xen.org>
On 01/12/16 11:39, Tim Deegan wrote: > At 13:50 +0000 on 30 Nov (1480513835), Andrew Cooper wrote: >> Use x86_emul_{hw_exception,pagefault}() rather than >> {pv,hvm}_inject_page_fault() and hvm_inject_hw_exception() to cause raised >> faults to be known to the emulator. This requires altering the callers of >> x86_emulate() to properly re-inject the event. >> >> While fixing this, fix the singlestep behaviour. Previously, an otherwise >> successful emulation would fail if singlestepping was active, as the emulator >> couldn't raise #DB. This is unreasonable from the point of view of the guest. >> >> We therefore tolerate #PF/#GP/SS and #DB being raised by the emulator, but >> reject anything else as unexpected. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Please update the patch description to remove the bits about > singlestepping and #DB. With that, > > Acked-by: Tim Deegan <tim@xen.org> Oh of course. I managed to do that on the previous patch, but not this one. Sorry and thanks.
>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3373,18 +3373,35 @@ static int sh_page_fault(struct vcpu *v, > > r = x86_emulate(&emul_ctxt.ctxt, emul_ops); > > - /* > - * 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 ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending ) > - r = X86EMUL_UNHANDLEABLE; > + { > + /* > + * This emulation covers writes to shadow pagetables. We tolerate #PF > + * (from hitting adjacent pages) and #GP/#SS (from segmentation > + * errors). Anything else is an emulation bug, or a guest playing > + * with the instruction stream under Xen's feet. > + */ Same comment here regarding "adjacent". > + if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION && > + (emul_ctxt.ctxt.event.vector < 32) && > + ((1u << emul_ctxt.ctxt.event.vector) & > + ((1u << TRAP_stack_error) | (1u << TRAP_gp_fault) | > + (1u << TRAP_page_fault))) ) May I suggest to also demand an error code of zero for #GP/#SS? > + { > + if ( is_hvm_vcpu(v) ) has_hvm_container_domain()? Jan
On 01/12/16 13:00, Jan Beulich wrote: >>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/mm/shadow/multi.c >> +++ b/xen/arch/x86/mm/shadow/multi.c >> @@ -3373,18 +3373,35 @@ static int sh_page_fault(struct vcpu *v, >> >> r = x86_emulate(&emul_ctxt.ctxt, emul_ops); >> >> - /* >> - * 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 ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending ) >> - r = X86EMUL_UNHANDLEABLE; >> + { >> + /* >> + * This emulation covers writes to shadow pagetables. We tolerate #PF >> + * (from hitting adjacent pages) and #GP/#SS (from segmentation >> + * errors). Anything else is an emulation bug, or a guest playing >> + * with the instruction stream under Xen's feet. >> + */ > Same comment here regarding "adjacent". In this case, the answer is different. A misaligned write across the end of a shadow pagetable may legitimately trigger a #PF. > >> + if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION && >> + (emul_ctxt.ctxt.event.vector < 32) && >> + ((1u << emul_ctxt.ctxt.event.vector) & >> + ((1u << TRAP_stack_error) | (1u << TRAP_gp_fault) | >> + (1u << TRAP_page_fault))) ) > May I suggest to also demand an error code of zero for #GP/#SS? Ok. > >> + { >> + if ( is_hvm_vcpu(v) ) > has_hvm_container_domain()? Very good point. Will fix. ~Andrew
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c index f07803b..e509cc1 100644 --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -162,8 +162,9 @@ static int hvm_translate_linear_addr( if ( !okay ) { - hvm_inject_hw_exception( - (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0); + x86_emul_hw_exception( + (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, + 0, &sh_ctxt->ctxt); return X86EMUL_EXCEPTION; } @@ -323,7 +324,7 @@ pv_emulate_read(enum x86_segment seg, if ( (rc = copy_from_user(p_data, (void *)offset, bytes)) != 0 ) { - pv_inject_page_fault(0, offset + bytes - rc); /* Read fault. */ + x86_emul_pagefault(0, offset + bytes - rc, ctxt); /* Read fault. */ return X86EMUL_EXCEPTION; } @@ -1720,10 +1721,8 @@ static mfn_t emulate_gva_to_mfn(struct vcpu *v, unsigned long vaddr, gfn = paging_get_hostmode(v)->gva_to_gfn(v, NULL, vaddr, &pfec); if ( gfn == gfn_x(INVALID_GFN) ) { - if ( is_hvm_vcpu(v) ) - hvm_inject_page_fault(pfec, vaddr); - else - pv_inject_page_fault(pfec, vaddr); + x86_emul_pagefault(pfec, vaddr, &sh_ctxt->ctxt); + return _mfn(BAD_GVA_TO_GFN); } diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index 56c40f8..098b653 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3373,18 +3373,35 @@ static int sh_page_fault(struct vcpu *v, r = x86_emulate(&emul_ctxt.ctxt, emul_ops); - /* - * 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 ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending ) - r = X86EMUL_UNHANDLEABLE; + { + /* + * This emulation covers writes to shadow pagetables. We tolerate #PF + * (from hitting adjacent pages) and #GP/#SS (from segmentation + * errors). Anything else is an emulation bug, or a guest playing + * with the instruction stream under Xen's feet. + */ + if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION && + (emul_ctxt.ctxt.event.vector < 32) && + ((1u << emul_ctxt.ctxt.event.vector) & + ((1u << TRAP_stack_error) | (1u << TRAP_gp_fault) | + (1u << TRAP_page_fault))) ) + { + if ( is_hvm_vcpu(v) ) + hvm_inject_event(&emul_ctxt.ctxt.event); + else + pv_inject_event(&emul_ctxt.ctxt.event); + + goto emulate_done; + } + else + { + SHADOW_PRINTK( + "Unexpected event (type %u, vector %#x) from emulation\n", + emul_ctxt.ctxt.event.type, emul_ctxt.ctxt.event.vector); + r = X86EMUL_UNHANDLEABLE; + } + } /* * NB. We do not unshadow on X86EMUL_EXCEPTION. It's not clear that it
Use x86_emul_{hw_exception,pagefault}() rather than {pv,hvm}_inject_page_fault() and hvm_inject_hw_exception() to cause raised faults to be known to the emulator. This requires altering the callers of x86_emulate() to properly re-inject the event. While fixing this, fix the singlestep behaviour. Previously, an otherwise successful emulation would fail if singlestepping was active, as the emulator couldn't raise #DB. This is unreasonable from the point of view of the guest. We therefore tolerate #PF/#GP/SS and #DB being raised by the emulator, but reject anything else as unexpected. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> v3: * Split out #DB handling to an earlier part of the series * Don't inject #GP faults for unexpected events, but do reenter the guest. v2: * New --- xen/arch/x86/mm/shadow/common.c | 13 ++++++------- xen/arch/x86/mm/shadow/multi.c | 39 ++++++++++++++++++++++++++++----------- 2 files changed, 34 insertions(+), 18 deletions(-)