trap bounce flags

trap bounce flags

[Jan Beulich]

With the severe stability issues we are having with SLE10sp1 on x86-64, things
start pointing pretty closely at the int80 direct trap patch we imported from
-unstable. While I just now realized that there's been a fix for these problems
for quite a while (don't know how this slipped my attention), I still have a few
notes:

- even compat_restore_all_guest now asserts interrupts are disabled, despite
  32-bit restore_all_guest not doing so (and the iret path not generally needing
  this)
- int80_direct_trap checks for non-zero TRAPBOUNCE_flags, yet
  {,compat_}create_bounce_frame clear the low byte of these flags (i.e.
  including TBF_exception, which is in this lower byte); it appears to be only a
  lucky coincidence that this still works as the cmp (again!) is suffix-less and
  hence gets sized as a 32-bit compare, accidentally covering TRAPBOUNCE_cs
- from the above, why is it that only the lower byte (if anything) needs clearing?

Jan

Re: trap bounce flags

[Keir Fraser]

On 25/4/07 10:56, "Jan Beulich" <jbeulich@novell.com> wrote:

> - even compat_restore_all_guest now asserts interrupts are disabled, despite
>   32-bit restore_all_guest not doing so (and the iret path not generally
>   needing this)

The 32-bit restore_all_guest ought to have the assertion added. I must have
forgotten about it. Interrupts need to be disabled during return to guest so
that the tests for softirq work and event-notification to the guest do not
race against new interrupts. Of course this issue is much less fatal than
the restore-rsp;sysret bug!

> - int80_direct_trap checks for non-zero TRAPBOUNCE_flags, yet
>   {,compat_}create_bounce_frame clear the low byte of these flags (i.e.
>   including TBF_exception, which is in this lower byte); it appears to be only
>   a lucky coincidence that this still works as the cmp (again!) is suffix-less
>   and hence gets sized as a 32-bit compare, accidentally coveringTRAPBOUNCE_cs

Ooo, good catch. It's a tiny bit gross but the best fix is probably just to
restore the flags field after the call to create_bounce_frame. And of course
change the cmp to a cmpb. Agree?

> - from the above, why is it that only the lower byte (if anything) needs
> clearing?

Really it's a one-byte field: it's consistently treated that way in asm
code. The upper byte is always zero. We should probably make the field
explicitly uint8_t. Agree?

 -- Keir

Re: trap bounce flags

[Keir Fraser]

On 25/4/07 11:10, "Keir Fraser" <keir@xensource.com> wrote:

>> - from the above, why is it that only the lower byte (if anything) needs
>> clearing?
> 
> Really it's a one-byte field: it's consistently treated that way in asm
> code. The upper byte is always zero. We should probably make the field
> explicitly uint8_t. Agree?

FYI, we need to clear that field usually because the contents are one-shot
but the structure itself is permanent and checked on every return to guest.
A non-zero flags field is what we use to indicate whether the structure is
'primed' or not. So the passing in of a different, non-one-shot, structure
on the direct-int80 path is a bit abusive of the interface. But I think we
can live with it if we reset the flags field manually and include a comment.
:-)

 -- Keir

Re: trap bounce flags

[Jan Beulich]

>> - int80_direct_trap checks for non-zero TRAPBOUNCE_flags, yet
>>   {,compat_}create_bounce_frame clear the low byte of these flags (i.e.
>>   including TBF_exception, which is in this lower byte); it appears to be only
>>   a lucky coincidence that this still works as the cmp (again!) is suffix-less
>>   and hence gets sized as a 32-bit compare, accidentally coveringTRAPBOUNCE_cs
>
>Ooo, good catch. It's a tiny bit gross but the best fix is probably just to
>restore the flags field after the call to create_bounce_frame. And of course
>change the cmp to a cmpb. Agree?

That's the alternative solution I considered. The preferable one is to do the
compat/native distinction before the null check, and then be consistent with
the rest of the code and check cs for 32-bit guest and eip for 64-bit ones.
That's how I'm preparing a patch right now.

>> - from the above, why is it that only the lower byte (if anything) needs
>> clearing?
>
>Really it's a one-byte field: it's consistently treated that way in asm
>code. The upper byte is always zero. We should probably make the field
>explicitly uint8_t. Agree?

Making it a uint8_t is fine. It is, however, far from being consistently handled
in assembly code:
x86_32/entry.S: 4 word refs and 3 byte refs
x86_64/entry.S: 6 word refs, 3 byte refs, and one size-less ref
x86_64/compat/entry.S: 4 word refs and 3 byte refs

Jan

Re: trap bounce flags

[Keir Fraser]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On 25/4/07 11:33, "Jan Beulich" <jbeulich@novell.com> wrote:

> That's the alternative solution I considered. The preferable one is to do the
> compat/native distinction before the null check, and then be consistent with
> the rest of the code and check cs for 32-bit guest and eip for 64-bit ones.
> That's how I'm preparing a patch right now.

Attached is my own proposed patch which I think cleans up all the issues.
Checking just flags in asm and keeping the null-bounce check in
init_int80_direct_trap() seems fine to me.

 -- Keir

>>> - from the above, why is it that only the lower byte (if anything) needs
>>> clearing?
>> 
>> Really it's a one-byte field: it's consistently treated that way in asm
>> code. The upper byte is always zero. We should probably make the field
>> explicitly uint8_t. Agree?
> 
> Making it a uint8_t is fine. It is, however, far from being consistently
> handled
> in assembly code:
> x86_32/entry.S: 4 word refs and 3 byte refs
> x86_64/entry.S: 6 word refs, 3 byte refs, and one size-less ref
> x86_64/compat/entry.S: 4 word refs and 3 byte refs


[-- Attachment #2: 00-fix-trapbounce --]
[-- Type: application/octet-stream, Size: 8739 bytes --]

diff -r 867965efcbd2 xen/arch/x86/x86_32/entry.S
--- a/xen/arch/x86/x86_32/entry.S	Wed Apr 25 09:49:18 2007 +0100
+++ b/xen/arch/x86/x86_32/entry.S	Wed Apr 25 11:32:26 2007 +0100
@@ -75,6 +75,7 @@
 
         ALIGN
 restore_all_guest:
+        ASSERT_INTERRUPTS_DISABLED
         testl $X86_EFLAGS_VM,UREGS_eflags(%esp)
         jnz  restore_all_vm86
 #ifdef CONFIG_X86_SUPERVISOR_MODE_KERNEL
@@ -129,10 +130,10 @@ failsafe_callback:
         movl  %eax,TRAPBOUNCE_eip(%edx)
         movl  VCPU_failsafe_sel(%ebx),%eax
         movw  %ax,TRAPBOUNCE_cs(%edx)
-        movw  $TBF_FAILSAFE,TRAPBOUNCE_flags(%edx)
+        movb  $TBF_FAILSAFE,TRAPBOUNCE_flags(%edx)
         bt    $_VGCF_failsafe_disables_events,VCPU_guest_context_flags(%ebx)
         jnc   1f
-        orw   $TBF_INTERRUPT,TRAPBOUNCE_flags(%edx)
+        orb   $TBF_INTERRUPT,TRAPBOUNCE_flags(%edx)
 1:      call  create_bounce_frame
         xorl  %eax,%eax
         movl  %eax,UREGS_ds(%esp)
@@ -247,7 +248,7 @@ test_guest_events:
         movl %eax,TRAPBOUNCE_eip(%edx)
         movl VCPU_event_sel(%ebx),%eax
         movw %ax,TRAPBOUNCE_cs(%edx)
-        movw $TBF_INTERRUPT,TRAPBOUNCE_flags(%edx)
+        movb $TBF_INTERRUPT,TRAPBOUNCE_flags(%edx)
         call create_bounce_frame
         jmp  test_all_events
 
@@ -270,7 +271,7 @@ process_nmi:
         leal VCPU_trap_bounce(%ebx),%edx
         movl %eax,TRAPBOUNCE_eip(%edx)
         movw $FLAT_KERNEL_CS,TRAPBOUNCE_cs(%edx)
-        movw $TBF_INTERRUPT,TRAPBOUNCE_flags(%edx)
+        movb $TBF_INTERRUPT,TRAPBOUNCE_flags(%edx)
         call create_bounce_frame
         jmp  test_all_events
 
@@ -383,7 +384,6 @@ 2:      testl $X86_EFLAGS_VM,UREGS_eflag
         movl %eax,UREGS_cs+4(%esp)
         movl TRAPBOUNCE_eip(%edx),%eax
         movl %eax,UREGS_eip+4(%esp)
-        movb $0,TRAPBOUNCE_flags(%edx)
         ret
 .section __ex_table,"a"
         .long  .Lft6,domain_crash_synchronous ,  .Lft7,domain_crash_synchronous
@@ -441,6 +441,7 @@ 1:      xorl  %eax,%eax
         testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%edx)
         jz    test_all_events
         call  create_bounce_frame
+        movb  $0,TRAPBOUNCE_flags(%edx)
         jmp   test_all_events
 
 exception_with_ints_disabled:
diff -r 867965efcbd2 xen/arch/x86/x86_64/compat/entry.S
--- a/xen/arch/x86/x86_64/compat/entry.S	Wed Apr 25 09:49:18 2007 +0100
+++ b/xen/arch/x86/x86_64/compat/entry.S	Wed Apr 25 11:33:14 2007 +0100
@@ -102,7 +102,7 @@ compat_test_guest_events:
         movl  %eax,TRAPBOUNCE_eip(%rdx)
         movl  VCPU_event_sel(%rbx),%eax
         movl  %eax,TRAPBOUNCE_cs(%rdx)
-        movw  $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
+        movb  $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
         call  compat_create_bounce_frame
         jmp   compat_test_all_events
 
@@ -127,7 +127,7 @@ compat_process_nmi:
         leaq  VCPU_trap_bounce(%rbx),%rdx
         movl  %eax,TRAPBOUNCE_eip(%rdx)
         movl  $FLAT_COMPAT_KERNEL_CS,TRAPBOUNCE_cs(%rdx)
-        movw  $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
+        movb  $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
         call  compat_create_bounce_frame
         jmp   compat_test_all_events
 
@@ -165,12 +165,11 @@ compat_failsafe_callback:
         movl  %eax,TRAPBOUNCE_eip(%rdx)
         movl  VCPU_failsafe_sel(%rbx),%eax
         movl  %eax,TRAPBOUNCE_cs(%rdx)
-        movw  $TBF_FAILSAFE,TRAPBOUNCE_flags(%rdx)
+        movb  $TBF_FAILSAFE,TRAPBOUNCE_flags(%rdx)
         btq   $_VGCF_failsafe_disables_events,VCPU_guest_context_flags(%rbx)
         jnc   1f
-        orw   $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
-1:
-        call  compat_create_bounce_frame
+        orb   $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
+1:      call  compat_create_bounce_frame
         jmp   compat_test_all_events
 .previous
 .section __pre_ex_table,"a"
@@ -185,6 +184,7 @@ ENTRY(compat_post_handle_exception)
         testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
         jz    compat_test_all_events
         call  compat_create_bounce_frame
+        movb  $0,TRAPBOUNCE_flags(%rdx)
         jmp   compat_test_all_events
 
 ENTRY(compat_int80_direct_trap)
@@ -194,7 +194,7 @@ ENTRY(compat_int80_direct_trap)
 /* CREATE A BASIC EXCEPTION FRAME ON GUEST OS (RING-1) STACK:            */
 /*   {[ERRCODE,] EIP, CS, EFLAGS, [ESP, SS]}                             */
 /* %rdx: trap_bounce, %rbx: struct vcpu                                  */
-/* On return only %rbx is guaranteed non-clobbered.                      */
+/* On return only %rbx and %rdx are guaranteed non-clobbered.            */
 compat_create_bounce_frame:
         ASSERT_INTERRUPTS_ENABLED
         mov   %fs,%edi
@@ -266,7 +266,6 @@ 2:
         movl  %eax,UREGS_cs+8(%rsp)
         movl  TRAPBOUNCE_eip(%rdx),%eax
         movl  %eax,UREGS_rip+8(%rsp)
-        movb  $0,TRAPBOUNCE_flags(%rdx)
         ret
 .section .fixup,"ax"
 .Lfx13:
diff -r 867965efcbd2 xen/arch/x86/x86_64/entry.S
--- a/xen/arch/x86/x86_64/entry.S	Wed Apr 25 09:49:18 2007 +0100
+++ b/xen/arch/x86/x86_64/entry.S	Wed Apr 25 11:31:46 2007 +0100
@@ -29,10 +29,10 @@ switch_to_kernel:
         leaq  VCPU_trap_bounce(%rbx),%rdx
         movq  VCPU_syscall_addr(%rbx),%rax
         movq  %rax,TRAPBOUNCE_eip(%rdx)
-        movw  $0,TRAPBOUNCE_flags(%rdx)
+        movb  $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
         bt    $_VGCF_syscall_disables_events,VCPU_guest_context_flags(%rbx)
         jnc   1f
-        orw   $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
+        movb  $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
 1:      call  create_bounce_frame
         jmp   test_all_events
 
@@ -80,10 +80,10 @@ failsafe_callback:
         leaq  VCPU_trap_bounce(%rbx),%rdx
         movq  VCPU_failsafe_addr(%rbx),%rax
         movq  %rax,TRAPBOUNCE_eip(%rdx)
-        movw  $TBF_FAILSAFE,TRAPBOUNCE_flags(%rdx)
+        movb  $TBF_FAILSAFE,TRAPBOUNCE_flags(%rdx)
         bt    $_VGCF_failsafe_disables_events,VCPU_guest_context_flags(%rbx)
         jnc   1f
-        orw   $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
+        orb   $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
 1:      call  create_bounce_frame
         jmp   test_all_events
 .previous
@@ -191,7 +191,7 @@ test_guest_events:
         leaq  VCPU_trap_bounce(%rbx),%rdx
         movq  VCPU_event_addr(%rbx),%rax
         movq  %rax,TRAPBOUNCE_eip(%rdx)
-        movw  $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
+        movb  $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
         call  create_bounce_frame
         jmp   test_all_events
 
@@ -215,7 +215,7 @@ process_nmi:
         sti
         leaq VCPU_trap_bounce(%rbx),%rdx
         movq %rax,TRAPBOUNCE_eip(%rdx)
-        movw $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
+        movb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
         call create_bounce_frame
         jmp  test_all_events
 
@@ -231,7 +231,7 @@ ENTRY(int80_direct_trap)
 
         /* Check that the callback is non-null. */
         leaq  VCPU_int80_bounce(%rbx),%rdx
-        cmp   $0,TRAPBOUNCE_flags(%rdx)
+        cmpb  $0,TRAPBOUNCE_flags(%rdx)
         jz    int80_slow_path
 
         movq  VCPU_domain(%rbx),%rax
@@ -254,8 +254,8 @@ int80_slow_path:
 
 /* CREATE A BASIC EXCEPTION FRAME ON GUEST OS STACK:                     */
 /*   { RCX, R11, [DS-GS,] [CR2,] [ERRCODE,] RIP, CS, RFLAGS, RSP, SS }   */
-/* %rdx: trap_bounce, %rbx: struct vcpu                           */
-/* On return only %rbx is guaranteed non-clobbered.                      */
+/* %rdx: trap_bounce, %rbx: struct vcpu                                  */
+/* On return only %rbx and %rdx are guaranteed non-clobbered.            */
 create_bounce_frame:
         ASSERT_INTERRUPTS_ENABLED
         testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
@@ -336,7 +336,6 @@ 2:      subq  $16,%rsi
         testq %rax,%rax
         jz    domain_crash_synchronous
         movq  %rax,UREGS_rip+8(%rsp)
-        movb  $0,TRAPBOUNCE_flags(%rdx)
         ret
 .section __ex_table,"a"
         .quad  .Lft2,domain_crash_synchronous ,  .Lft3,domain_crash_synchronous
@@ -401,6 +400,7 @@ 1:      movq  %rsp,%rdi
         testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
         jz    test_all_events
         call  create_bounce_frame
+        movb  $0,TRAPBOUNCE_flags(%rdx)
         jmp   test_all_events
 
 /* No special register assumptions. */
diff -r 867965efcbd2 xen/include/asm-x86/domain.h
--- a/xen/include/asm-x86/domain.h	Wed Apr 25 09:49:18 2007 +0100
+++ b/xen/include/asm-x86/domain.h	Wed Apr 25 11:36:10 2007 +0100
@@ -8,10 +8,10 @@
 #include <asm/e820.h>
 
 struct trap_bounce {
-    unsigned long  error_code;
-    unsigned short flags; /* TBF_ */
-    unsigned short cs;
-    unsigned long  eip;
+    uint32_t      error_code;
+    uint8_t       flags; /* TBF_ */
+    uint16_t      cs;
+    unsigned long eip;
 };
 
 #define MAPHASH_ENTRIES 8

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: trap bounce flags

[Keir Fraser]

On 25/4/07 11:41, "Keir Fraser" <keir@xensource.com> wrote:

> Attached is my own proposed patch which I think cleans up all the issues.
> Checking just flags in asm and keeping the null-bounce check in
> init_int80_direct_trap() seems fine to me.

The change of a movw $0 to a movb $TBF_EXCEPTION in that patch is wrong, by
the way. Should be movb $0.

 -- Keir

Re: trap bounce flags

[Jan Beulich]

>>> Keir Fraser <keir@xensource.com> 25.04.07 12:56 >>>
>On 25/4/07 11:41, "Keir Fraser" <keir@xensource.com> wrote:
>
>> Attached is my own proposed patch which I think cleans up all the issues.
>> Checking just flags in asm and keeping the null-bounce check in
>> init_int80_direct_trap() seems fine to me.
>
>The change of a movw $0 to a movb $TBF_EXCEPTION in that patch is wrong, by
>the way. Should be movb $0.

Which means there's not really a dependency on this being non-zero...

The patch looks otherwise okay to me, though I think there's one more
issue here: There's another suffix-less instruction (updating UREGS_rip
in int80_slow_path) - this must be a subq, and it must imply that no 32-bit
guest places an int $0x80 at 0xfffffffe.

And my patch has a not directly related adjustment removing the

        movl  $TRAP_syscall,UREGS_entry_vector+8(%rsp)

close to the end of compat_create_bounce_frame, as this is meaningless
here.


Jan

Re: trap bounce flags

[Keir Fraser]

On 25/4/07 12:11, "Jan Beulich" <jbeulich@novell.com> wrote:

> Which means there's not really a dependency on this being non-zero...

Yeah, I limited that dependency just to the handle_exception path. It now
zeroes the flags field itself which. It might be even cleaner to host the
zero of the flags field to before the indirect call to the
exception-specific C handler (that's the only thing that might set up the
bounce structure).

> The patch looks otherwise okay to me, though I think there's one more
> issue here: There's another suffix-less instruction (updating UREGS_rip
> in int80_slow_path) - this must be a subq, and it must imply that no 32-bit
> guest places an int $0x80 at 0xfffffffe.

Yep.

> And my patch has a not directly related adjustment removing the
> 
>         movl  $TRAP_syscall,UREGS_entry_vector+8(%rsp)
> 
> close to the end of compat_create_bounce_frame, as this is meaningless
> here.

Should we also remove it from compat_hypercall?

 -- Keir

Re: trap bounce flags

[Jan Beulich]

>> And my patch has a not directly related adjustment removing the
>> 
>>         movl  $TRAP_syscall,UREGS_entry_vector+8(%rsp)
>> 
>> close to the end of compat_create_bounce_frame, as this is meaningless
>> here.
>
>Should we also remove it from compat_hypercall?

Probably not, to be able to identify the frame properly in case something
crashes in the process of handling the hypercall.

Jan