Re: [Xen-devel] [PATCH v2 2/4] VMX: Fix the mistake of exception execution

["Hao, Xudong" <xudong.hao@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, May 30, 2012 5:19 PM
> To: Hao, Xudong
> Cc: Ian.Jackson@xxxxxxxxxxxxx; keir.xen@xxxxxxxxx; Dong, Eddie; Zhang,
> Xiantao; xen-devel@xxxxxxxxxxxxx; aravindh@xxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/4] VMX: Fix the mistake of exception execution
> 
> >>> On 30.05.12 at 04:35, Xudong Hao <xudong.hao@xxxxxxxxx> wrote:
> > Fix the mistake for debug exception(#DB), overflow exception(#OF) and
> > INT3(#BP), INTn instruction emulation.
> >
> > Add inslen field in struct hvm_trap. According to instruction length,
> > to distinguish INT3 is generated by opcode 'CC' or 'CD ib =3',
> > so do INTO and #DB(debug exception).
> 
> As noted previously, using the instruction length here is not fully
> correct. Depending on how significant the distinction between
> software interrupt and software exception is, taking into account
> something like the opcode sequence 66 CC may be necessary. If
> the distinction is insignificant, then perhaps a code comment
> should say so.
> 

Jan, thanks comments, Keir added a trap type and both type and inslen can be 
specified by the caller now.

> More comments inline...
> 
> > Note:
> >  * For INTn (CD ib), it should use type 4 (software interrupt).
> >
> >  * For INT3 (CC; NOT CD ib with ib=3) and INTO (CE; NOT CD ib with ib=4),
> >    it should use type 6 (software exception).
> >
> >  * For other exceptions (#DE, #DB, #BR, #UD, #NM, #TS, #NP, #SS, #GP, #PF,
> > #MF,
> >    #AC, #MC, and #XM), it should use type 3 (hardware exception).
> >
> >  * In the unlikely event that you are emulating the undocumented opcode F1
> >    (informally called INT1 or ICEBP), it would use type 5 (privileged
> > software
> >    exception).
> >
> > Signed-off-by: Xudong Hao <xudong.hao@xxxxxxxxx>
> > Signed-off-by: Eddie Dong <eddie.dong@xxxxxxxxx>
> > Signed-off-by: Xiantao Zhang <xiantao.zhang@xxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/vmx/vmx.c    |   43
> > ++++++++++++++++++++++++++++++++++++++++-
> >  xen/include/asm-x86/hvm/hvm.h |    2 +
> >  2 files changed, 44 insertions(+), 1 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index c96d18b..cf08a11 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -1381,6 +1381,19 @@ void vmx_inject_nmi(void)
> >                             HVM_DELIVER_NO_ERROR_CODE);
> >  }
> >
> > +/*
> > + * Generate the virtual event to guest.
> > + * NOTE:
> > + *    This is for processor execution generated exceptions,
> > + * and handle #DB hardware exception and all software
> > + * exception/interrupt, which include:
> > + *  - INT 3(CC), INTO (CE) instruction emulation, which should
> > + *    use X86_EVENTTYPE_SW_EXCEPTION;
> > + *  - INT nn (CD nn) instruction emulation, which should use
> > + *    X86_EVENTTYPE_SW_INTERRUPT as interrupt type;
> > + *  - opcode 0xf1 generated #DB should use privileged software
> > + *    exception.
> > + */
> >  static void vmx_inject_trap(struct hvm_trap *trap)
> >  {
> >      unsigned long intr_info;
> > @@ -1399,6 +1412,12 @@ static void vmx_inject_trap(struct hvm_trap
> *trap)
> >      switch ( _trap.vector )
> >      {
> >      case TRAP_debug:
> > +        _trap.type = X86_EVENTTYPE_HW_EXCEPTION;
> > +        if ( _trap.inslen != 1 ) {
> 
> Opcode F1 certainly has length 1, so perhaps the condition is
> inverted? Perhaps length 0 should be used here to distinguish
> the hardware exception case from the F1-generated one.
> 

Yes, it's inverted..

> > +            _trap.type = X86_EVENTTYPE_PRI_SW_EXCEPTION;  /*
> opcode 0xf1 */
> > +            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, _trap.inslen);
> > +        }
> > +
> >          if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> >          {
> >              __restore_debug_registers(curr);
> > @@ -1414,6 +1433,27 @@ static void vmx_inject_trap(struct hvm_trap
> *trap)
> >              domain_pause_for_debugger();
> >              return;
> >          }
> > +        _trap.type = X86_EVENTTYPE_SW_EXCEPTION;  /* CC */
> > +        if ( _trap.inslen != 1 )
> > +            _trap.type = X86_EVENTTYPE_SW_INTERRUPT;  /* CD ib
> with ib=3 */
> > +        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, _trap.inslen);
> > +        break;
> > +
> > +    case TRAP_overflow:
> > +        _trap.type = X86_EVENTTYPE_SW_EXCEPTION;  /* CE */
> > +        if ( _trap.inslen != 1 )
> > +            _trap.type = X86_EVENTTYPE_SW_INTERRUPT;  /* CD ib
> with ib=4 */
> > +        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, _trap.inslen);
> > +        break;
> > +
> > +    default:
> > +        if ( _trap.vector > TRAP_last_reserved ) /* int imm8 */
> > +        {
> > +            _trap.type = X86_EVENTTYPE_SW_INTERRUPT;
> > +            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, _trap.inslen);
> > +        }
> > +        break;
> > +
> >      }
> >
> >      if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
> > @@ -2424,7 +2464,8 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
> >                      struct hvm_trap trap = {
> >                          .vector = TRAP_int3,
> >                          .type = X86_EVENTTYPE_SW_EXCEPTION,
> > -                        .error_code =
> HVM_DELIVER_NO_ERROR_CODE
> > +                        .error_code =
> HVM_DELIVER_NO_ERROR_CODE,
> > +                        .inslen =
> __vmread(VM_EXIT_INSTRUCTION_LEN)
> >                      };
> >                      hvm_inject_trap(&trap);
> >                      break;
> > diff --git a/xen/include/asm-x86/hvm/hvm.h
> b/xen/include/asm-x86/hvm/hvm.h
> > index 65f7e20..a3d8bf1 100644
> > --- a/xen/include/asm-x86/hvm/hvm.h
> > +++ b/xen/include/asm-x86/hvm/hvm.h
> > @@ -76,6 +76,7 @@ struct hvm_trap {
> >      unsigned int  type;         /* X86_EVENTTYPE_* */
> >      int           error_code;   /* HVM_DELIVER_NO_ERROR_CODE
> if n/a */
> >      unsigned long cr2;          /* Only for TRAP_page_fault h/w
> exception
> > */
> > +    int           inslen;       /* Instruction length */
> 
> May I suggest using "insnlen" instead?
> 
> Jan
> 
> >  };
> >
> >  /*
> > @@ -375,6 +376,7 @@ static inline int hvm_do_pmu_interrupt(struct
> > cpu_user_regs *regs)
> >  #define X86_EVENTTYPE_NMI                   2    /* NMI
> */
> >  #define X86_EVENTTYPE_HW_EXCEPTION          3    /* hardware
> exception */
> >  #define X86_EVENTTYPE_SW_INTERRUPT          4    /* software
> interrupt */
> > +#define X86_EVENTTYPE_PRI_SW_EXCEPTION      5    /* privileged
> software exception */
> >  #define X86_EVENTTYPE_SW_EXCEPTION          6    /* software
> exception */
> >
> >  int hvm_event_needs_reinjection(uint8_t type, uint8_t vector);
> > --
> > 1.5.5
> 


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