Address space creation/deletion implemented

- Proper releasing of user pmd and pgds when a space is not used.
- Proper releasing of task, space ids.

- At occasions a starting thread gets bogus SPSR, this needs investigating.
- At a very rare occasion arch_setup_new_thread() had a kernel data abort during
  register copying from one task to another. Needs investigating.

include/l4/arch/arm/v5/mm.h

@@ -138,6 +138,7 @@ void add_section_mapping_init(unsigned int paddr, unsigned int vaddr,
 			      unsigned int size, unsigned int flags);
 
 struct address_space;
+int delete_page_tables(struct address_space *space);
 int copy_user_tables(struct address_space *new, struct address_space *orig);
 pgd_table_t *copy_page_tables(pgd_table_t *from);
 void remap_as_pages(void *vstart, void *vend);

include/l4/generic/tcb.h

@@ -56,8 +56,8 @@ struct ktcb {
 
 	/* Thread information */
 	l4id_t tid;		/* Global thread id */
-	l4id_t spid;		/* Global space id */
 	l4id_t tgid;		/* Global thread group id */
+	/* See space for space id */
 
 	/* Flags to indicate various task status */
 	unsigned int flags;
@@ -120,7 +120,6 @@ union ktcb_union {
 static inline void set_task_ids(struct ktcb *task, struct task_ids *ids)
 {
 	task->tid = ids->tid;
-	task->spid = ids->spid;
 	task->tgid = ids->tgid;
 }
 
@@ -134,6 +133,7 @@ extern struct id_pool *space_id_pool;
 extern struct id_pool *tgroup_id_pool;
 
 struct ktcb *tcb_find(l4id_t tid);
+struct ktcb *tcb_find_by_space(l4id_t tid);
 void tcb_add(struct ktcb *tcb);
 void tcb_remove(struct ktcb *tcb);
 

src/api/syscall.c

@@ -167,7 +167,7 @@ int sys_getid(syscall_context_t *regs)
 	struct ktcb *this = current;
 
 	ids->tid = this->tid;
-	ids->spid = this->spid;
+	ids->spid = this->space->spid;
 	ids->tgid = this->tgid;
 
 	return 0;

src/api/thread.c

@@ -172,7 +172,7 @@ int arch_setup_new_thread(struct ktcb *new, struct ktcb *orig,
 	 * We don't lock for context modification because the
 	 * thread is not known to the system yet.
 	 */
-	new->context.spsr = orig->syscall_regs->spsr; /* User mode */
+	BUG_ON(!(new->context.spsr = orig->syscall_regs->spsr)); /* User mode */
 	new->context.r0 = orig->syscall_regs->r0;
 	new->context.r1 = orig->syscall_regs->r1;
 	new->context.r2 = orig->syscall_regs->r2;
@@ -213,32 +213,20 @@ int thread_setup_new_ids(struct task_ids *ids, unsigned int flags,
 
 	/*
 	 * If thread space is new or copied,
-	 * allocate a new space id and tgid
+	 * thread gets same group id as its thread id
 	 */
 	if (flags == THREAD_NEW_SPACE ||
-	    flags == THREAD_COPY_SPACE) {
+	    flags == THREAD_COPY_SPACE)
-		/*
-		 * Allocate requested id if
-		 * it's available, else a new one
-		 */
-		if ((ids->spid = id_get(space_id_pool,
-					ids->spid)) < 0)
-			ids->spid = id_new(space_id_pool);
-
-		/* Thread gets same group id as its thread id */
 		ids->tgid = ids->tid;
-	}
 
-	if (flags == THREAD_SAME_SPACE) {
+	/*
-		/*
+	 * If the tgid of original thread is supplied, that implies the
-		 * If the tgid of original thread is supplied, that
+	 * new thread wants to be in the same group, and we leave it as
-		 * implies the new thread wants to be in the same group,
+	 * it is. Otherwise the thread gets the same group id as its
-		 * and we leave it as it is. Otherwise the thread gets
+	 * unique thread id.
-		 * the same group id as its unique thread id.
+	 */
-		 */
+	if (flags == THREAD_SAME_SPACE && ids->tgid != orig->tgid)
-		if (ids->tgid != orig->tgid)
+		ids->tgid = ids->tid;
-			ids->tgid = ids->tid;
-	}
 
 	/* Set all ids */
 	set_task_ids(new, ids);
@@ -249,31 +237,43 @@ int thread_setup_new_ids(struct task_ids *ids, unsigned int flags,
 int thread_setup_space(struct ktcb *tcb, struct task_ids *ids, unsigned int flags)
 {
 	struct address_space *space, *new;
+	int ret = 0;
 
 	address_space_reference_lock();
 
 	if (flags == THREAD_SAME_SPACE) {
-		if (!(space = address_space_find(ids->spid)))
+		if (!(space = address_space_find(ids->spid))) {
-			return -ESRCH;
+			ret = -ESRCH;
+			goto out;
+		}
 		address_space_attach(tcb, space);
 	}
 	if (flags == THREAD_COPY_SPACE) {
-		if (!(space = address_space_find(ids->spid)))
+		if (!(space = address_space_find(ids->spid))) {
-			return -ESRCH;
+			ret = -ESRCH;
-		if (IS_ERR(new = address_space_create(space)))
+			goto out;
-			return (int)new;
+		}
+		if (IS_ERR(new = address_space_create(space))) {
+			ret = (int)new;
+			goto out;
+		}
+		ids->spid = new->spid;	/* Returned back to caller */
 		address_space_attach(tcb, new);
 		address_space_add(new);
 	}
 	if (flags == THREAD_NEW_SPACE) {
-		if (IS_ERR(new = address_space_create(0)))
+		if (IS_ERR(new = address_space_create(0))) {
-			return (int)new;
+			ret = (int)new;
+			goto out;
+		}
+		ids->spid = new->spid;	/* Returned back to caller */
 		address_space_attach(tcb, new);
 		address_space_add(new);
 	}
 
+out:
 	address_space_reference_unlock();
-	return 0;
+	return ret;
 }
 
 int thread_create(struct task_ids *ids, unsigned int flags)
@@ -286,22 +286,20 @@ int thread_create(struct task_ids *ids, unsigned int flags)
 	if (!(new = tcb_alloc_init()))
 		return -ENOMEM;
 
-	if ((err = thread_setup_space(new, ids, flags))) {
+	if (flags != THREAD_NEW_SPACE) {
-		tcb_delete(new);
+		BUG_ON(!(orig_task = tcb_find_by_space(ids->spid)));
+	} else
+		orig_task = 0;
+
+	if ((err = thread_setup_space(new, ids, flags)) < 0) {
+		/* Since it hasn't initialised maturely, we delete it this way */
+		free_page(new);	
 		return err;
 	}
 
-	if (flags != THREAD_NEW_SPACE) {
+	/* Set up ids and context using original tcb or from scratch */
-		BUG_ON(!(orig_task = tcb_find(ids->tid)));
+	thread_setup_new_ids(ids, flags, new, (orig_task) ? orig_task : 0);
-
+	arch_setup_new_thread(new, (orig_task) ? orig_task : 0, flags);
-		/* Set up ids and context using original tcb */
-		thread_setup_new_ids(ids, flags, new, orig_task);
-		arch_setup_new_thread(new, orig_task, flags);
-	} else {
-		/* Set up ids and context from scratch */
-		thread_setup_new_ids(ids, flags, new, 0);
-		arch_setup_new_thread(new, 0, flags);
-	}
 
 	tcb_add(new);
 

src/arch/arm/v5/mm.c

@@ -419,6 +419,12 @@ int remove_mapping(unsigned long vaddr)
 	return remove_mapping_pgd(vaddr, TASK_PGD(current));
 }
 
+int delete_page_tables(struct address_space *space)
+{
+	remove_mapping_pgd_all_user(space->pgd);
+	free_pgd(space->pgd);
+	return 0;
+}
 
 /*
  * Copies userspace entries of one task to another. In order to do that,

src/generic/space.c

@@ -6,12 +6,14 @@
 #include INC_GLUE(memory.h)
 #include INC_GLUE(memlayout.h)
 #include INC_ARCH(exception.h)
+#include INC_SUBARCH(mm.h)
 #include <l4/generic/space.h>
 #include <l4/generic/tcb.h>
 #include <l4/generic/kmalloc.h>
 #include <l4/api/space.h>
 #include <l4/api/errno.h>
 #include <l4/api/kip.h>
+#include <l4/lib/idpool.h>
 
 struct address_space_list {
 	struct list_head list;
@@ -69,7 +71,9 @@ struct address_space *address_space_find(l4id_t spid)
 void address_space_add(struct address_space *space)
 {
 	spin_lock(&address_space_list.list_lock);
+	BUG_ON(!list_empty(&space->list));
 	list_add(&space->list, &address_space_list.list);
+	BUG_ON(!++address_space_list.count);
 	spin_unlock(&address_space_list.list_lock);
 }
 
@@ -77,23 +81,24 @@ void address_space_remove(struct address_space *space)
 {
 	spin_lock(&address_space_list.list_lock);
 	BUG_ON(list_empty(&space->list));
+	BUG_ON(--address_space_list.count < 0);
 	list_del_init(&space->list);
 	spin_unlock(&address_space_list.list_lock);
 }
 
+/* Assumes address space reflock is already held */
 void address_space_delete(struct address_space *space)
 {
-	/* Address space refcount lock must be held */
+	BUG_ON(space->ktcb_refs);
-
-	/* Sanity checks ??? */
 
 	/* Traverse the page tables and delete private pmds */
+	delete_page_tables(space);
 
-	/* Delete the top-level pgd */
+	/* Return the space id */
-
+	id_del(space_id_pool, space->spid);
-	/* Return the space id ??? */
 
 	/* Deallocate the space structure */
+	kfree(space);
 }
 
 struct address_space *address_space_create(struct address_space *orig)
@@ -120,6 +125,13 @@ struct address_space *address_space_create(struct address_space *orig)
 	/* Copy all kernel entries */
 	copy_pgd_kern_all(pgd);
 
+	/*
+	 * Set up space id: Always allocate a new one. Specifying a space id
+	 * is not allowed since spid field is used to indicate the space to
+	 * copy from.
+	 */
+	space->spid = id_new(space_id_pool);
+
 	/* If an original space is supplied */
 	if (orig) {
 		/* Copy its user entries/tables */

src/generic/tcb.c

@@ -75,6 +75,7 @@ void tcb_delete(struct ktcb *tcb)
 	BUG_ON(tcb->wqh_recv.sleepers > 0);
 	BUG_ON(!list_empty(&tcb->task_list));
 	BUG_ON(!list_empty(&tcb->rq_list));
+	BUG_ON(tcb->rq);
 	BUG_ON(tcb->nlocks);
 	BUG_ON(tcb->waiting_on);
 	BUG_ON(tcb->wq);
@@ -87,15 +88,35 @@ void tcb_delete(struct ktcb *tcb)
 	BUG_ON(--tcb->space->ktcb_refs < 0);
 
 	/* No refs left for the space, delete it */
-	if (tcb->space->ktcb_refs == 0)
+	if (tcb->space->ktcb_refs == 0) {
+		address_space_remove(tcb->space);
 		address_space_delete(tcb->space);
+	}
 
 	address_space_reference_unlock();
 
+	/* Deallocate tcb ids */
+	id_del(thread_id_pool, tcb->tid);
+
 	/* Free the tcb */
 	free_page(tcb);
 }
 
+struct ktcb *tcb_find_by_space(l4id_t spid)
+{
+	struct ktcb *task;
+
+	spin_lock(&ktcb_list.list_lock);
+	list_for_each_entry(task, &ktcb_list.list, task_list) {
+		if (task->space->spid == spid) {
+			spin_unlock(&ktcb_list.list_lock);
+			return task;
+		}
+	}
+	spin_unlock(&ktcb_list.list_lock);
+	return 0;
+}
+
 struct ktcb *tcb_find(l4id_t tid)
 {
 	struct ktcb *task;
@@ -115,6 +136,7 @@ void tcb_add(struct ktcb *new)
 {
 	spin_lock(&ktcb_list.list_lock);
 	BUG_ON(!list_empty(&new->task_list));
+	BUG_ON(!++ktcb_list.count);
 	list_add(&new->task_list, &ktcb_list.list);
 	spin_unlock(&ktcb_list.list_lock);
 }
@@ -123,6 +145,7 @@ void tcb_remove(struct ktcb *new)
 {
 	spin_lock(&ktcb_list.list_lock);
 	BUG_ON(list_empty(&new->task_list));
+	BUG_ON(--ktcb_list.count < 0);
 	list_del_init(&new->task_list);
 	spin_unlock(&ktcb_list.list_lock);
 }