From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/vmx: Fix injection of #DB traps following XSA-165 Date: Tue, 29 Dec 2015 10:50:30 +0000 Message-ID: <56826576.6060505@citrix.com> References: <1450981384-2966-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Tian, Kevin" , Xen-devel Cc: "Nakajima, Jun" , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 >> --- >> CC: Jan Beulich >> CC: Jun Nakajima >> CC: Kevin Tian >> --- >> 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