xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] Fix the mistake of exception execution
@ 2012-05-14 10:41 Hao, Xudong
  2012-05-14 11:06 ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: Hao, Xudong @ 2012-05-14 10:41 UTC (permalink / raw)
  To: Jan Beulich (JBeulich@suse.com), Keir Fraser (keir.xen@gmail.com)
  Cc: Aravindh Puthiyaparambil, Dong, Eddie, Zhang, Xiantao,
	Nakajima, Jun, xen-devel	(xen-devel@lists.xen.org)

Fix the mistake for debug exception(#DB), overflow exception(#OF; generated by INTO) and int 3(#BP) instruction emulation.

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: Eddie Dong<eddie.dong@intel.com>
Signed-off-by: Xudong Hao <xudong.hao@intel.com>

diff -r cd4dd23a831d xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Fri May 11 18:59:07 2012 +0100
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed May 15 02:31:34 2013 +0800
@@ -1350,6 +1350,19 @@ static void __vmx_inject_exception(int t
         curr->arch.hvm_vmx.vmx_emulate = 1;
 }
 
+/*
+ * Generate the virtual event to guest.
+ * NOTE: 
+ *    This is for processor execution generated exceptions,
+ * and INT 3(CC), INTO (CE) instruction emulation. It is
+ * not intended for the delivery of event due to emulation 
+ * of INT nn (CD nn) instruction, which should use 
+ * X86_EVENTTYPE_SW_INTERRUPT as interrupt type; opcode
+ * 0xf1 generated #DB should use privileged software
+ * exception, which is not deliverd here either.
+ *    The caller of this function should set correct instruction
+ * length.
+ */
 void vmx_inject_hw_exception(int trap, int error_code)
 {
     unsigned long intr_info;
@@ -1365,7 +1378,6 @@ void vmx_inject_hw_exception(int trap, i
     switch ( trap )
     {
     case TRAP_debug:
-        type = X86_EVENTTYPE_SW_EXCEPTION;
         if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
         {
             __restore_debug_registers(curr);
@@ -1383,16 +1395,14 @@ void vmx_inject_hw_exception(int trap, i
             return;
         }
 
-        type = X86_EVENTTYPE_SW_EXCEPTION;
-        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
-        break;
-
+        type = X86_EVENTTYPE_SW_EXCEPTION; /* int3; CC */
+        break;
+
+    case TRAP_overflow:
+        type = X86_EVENTTYPE_SW_EXCEPTION;  /* into; CE */
+        break;
+	
     default:
-        if ( trap > TRAP_last_reserved )
-        {
-            type = X86_EVENTTYPE_SW_EXCEPTION;
-            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */
-        }
         break;
     }
 
@@ -2447,6 +2457,11 @@ void vmx_vmexit_handler(struct cpu_user_
                 if ( handled < 0 ) 
                 {
                     vmx_inject_exception(TRAP_int3, HVM_DELIVER_NO_ERROR_CODE, 0);
+                    /*
+                     * According to the vmx_inject_hw_exception() description,
+                     * it must set correct instruction length by caller itself.
+                     */
+                    __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3, CC */
                     break;
                 }
                 else if ( handled )

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Fix the mistake of exception execution
  2012-05-14 10:41 [PATCH v3] Fix the mistake of exception execution Hao, Xudong
@ 2012-05-14 11:06 ` Jan Beulich
  2012-05-15  5:59   ` Hao, Xudong
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2012-05-14 11:06 UTC (permalink / raw)
  To: Xudong Hao
  Cc: Aravindh Puthiyaparambil, Eddie Dong,
	xen-devel (xen-devel@lists.xen.org),
	Keir Fraser(keir.xen@gmail.com), Jun Nakajima, Xiantao Zhang

>>> On 14.05.12 at 12:41, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> Fix the mistake for debug exception(#DB), overflow exception(#OF; generated 
> by INTO) and int 3(#BP) instruction emulation.
> 
> 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: Eddie Dong<eddie.dong@intel.com>
> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> 
> diff -r cd4dd23a831d xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c	Fri May 11 18:59:07 2012 +0100
> +++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed May 15 02:31:34 2013 +0800
> @@ -1350,6 +1350,19 @@ static void __vmx_inject_exception(int t
>          curr->arch.hvm_vmx.vmx_emulate = 1;
>  }
>  
> +/*
> + * Generate the virtual event to guest.
> + * NOTE: 
> + *    This is for processor execution generated exceptions,
> + * and INT 3(CC), INTO (CE) instruction emulation. It is
> + * not intended for the delivery of event due to emulation 
> + * of INT nn (CD nn) instruction, which should use 
> + * X86_EVENTTYPE_SW_INTERRUPT as interrupt type; opcode
> + * 0xf1 generated #DB should use privileged software
> + * exception, which is not deliverd here either.
> + *    The caller of this function should set correct instruction
> + * length.
> + */
>  void vmx_inject_hw_exception(int trap, int error_code)
>  {
>      unsigned long intr_info;
> @@ -1365,7 +1378,6 @@ void vmx_inject_hw_exception(int trap, i
>      switch ( trap )
>      {
>      case TRAP_debug:
> -        type = X86_EVENTTYPE_SW_EXCEPTION;
>          if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>          {
>              __restore_debug_registers(curr);
> @@ -1383,16 +1395,14 @@ void vmx_inject_hw_exception(int trap, i
>              return;
>          }
>  
> -        type = X86_EVENTTYPE_SW_EXCEPTION;
> -        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
> -        break;
> -
> +        type = X86_EVENTTYPE_SW_EXCEPTION; /* int3; CC */
> +        break;
> +
> +    case TRAP_overflow:
> +        type = X86_EVENTTYPE_SW_EXCEPTION;  /* into; CE */
> +        break;
> +	
>      default:
> -        if ( trap > TRAP_last_reserved )
> -        {
> -            type = X86_EVENTTYPE_SW_EXCEPTION;
> -            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */
> -        }

So this undoes Aravindh's earlier change, without replacement. I
don't think that's acceptable.

>          break;
>      }
>  
> @@ -2447,6 +2457,11 @@ void vmx_vmexit_handler(struct cpu_user_
>                  if ( handled < 0 ) 
>                  {
>                      vmx_inject_exception(TRAP_int3, HVM_DELIVER_NO_ERROR_CODE, 0);
> +                    /*
> +                     * According to the vmx_inject_hw_exception() description,
> +                     * it must set correct instruction length by caller itself.
> +                     */
> +                    __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3, CC */

Still using a hard-coded 1 here, the more that afaict you can't
distinguish CC and CD 03 here.

Furthermore - is this really the only place needing adjustment after
the removal of the corresponding writes above?

Jan

>                      break;
>                  }
>                  else if ( handled )

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Fix the mistake of exception execution
  2012-05-14 11:06 ` Jan Beulich
@ 2012-05-15  5:59   ` Hao, Xudong
  2012-05-15  6:48     ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: Hao, Xudong @ 2012-05-15  5:59 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Aravindh Puthiyaparambil, Dong, Eddie,
	xen-devel (xen-devel@lists.xen.org),
	Keir Fraser(keir.xen@gmail.com), Nakajima, Jun, Zhang, Xiantao

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, May 14, 2012 7:07 PM
> To: Hao, Xudong
> Cc: Keir Fraser(keir.xen@gmail.com); Dong, Eddie; Nakajima, Jun; Zhang,
> Xiantao; xen-devel (xen-devel@lists.xen.org); Aravindh Puthiyaparambil
> Subject: Re: [PATCH v3] Fix the mistake of exception execution
> 
> >>> On 14.05.12 at 12:41, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> > Fix the mistake for debug exception(#DB), overflow exception(#OF; generated
> > by INTO) and int 3(#BP) instruction emulation.
> >
> > 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: Eddie Dong<eddie.dong@intel.com>
> > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> >
> > diff -r cd4dd23a831d xen/arch/x86/hvm/vmx/vmx.c
> > --- a/xen/arch/x86/hvm/vmx/vmx.c	Fri May 11 18:59:07 2012 +0100
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed May 15 02:31:34 2013 +0800
> > @@ -1350,6 +1350,19 @@ static void __vmx_inject_exception(int t
> >          curr->arch.hvm_vmx.vmx_emulate = 1;
> >  }
> >
> > +/*
> > + * Generate the virtual event to guest.
> > + * NOTE:
> > + *    This is for processor execution generated exceptions,
> > + * and INT 3(CC), INTO (CE) instruction emulation. It is
> > + * not intended for the delivery of event due to emulation
> > + * of INT nn (CD nn) instruction, which should use
> > + * X86_EVENTTYPE_SW_INTERRUPT as interrupt type; opcode
> > + * 0xf1 generated #DB should use privileged software
> > + * exception, which is not deliverd here either.
> > + *    The caller of this function should set correct instruction
> > + * length.
> > + */
> >  void vmx_inject_hw_exception(int trap, int error_code)
> >  {
> >      unsigned long intr_info;
> > @@ -1365,7 +1378,6 @@ void vmx_inject_hw_exception(int trap, i
> >      switch ( trap )
> >      {
> >      case TRAP_debug:
> > -        type = X86_EVENTTYPE_SW_EXCEPTION;
> >          if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> >          {
> >              __restore_debug_registers(curr);
> > @@ -1383,16 +1395,14 @@ void vmx_inject_hw_exception(int trap, i
> >              return;
> >          }
> >
> > -        type = X86_EVENTTYPE_SW_EXCEPTION;
> > -        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
> > -        break;
> > -
> > +        type = X86_EVENTTYPE_SW_EXCEPTION; /* int3; CC */
> > +        break;
> > +
> > +    case TRAP_overflow:
> > +        type = X86_EVENTTYPE_SW_EXCEPTION;  /* into; CE */
> > +        break;
> > +
> >      default:
> > -        if ( trap > TRAP_last_reserved )
> > -        {
> > -            type = X86_EVENTTYPE_SW_EXCEPTION;
> > -            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */
> > -        }
> 
> So this undoes Aravindh's earlier change, without replacement. I
> don't think that's acceptable.
> 

This is the first patch that just correct some instruction in hw exception function, as function description above, int n (n > 32) is not delivered by this function. 
I'll write another patch of new function for int n handler.

> >          break;
> >      }
> >
> > @@ -2447,6 +2457,11 @@ void vmx_vmexit_handler(struct cpu_user_
> >                  if ( handled < 0 )
> >                  {
> >                      vmx_inject_exception(TRAP_int3,
> HVM_DELIVER_NO_ERROR_CODE, 0);
> > +                    /*
> > +                     * According to the vmx_inject_hw_exception()
> description,
> > +                     * it must set correct instruction length by caller
> itself.
> > +                     */
> > +                    __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /*
> int3, CC */
> 
> Still using a hard-coded 1 here, the more that afaict you can't
> distinguish CC and CD 03 here.
> 

Just copied it from original code, how about this replacement:

+     __vmwrite(VM_ENTRY_INSTRUCTION_LEN, __vmread(VM_EXIT_INSTRUCTION_LEN));

> Furthermore - is this really the only place needing adjustment after
> the removal of the corresponding writes above?
> 

Yes, I searched whole code, only this place need to do adjustment after function changes.

> Jan
> 
> >                      break;
> >                  }
> >                  else if ( handled )
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Fix the mistake of exception execution
  2012-05-15  5:59   ` Hao, Xudong
@ 2012-05-15  6:48     ` Jan Beulich
  2012-05-15  7:22       ` Aravindh Puthiyaparambil
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2012-05-15  6:48 UTC (permalink / raw)
  To: Xudong Hao
  Cc: Aravindh Puthiyaparambil, Eddie Dong,
	xen-devel (xen-devel@lists.xen.org),
	Keir Fraser(keir.xen@gmail.com), Jun Nakajima, Xiantao Zhang

>>> On 15.05.12 at 07:59, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>> On 14.05.12 at 12:41, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>> >      default:
>> > -        if ( trap > TRAP_last_reserved )
>> > -        {
>> > -            type = X86_EVENTTYPE_SW_EXCEPTION;
>> > -            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */
>> > -        }
>> 
>> So this undoes Aravindh's earlier change, without replacement. I
>> don't think that's acceptable.
>> 
> 
> This is the first patch that just correct some instruction in hw exception 
> function, as function description above, int n (n > 32) is not delivered by 
> this function. 
> I'll write another patch of new function for int n handler.

In that case it would have been nice to indicate that you don't expect
this to be applied just yet (i.e. by marking the patch RFC).

>> > +                    __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3, CC */
>> 
>> Still using a hard-coded 1 here, the more that afaict you can't
>> distinguish CC and CD 03 here.
>> 
> 
> Just copied it from original code, how about this replacement:
> 
> +     __vmwrite(VM_ENTRY_INSTRUCTION_LEN, __vmread(VM_EXIT_INSTRUCTION_LEN));

That's okay as long as on all possible code paths arriving here
VM_EXIT_INSTRUCTION_LEN is actually valid. I'm suspicious this might
not be the case (especially in the case of injection originating from
libxc).

>> Furthermore - is this really the only place needing adjustment after
>> the removal of the corresponding writes above?
>> 
> 
> Yes, I searched whole code, only this place need to do adjustment after 
> function changes.

Thanks, that's good to be sure of.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Fix the mistake of exception execution
  2012-05-15  6:48     ` Jan Beulich
@ 2012-05-15  7:22       ` Aravindh Puthiyaparambil
  2012-05-15  7:32         ` Jan Beulich
  2012-05-15  8:14         ` Hao, Xudong
  0 siblings, 2 replies; 8+ messages in thread
From: Aravindh Puthiyaparambil @ 2012-05-15  7:22 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Xudong Hao, Eddie Dong, xen-devel (xen-devel@lists.xen.org),
	Keir Fraser(keir.xen@gmail.com), Jun Nakajima, Xiantao Zhang

On Mon, May 14, 2012 at 11:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 15.05.12 at 07:59, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >>> On 14.05.12 at 12:41, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> >> >      default:
> >> > -        if ( trap > TRAP_last_reserved )
> >> > -        {
> >> > -            type = X86_EVENTTYPE_SW_EXCEPTION;
> >> > -            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */
> >> > -        }
> >>
> >> So this undoes Aravindh's earlier change, without replacement. I
> >> don't think that's acceptable.
> >>
> >
> > This is the first patch that just correct some instruction in hw exception
> > function, as function description above, int n (n > 32) is not delivered by
> > this function.
> > I'll write another patch of new function for int n handler.
>
> In that case it would have been nice to indicate that you don't expect
> this to be applied just yet (i.e. by marking the patch RFC).
>
> >> > +                    __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3, CC */
> >>
> >> Still using a hard-coded 1 here, the more that afaict you can't
> >> distinguish CC and CD 03 here.
> >>
> >
> > Just copied it from original code, how about this replacement:
> >
> > +     __vmwrite(VM_ENTRY_INSTRUCTION_LEN, __vmread(VM_EXIT_INSTRUCTION_LEN));
>
> That's okay as long as on all possible code paths arriving here
> VM_EXIT_INSTRUCTION_LEN is actually valid. I'm suspicious this might
> not be the case (especially in the case of injection originating from
> libxc).

Your suspicion is warranted. IIRC this did not work for the libxc case
injecting software interrupts. That is why I hard coded the
instruction length. Maybe the instruction length can be made caller
specific?

Thanks,
Aravindh

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Fix the mistake of exception execution
  2012-05-15  7:22       ` Aravindh Puthiyaparambil
@ 2012-05-15  7:32         ` Jan Beulich
  2012-05-15  8:14         ` Hao, Xudong
  1 sibling, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2012-05-15  7:32 UTC (permalink / raw)
  To: Aravindh Puthiyaparambil
  Cc: Eddie Dong, Xudong Hao, xen-devel (xen-devel@lists.xen.org),
	Keir Fraser(keir.xen@gmail.com), Jun Nakajima, Xiantao Zhang

>>> On 15.05.12 at 09:22, Aravindh Puthiyaparambil <aravindh@virtuata.com> wrote:
> On Mon, May 14, 2012 at 11:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 15.05.12 at 07:59, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> >>> On 14.05.12 at 12:41, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>> >> >      default:
>> >> > -        if ( trap > TRAP_last_reserved )
>> >> > -        {
>> >> > -            type = X86_EVENTTYPE_SW_EXCEPTION;
>> >> > -            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */
>> >> > -        }
>> >>
>> >> So this undoes Aravindh's earlier change, without replacement. I
>> >> don't think that's acceptable.
>> >>
>> >
>> > This is the first patch that just correct some instruction in hw exception
>> > function, as function description above, int n (n > 32) is not delivered by
>> > this function.
>> > I'll write another patch of new function for int n handler.
>>
>> In that case it would have been nice to indicate that you don't expect
>> this to be applied just yet (i.e. by marking the patch RFC).
>>
>> >> > +                    __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3, CC 
> */
>> >>
>> >> Still using a hard-coded 1 here, the more that afaict you can't
>> >> distinguish CC and CD 03 here.
>> >>
>> >
>> > Just copied it from original code, how about this replacement:
>> >
>> > +     __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 
> __vmread(VM_EXIT_INSTRUCTION_LEN));
>>
>> That's okay as long as on all possible code paths arriving here
>> VM_EXIT_INSTRUCTION_LEN is actually valid. I'm suspicious this might
>> not be the case (especially in the case of injection originating from
>> libxc).
> 
> Your suspicion is warranted. IIRC this did not work for the libxc case
> injecting software interrupts. That is why I hard coded the
> instruction length. Maybe the instruction length can be made caller
> specific?

Yes, this is what I think is needed. It should probably be an input,
with some special value (negative? zero?) indicating to use the
__vmread() as above (so that instruction emulation callers don't
need to care).

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Fix the mistake of exception execution
  2012-05-15  7:22       ` Aravindh Puthiyaparambil
  2012-05-15  7:32         ` Jan Beulich
@ 2012-05-15  8:14         ` Hao, Xudong
  2012-05-15  8:19           ` Aravindh Puthiyaparambil
  1 sibling, 1 reply; 8+ messages in thread
From: Hao, Xudong @ 2012-05-15  8:14 UTC (permalink / raw)
  To: Aravindh Puthiyaparambil
  Cc: Nakajima, Jun, Dong, Eddie, xen-devel (xen-devel@lists.xen.org),
	Keir Fraser(keir.xen@gmail.com), Jan Beulich, Zhang, Xiantao

> -----Original Message-----
> From: Aravindh Puthiyaparambil [mailto:aravindh@virtuata.com]
> Sent: Tuesday, May 15, 2012 3:23 PM
> To: Jan Beulich
> Cc: Hao, Xudong; Keir Fraser(keir.xen@gmail.com); Dong, Eddie; Nakajima, Jun;
> Zhang, Xiantao; xen-devel (xen-devel@lists.xen.org)
> Subject: Re: [PATCH v3] Fix the mistake of exception execution
> 
> On Mon, May 14, 2012 at 11:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >
> > >>> On 15.05.12 at 07:59, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> > >> From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> >>> On 14.05.12 at 12:41, "Hao, Xudong" <xudong.hao@intel.com>
> wrote:
> > >> >      default:
> > >> > -        if ( trap > TRAP_last_reserved )
> > >> > -        {
> > >> > -            type = X86_EVENTTYPE_SW_EXCEPTION;
> > >> > -            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int
> imm8 */
> > >> > -        }
> > >>
> > >> So this undoes Aravindh's earlier change, without replacement. I
> > >> don't think that's acceptable.
> > >>
> > >
> > > This is the first patch that just correct some instruction in hw exception
> > > function, as function description above, int n (n > 32) is not delivered by
> > > this function.
> > > I'll write another patch of new function for int n handler.
> >
> > In that case it would have been nice to indicate that you don't expect
> > this to be applied just yet (i.e. by marking the patch RFC).
> >
> > >> > +                    __vmwrite(VM_ENTRY_INSTRUCTION_LEN,
> 1); /* int3, CC */
> > >>
> > >> Still using a hard-coded 1 here, the more that afaict you can't
> > >> distinguish CC and CD 03 here.
> > >>
> > >
> > > Just copied it from original code, how about this replacement:
> > >
> > > +     __vmwrite(VM_ENTRY_INSTRUCTION_LEN,
> __vmread(VM_EXIT_INSTRUCTION_LEN));
> >
> > That's okay as long as on all possible code paths arriving here
> > VM_EXIT_INSTRUCTION_LEN is actually valid. I'm suspicious this might
> > not be the case (especially in the case of injection originating from
> > libxc).
> 
> Your suspicion is warranted. IIRC this did not work for the libxc case
> injecting software interrupts. That is why I hard coded the
> instruction length. Maybe the instruction length can be made caller
> specific?
> 

What's traps did you inject? This patch has not handle the software interrupts, but hardware exceptions and #BP, #OF software exceptions.

> Thanks,
> Aravindh

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Fix the mistake of exception execution
  2012-05-15  8:14         ` Hao, Xudong
@ 2012-05-15  8:19           ` Aravindh Puthiyaparambil
  0 siblings, 0 replies; 8+ messages in thread
From: Aravindh Puthiyaparambil @ 2012-05-15  8:19 UTC (permalink / raw)
  To: Hao, Xudong
  Cc: Nakajima, Jun, Dong, Eddie, xen-devel (xen-devel@lists.xen.org),
	Keir Fraser(keir.xen@gmail.com), Jan Beulich, Zhang, Xiantao


[-- Attachment #1.1: Type: text/plain, Size: 2731 bytes --]

On May 15, 2012 1:15 AM, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>
> > -----Original Message-----
> > From: Aravindh Puthiyaparambil [mailto:aravindh@virtuata.com]
> > Sent: Tuesday, May 15, 2012 3:23 PM
> > To: Jan Beulich
> > Cc: Hao, Xudong; Keir Fraser(keir.xen@gmail.com); Dong, Eddie;
Nakajima, Jun;
> > Zhang, Xiantao; xen-devel (xen-devel@lists.xen.org)
> > Subject: Re: [PATCH v3] Fix the mistake of exception execution
> >
> > On Mon, May 14, 2012 at 11:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > >
> > > >>> On 15.05.12 at 07:59, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> > > >> From: Jan Beulich [mailto:JBeulich@suse.com]
> > > >> >>> On 14.05.12 at 12:41, "Hao, Xudong" <xudong.hao@intel.com>
> > wrote:
> > > >> >      default:
> > > >> > -        if ( trap > TRAP_last_reserved )
> > > >> > -        {
> > > >> > -            type = X86_EVENTTYPE_SW_EXCEPTION;
> > > >> > -            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int
> > imm8 */
> > > >> > -        }
> > > >>
> > > >> So this undoes Aravindh's earlier change, without replacement. I
> > > >> don't think that's acceptable.
> > > >>
> > > >
> > > > This is the first patch that just correct some instruction in hw
exception
> > > > function, as function description above, int n (n > 32) is not
delivered by
> > > > this function.
> > > > I'll write another patch of new function for int n handler.
> > >
> > > In that case it would have been nice to indicate that you don't expect
> > > this to be applied just yet (i.e. by marking the patch RFC).
> > >
> > > >> > +                    __vmwrite(VM_ENTRY_INSTRUCTION_LEN,
> > 1); /* int3, CC */
> > > >>
> > > >> Still using a hard-coded 1 here, the more that afaict you can't
> > > >> distinguish CC and CD 03 here.
> > > >>
> > > >
> > > > Just copied it from original code, how about this replacement:
> > > >
> > > > +     __vmwrite(VM_ENTRY_INSTRUCTION_LEN,
> > __vmread(VM_EXIT_INSTRUCTION_LEN));
> > >
> > > That's okay as long as on all possible code paths arriving here
> > > VM_EXIT_INSTRUCTION_LEN is actually valid. I'm suspicious this might
> > > not be the case (especially in the case of injection originating from
> > > libxc).
> >
> > Your suspicion is warranted. IIRC this did not work for the libxc case
> > injecting software interrupts. That is why I hard coded the
> > instruction length. Maybe the instruction length can be made caller
> > specific?
> >
>
> What's traps did you inject? This patch has not handle the software
interrupts, but hardware exceptions and #BP, #OF software exceptions.
>

The function handles software interrupts though marked as software
exception. Incorrect it might be but it works. Your patch removes that code.

Thanks,
Aravindh

[-- Attachment #1.2: Type: text/html, Size: 4125 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2012-05-15  8:19 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-14 10:41 [PATCH v3] Fix the mistake of exception execution Hao, Xudong
2012-05-14 11:06 ` Jan Beulich
2012-05-15  5:59   ` Hao, Xudong
2012-05-15  6:48     ` Jan Beulich
2012-05-15  7:22       ` Aravindh Puthiyaparambil
2012-05-15  7:32         ` Jan Beulich
2012-05-15  8:14         ` Hao, Xudong
2012-05-15  8:19           ` Aravindh Puthiyaparambil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).