[PATCH] Fixing stack alignment in x86-64 Xen

[PATCH] Fixing stack alignment in x86-64 Xen

[Nakajima, Jun]

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

Long mode needs to align the stack on a 16-byte boundary. Recent changes
to Xen broke the requirement, and x86-64 XenLinux stopped booting. The
attached fixes the problem. 

Signed-off-by: Jun Nakajima <jun.nakajima@intel.com>

Jun

[-- Attachment #2: stack_alignment.patch --]
[-- Type: application/octet-stream, Size: 1555 bytes --]

diff -ur xen-unstable.orig/xen/include/asm-x86/x86_64/current.h xen-unstable/xen/include/asm-x86/x86_64/current.h
--- xen-unstable.orig/xen/include/asm-x86/x86_64/current.h	2005-05-16 20:27:42.000000000 -0700
+++ xen-unstable/xen/include/asm-x86/x86_64/current.h	2005-05-17 18:53:25.264794104 -0700
@@ -34,15 +34,16 @@
 
 /*
  * Get the bottom-of-stack, as stored in the per-CPU TSS. This is actually
- * 40 bytes before the real bottom of the stack to allow space for:
- *  domain pointer, DS, ES, FS, GS
+ * 48 bytes before the real bottom of the stack to allow space for:
+ * domain pointer, padding, DS, ES, FS, GS. The padding is required to
+ * have the stack pointer 16-byte aligned.
  */
 static inline unsigned long get_stack_bottom(void)
 {
     unsigned long p;
     __asm__( "andq %%rsp,%0; addq %2,%0"
 	    : "=r" (p)
-	    : "0" (~(STACK_SIZE-1)), "i" (STACK_SIZE-40) );
+	    : "0" (~(STACK_SIZE-1)), "i" (STACK_SIZE-48) );
     return p;
 }
 
diff -ur xen-unstable.orig/xen/include/public/arch-x86_64.h xen-unstable/xen/include/public/arch-x86_64.h
--- xen-unstable.orig/xen/include/public/arch-x86_64.h	2005-05-16 20:27:43.000000000 -0700
+++ xen-unstable/xen/include/public/arch-x86_64.h	2005-05-17 18:51:32.350959616 -0700
@@ -169,6 +169,7 @@
     u64 ds;
     u64 fs;      /* Non-zero => takes precedence over fs_base.      */
     u64 gs;      /* Non-zero => takes precedence over gs_base_user. */
+    u64 padding; /* Get the stack bottom 16-byte aligned */
 } cpu_user_regs_t;
 
 typedef u64 tsc_timestamp_t; /* RDTSC timestamp */

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

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

Re: [PATCH] Fixing stack alignment in x86-64 Xen

[Scott Parish]

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

On Tue, May 17, 2005 at 07:25:46PM -0700, Nakajima, Jun wrote:

> Long mode needs to align the stack on a 16-byte boundary. Recent changes
> to Xen broke the requirement, and x86-64 XenLinux stopped booting. The
> attached fixes the problem. 

Awesome!

I also needed the attached patch, which between the two of these,
makes one wonder how stuff like this is ending up in the tree. Is
code getting checked in without even being compile tested?

sRp

-- 
Scott Parish
Signed-off-by: srparish@us.ibm.com

[-- Attachment #2: rex.diff --]
[-- Type: text/plain, Size: 948 bytes --]

diff -rN -u -p old-xen-64/xen/arch/x86/x86_64/entry.S new-xen-64/xen/arch/x86/x86_64/entry.S
--- old-xen-64/xen/arch/x86/x86_64/entry.S	2005-05-17 19:23:04.000000000 +0000
+++ new-xen-64/xen/arch/x86/x86_64/entry.S	2005-05-18 00:44:21.000000000 +0000
@@ -314,10 +314,10 @@ FLT4:   movq  %rax,16(%rsi)             
         movq  EDOMAIN_vcpu_info(%rbx),%rax
         pushq VCPUINFO_upcall_mask(%rax)
         testb $TBF_INTERRUPT,%cl
-        setnz VCPUINFO_upcall_mask(%eax)# TBF_INTERRUPT -> clear upcall mask
+        setnz VCPUINFO_upcall_mask(%rax)# TBF_INTERRUPT -> clear upcall mask
         popq  %rax
         shll  $16,%eax                  # Bits 16-23: saved_upcall_mask
-        movw  UREGS_cs+8(%esp),%ax      # Bits  0-15: CS
+        movw  UREGS_cs+8(%rsp),%ax      # Bits  0-15: CS
 FLT5:   movq  %rax,8(%rsi)              # CS/saved_upcall_mask
         movq  UREGS_rip+8(%rsp),%rax
 FLT6:   movq  %rax,(%rsi)               # RIP

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

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

Re: [PATCH] Fixing stack alignment in x86-64 Xen

[Keir Fraser]

On 18 May 2005, at 03:25, Nakajima, Jun wrote:

> Long mode needs to align the stack on a 16-byte boundary. Recent 
> changes
> to Xen broke the requirement, and x86-64 XenLinux stopped booting. The
> attached fixes the problem.
>
> Signed-off-by: Jun Nakajima <jun.nakajima@intel.com>

Thanks Jun, that was a nasty bug to have to find. I keep forgetting 
about that annoying automatic 16-byte alignment...

  -- Keir

Re: [PATCH] Fixing stack alignment in x86-64 Xen

[Andi Kleen]

"Nakajima, Jun" <jun.nakajima@intel.com> writes:

> Long mode needs to align the stack on a 16-byte boundary. Recent changes
> to Xen broke the requirement, and x86-64 XenLinux stopped booting. The
> attached fixes the problem. 

Normally it should be only needed in user space for saving FP registers.
The kernel and Xen which should not do this probably don't need it.

Unless you save FP registers on the stack somewhere. Then I would
rather fix that place only.

At least the main linux kernel does not try to keep the stack
always 16byte aligned. There is even a gcc option to turn it off
and it saves some code (and probably stack) size.

-Andi

RE: [PATCH] Fixing stack alignment in x86-64 Xen

[Ian Pratt]

> > Long mode needs to align the stack on a 16-byte boundary. Recent 
> > changes to Xen broke the requirement, and x86-64 XenLinux stopped 
> > booting. The attached fixes the problem.
> >
> > Signed-off-by: Jun Nakajima <jun.nakajima@intel.com>
> 
> Thanks Jun, that was a nasty bug to have to find. I keep 
> forgetting about that annoying automatic 16-byte alignment...

Let's put an assert in there.

Ian

RE: [PATCH] Fixing stack alignment in x86-64 Xen

[Nakajima, Jun]

Andi Kleen wrote:
> "Nakajima, Jun" <jun.nakajima@intel.com> writes:
> 
>> Long mode needs to align the stack on a 16-byte boundary. Recent
>> changes to Xen broke the requirement, and x86-64 XenLinux stopped
>> booting. The attached fixes the problem.
> 
> Normally it should be only needed in user space for saving FP
> registers. The kernel and Xen which should not do this probably don't
> need it. 

Normally they don't care. The problem was that Xen set rsp0 in TSS on a
8-byte boundary and upon interrupts in Ring3 (XenLinux or user
processes) the processor started pushing registers (ss, rsp, rflags, ..,
rip) on the 16-byte boundary in Xen.  

The recent optimization reset_stack_and_jump() code needs to know the
exact address of the interrupt stack (because it resets %rsp), and
calculates it based on the value that Xen set (i.e. 8-byte boundary).
Since the processor forces the rsp0 on a 16-byte boundary (i.e. moves it
down by 8 bytes), Xen sees a wrong stack when returning from the
interrupt.

Jun

> 
> Unless you save FP registers on the stack somewhere. Then I would
> rather fix that place only.
> 
> At least the main linux kernel does not try to keep the stack
> always 16byte aligned. There is even a gcc option to turn it off
> and it saves some code (and probably stack) size.
> 
> -Andi

Re: [PATCH] Fixing stack alignment in x86-64 Xen

[Andi Kleen]

"Nakajima, Jun" <jun.nakajima@intel.com> writes:
>
> The recent optimization reset_stack_and_jump() code needs to know the
> exact address of the interrupt stack (because it resets %rsp), and
> calculates it based on the value that Xen set (i.e. 8-byte boundary).
> Since the processor forces the rsp0 on a 16-byte boundary (i.e. moves it
> down by 8 bytes), Xen sees a wrong stack when returning from the
> interrupt.

I would rather fix reset_stack_and_jump then to do the necessary
rounding or better look at the original RSP the processor stored into the
stack frame. Otherwise the 16 byte alignment will probably bite you
later again.

-Andi

RE: [PATCH] Fixing stack alignment in x86-64 Xen

[Nakajima, Jun]

Andi Kleen wrote:
> "Nakajima, Jun" <jun.nakajima@intel.com> writes:
>> 
>> The recent optimization reset_stack_and_jump() code needs to know the
>> exact address of the interrupt stack (because it resets %rsp), and
>> calculates it based on the value that Xen set (i.e. 8-byte boundary).
>> Since the processor forces the rsp0 on a 16-byte boundary (i.e.
>> moves it down by 8 bytes), Xen sees a wrong stack when returning
>> from the interrupt.
> 
> I would rather fix reset_stack_and_jump then to do the necessary
> rounding or better look at the original RSP the processor stored into
> the stack frame. Otherwise the 16 byte alignment will probably bite
> you later again.
> 
> -Andi

I think the right thing is to get rsp0 in TSS on a 16-byte boundary by
getting get_stack_bottom() and get_cpu_user_regs() see the correct
stack. That will fix the reset_stack_and_jump() as well. It's basically
what my patch does. 

Jun

Re: [PATCH] Fixing stack alignment in x86-64 Xen

[Andi Kleen]

> I think the right thing is to get rsp0 in TSS on a 16-byte boundary by
> getting get_stack_bottom() and get_cpu_user_regs() see the correct
> stack. That will fix the reset_stack_and_jump() as well. It's basically
> what my patch does. 

This means you cannot disable the 16 byte stack alignment in gcc.
Probably does not matter too much today (I guess Xen is not that
bad a stack pig), but in the far future it might come in useful.
Also it would generate smaller code.

-Andi

RE: [PATCH] Fixing stack alignment in x86-64 Xen

[Nakajima, Jun]

Andi Kleen wrote:
>> I think the right thing is to get rsp0 in TSS on a 16-byte boundary
>> by getting get_stack_bottom() and get_cpu_user_regs() see the correct
>> stack. That will fix the reset_stack_and_jump() as well. It's
>> basically what my patch does.
> 
> This means you cannot disable the 16 byte stack alignment in gcc.
> Probably does not matter too much today (I guess Xen is not that
> bad a stack pig), but in the far future it might come in useful.
> Also it would generate smaller code.
> 
> -Andi

I don't think that's correct. If you look at how they calculate the
stack pointer, fortunately they depend only on STACK_RESERVED and the
magic number 48 (see below). It does not matter if gcc used 16 byte
stack alignment or not because the current RSP will be rounded down to
the 8KB boundary when calculate the stack pointer.
  
#define STACK_RESERVED \
    (sizeof(struct cpu_user_regs) + sizeof(struct domain *))

static inline struct cpu_user_regs *get_cpu_user_regs(void)
{
    struct cpu_user_regs *cpu_user_regs;
    __asm__( "andq %%rsp,%0; addq %2,%0"
	    : "=r" (cpu_user_regs)
	    : "0" (~(STACK_SIZE-1)), "i" (STACK_SIZE-STACK_RESERVED) ); 
    return cpu_user_regs;
}

/*
 * Get the bottom-of-stack, as stored in the per-CPU TSS. This is
actually
 * 48 bytes before the real bottom of the stack to allow space for:
 * domain pointer, padding, DS, ES, FS, GS. The padding is required to
 * have the stack pointer 16-byte aligned.
 */
static inline unsigned long get_stack_bottom(void)
{
    unsigned long p;
    __asm__( "andq %%rsp,%0; addq %2,%0"
	    : "=r" (p)
	    : "0" (~(STACK_SIZE-1)), "i" (STACK_SIZE-48) );
    return p;
}

Jun