Re: [Xen-devel] [PATCH] x86/vmx: Fix injection of #DB traps following XSA-165

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> 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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel