| Message ID | 1450981384-2966-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/12/2015 18:23, Andrew Cooper wrote: > Most debug exceptions are traps rather than faults. Blindly re-injecting them > as hardware exception causes the instruction pointer in the exception frame > to point at the target instruct, rather than after it. This in turn breaks > debuggers in the guests. > > Introduce a helper which copies VM_EXIT_INTR_INTO to VM_ENTRY_INTR_INFO, and > use it to mirror the intercepted interrupt back to the guest. As part of > doing so, introduce vmx_intr_info_t with a bitfield representation of an > INTR_INFO field. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> In principle, the same logical bug exists for SVM, but things are a little more complicated. In VT-x, all exception intercepts have fault semantics, but for SVM, exception intercepts for traps take place after the trapped instruction has completed. Therefore, despite being conceptually incorrect, the XSA-165 patch of: @@ -2434,8 +2435,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) case VMEXIT_EXCEPTION_DB: if ( !v->domain->debugger_attached ) - goto unexpected_exit_type; - domain_pause_for_debugger(); + hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE); + else + domain_pause_for_debugger(); break; case VMEXIT_EXCEPTION_BP: is correct, as whether the intercepted exception was a trap or a fault, it must be in-injected at the current %rip, as well as needing nrip = rip. Could I please get a second opinion? I don't actually have a unit test for this at the moment (my existing unit test uses ICEBP for #BP traps, which have different intercept semantics in SVM), but I will be working on one in due course. ~Andrew
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: Friday, December 25, 2015 2:23 AM > > Most debug exceptions are traps rather than faults. Blindly re-injecting them > as hardware exception causes the instruction pointer in the exception frame > to point at the target instruct, rather than after it. This in turn breaks > debuggers in the guests. > > Introduce a helper which copies VM_EXIT_INTR_INTO to VM_ENTRY_INTR_INFO, and > use it to mirror the intercepted interrupt back to the guest. As part of > doing so, introduce vmx_intr_info_t with a bitfield representation of an > INTR_INFO field. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Jun Nakajima <jun.nakajima@intel.com> > CC: Kevin Tian <kevin.tian@intel.com> > --- > xen/arch/x86/hvm/vmx/vmx.c | 33 > ++++++++++++++++++++++++++++++--- > xen/include/asm-x86/hvm/vmx/vmx.h | 12 ++++++++++++ > 2 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index b918b8a..aacf07a 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2877,6 +2877,34 @@ static int vmx_handle_eoi_write(void) > return 0; > } > > +/* > + * Propagate VM_EXIT_INTR_INFO to VM_ENTRY_INTR_INFO. Used to mirror an > + * intercepted exception back to the guest as if Xen hadn't intercepted it. > + * > + * 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(void) > +{ > + vmx_intr_info_t intr; > + unsigned long tmp; > + > + __vmread(VM_EXIT_INTR_INFO, &intr.raw); > + > + ASSERT(intr.valid); > + > + __vmwrite(VM_ENTRY_INTR_INFO, intr.raw); > + > + if ( intr.ec_valid ) > + { > + __vmread(VM_EXIT_INTR_ERROR_CODE, &tmp); > + __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, tmp); > + } > + > + __vmread(VM_EXIT_INSTRUCTION_LEN, &tmp); > + __vmwrite(VM_ENTRY_INSTRUCTION_LEN, tmp); > +} > + > static void vmx_idtv_reinject(unsigned long idtv_info) > { > > @@ -3137,7 +3165,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > HVMTRACE_1D(TRAP_DEBUG, exit_qualification); > write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); > if ( !v->domain->debugger_attached ) > - hvm_inject_hw_exception(vector, HVM_DELIVER_NO_ERROR_CODE); > + vmx_propagate_intr(); > else > domain_pause_for_debugger(); > break; > @@ -3206,8 +3234,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > break; > case TRAP_alignment_check: > HVMTRACE_1D(TRAP, vector); > - __vmread(VM_EXIT_INTR_ERROR_CODE, &ecode); > - hvm_inject_hw_exception(vector, ecode); > + vmx_propagate_intr(); > break; > case TRAP_nmi: > if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) != > diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h > b/xen/include/asm-x86/hvm/vmx/vmx.h > index 1719965..ad2018f 100644 > --- a/xen/include/asm-x86/hvm/vmx/vmx.h > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h > @@ -224,6 +224,18 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc) > #define INTR_INFO_VALID_MASK 0x80000000 /* 31 */ > #define INTR_INFO_RESVD_BITS_MASK 0x7ffff000 > > +typedef union vmx_intr_info { > + unsigned long raw; > + struct { > + uint8_t vector; > + uint32_t type: 3, > + ec_valid: 1, > + nmi_unblocked: 1, > + rsvd: 18, > + valid: 1; > + }; > +} vmx_intr_info_t; > + Is there a value to separate vector from bitfield definition? Although it works, spec defines all fields in one 32bits format... btw seems there's no other users of this new structure. To be consistent I'd suggest either staying same with other place reading intr_info or split into a new patch to change all references together. Thanks Kevin
On 25/12/2015 01:36, Tian, Kevin wrote: >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] >> Sent: Friday, December 25, 2015 2:23 AM >> >> Most debug exceptions are traps rather than faults. Blindly re-injecting them >> as hardware exception causes the instruction pointer in the exception frame >> to point at the target instruct, rather than after it. This in turn breaks >> debuggers in the guests. >> >> Introduce a helper which copies VM_EXIT_INTR_INTO to VM_ENTRY_INTR_INFO, and >> use it to mirror the intercepted interrupt back to the guest. As part of >> doing so, introduce vmx_intr_info_t with a bitfield representation of an >> INTR_INFO field. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Jun Nakajima <jun.nakajima@intel.com> >> CC: Kevin Tian <kevin.tian@intel.com> >> --- >> xen/arch/x86/hvm/vmx/vmx.c | 33 >> ++++++++++++++++++++++++++++++--- >> xen/include/asm-x86/hvm/vmx/vmx.h | 12 ++++++++++++ >> 2 files changed, 42 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >> index b918b8a..aacf07a 100644 >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -2877,6 +2877,34 @@ static int vmx_handle_eoi_write(void) >> return 0; >> } >> >> +/* >> + * Propagate VM_EXIT_INTR_INFO to VM_ENTRY_INTR_INFO. Used to mirror an >> + * intercepted exception back to the guest as if Xen hadn't intercepted it. >> + * >> + * 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(void) >> +{ >> + vmx_intr_info_t intr; >> + unsigned long tmp; >> + >> + __vmread(VM_EXIT_INTR_INFO, &intr.raw); >> + >> + ASSERT(intr.valid); >> + >> + __vmwrite(VM_ENTRY_INTR_INFO, intr.raw); >> + >> + if ( intr.ec_valid ) >> + { >> + __vmread(VM_EXIT_INTR_ERROR_CODE, &tmp); >> + __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, tmp); >> + } >> + >> + __vmread(VM_EXIT_INSTRUCTION_LEN, &tmp); >> + __vmwrite(VM_ENTRY_INSTRUCTION_LEN, tmp); >> +} >> + >> static void vmx_idtv_reinject(unsigned long idtv_info) >> { >> >> @@ -3137,7 +3165,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) >> HVMTRACE_1D(TRAP_DEBUG, exit_qualification); >> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); >> if ( !v->domain->debugger_attached ) >> - hvm_inject_hw_exception(vector, HVM_DELIVER_NO_ERROR_CODE); >> + vmx_propagate_intr(); >> else >> domain_pause_for_debugger(); >> break; >> @@ -3206,8 +3234,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) >> break; >> case TRAP_alignment_check: >> HVMTRACE_1D(TRAP, vector); >> - __vmread(VM_EXIT_INTR_ERROR_CODE, &ecode); >> - hvm_inject_hw_exception(vector, ecode); >> + vmx_propagate_intr(); >> break; >> case TRAP_nmi: >> if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) != >> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h >> b/xen/include/asm-x86/hvm/vmx/vmx.h >> index 1719965..ad2018f 100644 >> --- a/xen/include/asm-x86/hvm/vmx/vmx.h >> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h >> @@ -224,6 +224,18 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc) >> #define INTR_INFO_VALID_MASK 0x80000000 /* 31 */ >> #define INTR_INFO_RESVD_BITS_MASK 0x7ffff000 >> >> +typedef union vmx_intr_info { >> + unsigned long raw; >> + struct { >> + uint8_t vector; >> + uint32_t type: 3, >> + ec_valid: 1, >> + nmi_unblocked: 1, >> + rsvd: 18, >> + valid: 1; >> + }; >> +} vmx_intr_info_t; >> + > Is there a value to separate vector from bitfield definition? Although > it works, spec defines all fields in one 32bits format... Not specifically - I can certainly replace it with a single bitfield list. > > btw seems there's no other users of this new structure. To be consistent > I'd suggest either staying same with other place reading intr_info or > split into a new patch to change all references together. I will split into two patches. A bugfix first, and a cleanup second. The former will then be easier to backport. ~Andrew
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index b918b8a..aacf07a 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2877,6 +2877,34 @@ static int vmx_handle_eoi_write(void) return 0; } +/* + * Propagate VM_EXIT_INTR_INFO to VM_ENTRY_INTR_INFO. Used to mirror an + * intercepted exception back to the guest as if Xen hadn't intercepted it. + * + * 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(void) +{ + vmx_intr_info_t intr; + unsigned long tmp; + + __vmread(VM_EXIT_INTR_INFO, &intr.raw); + + ASSERT(intr.valid); + + __vmwrite(VM_ENTRY_INTR_INFO, intr.raw); + + if ( intr.ec_valid ) + { + __vmread(VM_EXIT_INTR_ERROR_CODE, &tmp); + __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, tmp); + } + + __vmread(VM_EXIT_INSTRUCTION_LEN, &tmp); + __vmwrite(VM_ENTRY_INSTRUCTION_LEN, tmp); +} + static void vmx_idtv_reinject(unsigned long idtv_info) { @@ -3137,7 +3165,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) HVMTRACE_1D(TRAP_DEBUG, exit_qualification); write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); if ( !v->domain->debugger_attached ) - hvm_inject_hw_exception(vector, HVM_DELIVER_NO_ERROR_CODE); + vmx_propagate_intr(); else domain_pause_for_debugger(); break; @@ -3206,8 +3234,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) break; case TRAP_alignment_check: HVMTRACE_1D(TRAP, vector); - __vmread(VM_EXIT_INTR_ERROR_CODE, &ecode); - hvm_inject_hw_exception(vector, ecode); + vmx_propagate_intr(); break; case TRAP_nmi: if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) != diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h index 1719965..ad2018f 100644 --- a/xen/include/asm-x86/hvm/vmx/vmx.h +++ b/xen/include/asm-x86/hvm/vmx/vmx.h @@ -224,6 +224,18 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc) #define INTR_INFO_VALID_MASK 0x80000000 /* 31 */ #define INTR_INFO_RESVD_BITS_MASK 0x7ffff000 +typedef union vmx_intr_info { + unsigned long raw; + struct { + uint8_t vector; + uint32_t type: 3, + ec_valid: 1, + nmi_unblocked: 1, + rsvd: 18, + valid: 1; + }; +} vmx_intr_info_t; + /* * Exit Qualifications for MOV for Control Register Access */
Most debug exceptions are traps rather than faults. Blindly re-injecting them as hardware exception causes the instruction pointer in the exception frame to point at the target instruct, rather than after it. This in turn breaks debuggers in the guests. Introduce a helper which copies VM_EXIT_INTR_INTO to VM_ENTRY_INTR_INFO, and use it to mirror the intercepted interrupt back to the guest. As part of doing so, introduce vmx_intr_info_t with a bitfield representation of an INTR_INFO field. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> --- xen/arch/x86/hvm/vmx/vmx.c | 33 ++++++++++++++++++++++++++++++--- xen/include/asm-x86/hvm/vmx/vmx.h | 12 ++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-)