On RISC architecture like aarch64 the thread register tpidr_el0 that holds an address of thread local storage memory for current thread, is never accessed directly in one instruction like on x86_64. Instead compiler generates machone code that copies value of the tpidr_el0 to another generic register when reading and does the opposite when writing.
As the issue #1121 explains, the reschedule_from_interrupt() method uses thread local variable 'current_cpu' to detect any pending thread to de destroyed for current cpu after switching from old thread to a new one. It must use TLS variable instead of any stack variables because those would be invalid at this point as they would be on the stack of the old thread switched from. This all works fine on x86_64, where every read or write of thread register fs is performed in one instruction like so: mov %fs:0xfffffffffffff948,%rax On aarch64, the value of the thread register tpidr_el0 is copied once to another register x21 before access of first TLS variable in this method and never refreshed after that. When reschedule_from_interrupt() calls switch_to() to switch to new thread, the update of the tpidr_el0 register is done in assembly and compiler has no idea that we are changing TLS register. So after return to reschedule_from_interrupt() before it calls cpu::current(), the register x21 still holds old value of tpidr_el0 register. In most cases this does not cause any harm, as typically the old and new thread holds the same current_cpu address. But it leads to very ugly crashes when sched::pin() is used which calls reschedule_from_interrupt() after changing its current_cpu to a new one it is pinning the current thread to. So in order to fix this problem, this patch delegates the code to destroy current cpu terminating thread to a new class method destroy_current_cpu_thread() to enforce fresh reading of the TLS register. Fixes #1121 Signed-off-by: Waldemar Kozaczuk <[email protected]> --- core/sched.cc | 19 ++++++++++++++----- include/osv/sched.hh | 1 + 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/core/sched.cc b/core/sched.cc index 06f849d1..45427cac 100644 --- a/core/sched.cc +++ b/core/sched.cc @@ -229,6 +229,13 @@ void cpu::schedule() } } +void __attribute__ ((noinline)) cpu::destroy_current_cpu_thread() { + if (cpu::current()->terminating_thread) { + cpu::current()->terminating_thread->destroy(); + cpu::current()->terminating_thread = nullptr; + } +} + void cpu::reschedule_from_interrupt(bool called_from_yield, thread_runtime::duration preempt_after) { @@ -338,15 +345,17 @@ void cpu::reschedule_from_interrupt(bool called_from_yield, } n->switch_to(); - // Note: after the call to n->switch_to(), we should no longer use any of + // Note 1: after the call to n->switch_to(), we should no longer use any of // the local variables, nor "this" object, because we just switched to n's // stack and the values we can access now are those that existed in the // reschedule call which scheduled n out, and will now be returning. // So to get the current cpu, we must use cpu::current(), not "this". - if (cpu::current()->terminating_thread) { - cpu::current()->terminating_thread->destroy(); - cpu::current()->terminating_thread = nullptr; - } + // Note 2: in addition, in order to destroy any pending thread to be destroyed + // for current CPU, we delegate to a class static method destroy_current_cpu_thread + // to enforce that TLS register is refreshed before we reference current cpu + // using new thread TLS variable we switched to. This is necessary for architectures + // like aarch64 where tpidr_el0 register is copied to other one before use. + destroy_current_cpu_thread(); } void cpu::timer_fired() diff --git a/include/osv/sched.hh b/include/osv/sched.hh index 0fb44f77..78fed622 100644 --- a/include/osv/sched.hh +++ b/include/osv/sched.hh @@ -886,6 +886,7 @@ struct cpu : private timer_base::client { */ void reschedule_from_interrupt(bool called_from_yield = false, thread_runtime::duration preempt_after = thyst); + static void destroy_current_cpu_thread(); void enqueue(thread& t); void init_idle_thread(); virtual void timer_fired() override; -- 2.29.2 -- 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20210208055529.1339473-1-jwkozaczuk%40gmail.com.
