Fixes bug in pop_ros_tf
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 16 Mar 2011 02:38:39 +0000 (19:38 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:00 +0000 (17:36 -0700)
Brutal bug, where branches would make the wrong decision and syscall
structs would get clobbered.  EFLAGS wasn't restored, so an unlucky
IPI/notification would result in bad CCs.  Note: subl and addl affect
the flags.

Also, don't use the vcore stack for things like syscall struct storage
after you enable notifs!  Once notifs are enabled, you should not be
touching the vcore stack.

kern/src/kthread.c
kern/src/process.c
tests/hello.c
user/parlib/include/i686/vcore.h
user/parlib/include/vcore.h
user/parlib/uthread.c
user/pthread/pthread.c

index bf70264..b23c9f5 100644 (file)
@@ -199,8 +199,9 @@ void __launch_kthread(struct trapframe *tf, uint32_t srcid, void *a0, void *a1,
                         * process another process's work. */
                        /* Keep in mind this can happen if you yield your core after
                         * submitting work (like sys_block()) that will complete on the
-                        * current core, and then someone else gets it.  TODO: might be
-                        * other cases. */
+                        * current core, and then someone else gets it.
+                        * TODO: This can also happen if we started running another _M
+                        * here... */
                        if (cur_proc->state != PROC_RUNNING_S) {
                                printk("cur_proc: %08p, kthread->proc: %08p\n", cur_proc,
                                       kthread->proc);
@@ -214,10 +215,15 @@ void __launch_kthread(struct trapframe *tf, uint32_t srcid, void *a0, void *a1,
                } else {
                        /* possible to get here if there is only one _S proc that blocked */
                        //assert(cur_proc->state == PROC_RUNNING_M);
-                       /* TODO: might need to do something here, though it will depend on
-                        * how we handle async local calls. */
+                       /* Our proc was current, but also wants an old kthread restarted.
+                        * This could happen if we are in the kernel servicing a call, and a
+                        * kthread tries to get restarted here on the way out.  cur_tf ought
+                        * to be the TF that needs to be restarted when the kernel is done
+                        * (regardless of whether or not we rerun kthreads). */
+                       assert(tf == current_tf);
+                       /* And just let the kthread restart, abandoning the call path via
+                        * proc_restartcore (or however we got here). */
                }
-
        }
        restart_kthread(kthread);
        assert(0);
index ea74210..d04607e 100644 (file)
@@ -1417,6 +1417,7 @@ void __notify(trapframe_t *tf, uint32_t srcid, void *a0, void *a1, void *a2)
        /* We shouldn't need to lock here, since unmapping happens on the pcore and
         * mapping would only happen if the vcore was free, which it isn't until
         * after we unmap. */
+       assert(tf == current_tf);
        vcoreid = get_vcoreid(p, core_id());
        vcpd = &p->procdata->vcore_preempt_data[vcoreid];
        printd("received active notification for proc %d's vcore %d on pcore %d\n",
index 7891ffb..32fb773 100644 (file)
@@ -1,8 +1,11 @@
 #include <stdlib.h>
 #include <stdio.h>
+#include <ros/syscall.h>
 
 int main(int argc, char** argv)
 {
        printf("Hello world from program %s!!\n", argv[0]);
+       ros_syscall(SYS_block, 0, 0, 0, 0, 0, 0);
+       printf("Done\n");
        return 0;
 }
index 5ccbbe7..47a5761 100644 (file)
@@ -26,65 +26,98 @@ extern __thread int __vcoreid;
  *
  * This is what the target notif_tf's stack will look like (growing down):
  *
- * Target ESP -> |   u_thread's old stuff   |
- *               |   new eip                |
- *               |   eax save space         |
- *               |   &sysc (async syscall)  |
- *               |   notif_pending_loc      |
- *               |   notif_enabled_loc      |
+ * Target ESP -> |   u_thread's old stuff   | the future %esp, tf->tf_esp
+ *               |   new eip                | 0x04 below %esp (one slot is 0x04)
+ *               |   eflags space           | 0x08 below
+ *               |   eax save space         | 0x0c below
+ *               |   actual syscall         | 0x10 below (0x30 space)
+ *               |   *sysc ptr to syscall   | 0x40 below (0x10 + 0x30)
+ *               |   notif_pending_loc      | 0x44 below (0x10 + 0x30)
+ *               |   notif_enabled_loc      | 0x48 below (0x10 + 0x30)
  *
  * The important thing is that it can handle a notification after it enables
  * notifications, and when it gets resumed it can ultimately run the new
  * context.  Enough state is saved in the running context and stack to continue
- * running. */
-#include <stdio.h>
+ * running.
+ *
+ * Related to that is whether or not our stack pointer is sufficiently far down
+ * so that restarting *this* code won't clobber shit we need later.  The way we
+ * do this is that we do any "stack jumping" after we enable interrupts/notifs.
+ * These jumps are when we directly modify esp, specifically in the down
+ * direction (subtracts).  Adds would be okay.
+ *
+ * Another related concern is the storage for sysc.  It used to be on the
+ * vcore's stack, but if an interrupt comes in before we use it, we trash the
+ * vcore's stack (and thus the storage for sysc!).  Instead, we put it on the
+ * stack of the user tf.  Moral: don't touch a vcore's stack with notifs
+ * enabled. */
+
+/* Helper for writing the info we need later to the u_tf's stack.  Note, this
+ * could get fucked if the struct syscall isn't a multiple of 4-bytes.  Also,
+ * note this goes backwards, since memory reads up the stack. */
+struct restart_helper {
+       uint32_t                                        notif_enab_loc;
+       uint32_t                                        notif_pend_loc;
+       struct syscall                          *sysc;
+       struct syscall                          local_sysc;
+       uint32_t                                        eax_save;
+       uint32_t                                        eflags;
+       uint32_t                                        eip;
+};
+
 static inline void pop_ros_tf(struct user_trapframe *tf, uint32_t vcoreid)
 {
-       struct syscall sysc = {0};
+       struct restart_helper *rst;
        struct preempt_data *vcpd = &__procdata.vcore_preempt_data[vcoreid];
-       /* need to prep the async sysc in case we need to notify ourselves */
-       sysc.num = SYS_self_notify;
-       sysc.arg0 = vcoreid;    /* arg1 and 2 already = 0 (null notif, no u_ne) */
        if (!tf->tf_cs) { /* sysenter TF.  esp and eip are in other regs. */
                tf->tf_esp = tf->tf_regs.reg_ebp;
                tf->tf_eip = tf->tf_regs.reg_edx;
        }
+       /* The stuff we need to write will be below the current stack of the utf */
+       rst = (struct restart_helper*)((void*)tf->tf_esp -
+                                      sizeof(struct restart_helper));
+       /* Fill in the info we'll need later */
+       rst->notif_enab_loc = (uint32_t)&vcpd->notif_enabled;
+       rst->notif_pend_loc = (uint32_t)&vcpd->notif_pending;
+       rst->sysc = &rst->local_sysc;   /* point to the local one */
+       memset(rst->sysc, 0, sizeof(struct syscall));
+       /* Need to prep the async sysc in case we need to notify ourselves */
+       rst->sysc->num = SYS_self_notify;
+       rst->sysc->arg0 = vcoreid;      /* arg 1 & 2 already = 0 (null notif, no u_ne)*/
+       rst->eax_save = 0;                      /* avoid bugs */
+       rst->eflags = tf->tf_eflags;
+       rst->eip = tf->tf_eip;
 
-       asm volatile ("movl %2,-0x04(%1);    " /* push the PC */
-                     "movl %3,-0x0c(%1);    " /* room for eax, push &sysc */
-                     "movl %4,-0x10(%1);    " /* push notif_pending loc */
-                     "movl %5,-0x14(%1);    " /* push notif_enabled loc */
-                     "movl %0,%%esp;        " /* pop the real tf */
+       asm volatile ("movl %0,%%esp;        " /* jump esp to the utf */
                      "popal;                " /* restore normal registers */
-                     "addl $0x20,%%esp;     " /* move to the eflags in the tf */
-                     "popfl;                " /* restore eflags */
-                     "popl %%esp;           " /* change to the new %esp */
-                     "subl $0x4,%%esp;      " /* move esp to the slot for eax */
+                     "addl $0x24,%%esp;     " /* move to the esp slot in the tf */
+                     "popl %%esp;           " /* change to the utf's %esp */
+                     "subl $0x08,%%esp;     " /* move esp to below eax's slot */
                      "pushl %%eax;          " /* save eax, will clobber soon */
-                     "subl $0xc,%%esp;      " /* move to notif_en_loc slot */
+                                 "movl %2,%%eax;        " /* sizeof struct syscall */
+                                 "addl $0x0c,%%eax;     " /* more offset btw eax/notif_en_loc*/
+                     "subl %%eax,%%esp;     " /* move to notif_en_loc slot */
                      "popl %%eax;           " /* load notif_enabaled addr */
                      "movb $0x01,(%%eax);   " /* enable notifications */
+                                 /* From here down, we can get interrupted and restarted */
                      "popl %%eax;           " /* get notif_pending status */
-                     "pushfl;               " /* save eflags */
                      "testb $0x01,(%%eax);  " /* test if a notif is pending */
                      "jz 1f;                " /* if not pending, skip syscall */
-                     "popfl;                " /* restore eflags */
                      "movb $0x00,(%%eax);   " /* clear pending */
                                  /* Actual syscall.  Note we don't wait on the async call */
                      "popl %%eax;           " /* &sysc, trap arg0 */
                      "pushl %%edx;          " /* save edx, will be trap arg1 */
                      "movl $0x1,%%edx;      " /* sending one async syscall: arg1 */
-                     "int %6;               " /* fire the syscall */
+                     "int %1;               " /* fire the syscall */
                      "popl %%edx;           " /* restore regs after syscall */
                      "jmp 2f;               " /* skip 1:, already popped */
-                     "1: popfl;             " /* restore eflags (on non-sc path) */
-                                 "popl %%eax;           " /* discard &sysc (on non-sc path) */
-                     "2: popl %%eax;        " /* restore tf's %eax (both paths) */
+                                 "1: popl %%eax;        " /* discard &sysc (on non-sc path) */
+                     "2: addl %2,%%esp;     " /* jump over the sysc (both paths) */
+                     "popl %%eax;           " /* restore tf's %eax */
+                                 "popfl;                " /* restore utf's eflags */
                      "ret;                  " /* return to the new PC */
                      :
-                     : "g"(tf), "r"(tf->tf_esp), "r"(tf->tf_eip), "r"(&sysc),
-                       "r"(&vcpd->notif_pending), "r"(&vcpd->notif_enabled),
-                       "i"(T_SYSCALL)
+                     : "g"(tf), "i"(T_SYSCALL), "i"(sizeof(struct syscall))
                      : "memory");
 }
 
index b28ebfd..4afad10 100644 (file)
@@ -43,6 +43,7 @@ static inline int vcore_id(void);
 static inline bool in_vcore_context(void);
 static inline void __enable_notifs(uint32_t vcoreid);
 static inline void disable_notifs(uint32_t vcoreid);
+static inline bool notif_is_enabled(uint32_t vcoreid);
 int vcore_init(void);
 int vcore_request(size_t k);
 void vcore_yield(void);
@@ -82,6 +83,11 @@ static inline void disable_notifs(uint32_t vcoreid)
        __procdata.vcore_preempt_data[vcoreid].notif_enabled = FALSE;
 }
 
+static inline bool notif_is_enabled(uint32_t vcoreid)
+{
+       return __procdata.vcore_preempt_data[vcoreid].notif_enabled;
+}
+
 #ifdef __cplusplus
 }
 #endif
index 2a8f992..a255e96 100644 (file)
@@ -58,10 +58,8 @@ void __attribute__((noreturn)) uthread_vcore_entry(void)
 {
        uint32_t vcoreid = vcore_id();
 
-       struct preempt_data *vcpd = &__procdata.vcore_preempt_data[vcoreid];
-
        /* Should always have notifications disabled when coming in here. */
-       assert(vcpd->notif_enabled == FALSE);
+       assert(!notif_is_enabled(vcoreid));
        assert(in_vcore_context());
 
        check_preempt_pending(vcoreid);
@@ -129,8 +127,7 @@ __uthread_yield(void)
 {
        struct uthread *uthread = current_uthread;
        assert(in_vcore_context());
-       assert(uthread->state == UT_RUNNING);
-       assert(uthread == current_uthread);
+       assert(!notif_is_enabled(vcore_id()));
        /* Do slightly different things depending on whether or not we're exiting.
         * While it is tempting to get rid of the UTHREAD_DYING flag and have
         * callers just set the thread state, we'd lose the ability to assert
@@ -177,6 +174,7 @@ void uthread_yield(void)
        // if (!(uthread->flags & UTHREAD_DYING))
        //      save_fp_state(&t->as);
        assert(!in_vcore_context());
+       assert(uthread->state == UT_RUNNING);
        /* Don't migrate this thread to another vcore, since it depends on being on
         * the same vcore throughout (once it disables notifs).  The race is that we
         * read vcoreid, then get interrupted / migrated before disabling notifs. */
@@ -225,7 +223,6 @@ yield_return_path:
 void uthread_exit(void)
 {
        current_uthread->flags |= UTHREAD_DYING;
-       assert(!in_vcore_context());    /* try and catch the yield bug */
        uthread_yield();
 }
 
@@ -248,7 +245,6 @@ void ros_syscall_blockon(struct syscall *sysc)
                return;
        /* So yield knows we are blocking on something */
        current_uthread->sysc = sysc;
-       assert(!in_vcore_context());    /* try and catch the yield bug */
        uthread_yield();
 }
 
@@ -259,7 +255,8 @@ void run_current_uthread(void)
        struct preempt_data *vcpd = &__procdata.vcore_preempt_data[vcoreid];
        assert(current_uthread);
        assert(current_uthread->state == UT_RUNNING);
-       printd("[U] Vcore %d is restarting uthread %d\n", vcoreid, uthread->id);
+       printd("[U] Vcore %d is restarting uthread %08p\n", vcoreid,
+              current_uthread);
        clear_notif_pending(vcoreid);
        set_tls_desc(current_uthread->tls_desc, vcoreid);
        /* Pop the user trap frame */
index 16c5553..d181832 100644 (file)
@@ -115,20 +115,27 @@ void __attribute__((noreturn)) pth_sched_entry(void)
        /* no one currently running, so lets get someone from the ready queue */
        struct pthread_tcb *new_thread = NULL;
        struct mcs_lock_qnode local_qn = {0};
-       mcs_lock_notifsafe(&queue_lock, &local_qn);
-       new_thread = TAILQ_FIRST(&ready_queue);
-       if (new_thread) {
-               TAILQ_REMOVE(&ready_queue, new_thread, next);
-               TAILQ_INSERT_TAIL(&active_queue, new_thread, next);
-               threads_active++;
-               threads_ready--;
+       /* For now, let's spin and handle events til we get a thread to run.  This
+        * will help catch races, instead of only having one core ever run a thread
+        * (if there is just one, etc).  Also, we don't need the EVENT_IPIs for this
+        * to work (since we poll handle_events() */
+       while (!new_thread) {
+               handle_events(vcoreid);
+               mcs_lock_notifsafe(&queue_lock, &local_qn);
+               new_thread = TAILQ_FIRST(&ready_queue);
+               if (new_thread) {
+                       TAILQ_REMOVE(&ready_queue, new_thread, next);
+                       TAILQ_INSERT_TAIL(&active_queue, new_thread, next);
+                       threads_active++;
+                       threads_ready--;
+               }
+               mcs_unlock_notifsafe(&queue_lock, &local_qn);
        }
-       mcs_unlock_notifsafe(&queue_lock, &local_qn);
        /* Instead of yielding, you could spin, turn off the core, set an alarm,
         * whatever.  You want some logic to decide this.  Uthread code wil have
         * helpers for this (like how we provide run_uthread()) */
        if (!new_thread) {
-               /* TODO: consider doing something more intelligent here */
+               /* Note, we currently don't get here (due to the while loop) */
                printd("[P] No threads, vcore %d is yielding\n", vcore_id());
                /* Not actually yielding - just spin for now, so we can get syscall
                 * unblocking events */
@@ -383,7 +390,6 @@ int pthread_join(pthread_t thread, void** retval)
 
 int pthread_yield(void)
 {
-       assert(!in_vcore_context());    /* try and catch the yield bug */
        uthread_yield();
        return 0;
 }