I've been working on this issue
<https://github.com/cloudius-systems/osv/issues/143> for the past couple
days, and I have a couple questions as well as a WIP solution.
I figured I would start by implementing a simple solution that accesses the
next stack page in preempt_disable() and irq_disable() to ensure that the
next stack page is mapped.
Here's my WIP:
diff --git a/arch/x64/arch-switch.hh b/arch/x64/arch-switch.hh
index 6803498f..11d8d3f8 100644
--- a/arch/x64/arch-switch.hh
+++ b/arch/x64/arch-switch.hh
@@ -144,10 +144,17 @@ void thread::init_stack()
stack.deleter = stack.default_deleter;
}
void** stacktop = reinterpret_cast<void**>(stack.begin + stack.size);
+
_state.rbp = this;
_state.rip = reinterpret_cast<void*>(thread_main);
_state.rsp = stacktop;
_state.exception_stack = _arch.exception_stack +
sizeof(_arch.exception_stack);
+
+ // We switch to a new thread with preemption disabled, so if the stack
+ // starts with a yet-unpopulated page, we will get a page fault when
the
+ // thread starts. Read the first byte of the stack to get the fault
now,
+ // when we're with preemption enabled.
+ asm volatile ("" : : "r" ( *((char*)stacktop - 1) ));
}
This change is referenced in the original email thread
(https://groups.google.com/d/msg/osv-dev/GCY8Q6vidPU/qsbaEU_gdsAJ) as a
solution to the boot failure on the sched::preemptable() assertion in
arch/x86/mmu.cc, however when it didn't resolve the issue for me. With this
WIP, my code is failing on that assertion.
diff --git a/arch/x64/arch.hh b/arch/x64/arch.hh
index 17df5f5c..35e339cc 100644
--- a/arch/x64/arch.hh
+++ b/arch/x64/arch.hh
@@ -16,12 +16,22 @@
namespace arch {
+inline void access_next_stack_page() {
+ uint64_t rsp;
+ asm volatile("movq %%rsp, %[Var]" : [Var] "=r" (rsp));
+ char *next_stack_page = (char*) rsp - 4096;
+ *next_stack_page = 0;
+}
+
#define CACHELINE_ALIGNED __attribute__((aligned(64)))
#define INSTR_SIZE_MIN 1
#define ELF_IMAGE_START OSV_KERNEL_BASE
inline void irq_disable()
{
+ // To allow stack pages to be dynamically allocated,
+ // we must check that the next stack page is allocated before
disabling interrupts
+ access_next_stack_page();
processor::cli();
}
These changes reflect the need to access the next stack page before
disabling interrupts. Would it be more clean to implement
access_next_stack_page in a one-line assembly function, or leave it more
human-readable?
diff --git a/include/osv/mmu-defs.hh b/include/osv/mmu-defs.hh
index 18edf441..694815f8 100644
--- a/include/osv/mmu-defs.hh
+++ b/include/osv/mmu-defs.hh
@@ -84,6 +84,7 @@ enum {
mmap_small = 1ul << 5,
mmap_jvm_balloon = 1ul << 6,
mmap_file = 1ul << 7,
+ mmap_stack = 1ul << 8,
};
enum {
diff --git a/include/osv/sched.hh b/include/osv/sched.hh
index 66c0a44a..f24df875 100644
--- a/include/osv/sched.hh
+++ b/include/osv/sched.hh
@@ -1001,6 +1001,10 @@ inline void preempt()
inline void preempt_disable()
{
++preempt_counter;
+ // Since stack pages are dynamically allocated,
+ // we must ensure that the next stack page is allocated before
disabling interrupts
+ arch::access_next_stack_page();
+
barrier();
}
diff --git a/libc/mman.cc b/libc/mman.cc
index bb573a80..5b0436e2 100644
--- a/libc/mman.cc
+++ b/libc/mman.cc
@@ -44,7 +44,7 @@ unsigned libc_flags_to_mmap(int flags)
// and did us the courtesy of telling this to ue (via MAP_STACK),
// let's return the courtesy by returning pre-faulted memory.
// FIXME: If issue #143 is fixed, this workaround should be
removed.
- mmap_flags |= mmu::mmap_populate;
+ mmap_flags |= mmu::mmap_stack;
}
if (flags & MAP_SHARED) {
mmap_flags |= mmu::mmap_shared;
diff --git a/libc/pthread.cc b/libc/pthread.cc
index 44b93b83..f063754b 100644
--- a/libc/pthread.cc
+++ b/libc/pthread.cc
@@ -139,7 +139,7 @@ namespace pthread_private {
return {attr.stack_begin, attr.stack_size};
}
size_t size = attr.stack_size;
- void *addr = mmu::map_anon(nullptr, size, mmu::mmap_populate,
mmu::perm_rw);
+ void *addr = mmu::map_anon(nullptr, size, mmu::mmap_stack,
mmu::perm_rw);
mmu::mprotect(addr, attr.guard_size, 0);
sched::thread::stack_info si{addr, size};
si.deleter = free_stack;
These changes force dynamic allocation rather than the previous
mmap_populate solution.
Interestingly, I couldn't find any test cases that called mmap in libc with
the MAP_STACK flag - I'm assuming this is because it's a no-op in Linux.
Instead all the stack allocations came from pthread::allocate_stack.
Let me know what you think of my WIP and my questions,
Matthew
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/b9ca3a6e-5551-41fb-9aa5-304dda7fd305%40googlegroups.com.
