| Message ID | 1451936091-29247-9-git-send-email-cmetcalf@ezchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote: > This change is a prerequisite change for TASK_ISOLATION but also > stands on its own for readability and maintainability. I have also been looking into converting the userspace return path from assembly to C [1], for the latter two reasons. Based on that, I have a couple of comments. > The existing arm64 do_notify_resume() is called in a loop from > assembly on the slow path; this change moves the loop into C code as > well. For the x86 version see commit c5c46f59e4e7 ("x86/entry: Add > new, comprehensible entry and exit handlers written in C"). > > Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com> > --- > arch/arm64/kernel/entry.S | 6 +++--- > arch/arm64/kernel/signal.c | 32 ++++++++++++++++++++++---------- > 2 files changed, 25 insertions(+), 13 deletions(-) > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index 7ed3d75f6304..04eff4c4ac6e 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -630,9 +630,8 @@ work_pending: > mov x0, sp // 'regs' > tst x2, #PSR_MODE_MASK // user mode regs? > b.ne no_work_pending // returning to kernel > - enable_irq // enable interrupts for do_notify_resume() > - bl do_notify_resume > - b ret_to_user > + bl prepare_exit_to_usermode > + b no_user_work_pending > work_resched: > bl schedule > > @@ -644,6 +643,7 @@ ret_to_user: > ldr x1, [tsk, #TI_FLAGS] > and x2, x1, #_TIF_WORK_MASK > cbnz x2, work_pending > +no_user_work_pending: > enable_step_tsk x1, x2 > no_work_pending: > kernel_exit 0 It seems unfortunate to leave behind portions of the entry.S _TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall, and the majority of work_pending and ret_to_user). I think it would be nicer if we could handle all of that in one place (or at least all in C). > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > index e18c48cb6db1..fde59c1139a9 100644 > --- a/arch/arm64/kernel/signal.c > +++ b/arch/arm64/kernel/signal.c > @@ -399,18 +399,30 @@ static void do_signal(struct pt_regs *regs) > restore_saved_sigmask(); > } > > -asmlinkage void do_notify_resume(struct pt_regs *regs, > - unsigned int thread_flags) > +asmlinkage void prepare_exit_to_usermode(struct pt_regs *regs, > + unsigned int thread_flags) > { > - if (thread_flags & _TIF_SIGPENDING) > - do_signal(regs); > + do { > + local_irq_enable(); > > - if (thread_flags & _TIF_NOTIFY_RESUME) { > - clear_thread_flag(TIF_NOTIFY_RESUME); > - tracehook_notify_resume(regs); > - } > + if (thread_flags & _TIF_NEED_RESCHED) > + schedule(); Previously, had we called schedule(), we'd reload the thread info flags and start that state machine again, whereas now we'll handle all the cached flags before reloading. Are we sure nothing is relying on the prior behaviour? > + > + if (thread_flags & _TIF_SIGPENDING) > + do_signal(regs); > + > + if (thread_flags & _TIF_NOTIFY_RESUME) { > + clear_thread_flag(TIF_NOTIFY_RESUME); > + tracehook_notify_resume(regs); > + } > + > + if (thread_flags & _TIF_FOREIGN_FPSTATE) > + fpsimd_restore_current_state(); > + > + local_irq_disable(); > > - if (thread_flags & _TIF_FOREIGN_FPSTATE) > - fpsimd_restore_current_state(); > + thread_flags = READ_ONCE(current_thread_info()->flags) & > + _TIF_WORK_MASK; > > + } while (thread_flags); > } Other than that, this looks good to me. Thanks, Mark. [1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm
On 01/04/2016 03:33 PM, Mark Rutland wrote: > Hi, > > On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote: >> This change is a prerequisite change for TASK_ISOLATION but also >> stands on its own for readability and maintainability. > I have also been looking into converting the userspace return path from > assembly to C [1], for the latter two reasons. Based on that, I have a > couple of comments. Thanks! > It seems unfortunate to leave behind portions of the entry.S > _TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall, > and the majority of work_pending and ret_to_user). > > I think it would be nicer if we could handle all of that in one place > (or at least all in C). Yes, in principle I agree with this, and I think your deasm tree looks like an excellent idea. For this patch series I wanted to focus more on what was necessary for the various platforms to implement task isolation, and less on additional cleanups of the platforms in question. I think my changes don't make the TIF state machine any less clear, nor do they make it harder for an eventual further migration to C code along the lines of what you've done, so it seems plausible to me to commit them upstream independently of your work. >> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c >> index e18c48cb6db1..fde59c1139a9 100644 >> --- a/arch/arm64/kernel/signal.c >> +++ b/arch/arm64/kernel/signal.c >> @@ -399,18 +399,30 @@ static void do_signal(struct pt_regs *regs) >> restore_saved_sigmask(); >> } >> >> -asmlinkage void do_notify_resume(struct pt_regs *regs, >> - unsigned int thread_flags) >> +asmlinkage void prepare_exit_to_usermode(struct pt_regs *regs, >> + unsigned int thread_flags) >> { >> - if (thread_flags & _TIF_SIGPENDING) >> - do_signal(regs); >> + do { >> + local_irq_enable(); >> >> - if (thread_flags & _TIF_NOTIFY_RESUME) { >> - clear_thread_flag(TIF_NOTIFY_RESUME); >> - tracehook_notify_resume(regs); >> - } >> + if (thread_flags & _TIF_NEED_RESCHED) >> + schedule(); > Previously, had we called schedule(), we'd reload the thread info flags > and start that state machine again, whereas now we'll handle all the > cached flags before reloading. > > Are we sure nothing is relying on the prior behaviour? Good eye, and I probably should have called that out in the commit message. My best guess is that there should be nothing that depends on the old semantics. Other platforms (certainly x86 and tile, anyway) already have the semantics that you run out the old state machine on return from schedule(), so regardless, it's probably appropriate for arm to follow that same convention. >> + >> + if (thread_flags & _TIF_SIGPENDING) >> + do_signal(regs); >> + >> + if (thread_flags & _TIF_NOTIFY_RESUME) { >> + clear_thread_flag(TIF_NOTIFY_RESUME); >> + tracehook_notify_resume(regs); >> + } >> + >> + if (thread_flags & _TIF_FOREIGN_FPSTATE) >> + fpsimd_restore_current_state(); >> + >> + local_irq_disable(); >> >> - if (thread_flags & _TIF_FOREIGN_FPSTATE) >> - fpsimd_restore_current_state(); >> + thread_flags = READ_ONCE(current_thread_info()->flags) & >> + _TIF_WORK_MASK; >> >> + } while (thread_flags); >> } > Other than that, this looks good to me. > > Thanks, > Mark. > > [1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm Thanks again for the review - shall I add your Reviewed-by (or Acked-by?) to this patch?
On Mon, Jan 4, 2016 at 12:33 PM, Mark Rutland <mark.rutland@arm.com> wrote: > Hi, > > On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote: >> This change is a prerequisite change for TASK_ISOLATION but also >> stands on its own for readability and maintainability. > > I have also been looking into converting the userspace return path from > assembly to C [1], for the latter two reasons. Based on that, I have a > couple of comments. > > > [1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm Neat! In case you want to compare notes, I have a branch with the entire syscall path on x86 in C except for cleanly separated asm fast path optimizations: https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=x86/entry_compat Even in Linus' tree, the x86 32-bit syscalls are in C.
On Mon, Jan 04, 2016 at 04:01:05PM -0500, Chris Metcalf wrote: > On 01/04/2016 03:33 PM, Mark Rutland wrote: > >Hi, > > > >On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote: > >>This change is a prerequisite change for TASK_ISOLATION but also > >>stands on its own for readability and maintainability. > >I have also been looking into converting the userspace return path from > >assembly to C [1], for the latter two reasons. Based on that, I have a > >couple of comments. > > Thanks! > > >It seems unfortunate to leave behind portions of the entry.S > >_TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall, > >and the majority of work_pending and ret_to_user). > > > >I think it would be nicer if we could handle all of that in one place > >(or at least all in C). > > Yes, in principle I agree with this, and I think your deasm tree looks > like an excellent idea. > > For this patch series I wanted to focus more on what was necessary > for the various platforms to implement task isolation, and less on > additional cleanups of the platforms in question. I think my changes > don't make the TIF state machine any less clear, nor do they make > it harder for an eventual further migration to C code along the lines > of what you've done, so it seems plausible to me to commit them > upstream independently of your work. I appreciate that you don't want to rewrite all the code. However, I think it's easier to factor out a small amount of additional code now and evlove that as a whole than it will be to evolve part of it and try to put it back together later. I have a patch which I will reply with momentarily. Thanks, Mark.
On Mon, Jan 04, 2016 at 02:31:42PM -0800, Andy Lutomirski wrote: > On Mon, Jan 4, 2016 at 12:33 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > Hi, > > > > On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote: > >> This change is a prerequisite change for TASK_ISOLATION but also > >> stands on its own for readability and maintainability. > > > > I have also been looking into converting the userspace return path from > > assembly to C [1], for the latter two reasons. Based on that, I have a > > couple of comments. > > > > > > > [1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm > > Neat! > > In case you want to compare notes, I have a branch with the entire > syscall path on x86 in C except for cleanly separated asm fast path > optimizations: > > https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=x86/entry_compat It was in fact your x86 effort that inspired me to look at this! Thanks for the pointer, I'm almost certainly going to steal an idea or two. Currently it looks like arm64's conversion will be less painful than that for x86 as the entry assembly is smaller and relatively uniform. It looks like all but the register save/restore is possible in C. That said, I have yet to stress/validate everything with tracing, irq debugging, and so on, so my confidence may be misplaced. Thanks, Mark.
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 7ed3d75f6304..04eff4c4ac6e 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -630,9 +630,8 @@ work_pending: mov x0, sp // 'regs' tst x2, #PSR_MODE_MASK // user mode regs? b.ne no_work_pending // returning to kernel - enable_irq // enable interrupts for do_notify_resume() - bl do_notify_resume - b ret_to_user + bl prepare_exit_to_usermode + b no_user_work_pending work_resched: bl schedule @@ -644,6 +643,7 @@ ret_to_user: ldr x1, [tsk, #TI_FLAGS] and x2, x1, #_TIF_WORK_MASK cbnz x2, work_pending +no_user_work_pending: enable_step_tsk x1, x2 no_work_pending: kernel_exit 0 diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index e18c48cb6db1..fde59c1139a9 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -399,18 +399,30 @@ static void do_signal(struct pt_regs *regs) restore_saved_sigmask(); } -asmlinkage void do_notify_resume(struct pt_regs *regs, - unsigned int thread_flags) +asmlinkage void prepare_exit_to_usermode(struct pt_regs *regs, + unsigned int thread_flags) { - if (thread_flags & _TIF_SIGPENDING) - do_signal(regs); + do { + local_irq_enable(); - if (thread_flags & _TIF_NOTIFY_RESUME) { - clear_thread_flag(TIF_NOTIFY_RESUME); - tracehook_notify_resume(regs); - } + if (thread_flags & _TIF_NEED_RESCHED) + schedule(); + + if (thread_flags & _TIF_SIGPENDING) + do_signal(regs); + + if (thread_flags & _TIF_NOTIFY_RESUME) { + clear_thread_flag(TIF_NOTIFY_RESUME); + tracehook_notify_resume(regs); + } + + if (thread_flags & _TIF_FOREIGN_FPSTATE) + fpsimd_restore_current_state(); + + local_irq_disable(); - if (thread_flags & _TIF_FOREIGN_FPSTATE) - fpsimd_restore_current_state(); + thread_flags = READ_ONCE(current_thread_info()->flags) & + _TIF_WORK_MASK; + } while (thread_flags); }
This change is a prerequisite change for TASK_ISOLATION but also stands on its own for readability and maintainability. The existing arm64 do_notify_resume() is called in a loop from assembly on the slow path; this change moves the loop into C code as well. For the x86 version see commit c5c46f59e4e7 ("x86/entry: Add new, comprehensible entry and exit handlers written in C"). Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com> --- arch/arm64/kernel/entry.S | 6 +++--- arch/arm64/kernel/signal.c | 32 ++++++++++++++++++++++---------- 2 files changed, 25 insertions(+), 13 deletions(-)