[PATCH 0/3] arm64: cleanup DAIF handling for EL0 returns

[PATCH 0/3] arm64: cleanup DAIF handling for EL0 returns

[Mark Rutland]

Hi,

These patches rework DAIF handling when returning to EL0, simplifiying
the logic in do_notify_resume() and allowing Debug + SError to be
unmasked for a little longer during exception return.

My primary motivation for these changes is to centralize entry/exit DAIF
management within entry-common.c, as this will allow entry/exit
sequences to be specialised, and make it much easier to add support for
FEAT_NMI. With specialized entry DAIF management, it will also be
possible to simplify local_daif_{save,restore}() and
local_irq_{save,restore}().

I'm sending this out on its own as (IMO) it's a nice cleanup on its own,
and I don't believe that I'll have the remainder of the DAIF rework
ready for this cycle.

The series is based on v6.8-rc3.

Mark.

Mark Rutland (3):
  arm64: Simplify do_notify_resume() DAIF masking
  arm64: Move do_notify_resume() to entry-common.c
  arm64: Unmask Debug + SError in do_notify_resume()

 arch/arm64/include/asm/exception.h |  2 +-
 arch/arm64/kernel/entry-common.c   | 36 ++++++++++++++++++++++++++-
 arch/arm64/kernel/signal.c         | 39 ++----------------------------
 3 files changed, 38 insertions(+), 39 deletions(-)

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] arm64: Simplify do_notify_resume() DAIF masking

[Mark Rutland]

In do_notify_resume, we handle _TIF_NEED_RESCHED differently from all
other flags, leaving IRQ+FIQ masked when calling into schedule(). This
masking is a historical artifact, and it is not currently necessary
to mask IRQ+FIQ when calling into schedule (as evidenced by the generic
exit_to_user_mode_loop(), which unmasks IRQs before checking
_TIF_NEED_RESCHED and calling schedule()).

This patch removes the special case for _TIF_NEED_RESCHED, moving this
check into the main loop such that schedule() will be called from a
regular process context with IRQ+FIQ unmasked. This is a minor
simplification to do_notify_resume() and brings it into line with the
generic exit_to_user_mode_loop() logic. This will also aid subsequent
rework of DAIF management.

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/kernel/signal.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 0e8beb3349ea2..50e108741599d 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -1281,32 +1281,28 @@ static void do_signal(struct pt_regs *regs)
 void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags)
 {
 	do {
-		if (thread_flags & _TIF_NEED_RESCHED) {
-			/* Unmask Debug and SError for the next task */
-			local_daif_restore(DAIF_PROCCTX_NOIRQ);
+		local_daif_restore(DAIF_PROCCTX);
 
+		if (thread_flags & _TIF_NEED_RESCHED)
 			schedule();
-		} else {
-			local_daif_restore(DAIF_PROCCTX);
 
-			if (thread_flags & _TIF_UPROBE)
-				uprobe_notify_resume(regs);
+		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_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_SIGPENDING | _TIF_NOTIFY_SIGNAL))
+			do_signal(regs);
 
-			if (thread_flags & _TIF_NOTIFY_RESUME)
-				resume_user_mode_work(regs);
+		if (thread_flags & _TIF_NOTIFY_RESUME)
+			resume_user_mode_work(regs);
 
-			if (thread_flags & _TIF_FOREIGN_FPSTATE)
-				fpsimd_restore_current_state();
-		}
+		if (thread_flags & _TIF_FOREIGN_FPSTATE)
+			fpsimd_restore_current_state();
 
 		local_daif_mask();
 		thread_flags = read_thread_flags();
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] arm64: Move do_notify_resume() to entry-common.c

[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);
+}
+
 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;
 
 /*
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: Unmask Debug + SError in do_notify_resume()

[Mark Rutland]

When returning to a user context, the arm64 entry code masks all DAIF
exceptions before handling pending work in exit_to_user_mode_prepare()
and do_notify_resume(), where it will transiently unmask all DAIF
exceptions. This is a holdover from the old entry assembly, which
conservatively masked all DAIF exceptions, and it's only necessary to
mask interrupts at this point during the exception return path, so long
as we subsequently mask all DAIF exceptions before the actual exception
return.

While most DAIF manipulation follows a save...restore sequence, the
manipulation in do_notify_resume() is the other way around, unmasking
all DAIF exceptions before masking them again. This is unfortunate as we
unnecessarily mask Debug and SError exceptions, and it would be nice to
remove this special case to make DAIF manipulation simpler and most
consistent.

This patch changes exit_to_user_mode_prepare() and do_notify_resume() to
only mask interrupts while handling pending work, masking other DAIF
exceptions after this has completed. This removes the unusual DAIF
manipulation and allows Debug and SError exceptions to be taken for a
slightly longer window during the exception return path.

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/kernel/entry-common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 3c849ad03bf83..b77a15955f28b 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -130,7 +130,7 @@ static __always_inline void __exit_to_user_mode(void)
 static void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags)
 {
 	do {
-		local_daif_restore(DAIF_PROCCTX);
+		local_irq_enable();
 
 		if (thread_flags & _TIF_NEED_RESCHED)
 			schedule();
@@ -153,7 +153,7 @@ static void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags)
 		if (thread_flags & _TIF_FOREIGN_FPSTATE)
 			fpsimd_restore_current_state();
 
-		local_daif_mask();
+		local_irq_disable();
 		thread_flags = read_thread_flags();
 	} while (thread_flags & _TIF_WORK_MASK);
 }
@@ -162,12 +162,14 @@ static __always_inline void exit_to_user_mode_prepare(struct pt_regs *regs)
 {
 	unsigned long flags;
 
-	local_daif_mask();
+	local_irq_disable();
 
 	flags = read_thread_flags();
 	if (unlikely(flags & _TIF_WORK_MASK))
 		do_notify_resume(regs, flags);
 
+	local_daif_mask();
+
 	lockdep_sys_exit();
 }
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: Move do_notify_resume() to entry-common.c

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 480 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] arm64: cleanup DAIF handling for EL0 returns

[Itaru Kitayama]

On Tue, Feb 06, 2024 at 12:38:45PM +0000, Mark Rutland wrote:
> Hi,
> 
> These patches rework DAIF handling when returning to EL0, simplifiying
> the logic in do_notify_resume() and allowing Debug + SError to be
> unmasked for a little longer during exception return.
> 
> My primary motivation for these changes is to centralize entry/exit DAIF
> management within entry-common.c, as this will allow entry/exit
> sequences to be specialised, and make it much easier to add support for
> FEAT_NMI. With specialized entry DAIF management, it will also be
> possible to simplify local_daif_{save,restore}() and
> local_irq_{save,restore}().
> 
> I'm sending this out on its own as (IMO) it's a nice cleanup on its own,
> and I don't believe that I'll have the remainder of the DAIF rework
> ready for this cycle.
> 
> The series is based on v6.8-rc3.
> 
> Mark.
> 
> Mark Rutland (3):
>   arm64: Simplify do_notify_resume() DAIF masking
>   arm64: Move do_notify_resume() to entry-common.c
>   arm64: Unmask Debug + SError in do_notify_resume()
> 
>  arch/arm64/include/asm/exception.h |  2 +-
>  arch/arm64/kernel/entry-common.c   | 36 ++++++++++++++++++++++++++-
>  arch/arm64/kernel/signal.c         | 39 ++----------------------------
>  3 files changed, 38 insertions(+), 39 deletions(-)

Boot tested on RevC FVP and kselftest arm64 subcategory runs without an error
on RevC FVP too.

Itaru.

> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] arm64: cleanup DAIF handling for EL0 returns

[Mark Rutland]

On Thu, Feb 08, 2024 at 11:28:32AM +0900, Itaru Kitayama wrote:
> On Tue, Feb 06, 2024 at 12:38:45PM +0000, Mark Rutland wrote:
> > Hi,
> > 
> > These patches rework DAIF handling when returning to EL0, simplifiying
> > the logic in do_notify_resume() and allowing Debug + SError to be
> > unmasked for a little longer during exception return.
> > 
> > My primary motivation for these changes is to centralize entry/exit DAIF
> > management within entry-common.c, as this will allow entry/exit
> > sequences to be specialised, and make it much easier to add support for
> > FEAT_NMI. With specialized entry DAIF management, it will also be
> > possible to simplify local_daif_{save,restore}() and
> > local_irq_{save,restore}().
> > 
> > I'm sending this out on its own as (IMO) it's a nice cleanup on its own,
> > and I don't believe that I'll have the remainder of the DAIF rework
> > ready for this cycle.
> > 
> > The series is based on v6.8-rc3.
> > 
> > Mark.
> > 
> > Mark Rutland (3):
> >   arm64: Simplify do_notify_resume() DAIF masking
> >   arm64: Move do_notify_resume() to entry-common.c
> >   arm64: Unmask Debug + SError in do_notify_resume()
> > 
> >  arch/arm64/include/asm/exception.h |  2 +-
> >  arch/arm64/kernel/entry-common.c   | 36 ++++++++++++++++++++++++++-
> >  arch/arm64/kernel/signal.c         | 39 ++----------------------------
> >  3 files changed, 38 insertions(+), 39 deletions(-)
> 
> Boot tested on RevC FVP and kselftest arm64 subcategory runs without an error
> on RevC FVP too.

Thanks for testing! Would you be happy for that to imply a tested-by tag, e.g.

  Tested-by: Itaru Kitayama <itaru.kitayama@linux.dev>

... ?

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] arm64: Simplify do_notify_resume() DAIF masking

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 505 bytes --]

On Tue, Feb 06, 2024 at 12:38:46PM +0000, Mark Rutland wrote:
> In do_notify_resume, we handle _TIF_NEED_RESCHED differently from all
> other flags, leaving IRQ+FIQ masked when calling into schedule(). This
> masking is a historical artifact, and it is not currently necessary
> to mask IRQ+FIQ when calling into schedule (as evidenced by the generic
> exit_to_user_mode_loop(), which unmasks IRQs before checking
> _TIF_NEED_RESCHED and calling schedule()).

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] arm64: Unmask Debug + SError in do_notify_resume()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 619 bytes --]

On Tue, Feb 06, 2024 at 12:38:48PM +0000, Mark Rutland wrote:
> When returning to a user context, the arm64 entry code masks all DAIF
> exceptions before handling pending work in exit_to_user_mode_prepare()
> and do_notify_resume(), where it will transiently unmask all DAIF
> exceptions. This is a holdover from the old entry assembly, which
> conservatively masked all DAIF exceptions, and it's only necessary to
> mask interrupts at this point during the exception return path, so long
> as we subsequently mask all DAIF exceptions before the actual exception
> return.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] arm64: cleanup DAIF handling for EL0 returns

[Itaru Kitayama]

On Thu, Feb 08, 2024 at 12:38:12PM +0000, Mark Rutland wrote:
> On Thu, Feb 08, 2024 at 11:28:32AM +0900, Itaru Kitayama wrote:
> > On Tue, Feb 06, 2024 at 12:38:45PM +0000, Mark Rutland wrote:
> > > Hi,
> > > 
> > > These patches rework DAIF handling when returning to EL0, simplifiying
> > > the logic in do_notify_resume() and allowing Debug + SError to be
> > > unmasked for a little longer during exception return.
> > > 
> > > My primary motivation for these changes is to centralize entry/exit DAIF
> > > management within entry-common.c, as this will allow entry/exit
> > > sequences to be specialised, and make it much easier to add support for
> > > FEAT_NMI. With specialized entry DAIF management, it will also be
> > > possible to simplify local_daif_{save,restore}() and
> > > local_irq_{save,restore}().
> > > 
> > > I'm sending this out on its own as (IMO) it's a nice cleanup on its own,
> > > and I don't believe that I'll have the remainder of the DAIF rework
> > > ready for this cycle.
> > > 
> > > The series is based on v6.8-rc3.
> > > 
> > > Mark.
> > > 
> > > Mark Rutland (3):
> > >   arm64: Simplify do_notify_resume() DAIF masking
> > >   arm64: Move do_notify_resume() to entry-common.c
> > >   arm64: Unmask Debug + SError in do_notify_resume()
> > > 
> > >  arch/arm64/include/asm/exception.h |  2 +-
> > >  arch/arm64/kernel/entry-common.c   | 36 ++++++++++++++++++++++++++-
> > >  arch/arm64/kernel/signal.c         | 39 ++----------------------------
> > >  3 files changed, 38 insertions(+), 39 deletions(-)
> > 
> > Boot tested on RevC FVP and kselftest arm64 subcategory runs without an error
> > on RevC FVP too.
> 
> Thanks for testing! Would you be happy for that to imply a tested-by tag, e.g.
> 
>   Tested-by: Itaru Kitayama <itaru.kitayama@linux.dev>
> 
> ... ?

If it's worth it, yes please.

Thanks,
Itaru.
> 
> Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] arm64: cleanup DAIF handling for EL0 returns

[Catalin Marinas]

On Tue, 06 Feb 2024 12:38:45 +0000, Mark Rutland wrote:
> These patches rework DAIF handling when returning to EL0, simplifiying
> the logic in do_notify_resume() and allowing Debug + SError to be
> unmasked for a little longer during exception return.
> 
> My primary motivation for these changes is to centralize entry/exit DAIF
> management within entry-common.c, as this will allow entry/exit
> sequences to be specialised, and make it much easier to add support for
> FEAT_NMI. With specialized entry DAIF management, it will also be
> possible to simplify local_daif_{save,restore}() and
> local_irq_{save,restore}().
> 
> [...]

Applied to arm64 (for-next/daif-cleanup), thanks!

[1/3] arm64: Simplify do_notify_resume() DAIF masking
      https://git.kernel.org/arm64/c/270de609ae2a
[2/3] arm64: Move do_notify_resume() to entry-common.c
      https://git.kernel.org/arm64/c/997d79eb938e
[3/3] arm64: Unmask Debug + SError in do_notify_resume()
      https://git.kernel.org/arm64/c/97d935faacde

-- 
Catalin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: Move do_notify_resume() to entry-common.c

[Liao, Chang]

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;
>  
>  /*

-- 
BR
Liao, Chang

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: Move do_notify_resume() to entry-common.c

[Mark Rutland]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel