| Message ID | 20240206123848.1696480-3-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: cleanup DAIF handling for EL0 returns | expand |
On Tue, Feb 06, 2024 at 12:38:47PM +0000, Mark Rutland wrote: > Currently do_notify_resume() lives in arch/arm64/kernel/signal.c, but it would > make more sense for it to live in entry-common.c as it handles more than > signals, and is coupled with the rest of the return-to-userspace sequence (e.g. > with unusual DAIF masking that matches the exception return requirements). > > Move do_notify_resume() to entry-common.c. Reviewed-by: Mark Brown <broonie@kernel.org>
Hi, Mark 在 2024/2/6 20:38, Mark Rutland 写道: > Currently do_notify_resume() lives in arch/arm64/kernel/signal.c, but it would > make more sense for it to live in entry-common.c as it handles more than > signals, and is coupled with the rest of the return-to-userspace sequence (e.g. > with unusual DAIF masking that matches the exception return requirements). > > Move do_notify_resume() to entry-common.c. > > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: James Morse <james.morse@arm.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Will Deacon <will@kernel.org> > --- > arch/arm64/include/asm/exception.h | 2 +- > arch/arm64/kernel/entry-common.c | 32 +++++++++++++++++++++++++++ > arch/arm64/kernel/signal.c | 35 ++---------------------------- > 3 files changed, 35 insertions(+), 34 deletions(-) > > diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h > index ad688e157c9be..f296662590c7f 100644 > --- a/arch/arm64/include/asm/exception.h > +++ b/arch/arm64/include/asm/exception.h > @@ -74,7 +74,7 @@ void do_el0_fpac(struct pt_regs *regs, unsigned long esr); > void do_el1_fpac(struct pt_regs *regs, unsigned long esr); > void do_el0_mops(struct pt_regs *regs, unsigned long esr); > void do_serror(struct pt_regs *regs, unsigned long esr); > -void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags); > +void do_signal(struct pt_regs *regs); > > void __noreturn panic_bad_stack(struct pt_regs *regs, unsigned long esr, unsigned long far); > #endif /* __ASM_EXCEPTION_H */ > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > index 0fc94207e69a8..3c849ad03bf83 100644 > --- a/arch/arm64/kernel/entry-common.c > +++ b/arch/arm64/kernel/entry-common.c > @@ -10,6 +10,7 @@ > #include <linux/linkage.h> > #include <linux/lockdep.h> > #include <linux/ptrace.h> > +#include <linux/resume_user_mode.h> > #include <linux/sched.h> > #include <linux/sched/debug.h> > #include <linux/thread_info.h> > @@ -126,6 +127,37 @@ static __always_inline void __exit_to_user_mode(void) > lockdep_hardirqs_on(CALLER_ADDR0); > } > > +static void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags) > +{ > + do { > + local_daif_restore(DAIF_PROCCTX); > + > + if (thread_flags & _TIF_NEED_RESCHED) > + schedule(); > + > + if (thread_flags & _TIF_UPROBE) > + uprobe_notify_resume(regs); > + > + if (thread_flags & _TIF_MTE_ASYNC_FAULT) { > + clear_thread_flag(TIF_MTE_ASYNC_FAULT); > + send_sig_fault(SIGSEGV, SEGV_MTEAERR, > + (void __user *)NULL, current); > + } > + > + if (thread_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) > + do_signal(regs); > + > + if (thread_flags & _TIF_NOTIFY_RESUME) > + resume_user_mode_work(regs); > + > + if (thread_flags & _TIF_FOREIGN_FPSTATE) > + fpsimd_restore_current_state(); > + > + local_daif_mask(); > + thread_flags = read_thread_flags(); > + } while (thread_flags & _TIF_WORK_MASK); > +} What about moving load_daif_restore() and load_daif_mask() outside of the do-while loop? Invoking them repeatedlly within the loop is redundant, right? Thanks. > + > static __always_inline void exit_to_user_mode_prepare(struct pt_regs *regs) > { > unsigned long flags; > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > index 50e108741599d..c08e6465e0f49 100644 > --- a/arch/arm64/kernel/signal.c > +++ b/arch/arm64/kernel/signal.c > @@ -16,8 +16,8 @@ > #include <linux/uaccess.h> > #include <linux/sizes.h> > #include <linux/string.h> > -#include <linux/resume_user_mode.h> > #include <linux/ratelimit.h> > +#include <linux/rseq.h> > #include <linux/syscalls.h> > > #include <asm/daifflags.h> > @@ -1207,7 +1207,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs) > * the kernel can handle, and then we build all the user-level signal handling > * stack-frames in one go after that. > */ > -static void do_signal(struct pt_regs *regs) > +void do_signal(struct pt_regs *regs) > { > unsigned long continue_addr = 0, restart_addr = 0; > int retval = 0; > @@ -1278,37 +1278,6 @@ static void do_signal(struct pt_regs *regs) > restore_saved_sigmask(); > } > > -void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags) > -{ > - do { > - local_daif_restore(DAIF_PROCCTX); > - > - if (thread_flags & _TIF_NEED_RESCHED) > - schedule(); > - > - if (thread_flags & _TIF_UPROBE) > - uprobe_notify_resume(regs); > - > - if (thread_flags & _TIF_MTE_ASYNC_FAULT) { > - clear_thread_flag(TIF_MTE_ASYNC_FAULT); > - send_sig_fault(SIGSEGV, SEGV_MTEAERR, > - (void __user *)NULL, current); > - } > - > - if (thread_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) > - do_signal(regs); > - > - if (thread_flags & _TIF_NOTIFY_RESUME) > - resume_user_mode_work(regs); > - > - if (thread_flags & _TIF_FOREIGN_FPSTATE) > - fpsimd_restore_current_state(); > - > - local_daif_mask(); > - thread_flags = read_thread_flags(); > - } while (thread_flags & _TIF_WORK_MASK); > -} > - > unsigned long __ro_after_init signal_minsigstksz; > > /*
On Tue, Feb 27, 2024 at 11:25:32AM +0800, Liao, Chang wrote: > Hi, Mark > > 在 2024/2/6 20:38, Mark Rutland 写道: > > +static void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags) > > +{ > > + do { > > + local_daif_restore(DAIF_PROCCTX); > > + > > + if (thread_flags & _TIF_NEED_RESCHED) > > + schedule(); > > + > > + if (thread_flags & _TIF_UPROBE) > > + uprobe_notify_resume(regs); > > + > > + if (thread_flags & _TIF_MTE_ASYNC_FAULT) { > > + clear_thread_flag(TIF_MTE_ASYNC_FAULT); > > + send_sig_fault(SIGSEGV, SEGV_MTEAERR, > > + (void __user *)NULL, current); > > + } > > + > > + if (thread_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) > > + do_signal(regs); > > + > > + if (thread_flags & _TIF_NOTIFY_RESUME) > > + resume_user_mode_work(regs); > > + > > + if (thread_flags & _TIF_FOREIGN_FPSTATE) > > + fpsimd_restore_current_state(); > > + > > + local_daif_mask(); > > + thread_flags = read_thread_flags(); > > + } while (thread_flags & _TIF_WORK_MASK); > > +} > > What about moving load_daif_restore() and load_daif_mask() outside of the do-while loop? > Invoking them repeatedlly within the loop is redundant, right? It's not entirely redundant -- we don't want to take an interrupt between the last read of the thread flags and the actual return to userspace. The next patch moves the DAIF masking out, so that we only have to mask+unmask IRQ+FIQ here, but masking those is necessary. That matches the logic in the generic exit_to_user_mode_loop(). Mark.
diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h index ad688e157c9be..f296662590c7f 100644 --- a/arch/arm64/include/asm/exception.h +++ b/arch/arm64/include/asm/exception.h @@ -74,7 +74,7 @@ void do_el0_fpac(struct pt_regs *regs, unsigned long esr); void do_el1_fpac(struct pt_regs *regs, unsigned long esr); void do_el0_mops(struct pt_regs *regs, unsigned long esr); void do_serror(struct pt_regs *regs, unsigned long esr); -void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags); +void do_signal(struct pt_regs *regs); void __noreturn panic_bad_stack(struct pt_regs *regs, unsigned long esr, unsigned long far); #endif /* __ASM_EXCEPTION_H */ diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c index 0fc94207e69a8..3c849ad03bf83 100644 --- a/arch/arm64/kernel/entry-common.c +++ b/arch/arm64/kernel/entry-common.c @@ -10,6 +10,7 @@ #include <linux/linkage.h> #include <linux/lockdep.h> #include <linux/ptrace.h> +#include <linux/resume_user_mode.h> #include <linux/sched.h> #include <linux/sched/debug.h> #include <linux/thread_info.h> @@ -126,6 +127,37 @@ static __always_inline void __exit_to_user_mode(void) lockdep_hardirqs_on(CALLER_ADDR0); } +static void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags) +{ + do { + local_daif_restore(DAIF_PROCCTX); + + if (thread_flags & _TIF_NEED_RESCHED) + schedule(); + + if (thread_flags & _TIF_UPROBE) + uprobe_notify_resume(regs); + + if (thread_flags & _TIF_MTE_ASYNC_FAULT) { + clear_thread_flag(TIF_MTE_ASYNC_FAULT); + send_sig_fault(SIGSEGV, SEGV_MTEAERR, + (void __user *)NULL, current); + } + + if (thread_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) + do_signal(regs); + + if (thread_flags & _TIF_NOTIFY_RESUME) + resume_user_mode_work(regs); + + if (thread_flags & _TIF_FOREIGN_FPSTATE) + fpsimd_restore_current_state(); + + local_daif_mask(); + thread_flags = read_thread_flags(); + } while (thread_flags & _TIF_WORK_MASK); +} + static __always_inline void exit_to_user_mode_prepare(struct pt_regs *regs) { unsigned long flags; diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 50e108741599d..c08e6465e0f49 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -16,8 +16,8 @@ #include <linux/uaccess.h> #include <linux/sizes.h> #include <linux/string.h> -#include <linux/resume_user_mode.h> #include <linux/ratelimit.h> +#include <linux/rseq.h> #include <linux/syscalls.h> #include <asm/daifflags.h> @@ -1207,7 +1207,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs) * the kernel can handle, and then we build all the user-level signal handling * stack-frames in one go after that. */ -static void do_signal(struct pt_regs *regs) +void do_signal(struct pt_regs *regs) { unsigned long continue_addr = 0, restart_addr = 0; int retval = 0; @@ -1278,37 +1278,6 @@ static void do_signal(struct pt_regs *regs) restore_saved_sigmask(); } -void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags) -{ - do { - local_daif_restore(DAIF_PROCCTX); - - if (thread_flags & _TIF_NEED_RESCHED) - schedule(); - - if (thread_flags & _TIF_UPROBE) - uprobe_notify_resume(regs); - - if (thread_flags & _TIF_MTE_ASYNC_FAULT) { - clear_thread_flag(TIF_MTE_ASYNC_FAULT); - send_sig_fault(SIGSEGV, SEGV_MTEAERR, - (void __user *)NULL, current); - } - - if (thread_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) - do_signal(regs); - - if (thread_flags & _TIF_NOTIFY_RESUME) - resume_user_mode_work(regs); - - if (thread_flags & _TIF_FOREIGN_FPSTATE) - fpsimd_restore_current_state(); - - local_daif_mask(); - thread_flags = read_thread_flags(); - } while (thread_flags & _TIF_WORK_MASK); -} - unsigned long __ro_after_init signal_minsigstksz; /*
Currently do_notify_resume() lives in arch/arm64/kernel/signal.c, but it would make more sense for it to live in entry-common.c as it handles more than signals, and is coupled with the rest of the return-to-userspace sequence (e.g. with unusual DAIF masking that matches the exception return requirements). Move do_notify_resume() to entry-common.c. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Mark Brown <broonie@kernel.org> Cc: Will Deacon <will@kernel.org> --- arch/arm64/include/asm/exception.h | 2 +- arch/arm64/kernel/entry-common.c | 32 +++++++++++++++++++++++++++ arch/arm64/kernel/signal.c | 35 ++---------------------------- 3 files changed, 35 insertions(+), 34 deletions(-)