Refactor stack growth to be helper functions in exception for easier merging

src/Makefile.build

@@ -65,7 +65,6 @@ userprog_SRC += userprog/tss.c		# TSS management.
 vm_SRC += vm/frame.c                    # Frame table manager.
 vm_SRC += vm/page.c                     # Page table manager.
 vm_SRC += devices/swap.c		# Swap block manager.
-vm_SRC += vm/stackgrowth.c
 #vm_SRC = vm/file.c			# Some other file.
 
 # Filesystem code.

src/userprog/exception.c

@@ -2,9 +2,15 @@
 #include <inttypes.h>
 #include <stdio.h>
 #include "userprog/gdt.h"
+#include "userprog/pagedir.h"
+#include "userprog/process.h"
 #include "threads/interrupt.h"
+#include "threads/palloc.h"
 #include "threads/thread.h"
-#include "vm/stackgrowth.h"
+#include "threads/vaddr.h"
+
+#define MAX_STACK_SIZE (8 * 1024 * 1024) // 8MB
+#define MAX_STACK_OFFSET 32 // 32 bytes offset below stack pointer (ESP)
 
 /* Number of page faults processed. */
 static long long page_fault_cnt;
@@ -12,6 +18,9 @@ static long long page_fault_cnt;
 static void kill (struct intr_frame *);
 static void page_fault (struct intr_frame *);
 
+static bool is_valid_stack_access (const void *fault_addr, const void *esp);
+static bool grow_stack (void *upage);
+
 /* Registers handlers for interrupts that can be caused by user
    programs.
 
@@ -146,20 +155,27 @@ page_fault (struct intr_frame *f)
   write = (f->error_code & PF_W) != 0;
   user = (f->error_code & PF_U) != 0;
 
-  if (user && not_present)
+  if (!user || !not_present)
   {
-  	  if (try_alloc_new_page (fault_addr, f->esp))
-        return;
-    }
-  else
-    {
-      if (try_alloc_new_page (fault_addr, thread_current ()->curr_esp))
-        return;
     f->eip = (void *)f->eax;
     f->eax = 0xffffffff;
     return;
   }
 
+  /* If the fault address is in a user page that is not present, then it might
+   be just that the stack needs to grow. So we attempt to grow the stack. */
+  void *upage = pg_round_down (fault_addr);
+  if (not_present && is_user_vaddr (upage) && upage != NULL)
+    {
+      if (is_valid_stack_access (fault_addr, f->esp))
+        {
+          if (grow_stack (upage))
+            return;
+        }
+
+      /* TODO: Check SPT for the page. */
+    }
+
   /* To implement virtual memory, delete the rest of the function
      body, and replace it with code that brings in the page to
      which fault_addr refers. */
@@ -171,3 +187,50 @@ page_fault (struct intr_frame *f)
   kill (f);
 }
 
+/* Validates whether the fault address is a valid stack access. Access is a
+   valid stack access under the following two conditions:
+     1. The fault address must be within MAX_STACK_OFFSET (32) bytes below
+      the current stack pointer. (Accounts for both PUSH and PUSHA instructions)
+     2. Growing this stack to this address does not cause it to exceed the
+      MAX_STACK_SIZE (8MB) limit.
+
+   Returns true if both conditions are met, false otherwise.
+
+   Pre: fault_addr is a valid user virtual address (so also not NULL). */
+static bool
+is_valid_stack_access (const void *fault_addr, const void *esp)
+{
+  uint32_t new_stack_size = PHYS_BASE - pg_round_down (fault_addr);
+
+  uint32_t *lowest_valid_push_addr = (uint32_t *)esp - MAX_STACK_OFFSET;
+  bool is_within_push_range = (uint32_t *)fault_addr >= lowest_valid_push_addr;
+
+  return is_within_push_range && new_stack_size <= MAX_STACK_SIZE;
+}
+
+/* Attempts to grow the stack by allocating and mapping a new page.
+  This involves:
+    1. Allocating a zeroed page from the user pool
+    2. Installing it into the page table with write permissions
+
+  Returns true if the stack was successfully grown, false if either
+  allocation or installation fails.
+
+  Pre: upage is a valid page-aligned address (so also not NULL). */
+static bool
+grow_stack (void *upage)
+{
+  /* Allocate new page for stack */
+  void *kpage = palloc_get_page (PAL_USER | PAL_ZERO);
+  if (kpage == NULL)
+    return false;
+
+  /* Install the page into user page table */
+  if (!install_page (upage, kpage, true))
+    {
+      palloc_free_page (kpage);
+      return false;
+    }
+
+  return true;
+}

src/userprog/process.c

@@ -118,7 +118,7 @@ process_execute (const char *cmd)
 
 static void *get_usr_kpage (enum palloc_flags flags, void *upage);
 static void free_usr_kpage (void *kpage);
-static bool install_page (void *upage, void *kpage, bool writable);
+bool install_page (void *upage, void *kpage, bool writable);
 
 static bool process_init_stack (char *cmd_saveptr, void **esp, char *file_name);
 static void *push_to_stack (void **esp, void *data, size_t data_size);
@@ -809,7 +809,7 @@ free_usr_kpage (void *kpage)
    with palloc_get_page().
    Returns true on success, false if UPAGE is already mapped or
    if memory allocation fails. */
-static bool
+bool
 install_page (void *upage, void *kpage, bool writable)
 {
   struct thread *t = thread_current ();

src/userprog/process.h

@@ -8,4 +8,6 @@ int process_wait (tid_t);
 void process_exit (void);
 void process_activate (void);
 
+bool install_page (void *upage, void *kpage, bool writable);
+
 #endif /* userprog/process.h */

src/vm/stackgrowth.c

@@ -1,50 +0,0 @@
-#include <stdio.h>
-#include "stackgrowth.h"
-#include "threads/palloc.h"
-#include "threads/thread.h"
-#include "threads/vaddr.h"
-#include "userprog/pagedir.h"
-
-#define MAX_STACK_ACCESS_DIST 32
-
-static bool needs_new_page (const void *addr, const void *esp);
-static bool grow_stack (const void *addr);
-
-bool
-try_alloc_new_page (const void *ptr, const void *esp)
-{
-  return needs_new_page (ptr, esp) && grow_stack (ptr);
-}
-
-/* Validates a given address for being a stack query and not a generic erroneous
-   address
- */
-static bool
-needs_new_page (const void *addr, const void *esp)
-{
-    return (is_user_vaddr (addr) &&
-            (uint32_t*)addr >= ((uint32_t*)esp - MAX_STACK_ACCESS_DIST) &&
-            ((PHYS_BASE - pg_round_down (addr))
-                          <= MAX_STACK_SIZE));
-}
-
-/* Extends the stack by the necessary number of pages */
-static bool
-grow_stack (const void *addr)
-{
-  struct thread *t = thread_current ();
-  void *last_page = pg_round_down (addr);
-
-  uint8_t *new_page = palloc_get_page (PAL_USER | PAL_ZERO);
-  if ( new_page == NULL)
-    return false;
-
-  bool added_page = pagedir_get_page (t->pagedir, last_page) == NULL
-                    && pagedir_set_page (t->pagedir, last_page, new_page, true);
-
-  if (!added_page) {
-    palloc_free_page (new_page);
-    return false;
-  }
-  return true;
-}

src/vm/stackgrowth.h

@@ -1,10 +0,0 @@
-#ifndef VM_GROWSTACK_H
-#define VM_GROWSTACK_H
-
-#include <stdio.h>
-
-#define MAX_STACK_SIZE 8388608 // (8MB)
-
-bool try_alloc_new_page (const void *ptr, const void *esp);
-
-#endif /* vm/frame.h */