Properly align vcore stacks on x86
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 24 Mar 2016 16:39:05 +0000 (12:39 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 31 Mar 2016 20:53:42 +0000 (16:53 -0400)
Glibc _start expects a 16-byte aligned stack, since it is working in asm.
However, the vcore stacks were also getting a 16-byte aligned stack, but
they were C functions and needed to be odd-8-byte aligned.

This commit creates arch-dependent vcore entry functions.  x86's is in
assembly, where it can restore the odd-8-byte invariant.

Note that we can't have both vcore.S and vcore.c - both will be built as
vcore.o.  I renamed the asm ones with _asm.

Incidentally, this happened on a printf with floats/xmms in vcore context,
and was triggered by the "movaps xmm,rbp" due to the variadic function ABI.
It also shows that we weren't touching xmm's in vcore context, since we
would have GP faulted.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/arch/x86/process64.c
user/parlib/include/parlib/vcore.h
user/parlib/riscv/vcore.S [deleted file]
user/parlib/riscv/vcore.c [new file with mode: 0644]
user/parlib/riscv/vcore_asm.S [new file with mode: 0644]
user/parlib/vcore.c
user/parlib/x86/vcore.c
user/parlib/x86/vcore_asm.S [new file with mode: 0644]

index 7de0db4..e141873 100644 (file)
@@ -223,16 +223,23 @@ void proc_init_ctx(struct user_context *ctx, uint32_t vcoreid, uintptr_t entryp,
        /* zero the entire structure for any type, prevent potential disclosure */
        memset(ctx, 0, sizeof(struct user_context));
        ctx->type = ROS_SW_CTX;
-       /* Stack pointers in a fresh stack frame need to be 16 byte aligned
-        * (AMD64 ABI). If we call this function from within load_elf(), it
-        * should already be aligned properly, but we round again here for good
-        * measure. We used to subtract an extra 8 bytes here to allow us to
-        * write our _start() function in C instead of assembly. This was
-        * necessary to account for a preamble inserted the compiler which
-        * assumed a return address was pushed on the stack. Now that we properly
-        * pass our arguments on the stack, we will have to rewrite our _start()
-        * function in assembly to handle things properly. */
-       sw_tf->tf_rsp = ROUNDDOWN(stack_top, 16);
+       /* Stack pointers in x86 C functions need to be such that adding or
+        * subtracting 8 will result in 16 byte alignment (AMD64 ABI), which we call
+        * an odd-8-byte alignment.  The reason is so that input arguments (on the
+        * stack) are 16 byte aligned.  The extra 8 bytes is the retaddr, pushed on
+        * the stack.  Compilers know they can subtract 8 to get 16 byte alignment
+        * for instructions like movaps.
+        *
+        * However, the kernel will start contexts at 16 byte aligned stacks.  This
+        * is because glibc's _start (in ASM) expects this.  Parlib x86's vcore
+        * entry does the same.
+        *
+        * We init contexts for both an elf startup as well as vcore entry.  It is
+        * up to the caller (including the user) to make sure the stack is aligned
+        * properly.  elf.c doesn't know about these concerns, so if it messes up,
+        * there's nothing we can really do, since the args are just wrong.  ld will
+        * fail immediately though, so we'll find out quickly. */
+       sw_tf->tf_rsp = stack_top;
        sw_tf->tf_rip = entryp;
        sw_tf->tf_rbp = 0;      /* for potential backtraces */
        sw_tf->tf_mxcsr = 0x00001f80;   /* x86 default mxcsr */
index 02bbab5..7c86936 100644 (file)
@@ -30,6 +30,9 @@ extern __thread bool __vcore_context;
 extern __thread int __vcoreid;
 extern __thread struct syscall __vcore_one_sysc;       /* see sys_change_vcore */
 
+/* Arch specific entry from the kernel */
+void __attribute__((noreturn)) __kernel_vcore_entry(void);
+
 /* Vcore API functions */
 static inline uint32_t max_vcores(void);
 static inline uint32_t num_vcores(void);
diff --git a/user/parlib/riscv/vcore.S b/user/parlib/riscv/vcore.S
deleted file mode 100644 (file)
index 3a2daeb..0000000
+++ /dev/null
@@ -1,43 +0,0 @@
-#include <sys/asm.h>
-
-.global __pop_ros_tf_regs
-__pop_ros_tf_regs:
-
-  REG_L s0,  2*SZREG(a0)
-  REG_L s1,  3*SZREG(a0)
-  REG_L s2,  4*SZREG(a0)
-  REG_L s3,  5*SZREG(a0)
-  REG_L s4,  6*SZREG(a0)
-  REG_L s5,  7*SZREG(a0)
-  REG_L s6,  8*SZREG(a0)
-  REG_L s7,  9*SZREG(a0)
-  REG_L s8, 10*SZREG(a0)
-  REG_L s9, 11*SZREG(a0)
-  REG_L s10,12*SZREG(a0)
-  REG_L s11,13*SZREG(a0)
-  REG_L sp, 14*SZREG(a0)
-
-  REG_L ra,33*SZREG(a0)
-
-  jr    a3
-
-.global __save_ros_tf_regs
-__save_ros_tf_regs:
-
-  REG_S s0,  2*SZREG(a0)
-  REG_S s1,  3*SZREG(a0)
-  REG_S s2,  4*SZREG(a0)
-  REG_S s3,  5*SZREG(a0)
-  REG_S s4,  6*SZREG(a0)
-  REG_S s5,  7*SZREG(a0)
-  REG_S s6,  8*SZREG(a0)
-  REG_S s7,  9*SZREG(a0)
-  REG_S s8, 10*SZREG(a0)
-  REG_S s9, 11*SZREG(a0)
-  REG_S s10,12*SZREG(a0)
-  REG_S s11,13*SZREG(a0)
-  REG_S sp, 14*SZREG(a0)
-
-  REG_S ra,33*SZREG(a0)
-
-  ret
diff --git a/user/parlib/riscv/vcore.c b/user/parlib/riscv/vcore.c
new file mode 100644 (file)
index 0000000..c08130d
--- /dev/null
@@ -0,0 +1,25 @@
+/* TODO: implement me */
+void __attribute__((noreturn)) __kernel_vcore_entry(void)
+{
+       /* The kernel sets the TLS desc for us, based on whatever is in VCPD.
+        *
+        * x86 32-bit TLS is pretty jacked up, so the kernel doesn't set the TLS
+        * desc for us.  it's a little more expensive to do it here, esp for
+        * amd64.  Can remove this when/if we overhaul 32 bit TLS.
+        *
+        * AFAIK, riscv's TLS changes are really cheap, and they don't do it in
+        * the kernel (yet/ever), so they can set their TLS here too. */
+       int id = __vcore_id_on_entry;
+       #ifndef __x86_64__
+       set_tls_desc(vcpd_of(id)->vcore_tls_desc);
+       #endif
+       /* Every time the vcore comes up, it must set that it is in vcore context.
+        * uthreads may share the same TLS as their vcore (when uthreads do not have
+        * their own TLS), and if a uthread was preempted, __vcore_context == FALSE,
+        * and that will continue to be true the next time the vcore pops up. */
+       __vcore_context = TRUE;
+       vcore_entry();
+       fprintf(stderr, "vcore_entry() should never return!\n");
+       abort();
+       __builtin_unreachable();
+}
diff --git a/user/parlib/riscv/vcore_asm.S b/user/parlib/riscv/vcore_asm.S
new file mode 100644 (file)
index 0000000..3a2daeb
--- /dev/null
@@ -0,0 +1,43 @@
+#include <sys/asm.h>
+
+.global __pop_ros_tf_regs
+__pop_ros_tf_regs:
+
+  REG_L s0,  2*SZREG(a0)
+  REG_L s1,  3*SZREG(a0)
+  REG_L s2,  4*SZREG(a0)
+  REG_L s3,  5*SZREG(a0)
+  REG_L s4,  6*SZREG(a0)
+  REG_L s5,  7*SZREG(a0)
+  REG_L s6,  8*SZREG(a0)
+  REG_L s7,  9*SZREG(a0)
+  REG_L s8, 10*SZREG(a0)
+  REG_L s9, 11*SZREG(a0)
+  REG_L s10,12*SZREG(a0)
+  REG_L s11,13*SZREG(a0)
+  REG_L sp, 14*SZREG(a0)
+
+  REG_L ra,33*SZREG(a0)
+
+  jr    a3
+
+.global __save_ros_tf_regs
+__save_ros_tf_regs:
+
+  REG_S s0,  2*SZREG(a0)
+  REG_S s1,  3*SZREG(a0)
+  REG_S s2,  4*SZREG(a0)
+  REG_S s3,  5*SZREG(a0)
+  REG_S s4,  6*SZREG(a0)
+  REG_S s5,  7*SZREG(a0)
+  REG_S s6,  8*SZREG(a0)
+  REG_S s7,  9*SZREG(a0)
+  REG_S s8, 10*SZREG(a0)
+  REG_S s9, 11*SZREG(a0)
+  REG_S s10,12*SZREG(a0)
+  REG_S s11,13*SZREG(a0)
+  REG_S sp, 14*SZREG(a0)
+
+  REG_S ra,33*SZREG(a0)
+
+  ret
index b1f8aa1..a31ae33 100644 (file)
@@ -39,37 +39,6 @@ void __attribute__((noreturn)) __vcore_entry(void)
 }
 void vcore_entry(void) __attribute__((weak, alias ("__vcore_entry")));
 
-/* The lowest level function jumped to by the kernel on every vcore_entry.
- * Currently, this function is only necessary so we can set the tls_desc from
- * the vcpd for non x86_64 architectures. We should consider removing this and
- * making it mandatory to set the tls_desc in the kernel. We wouldn't even
- * need to pass the vcore id to user space at all if we did this.  It would
- * already be set in the preinstalled TLS as __vcore_id. */
-static void __attribute__((noreturn)) __kernel_vcore_entry(void)
-{
-       /* The kernel sets the TLS desc for us, based on whatever is in VCPD.
-        *
-        * x86 32-bit TLS is pretty jacked up, so the kernel doesn't set the TLS
-        * desc for us.  it's a little more expensive to do it here, esp for
-        * amd64.  Can remove this when/if we overhaul 32 bit TLS.
-        *
-        * AFAIK, riscv's TLS changes are really cheap, and they don't do it in
-        * the kernel (yet/ever), so they can set their TLS here too. */
-       int id = __vcore_id_on_entry;
-       #ifndef __x86_64__
-       set_tls_desc(vcpd_of(id)->vcore_tls_desc);
-       #endif
-       /* Every time the vcore comes up, it must set that it is in vcore context.
-        * uthreads may share the same TLS as their vcore (when uthreads do not have
-        * their own TLS), and if a uthread was preempted, __vcore_context == FALSE,
-        * and that will continue to be true the next time the vcore pops up. */
-       __vcore_context = TRUE;
-       vcore_entry();
-       fprintf(stderr, "vcore_entry() should never return!\n");
-       abort();
-       __builtin_unreachable();
-}
-
 /* TODO: probably don't want to dealloc.  Considering caching */
 static void free_transition_tls(int id)
 {
index 05dec14..7cd9d60 100644 (file)
@@ -1,6 +1,7 @@
 #include <ros/syscall.h>
-#include <parlib/arch/vcore.h>
+#include <parlib/vcore.h>
 #include <parlib/stdio.h>
+#include <stdlib.h>
 
 struct syscall vc_entry = {
        .num = SYS_vc_entry,
@@ -113,3 +114,32 @@ void print_user_context(struct user_context *ctx)
                printf("Unknown context type %d\n", ctx->type);
        }
 }
+
+/* The second-lowest level function jumped to by the kernel on every vcore
+ * entry.  We get called from __kernel_vcore_entry.
+ *
+ * We should consider making it mandatory to set the tls_desc in the kernel. We
+ * wouldn't even need to pass the vcore id to user space at all if we did this.
+ * It would already be set in the preinstalled TLS as __vcore_id. */
+void __attribute__((noreturn)) __kvc_entry_c(void)
+{
+       /* The kernel sets the TLS desc for us, based on whatever is in VCPD.
+        *
+        * x86 32-bit TLS is pretty jacked up, so the kernel doesn't set the TLS
+        * desc for us.  it's a little more expensive to do it here, esp for
+        * amd64.  Can remove this when/if we overhaul 32 bit TLS. */
+       int id = __vcore_id_on_entry;
+
+       #ifndef __x86_64__
+       set_tls_desc(vcpd_of(id)->vcore_tls_desc);
+       #endif
+       /* Every time the vcore comes up, it must set that it is in vcore context.
+        * uthreads may share the same TLS as their vcore (when uthreads do not have
+        * their own TLS), and if a uthread was preempted, __vcore_context == FALSE,
+        * and that will continue to be true the next time the vcore pops up. */
+       __vcore_context = TRUE;
+       vcore_entry();
+       fprintf(stderr, "vcore_entry() should never return!\n");
+       abort();
+       __builtin_unreachable();
+}
diff --git a/user/parlib/x86/vcore_asm.S b/user/parlib/x86/vcore_asm.S
new file mode 100644 (file)
index 0000000..9eb654b
--- /dev/null
@@ -0,0 +1,14 @@
+/* Copyright (c) 2016 Google Inc.
+ * Barret Rhoden <brho@cs.berkeley.edu>
+ * See LICENSE for details.
+ *
+ * Vcore entry point from the kernel.  We need this in asm to fix our stack
+ * pointer offset.  The kernel will start us with a 16-byte aligned stack.  C
+ * code expects to be odd-8-byte aligned.  The 'call' will push a retaddr on the
+ * stack, which is the 8 bytes we need. */
+
+.globl __kernel_vcore_entry
+__kernel_vcore_entry:
+       call __kvc_entry_c
+1:
+       jb 1