All #DB exceptions result in an update of %dr6, but this isn't captured in Xen's handling.
PV guests generally work by modifying %dr6 before raising #DB, whereas HVM guests do nothing and have a single-step special case in the lowest levels of {vmx,svm}_inject_event(). All of this is buggy, but in particular, task switches with the trace flag never end up signalling BT in %dr6. To begin resolving this issue, add a new pending_dbg field to x86_event (unioned with cr2 to avoid taking any extra space), and introduce {pv,hvm}_inject_debug_exn() helpers to replace the current callers using {pv,hvm}_inject_hw_exception(). A key property is that pending_dbg is taken with positive polarity to deal with RTM sensibly. Most callers pass in a constant, but callers passing in a hardware %dr6 value need to xor the value with X86_DR6_DEFAULT to flip the polarity of RTM and reserved fields. For PV guests, move the ad-hoc updating of %dr6 into pv_inject_event(). This in principle breaks the handing of RTM in do_debug(), but PV guests can't actually enable MSR_DEBUGCTL.RTM yet, so this doesn't matter in practice. Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> --- CC: Jan Beulich <jbeul...@suse.com> CC: Wei Liu <wei.l...@citrix.com> CC: Roger Pau Monné <roger....@citrix.com> CC: Jun Nakajima <jun.nakaj...@intel.com> CC: Kevin Tian <kevin.t...@intel.com> CC: Boris Ostrovsky <boris.ostrov...@oracle.com> CC: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com> CC: Brian Woods <brian.wo...@amd.com> CC: Tim Deegan <t...@xen.org> --- xen/arch/x86/hvm/emulate.c | 3 ++- xen/arch/x86/hvm/hvm.c | 2 +- xen/arch/x86/hvm/svm/svm.c | 9 ++++++--- xen/arch/x86/hvm/vmx/vmx.c | 13 ++++++++----- xen/arch/x86/mm/shadow/multi.c | 5 +++-- xen/arch/x86/pv/emul-priv-op.c | 11 +++++------ xen/arch/x86/pv/emulate.c | 6 ++---- xen/arch/x86/pv/ro-page-fault.c | 3 ++- xen/arch/x86/pv/traps.c | 16 ++++++++++++---- xen/arch/x86/traps.c | 7 +------ xen/arch/x86/x86_emulate/x86_emulate.h | 5 ++++- xen/include/asm-x86/domain.h | 12 ++++++++++++ xen/include/asm-x86/hvm/hvm.h | 15 ++++++++++++++- 13 files changed, 72 insertions(+), 35 deletions(-) diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index c9aa188..0292058 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -15,6 +15,7 @@ #include <xen/paging.h> #include <xen/trace.h> #include <xen/vm_event.h> +#include <asm/debugreg.h> #include <asm/event.h> #include <asm/i387.h> #include <asm/xstate.h> @@ -2283,7 +2284,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt, } if ( hvmemul_ctxt->ctxt.retire.singlestep ) - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + hvm_inject_debug_exn(X86_DR6_BS); new_intr_shadow = hvmemul_ctxt->intr_shadow; diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 4f825a2..b35cf54 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3115,7 +3115,7 @@ void hvm_task_switch( } if ( (tss.trace & 1) && !exn_raised ) - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + hvm_inject_debug_exn(X86_DR6_BT); out: hvm_unmap_entry(optss_desc); diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index dabb96f..c06bd68 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -119,7 +119,7 @@ void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len) curr->arch.hvm_svm.vmcb->interrupt_shadow = 0; if ( regs->eflags & X86_EFLAGS_TF ) - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + hvm_inject_debug_exn(X86_DR6_BS); } static void svm_cpu_down(void) @@ -2798,7 +2798,10 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) goto unexpected_exit_type; if ( !rc ) hvm_inject_exception(TRAP_debug, - trap_type, inst_len, X86_EVENT_NO_EC); + trap_type, inst_len, X86_EVENT_NO_EC, + exit_reason == VMEXIT_ICEBP ? 0 : + /* #DB - Hardware already updated dr6. */ + vmcb_get_dr6(vmcb) ^ X86_DR6_DEFAULT); } else domain_pause_for_debugger(); @@ -2830,7 +2833,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) if ( !rc ) hvm_inject_exception(TRAP_int3, X86_EVENTTYPE_SW_EXCEPTION, - inst_len, X86_EVENT_NO_EC); + inst_len, X86_EVENT_NO_EC, 0 /* N/A */); } break; diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index bfa3a0d..39c9ddc 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2483,7 +2483,7 @@ void update_guest_eip(void) } if ( regs->eflags & X86_EFLAGS_TF ) - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + hvm_inject_debug_exn(X86_DR6_BS); } static void vmx_fpu_dirty_intercept(void) @@ -3382,7 +3382,7 @@ static int vmx_handle_eoi_write(void) * It is the callers responsibility to ensure that this function is only used * in the context of an appropriate vmexit. */ -static void vmx_propagate_intr(unsigned long intr) +static void vmx_propagate_intr(unsigned long intr, unsigned long pending_dbg) { struct x86_event event = { .vector = MASK_EXTR(intr, INTR_INFO_VECTOR_MASK), @@ -3406,6 +3406,9 @@ static void vmx_propagate_intr(unsigned long intr) else event.insn_len = 0; + if ( event.vector == TRAP_debug ) + event.pending_dbg = pending_dbg; + hvm_inject_event(&event); } @@ -3715,7 +3718,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) if ( rc < 0 ) goto exit_and_crash; if ( !rc ) - vmx_propagate_intr(intr_info); + vmx_propagate_intr(intr_info, exit_qualification); } else domain_pause_for_debugger(); @@ -3736,7 +3739,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) if ( rc < 0 ) goto exit_and_crash; if ( !rc ) - vmx_propagate_intr(intr_info); + vmx_propagate_intr(intr_info, 0 /* N/A */); } else { @@ -3776,7 +3779,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) break; case TRAP_alignment_check: HVMTRACE_1D(TRAP, vector); - vmx_propagate_intr(intr_info); + vmx_propagate_intr(intr_info, 0 /* N/A */); break; case TRAP_nmi: if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) != diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index da586c2..57d24d9 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -30,6 +30,7 @@ asm(".file \"" __OBJECT_FILE__ "\""); #include <xen/perfc.h> #include <xen/domain_page.h> #include <xen/iocap.h> +#include <asm/debugreg.h> #include <xsm/xsm.h> #include <asm/page.h> #include <asm/current.h> @@ -3453,7 +3454,7 @@ static int sh_page_fault(struct vcpu *v, #endif if ( emul_ctxt.ctxt.retire.singlestep ) - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + hvm_inject_debug_exn(X86_DR6_BS); #if GUEST_PAGING_LEVELS == 3 /* PAE guest */ /* @@ -3494,7 +3495,7 @@ static int sh_page_fault(struct vcpu *v, TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED); if ( emul_ctxt.ctxt.retire.singlestep ) - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + hvm_inject_debug_exn(X86_DR6_BS); break; /* Don't emulate again if we failed! */ } diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index dfde70e..45788b2 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -1366,12 +1366,11 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs) if ( ctxt.ctxt.retire.singlestep ) ctxt.bpmatch |= DR_STEP; - if ( ctxt.bpmatch ) - { - curr->arch.dr6 |= ctxt.bpmatch | DR_STATUS_RESERVED_ONE; - if ( !(curr->arch.pv_vcpu.trap_bounce.flags & TBF_EXCEPTION) ) - pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); - } + + if ( ctxt.bpmatch && + !(curr->arch.pv_vcpu.trap_bounce.flags & TBF_EXCEPTION) ) + pv_inject_debug_exn(ctxt.bpmatch); + /* fall through */ case X86EMUL_RETRY: return EXCRET_fault_fixed; diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c index 757ffd1..542b6d0 100644 --- a/xen/arch/x86/pv/emulate.c +++ b/xen/arch/x86/pv/emulate.c @@ -76,11 +76,9 @@ void pv_emul_instruction_done(struct cpu_user_regs *regs, unsigned long rip) { regs->rip = rip; regs->eflags &= ~X86_EFLAGS_RF; + if ( regs->eflags & X86_EFLAGS_TF ) - { - current->arch.dr6 |= DR_STEP | DR_STATUS_RESERVED_ONE; - pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); - } + pv_inject_debug_exn(X86_DR6_BS); } /* diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c index aa8d5a7..33873c9 100644 --- a/xen/arch/x86/pv/ro-page-fault.c +++ b/xen/arch/x86/pv/ro-page-fault.c @@ -25,6 +25,7 @@ #include <xen/sched.h> #include <xen/trace.h> +#include <asm/debugreg.h> #include <asm/domain.h> #include <asm/mm.h> #include <asm/pci.h> @@ -387,7 +388,7 @@ int pv_ro_page_fault(unsigned long addr, struct cpu_user_regs *regs) /* Fallthrough */ case X86EMUL_OKAY: if ( ctxt.retire.singlestep ) - pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + pv_inject_debug_exn(X86_DR6_BS); /* Fallthrough */ case X86EMUL_RETRY: diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c index f48db92..7d48d83 100644 --- a/xen/arch/x86/pv/traps.c +++ b/xen/arch/x86/pv/traps.c @@ -26,6 +26,7 @@ #include <xen/softirq.h> #include <asm/apic.h> +#include <asm/debugreg.h> #include <asm/shared.h> #include <asm/traps.h> @@ -70,9 +71,9 @@ void pv_inject_event(const struct x86_event *event) tb->cs = ti->cs; tb->eip = ti->address; - if ( event->type == X86_EVENTTYPE_HW_EXCEPTION && - vector == TRAP_page_fault ) + switch ( vector | -(event->type == X86_EVENTTYPE_SW_INTERRUPT) ) { + case TRAP_page_fault: curr->arch.pv_vcpu.ctrlreg[2] = event->cr2; arch_set_cr2(curr, event->cr2); @@ -82,9 +83,16 @@ void pv_inject_event(const struct x86_event *event) error_code |= PFEC_user_mode; trace_pv_page_fault(event->cr2, error_code); - } - else + break; + + case TRAP_debug: + curr->arch.dr6 |= event->pending_dbg; + /* Fallthrough */ + + default: trace_pv_trap(vector, regs->rip, use_error_code, error_code); + break; + } if ( use_error_code ) { diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index d0d9011..8ef22b4 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1768,7 +1768,6 @@ void do_device_not_available(struct cpu_user_regs *regs) void do_debug(struct cpu_user_regs *regs) { unsigned long dr6; - struct vcpu *v = current; /* Stash dr6 as early as possible. */ dr6 = read_debugreg(6); @@ -1860,11 +1859,7 @@ void do_debug(struct cpu_user_regs *regs) return; } - /* Save debug status register where guest OS can peek at it */ - v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT); - v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT); - - pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + pv_inject_debug_exn(dr6 ^ X86_DR6_DEFAULT); } static void __init noinline __set_intr_gate(unsigned int n, diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index c22e774..d85a84a 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -88,7 +88,10 @@ struct x86_event { uint8_t type; /* X86_EVENTTYPE_* */ uint8_t insn_len; /* Instruction length */ int32_t error_code; /* X86_EVENT_NO_EC if n/a */ - unsigned long cr2; /* Only for TRAP_page_fault h/w exception */ + union { + unsigned long cr2; /* #PF */ + unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) */ + }; }; /* diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 59d5e4a..dfe995f 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -648,6 +648,18 @@ static inline void pv_inject_hw_exception(unsigned int vector, int errcode) pv_inject_event(&event); } +static inline void pv_inject_debug_exn(unsigned long pending_dbg) +{ + struct x86_event event = { + .vector = X86_EXC_DB, + .type = X86_EVENTTYPE_HW_EXCEPTION, + .error_code = X86_EVENT_NO_EC, + .pending_dbg = pending_dbg, + }; + + pv_inject_event(&event); +} + static inline void pv_inject_page_fault(int errcode, unsigned long cr2) { const struct x86_event event = { diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index ef5e198..65d512e 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -410,13 +410,14 @@ void hvm_inject_event(const struct x86_event *event); static inline void hvm_inject_exception( unsigned int vector, unsigned int type, - unsigned int insn_len, int error_code) + unsigned int insn_len, int error_code, unsigned long extra) { struct x86_event event = { .vector = vector, .type = type, .insn_len = insn_len, .error_code = error_code, + .cr2 = extra, /* Any union field will do. */ }; hvm_inject_event(&event); @@ -433,6 +434,18 @@ static inline void hvm_inject_hw_exception(unsigned int vector, int errcode) hvm_inject_event(&event); } +static inline void hvm_inject_debug_exn(unsigned long pending_dbg) +{ + struct x86_event event = { + .vector = X86_EXC_DB, + .type = X86_EVENTTYPE_HW_EXCEPTION, + .error_code = X86_EVENT_NO_EC, + .pending_dbg = pending_dbg, + }; + + hvm_inject_event(&event); +} + static inline void hvm_inject_page_fault(int errcode, unsigned long cr2) { struct x86_event event = { -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel