Xen devel - Apr 2012 - [PATCH] vmx: Allow software (user defined) interrupts to be injected in to the guest

If xc_hvm_inject_trap() is called on a software (user defined) interrupt, it
causes the guest to crash with a vmentry failure. The following patch fixes this
issue.

Signed-off-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>

diff -r 9036d6f974de -r f60377584f2d xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Apr 19 21:55:51 2012 -0700
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Thu Apr 19 22:01:50 2012 -0700
@@ -1374,6 +1374,13 @@ void vmx_inject_hw_exception(int trap, i
 
         type = X86_EVENTTYPE_SW_EXCEPTION;
         __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
+        break;
+    default:
+        if ( trap > TRAP_last_reserved )
+        {
+            type = X86_EVENTTYPE_SW_EXCEPTION;
+            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */
+        }
     }
 
     if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
diff -r 9036d6f974de -r f60377584f2d xen/include/asm-x86/processor.h
--- a/xen/include/asm-x86/processor.h	Thu Apr 19 21:55:51 2012 -0700
+++ b/xen/include/asm-x86/processor.h	Thu Apr 19 22:01:50 2012 -0700
@@ -111,6 +111,7 @@
 #define TRAP_alignment_check  17
 #define TRAP_machine_check    18
 #define TRAP_simd_error       19
+#define TRAP_last_reserved    31
 
 /* Set for entry via SYSCALL. Informs return code to use SYSRETQ not IRETQ. */
 /* NB. Same as VGCF_in_syscall. No bits in common with any other TRAP_ defn. */

>>> On 20.04.12 at 07:04, Aravindh Puthiyaparambil
<aravindh@virtuata.com> wrote:
> If xc_hvm_inject_trap() is called on a software (user defined) interrupt,
it
> causes the guest to crash with a vmentry failure. The following patch fixes
> this issue.
> 
> Signed-off-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>
> 
> diff -r 9036d6f974de -r f60377584f2d xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Apr 19 21:55:51 2012 -0700
> +++ b/xen/arch/x86/hvm/vmx/vmx.c	Thu Apr 19 22:01:50 2012 -0700
> @@ -1374,6 +1374,13 @@ void vmx_inject_hw_exception(int trap, i
>  
>          type = X86_EVENTTYPE_SW_EXCEPTION;
>          __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
> +        break;
> +    default:
> +        if ( trap > TRAP_last_reserved )
> +        {
> +            type = X86_EVENTTYPE_SW_EXCEPTION;
> +            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */
I doubt this is generally correct, in particular for the use you appear
to desire: When the injection is not the result of an INT nn
instruction (which I would guess to be the case when coming from
libxc), you shouldn''t set a non-zero instruction length. I believe this
is also wrong for the INT3 code above.

Additionally the problem should not be limited to injection coming
from libxc - injection originating from x86_emulate() should be
affected as much.

Jun, Eddie - I further wonder why #OF is not being handled according
to the documentation here either (should also result in
X86_EVENTTYPE_SW_EXCEPTION). And the fall-through from
TRAP_debug to TRAP_int3 is suspicious too (at the very minimum it
should be annotated with a comment saying why fall-through is
intended here). Nor does the documentation state that TRAP_debug
should ever result in X86_EVENTTYPE_SW_EXCEPTION.

Finally, the whole injection logic (including the patch here) doesn''t
appear to cope with INT nn being used by a guest with nn < 32, nor
with any (pointless) prefixes used on INT3 or INT nn.

Jan
> +        }
>      }
>  
>      if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
> diff -r 9036d6f974de -r f60377584f2d xen/include/asm-x86/processor.h
> --- a/xen/include/asm-x86/processor.h	Thu Apr 19 21:55:51 2012 -0700
> +++ b/xen/include/asm-x86/processor.h	Thu Apr 19 22:01:50 2012 -0700
> @@ -111,6 +111,7 @@
>  #define TRAP_alignment_check  17
>  #define TRAP_machine_check    18
>  #define TRAP_simd_error       19
> +#define TRAP_last_reserved    31
>  
>  /* Set for entry via SYSCALL. Informs return code to use SYSRETQ not
IRETQ.
> */
>  /* NB. Same as VGCF_in_syscall. No bits in common with any other TRAP_ 
> defn. */
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

On 20/04/2012 09:54, "Jan Beulich" <JBeulich@suse.com> wrote:
> I doubt this is generally correct, in particular for the use you appear
> to desire: When the injection is not the result of an INT nn
> instruction (which I would guess to be the case when coming from
> libxc), you shouldn''t set a non-zero instruction length. I believe
this
> is also wrong for the INT3 code above.
> 
> Additionally the problem should not be limited to injection coming
> from libxc - injection originating from x86_emulate() should be
> affected as much.
> 
> Jun, Eddie - I further wonder why #OF is not being handled according
> to the documentation here either (should also result in
> X86_EVENTTYPE_SW_EXCEPTION). And the fall-through from
> TRAP_debug to TRAP_int3 is suspicious too (at the very minimum it
> should be annotated with a comment saying why fall-through is
> intended here). Nor does the documentation state that TRAP_debug
> should ever result in X86_EVENTTYPE_SW_EXCEPTION.
> 
> Finally, the whole injection logic (including the patch here)
doesn''t
> appear to cope with INT nn being used by a guest with nn < 32, nor
> with any (pointless) prefixes used on INT3 or INT nn.
Agreed, I applied the patch because at least it doesn''t mess with any
existing logic for vectors < 32, but really this function is now an
overloaded mess. vmx_inject_hw_exception() should deal *only* with hw
exceptions, and a more general function should be provided for the more
general callers. Or something. It needs a bit of thought and is certainly
not 4.2 material now.

 -- Keir

> 
> Jun, Eddie - I further wonder why #OF is not being handled according
> to the documentation here either (should also result in
> X86_EVENTTYPE_SW_EXCEPTION). And the fall-through from
> TRAP_debug to TRAP_int3 is suspicious too (at the very minimum it
> should be annotated with a comment saying why fall-through is
> intended here). Nor does the documentation state that TRAP_debug
> should ever result in X86_EVENTTYPE_SW_EXCEPTION.
Mmm, SDM requires us to use X86_EVENTTYPE_SW_EXCEPTION for #OF & #BP, 
It seems we are slightly different here. Let me check w/ internal person.
> 
> Finally, the whole injection logic (including the patch here)
doesn''t
> appear to cope with INT nn being used by a guest with nn < 32, nor
The original code path works for the privilege violation introduced exceptions,
It seems we probbaly need a new code for INT n emulation for both interrupt
& exceptions.
> with any (pointless) prefixes used on INT3 or INT nn.
> 
What specific prefix do u mean here?

Thx, Eddie

>>> On 02.05.12 at 10:53, "Dong, Eddie"
<eddie.dong@intel.com> wrote:
>>  
>> Jun, Eddie - I further wonder why #OF is not being handled according
>> to the documentation here either (should also result in
>> X86_EVENTTYPE_SW_EXCEPTION). And the fall-through from
>> TRAP_debug to TRAP_int3 is suspicious too (at the very minimum it
>> should be annotated with a comment saying why fall-through is
>> intended here). Nor does the documentation state that TRAP_debug
>> should ever result in X86_EVENTTYPE_SW_EXCEPTION.
> 
> Mmm, SDM requires us to use X86_EVENTTYPE_SW_EXCEPTION for #OF & #BP, 
> It seems we are slightly different here. Let me check w/ internal person.
Thanks.
>> Finally, the whole injection logic (including the patch here)
doesn''t
>> appear to cope with INT nn being used by a guest with nn < 32, nor
> 
> The original code path works for the privilege violation introduced 
> exceptions,
> It seems we probbaly need a new code for INT n emulation for both interrupt
&
> exceptions. 
Indeed.
>> with any (pointless) prefixes used on INT3 or INT nn.
>> 
> What specific prefix do u mean here?
Anyone except perhaps LOCK - none of them should have any effect
other than making the instruction longer.

Jan

> >>
> >> Jun, Eddie - I further wonder why #OF is not being handled
according
> >> to the documentation here either (should also result in
> >> X86_EVENTTYPE_SW_EXCEPTION). And the fall-through from
> >> TRAP_debug to TRAP_int3 is suspicious too (at the very minimum it
> >> should be annotated with a comment saying why fall-through is
> >> intended here). Nor does the documentation state that TRAP_debug
> >> should ever result in X86_EVENTTYPE_SW_EXCEPTION.
> >
> > Mmm, SDM requires us to use X86_EVENTTYPE_SW_EXCEPTION for #OF &
> #BP,
> > It seems we are slightly different here. Let me check w/ internal
person.
> 
> Thanks.
The TRAP_debug should not use SW_EXCEPTION, it should use HW_EXCEPTION
Per SDM and confirmation from our HW guys. We will send fixes soon.

> 
> >> Finally, the whole injection logic (including the patch here)
doesn''t
> >> appear to cope with INT nn being used by a guest with nn < 32,
nor
> >
> > The original code path works for the privilege violation introduced
> > exceptions,
> > It seems we probbaly need a new code for INT n emulation for both
> interrupt &
> > exceptions.
> 
> Indeed.
This API vmx_inject_hw_exception is never intended to be used for INT nn
emulation,
Rather it is designed for the exceptions generated by processor-detected
program-error exceptions and machine check exceptions.

If the purpose of Aravindh''s patch is for INT nn emulation (CD nn), it
is incorrect. We need a new API for that purpose, and use software interrupt.
Of course, for INTO & INT 3 (CE & CC), we should use SW_EXCEPTION as SDM
mentioned.
> 
> >> with any (pointless) prefixes used on INT3 or INT nn.
> >>
> > What specific prefix do u mean here?
> 
> Anyone except perhaps LOCK - none of them should have any effect
> other than making the instruction longer.
> 
LOCK can never be used as prefix of INT nn instruction, nor can REPx prefix.
Can you provide more details as for this concern?

Thx, Eddie

On Wed, May 2, 2012 at 5:25 PM, Dong, Eddie <eddie.dong@intel.com>
wrote:
>> >>
>> >> Jun, Eddie - I further wonder why #OF is not being handled
according
>> >> to the documentation here either (should also result in
>> >> X86_EVENTTYPE_SW_EXCEPTION). And the fall-through from
>> >> TRAP_debug to TRAP_int3 is suspicious too (at the very minimum
it
>> >> should be annotated with a comment saying why fall-through is
>> >> intended here). Nor does the documentation state that
TRAP_debug
>> >> should ever result in X86_EVENTTYPE_SW_EXCEPTION.
>> >
>> > Mmm, SDM requires us to use X86_EVENTTYPE_SW_EXCEPTION for #OF
&
>> #BP,
>> > It seems we are slightly different here. Let me check w/ internal
person.
>>
>> Thanks.
>
> The TRAP_debug should not use SW_EXCEPTION, it should use HW_EXCEPTION
> Per SDM and confirmation from our HW guys. We will send fixes soon.
>
>
>>
>> >> Finally, the whole injection logic (including the patch here)
doesn''t
>> >> appear to cope with INT nn being used by a guest with nn <
32, nor
>> >
>> > The original code path works for the privilege violation
introduced
>> > exceptions,
>> > It seems we probbaly need a new code for INT n emulation for both
>> interrupt &
>> > exceptions.
>>
>> Indeed.
>
> This API vmx_inject_hw_exception is never intended to be used for INT nn
emulation,
> Rather it is designed for the exceptions generated by processor-detected
program-error exceptions and machine check exceptions.
>
> If the purpose of Aravindh''s patch is for INT nn emulation (CD
nn), it is incorrect. We need a new API for that purpose, and use software
interrupt.
> Of course, for INTO & INT 3 (CE & CC), we should use SW_EXCEPTION
as SDM mentioned.
>
The reason I submitted the patch was, calling xc_hvm_inject_trap() on
a software interrupt caused the guest to crash with a vmentry failure
because the interrupt was injected as a hardware interrupt. The patch
allowed me to inject a software interrupt successfully.

However I do agree that it is better if we have a separate API that
does not overload vmx_inject_hw_exception().

Thanks,
Aravindh

> 
> The reason I submitted the patch was, calling xc_hvm_inject_trap() on
> a software interrupt caused the guest to crash with a vmentry failure
That should use SW_INTERRUPT, not SW_EXCEPTION.
> because the interrupt was injected as a hardware interrupt. The patch
> allowed me to inject a software interrupt successfully.
> 
> However I do agree that it is better if we have a separate API that
> does not overload vmx_inject_hw_exception().
>

>>> On 03.05.12 at 02:25, "Dong, Eddie"
<eddie.dong@intel.com> wrote:
>> >>
>> >> Jun, Eddie - I further wonder why #OF is not being handled
according
>> >> to the documentation here either (should also result in
>> >> X86_EVENTTYPE_SW_EXCEPTION). And the fall-through from
>> >> TRAP_debug to TRAP_int3 is suspicious too (at the very minimum
it
>> >> should be annotated with a comment saying why fall-through is
>> >> intended here). Nor does the documentation state that
TRAP_debug
>> >> should ever result in X86_EVENTTYPE_SW_EXCEPTION.
>> >
>> > Mmm, SDM requires us to use X86_EVENTTYPE_SW_EXCEPTION for #OF
&
>> #BP,
>> > It seems we are slightly different here. Let me check w/ internal
person.
>> 
>> Thanks.
> 
> The TRAP_debug should not use SW_EXCEPTION, it should use HW_EXCEPTION
> Per SDM and confirmation from our HW guys. We will send fixes soon.
Please also have the opcode 0xF1 generated #DB addressed in
whatever is the appropriate way.
>> >> Finally, the whole injection logic (including the patch here)
doesn''t
>> >> appear to cope with INT nn being used by a guest with nn <
32, nor
>> >
>> > The original code path works for the privilege violation
introduced
>> > exceptions,
>> > It seems we probbaly need a new code for INT n emulation for both
>> interrupt &
>> > exceptions.
>> 
>> Indeed.
> 
> This API vmx_inject_hw_exception is never intended to be used for INT nn 
> emulation,
> Rather it is designed for the exceptions generated by processor-detected 
> program-error exceptions and machine check exceptions.
> 
> If the purpose of Aravindh''s patch is for INT nn emulation (CD
nn), it is
> incorrect. We need a new API for that purpose, and use software interrupt.
> Of course, for INTO & INT 3 (CE & CC), we should use SW_EXCEPTION
as SDM
> mentioned.
I''m sure he took it to be the correct one because it previously
handled #BP too.
>> >> with any (pointless) prefixes used on INT3 or INT nn.
>> >>
>> > What specific prefix do u mean here?
>> 
>> Anyone except perhaps LOCK - none of them should have any effect
>> other than making the instruction longer.
>> 
> LOCK can never be used as prefix of INT nn instruction, nor can REPx
prefix.
> Can you provide more details as for this concern?
The only prefix that is documented to cause #UD here is LOCK. All
other prefixes should consequently be considered ignored, and so
should the emulation do (and properly handle resulting instruction
lengths).

Jan

> > The TRAP_debug should not use SW_EXCEPTION, it should use
> HW_EXCEPTION
> > Per SDM and confirmation from our HW guys. We will send fixes soon.
> 
> Please also have the opcode 0xF1 generated #DB addressed in
> whatever is the appropriate way.
Opcode 0xf1 should use " privileged software exception".

What we can do probably include:
1: A patch to fix the mistake of #BP & #OF, plus additional comments to
state the usage of the API.
2: Another patch to provide a new API for 0xf1 & CD nn? But we
don''t have real usage case to test so far.

We will provide #1 quickly, but for #2, can Aravindh provide test if we get the
patch ready?
> 
> >>
> >> Anyone except perhaps LOCK - none of them should have any effect
> >> other than making the instruction longer.
> >>
> > LOCK can never be used as prefix of INT nn instruction, nor can REPx
> prefix.
> > Can you provide more details as for this concern?
> 
> The only prefix that is documented to cause #UD here is LOCK. All
In #UD case (fault), the guest RIP is not advanced per SDM, and therefore guest
will either
spin in the previous LOCK instruction, or advance the IP to next instruction by
guest #UD handler.
I didn''t see emulator could advance IP to the next instruction (INT nn)
for LOCK prefix.
Do I miss something?
> other prefixes should consequently be considered ignored, and so
> should the emulation do (and properly handle resulting instruction
> lengths).
> 
The behavior is un-defined per SDM in this case, so either solution should be
fine :)

Thx, Eddie

>>> On 03.05.12 at 15:42, "Dong, Eddie"
<eddie.dong@intel.com> wrote:
>> > The TRAP_debug should not use SW_EXCEPTION, it should use
>> HW_EXCEPTION
>> > Per SDM and confirmation from our HW guys. We will send fixes
soon.
>> 
>> Please also have the opcode 0xF1 generated #DB addressed in
>> whatever is the appropriate way.
> 
> Opcode 0xf1 should use " privileged software exception".
> 
> What we can do probably include:
> 1: A patch to fix the mistake of #BP & #OF, plus additional comments to
state
> the usage of the API.
> 2: Another patch to provide a new API for 0xf1 & CD nn? But we
don''t have
> real usage case to test so far.
> 
> We will provide #1 quickly, but for #2, can Aravindh provide test if we get
> the patch ready?
> 
>> 
>> >>
>> >> Anyone except perhaps LOCK - none of them should have any
effect
>> >> other than making the instruction longer.
>> >>
>> > LOCK can never be used as prefix of INT nn instruction, nor can
REPx
>> prefix.
>> > Can you provide more details as for this concern?
>> 
>> The only prefix that is documented to cause #UD here is LOCK. All
> 
> In #UD case (fault), the guest RIP is not advanced per SDM, and therefore 
> guest will either 
> spin in the previous LOCK instruction, or advance the IP to next
instruction
> by guest #UD handler.
> I didn''t see emulator could advance IP to the next instruction
(INT nn) for
> LOCK prefix.
> Do I miss something?
I''m sure you misunderstand me. I was saying that LOCK is the only
prefix we can validly assume was not present on the original
instruction.

Any other prefix could be present, and should count towards the
instruction length. Note the

        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */

and (after the recent change for INT nn)

            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 2); /* int imm8 */

which both use hard coded values.

Furthermore, for Aravindh''s use case where there might not even
be an "original instruction" (i.e. injecting an interrupt/exception
for
reasons other than emulating a respective instruction), advancing IP
seems bogus to me altogether.
>> other prefixes should consequently be considered ignored, and so
>> should the emulation do (and properly handle resulting instruction
>> lengths).
>> 
> The behavior is un-defined per SDM in this case, so either solution should
be
> fine :)
Can you please point me to where this is being stated? I particularly
doubt that for operand and address size prefixes as well as on 64-bit
- since they are documented to be ignored there - CS, DS, ES, and SS
segment prefixes...

Jan

On 03/05/2012 14:42, "Dong, Eddie" <eddie.dong@intel.com> wrote:
>>> The TRAP_debug should not use SW_EXCEPTION, it should use
>> HW_EXCEPTION
>>> Per SDM and confirmation from our HW guys. We will send fixes soon.
>> 
>> Please also have the opcode 0xF1 generated #DB addressed in
>> whatever is the appropriate way.
> 
> Opcode 0xf1 should use " privileged software exception".
> 
> What we can do probably include:
> 1: A patch to fix the mistake of #BP & #OF, plus additional comments to
state
> the usage of the API.
> 2: Another patch to provide a new API for 0xf1 & CD nn? But we
don''t have real
> usage case to test so far.
Yes, this sounds great.

 -- Keir
> We will provide #1 quickly, but for #2, can Aravindh provide test if we get
> the patch ready?
> 
>> 
>>>> 
>>>> Anyone except perhaps LOCK - none of them should have any
effect
>>>> other than making the instruction longer.
>>>> 
>>> LOCK can never be used as prefix of INT nn instruction, nor can
REPx
>> prefix.
>>> Can you provide more details as for this concern?
>> 
>> The only prefix that is documented to cause #UD here is LOCK. All
> 
> In #UD case (fault), the guest RIP is not advanced per SDM, and therefore
> guest will either
> spin in the previous LOCK instruction, or advance the IP to next
instruction
> by guest #UD handler.
> I didn''t see emulator could advance IP to the next instruction
(INT nn) for
> LOCK prefix.
> Do I miss something?
> 
>> other prefixes should consequently be considered ignored, and so
>> should the emulation do (and properly handle resulting instruction
>> lengths).
>> 
> The behavior is un-defined per SDM in this case, so either solution should
be
> fine :)
> 
> Thx, Eddie
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Thu, May 3, 2012 at 6:42 AM, Dong, Eddie <eddie.dong@intel.com>
wrote:
>> > The TRAP_debug should not use SW_EXCEPTION, it should use
>> HW_EXCEPTION
>> > Per SDM and confirmation from our HW guys. We will send fixes
soon.
>>
>> Please also have the opcode 0xF1 generated #DB addressed in
>> whatever is the appropriate way.
>
> Opcode 0xf1 should use " privileged software exception".
>
> What we can do probably include:
> 1: A patch to fix the mistake of #BP & #OF, plus additional comments to
state the usage of the API.
> 2: Another patch to provide a new API for 0xf1 & CD nn? But we
don''t have real usage case to test so far.
>
> We will provide #1 quickly, but for #2, can Aravindh provide test if we get
the patch ready?
I will gladly debug and test #2 for you.

Thanks,
Aravindh