| Message ID | 1479915538-15282-10-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At 15:38 +0000 on 23 Nov (1479915532), Andrew Cooper wrote: > Introduce a new x86_emul_pagefault() similar to x86_emul_hw_exception(), and > use this instead of hvm_inject_page_fault() from emulation codepaths. > > Replace one hvm_inject_hw_exception() in the shadow emulation codepaths. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > NOTE: this is a functional change for the shadow code, as a #PF previously > raised properly with the guest will now cause X86EMUL_UNHANDLABLE. It is my > understanding after a discusion with Tim that this is ok, but I haven't done > extenstive testing yet. Do you plan to? I think this is indeed OK, but there may be some edge case, e.g. an instruction that writes to both the current top-level pagetable (which can't be unshadowed) and an unmapped virtual address. That ought to raise #PF in the guest but might now spin retrying? Tim.
On 23/11/16 16:31, Tim Deegan wrote: > At 15:38 +0000 on 23 Nov (1479915532), Andrew Cooper wrote: >> Introduce a new x86_emul_pagefault() similar to x86_emul_hw_exception(), and >> use this instead of hvm_inject_page_fault() from emulation codepaths. >> >> Replace one hvm_inject_hw_exception() in the shadow emulation codepaths. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> NOTE: this is a functional change for the shadow code, as a #PF previously >> raised properly with the guest will now cause X86EMUL_UNHANDLABLE. It is my >> understanding after a discusion with Tim that this is ok, but I haven't done >> extenstive testing yet. > Do you plan to? I think this is indeed OK, but there may be some edge > case, e.g. an instruction that writes to both the current top-level > pagetable (which can't be unshadowed) and an unmapped virtual address. > That ought to raise #PF in the guest but might now spin retrying? That is a devious corner case. I take it you have been there before? The more I think about these changes, the more I think that the shadow code would be better by selectively looking a pending event, injecting pagefaults, but rejecting and retrying if any other event shows up. ~Andrew
At 16:40 +0000 on 23 Nov (1479919254), Andrew Cooper wrote: > On 23/11/16 16:31, Tim Deegan wrote: > > At 15:38 +0000 on 23 Nov (1479915532), Andrew Cooper wrote: > >> Introduce a new x86_emul_pagefault() similar to x86_emul_hw_exception(), and > >> use this instead of hvm_inject_page_fault() from emulation codepaths. > >> > >> Replace one hvm_inject_hw_exception() in the shadow emulation codepaths. > >> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> NOTE: this is a functional change for the shadow code, as a #PF previously > >> raised properly with the guest will now cause X86EMUL_UNHANDLABLE. It is my > >> understanding after a discusion with Tim that this is ok, but I haven't done > >> extenstive testing yet. > > Do you plan to? I think this is indeed OK, but there may be some edge > > case, e.g. an instruction that writes to both the current top-level > > pagetable (which can't be unshadowed) and an unmapped virtual address. > > That ought to raise #PF in the guest but might now spin retrying? > > That is a devious corner case. I take it you have been there before? In similar situations, yes. :) > The more I think about these changes, the more I think that the shadow > code would be better by selectively looking a pending event, injecting > pagefaults, but rejecting and retrying if any other event shows up. That sounds like a good idea, and seems like the smallest deviation from the current behaviour. It might also be OK to inject any event that the emulator raises. That's a bigger change but maybe a more coherent end result? Cheers, Tim.
On 23/11/16 16:50, Tim Deegan wrote: > At 16:40 +0000 on 23 Nov (1479919254), Andrew Cooper wrote: >> On 23/11/16 16:31, Tim Deegan wrote: >>> At 15:38 +0000 on 23 Nov (1479915532), Andrew Cooper wrote: >>>> Introduce a new x86_emul_pagefault() similar to x86_emul_hw_exception(), and >>>> use this instead of hvm_inject_page_fault() from emulation codepaths. >>>> >>>> Replace one hvm_inject_hw_exception() in the shadow emulation codepaths. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> NOTE: this is a functional change for the shadow code, as a #PF previously >>>> raised properly with the guest will now cause X86EMUL_UNHANDLABLE. It is my >>>> understanding after a discusion with Tim that this is ok, but I haven't done >>>> extenstive testing yet. >>> Do you plan to? I think this is indeed OK, but there may be some edge >>> case, e.g. an instruction that writes to both the current top-level >>> pagetable (which can't be unshadowed) and an unmapped virtual address. >>> That ought to raise #PF in the guest but might now spin retrying? >> That is a devious corner case. I take it you have been there before? > In similar situations, yes. :) > >> The more I think about these changes, the more I think that the shadow >> code would be better by selectively looking a pending event, injecting >> pagefaults, but rejecting and retrying if any other event shows up. > That sounds like a good idea, and seems like the smallest deviation > from the current behaviour. It might also be OK to inject any event > that the emulator raises. That's a bigger change but maybe a more > coherent end result? Well - now this isn't hidden in a security fix, I am less averse to functional changes. My only concern is that the previous lack of the ->inject_hw_exception() hook cut off large chunks of functionality from the shadow and PV PT emulation paths, and I am not sure opening this up in general is a good idea. Longterm the plan is to fully split the decode and emulate calls even for external callers, at which point the the pagetable code could check that the instruction is write which matches %cr2 before proceeding with emulation. Even then however, I am not sure it would be a good idea to follow anything other than a pagefault which surfaces from emulation. ~Andrew
>>> On 23.11.16 at 17:58, <andrew.cooper3@citrix.com> wrote: > Longterm the plan is to fully split the decode and emulate calls even > for external callers, This is news to me - why would that be? My PV priv-op series will add a hook called between decode and execute, but I don't see why we should bother all callers with invoking decode and execute separately. Jan
On 24/11/16 10:43, Jan Beulich wrote: >>>> On 23.11.16 at 17:58, <andrew.cooper3@citrix.com> wrote: >> Longterm the plan is to fully split the decode and emulate calls even >> for external callers, > This is news to me - why would that be? My PV priv-op series will > add a hook called between decode and execute, but I don't see > why we should bother all callers with invoking decode and execute > separately. Having an audit hook is also fine. It achieves the same overall effect. Without seeing how you planned to do this, I was referring back to the discussion which was had at the April Hackathon. ~Andrew
>>> On 24.11.16 at 11:46, <andrew.cooper3@citrix.com> wrote: > On 24/11/16 10:43, Jan Beulich wrote: >>>>> On 23.11.16 at 17:58, <andrew.cooper3@citrix.com> wrote: >>> Longterm the plan is to fully split the decode and emulate calls even >>> for external callers, >> This is news to me - why would that be? My PV priv-op series will >> add a hook called between decode and execute, but I don't see >> why we should bother all callers with invoking decode and execute >> separately. > > Having an audit hook is also fine. It achieves the same overall effect. > > Without seeing how you planned to do this, I was referring back to the > discussion which was had at the April Hackathon. Oh, I see. We actually did briefly discuss the hook later (see e.g. lists.xenproject.org/archives/html/xen-devel/2016-09/msg03367.html), and it doesn't make much sense for me to post v3 without your XSA-191 follow-up finished and in place. Jan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index c0fbde1..3ebb200 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -459,7 +459,7 @@ static int hvmemul_linear_to_phys( { if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) return X86EMUL_RETRY; - hvm_inject_page_fault(pfec, addr); + x86_emul_pagefault(pfec, addr, &hvmemul_ctxt->ctxt); return X86EMUL_EXCEPTION; } @@ -483,7 +483,7 @@ static int hvmemul_linear_to_phys( ASSERT(!reverse); if ( npfn != gfn_x(INVALID_GFN) ) return X86EMUL_UNHANDLEABLE; - hvm_inject_page_fault(pfec, addr & PAGE_MASK); + x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt); return X86EMUL_EXCEPTION; } *reps = done; diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c index 6f6668d..c8b61b9 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; } diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index ddcd93c..cc26e9d 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -600,6 +600,19 @@ static inline void x86_emul_hw_exception( ctxt->event_pending = true; } +static inline void x86_emul_pagefault( + unsigned int error_code, unsigned long cr2, struct x86_emulate_ctxt *ctxt) +{ + ASSERT(!ctxt->event_pending); + + ctxt->event.vector = 14; /* TRAP_page_fault */ + ctxt->event.type = X86_EVENTTYPE_HW_EXCEPTION; + ctxt->event.error_code = error_code; + ctxt->event.cr2 = cr2; + + ctxt->event_pending = true; +} + static inline void x86_emul_software_event( enum x86_swint_type type, uint8_t vector, uint8_t insn_len, struct x86_emulate_ctxt *ctxt)
Introduce a new x86_emul_pagefault() similar to x86_emul_hw_exception(), and use this instead of hvm_inject_page_fault() from emulation codepaths. Replace one hvm_inject_hw_exception() in the shadow emulation codepaths. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Paul Durrant <paul.durrant@citrix.com> CC: Tim Deegan <tim@xen.org> CC: George Dunlap <george.dunlap@eu.citrix.com> NOTE: this is a functional change for the shadow code, as a #PF previously raised properly with the guest will now cause X86EMUL_UNHANDLABLE. It is my understanding after a discusion with Tim that this is ok, but I haven't done extenstive testing yet. --- xen/arch/x86/hvm/emulate.c | 4 ++-- xen/arch/x86/mm/shadow/common.c | 5 +++-- xen/arch/x86/x86_emulate/x86_emulate.h | 13 +++++++++++++ 3 files changed, 18 insertions(+), 4 deletions(-)