set_current_tf() no longer sets the local *tf var
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 28 Sep 2011 00:44:24 +0000 (17:44 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:07 +0000 (17:36 -0700)
Meaning that we only copy out the *tf and set cur_tf, but do not change
tf in the calling function.  The contents are the same, but if something
happens that changes cur_tf's contents, you're still using the same one
you came in on (this will matter for a future commit).

The old style had a bug too.  If a kthread migrated, when it returned up
its stack and it looked at *tf, it would point to the cur_tf of possibly
another core (which is not what we want).  We want to point to the
original TF we came in on, so we can decide if we should return to the
kernel or not (and if we do, we just pop that TF).

Sparc and RISCV probably need a little work.  I put warns in there to
catch IRQs being enabled.  set_current_tf() probably needs to be called
more often too (like when handling other traps).

kern/arch/i686/trap.c
kern/arch/riscv/trap.c
kern/arch/sparc/trap.c

index 810ab8c..94e77ca 100644 (file)
@@ -297,14 +297,14 @@ env_pop_ancillary_state(env_t* e)
        // Here's where you'll restore FP/MMX/XMM regs
 }
 
-/* Helper.  For now, this copies out the TF to pcpui, and sets the tf to use it.
- * Eventually, we ought to do this in trapentry.S */
-static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe **tf)
+/* Helper.  For now, this copies out the TF to pcpui.  Eventually, we should
+ * consider doing this in trapentry.S */
+static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe *tf)
 {
        assert(!irq_is_enabled());
-       pcpui->actual_tf = **tf;
+       assert(!pcpui->cur_tf);
+       pcpui->actual_tf = *tf;
        pcpui->cur_tf = &pcpui->actual_tf;
-       *tf = &pcpui->actual_tf;
 }
 
 /* If the interrupt interrupted a halt, we advance past it.  Made to work with
@@ -325,7 +325,7 @@ void trap(struct trapframe *tf)
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        /* Copy out the TF for now, set tf to point to it.  */
        if (!in_kernel(tf))
-               set_current_tf(pcpui, &tf);
+               set_current_tf(pcpui, tf);
 
        printd("Incoming TRAP %d on core %d, TF at %p\n", tf->tf_trapno, core_id(),
               tf);
@@ -349,7 +349,7 @@ void irq_handler(struct trapframe *tf)
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        /* Copy out the TF for now, set tf to point to it. */
        if (!in_kernel(tf))
-               set_current_tf(pcpui, &tf);
+               set_current_tf(pcpui, tf);
        /* Coupled with cpu_halt() and smp_idle() */
        abort_halt(tf);
        //if (core_id())
@@ -436,7 +436,7 @@ void sysenter_callwrapper(struct trapframe *tf)
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        /* Copy out the TF for now, set tf to point to it. */
        if (!in_kernel(tf))
-               set_current_tf(pcpui, &tf);
+               set_current_tf(pcpui, tf);
        /* Once we've set_current_tf, we can enable interrupts */
        enable_irq();
 
index 2b66953..1caf969 100644 (file)
@@ -100,14 +100,15 @@ sysenter_init(void)
 {
 }
 
-/* Helper.  For now, this copies out the TF to pcpui, and sets the tf to use it.
- * Eventually, we ought to do this in trap_entry.S.  Honestly, do whatever you
- * want with this.  The **tf is for convenience in x86. */
-static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe **tf)
+/* Helper.  For now, this copies out the TF to pcpui, and sets cur_tf to point
+ * to it. */
+static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe *tf)
 {
-       pcpui->actual_tf = **tf;
+       if (irq_is_enabled())
+               warn("Turn off IRQs until cur_tf is set!");
+       assert(!pcpui->cur_tf);
+       pcpui->actual_tf = *tf;
        pcpui->cur_tf = &pcpui->actual_tf;
-       *tf = &pcpui->actual_tf;
 }
 
 static int
@@ -174,7 +175,7 @@ handle_ipi(trapframe_t* tf)
 
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        if (!in_kernel(tf))
-               set_current_tf(pcpui, &tf);
+               set_current_tf(pcpui, tf);
        else if((void*)tf->epc == &cpu_halt) // break out of the cpu_halt loop
                advance_pc(tf);
 
@@ -395,7 +396,7 @@ handle_syscall(trapframe_t* state)
        enable_irq();
        struct per_cpu_info* coreinfo = &per_cpu_info[core_id()];
 
-       set_current_tf(pcpui, &state);
+       set_current_tf(pcpui, state);
 
        prep_syscalls(current, (struct syscall*)a0, a1);
 
index 03d9721..aae53da 100644 (file)
@@ -122,14 +122,15 @@ sysenter_init(void)
 {
 }
 
-/* Helper.  For now, this copies out the TF to pcpui, and sets the tf to use it.
- * Eventually, we ought to do this in trap_entry.S.  Honestly, do whatever you
- * want with this.  The **tf is for convenience in x86. */
-static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe **tf)
-{
-       pcpui->actual_tf = **tf;
+/* Helper.  For now, this copies out the TF to pcpui, and sets cur_tf to point
+ * to it. */
+static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe *tf)
+{
+       if (irq_is_enabled())
+               warn("Turn off IRQs until cur_tf is set!");
+       assert(!pcpui->cur_tf);
+       pcpui->actual_tf = *tf;
        pcpui->cur_tf = &pcpui->actual_tf;
-       *tf = &pcpui->actual_tf;
 }
 
 static int
@@ -235,7 +236,7 @@ void handle_ipi(trapframe_t* tf)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        if (!in_kernel(tf))
-               set_current_tf(pcpui, &tf);
+               set_current_tf(pcpui, tf);
        else if((void*)tf->pc == &__cpu_halt) // break out of the __cpu_halt loop
                advance_pc(tf);
 
@@ -443,7 +444,7 @@ void
 handle_pop_tf(trapframe_t* state)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
-       set_current_tf(pcpui, &state);
+       set_current_tf(pcpui, state);
 
        trapframe_t tf, *tf_p = &tf;
        if (memcpy_from_user(current,&tf,(void*)state->gpr[8],sizeof(tf))) {
@@ -453,7 +454,7 @@ handle_pop_tf(trapframe_t* state)
        }
 
        proc_secure_trapframe(&tf);
-       set_current_tf(pcpui, &tf_p);
+       set_current_tf(pcpui, tf_p);
        proc_restartcore();
 }
 
@@ -479,7 +480,7 @@ handle_syscall(trapframe_t* state)
        enable_irq();
        struct per_cpu_info* coreinfo = &per_cpu_info[core_id()];
 
-       set_current_tf(pcpui, &state);
+       set_current_tf(pcpui, state);
 
        prep_syscalls(current, (struct syscall*)a0, a1);