[PATCH 1/2] powerpc/interrupt: Refactor interrupt_exit_user_prepare() and syscall_exit_prepare()

[PATCH 1/2] powerpc/interrupt: Refactor interrupt_exit_user_prepare() and syscall_exit_prepare()

[Christophe Leroy]

Last part of interrupt_exit_user_prepare() and syscall_exit_prepare()
are identical.

Create a __interrupt_exit_user_prepare() function that is called by
both.

Note that it replaces a local_irq_save(flags) by local_irq_disable().
This is similar because the flags are never used. On ppc 8xx it is
more efficient because it doesn't require reading MSR.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
It requires the following commits that are in powerpc/fixes-test:
5d510ed78bcf powerpc/syscall: Calling kuap_save_and_lock() is wrong
a78339698ab1 powerpc/interrupts: Fix kuep_unlock() call

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/interrupt.c | 147 ++++++++++----------------------
 1 file changed, 44 insertions(+), 103 deletions(-)

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index e0938ba298f2..d896fc6ed0be 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -231,56 +231,15 @@ static notrace void booke_load_dbcr0(void)
 #endif
 }
 
-/*
- * This should be called after a syscall returns, with r3 the return value
- * from the syscall. If this function returns non-zero, the system call
- * exit assembly should additionally load all GPR registers and CTR and XER
- * from the interrupt frame.
- *
- * The function graph tracer can not trace the return side of this function,
- * because RI=0 and soft mask state is "unreconciled", so it is marked notrace.
- */
-notrace unsigned long syscall_exit_prepare(unsigned long r3,
-					   struct pt_regs *regs,
-					   long scv)
+static notrace unsigned long __interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long ret,
+							   bool is_not_scv)
 {
 	unsigned long ti_flags;
-	unsigned long ret = 0;
-	bool is_not_scv = !IS_ENABLED(CONFIG_PPC_BOOK3S_64) || !scv;
 
 	CT_WARN_ON(ct_state() == CONTEXT_USER);
 
 	kuap_assert_locked();
 
-	regs->result = r3;
-
-	/* Check whether the syscall is issued inside a restartable sequence */
-	rseq_syscall(regs);
-
-	ti_flags = current_thread_info()->flags;
-
-	if (unlikely(r3 >= (unsigned long)-MAX_ERRNO) && is_not_scv) {
-		if (likely(!(ti_flags & (_TIF_NOERROR | _TIF_RESTOREALL)))) {
-			r3 = -r3;
-			regs->ccr |= 0x10000000; /* Set SO bit in CR */
-		}
-	}
-
-	if (unlikely(ti_flags & _TIF_PERSYSCALL_MASK)) {
-		if (ti_flags & _TIF_RESTOREALL)
-			ret = _TIF_RESTOREALL;
-		else
-			regs->gpr[3] = r3;
-		clear_bits(_TIF_PERSYSCALL_MASK, &current_thread_info()->flags);
-	} else {
-		regs->gpr[3] = r3;
-	}
-
-	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE)) {
-		do_syscall_trace_leave(regs);
-		ret |= _TIF_RESTOREALL;
-	}
-
 	local_irq_disable();
 
 again:
@@ -303,7 +262,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 		ti_flags = READ_ONCE(current_thread_info()->flags);
 	}
 
-	if (IS_ENABLED(CONFIG_PPC_BOOK3S) && IS_ENABLED(CONFIG_PPC_FPU)) {
+	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && IS_ENABLED(CONFIG_PPC_FPU)) {
 		if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) &&
 				unlikely((ti_flags & _TIF_RESTORE_TM))) {
 			restore_tm_state(regs);
@@ -352,81 +311,63 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 	return ret;
 }
 
-notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
+/*
+ * This should be called after a syscall returns, with r3 the return value
+ * from the syscall. If this function returns non-zero, the system call
+ * exit assembly should additionally load all GPR registers and CTR and XER
+ * from the interrupt frame.
+ *
+ * The function graph tracer can not trace the return side of this function,
+ * because RI=0 and soft mask state is "unreconciled", so it is marked notrace.
+ */
+notrace unsigned long syscall_exit_prepare(unsigned long r3,
+					   struct pt_regs *regs,
+					   long scv)
 {
 	unsigned long ti_flags;
-	unsigned long flags;
 	unsigned long ret = 0;
+	bool is_not_scv = !IS_ENABLED(CONFIG_PPC_BOOK3S_64) || !scv;
 
-	if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x))
-		BUG_ON(!(regs->msr & MSR_RI));
-	BUG_ON(!(regs->msr & MSR_PR));
-	BUG_ON(arch_irq_disabled_regs(regs));
-	CT_WARN_ON(ct_state() == CONTEXT_USER);
+	regs->result = r3;
 
-	/*
-	 * We don't need to restore AMR on the way back to userspace for KUAP.
-	 * AMR can only have been unlocked if we interrupted the kernel.
-	 */
-	kuap_assert_locked();
+	/* Check whether the syscall is issued inside a restartable sequence */
+	rseq_syscall(regs);
 
-	local_irq_save(flags);
+	ti_flags = current_thread_info()->flags;
 
-again:
-	ti_flags = READ_ONCE(current_thread_info()->flags);
-	while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
-		local_irq_enable(); /* returning to user: may enable */
-		if (ti_flags & _TIF_NEED_RESCHED) {
-			schedule();
-		} else {
-			if (ti_flags & _TIF_SIGPENDING)
-				ret |= _TIF_RESTOREALL;
-			do_notify_resume(regs, ti_flags);
+	if (unlikely(r3 >= (unsigned long)-MAX_ERRNO) && is_not_scv) {
+		if (likely(!(ti_flags & (_TIF_NOERROR | _TIF_RESTOREALL)))) {
+			r3 = -r3;
+			regs->ccr |= 0x10000000; /* Set SO bit in CR */
 		}
-		local_irq_disable();
-		ti_flags = READ_ONCE(current_thread_info()->flags);
 	}
 
-	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && IS_ENABLED(CONFIG_PPC_FPU)) {
-		if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) &&
-				unlikely((ti_flags & _TIF_RESTORE_TM))) {
-			restore_tm_state(regs);
-		} else {
-			unsigned long mathflags = MSR_FP;
-
-			if (cpu_has_feature(CPU_FTR_VSX))
-				mathflags |= MSR_VEC | MSR_VSX;
-			else if (cpu_has_feature(CPU_FTR_ALTIVEC))
-				mathflags |= MSR_VEC;
-
-			/* See above restore_math comment */
-			if ((regs->msr & mathflags) != mathflags)
-				restore_math(regs);
-		}
+	if (unlikely(ti_flags & _TIF_PERSYSCALL_MASK)) {
+		if (ti_flags & _TIF_RESTOREALL)
+			ret = _TIF_RESTOREALL;
+		else
+			regs->gpr[3] = r3;
+		clear_bits(_TIF_PERSYSCALL_MASK, &current_thread_info()->flags);
+	} else {
+		regs->gpr[3] = r3;
 	}
 
-	user_enter_irqoff();
-
-	if (unlikely(!__prep_irq_for_enabled_exit(true))) {
-		user_exit_irqoff();
-		local_irq_enable();
-		local_irq_disable();
-		goto again;
+	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE)) {
+		do_syscall_trace_leave(regs);
+		ret |= _TIF_RESTOREALL;
 	}
 
-	booke_load_dbcr0();
-
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	local_paca->tm_scratch = regs->msr;
-#endif
-
-	account_cpu_user_exit();
+	return __interrupt_exit_user_prepare(regs, ret, is_not_scv);
+}
 
-	/* Restore user access locks last */
-	kuap_user_restore(regs);
-	kuep_unlock();
+notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
+{
+	if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x))
+		BUG_ON(!(regs->msr & MSR_RI));
+	BUG_ON(!(regs->msr & MSR_PR));
+	BUG_ON(arch_irq_disabled_regs(regs));
 
-	return ret;
+	return __interrupt_exit_user_prepare(regs, 0, true);
 }
 
 void preempt_schedule_irq(void);
-- 
2.25.0

[PATCH 2/2] powerpc/interrupt: Use msr instead of regs->msr

[Christophe Leroy]

interrupt_exit_user_prepare() and interrupt_exit_kernel_prepare()
get msr as second parameter from ASM. Use it instead of reading
it again.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/interrupt.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index d896fc6ed0be..9541328a97b1 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -231,8 +231,8 @@ static notrace void booke_load_dbcr0(void)
 #endif
 }
 
-static notrace unsigned long __interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long ret,
-							   bool is_not_scv)
+static notrace unsigned long __interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr,
+							   unsigned long ret, bool is_not_scv)
 {
 	unsigned long ti_flags;
 
@@ -281,7 +281,7 @@ static notrace unsigned long __interrupt_exit_user_prepare(struct pt_regs *regs,
 			 * may decide to restore them (to avoid taking an FP
 			 * fault).
 			 */
-			if ((regs->msr & mathflags) != mathflags)
+			if ((msr & mathflags) != mathflags)
 				restore_math(regs);
 		}
 	}
@@ -297,7 +297,7 @@ static notrace unsigned long __interrupt_exit_user_prepare(struct pt_regs *regs,
 	}
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	local_paca->tm_scratch = regs->msr;
+	local_paca->tm_scratch = msr;
 #endif
 
 	booke_load_dbcr0();
@@ -357,17 +357,17 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 		ret |= _TIF_RESTOREALL;
 	}
 
-	return __interrupt_exit_user_prepare(regs, ret, is_not_scv);
+	return __interrupt_exit_user_prepare(regs, regs->msr, ret, is_not_scv);
 }
 
 notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
 {
 	if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x))
-		BUG_ON(!(regs->msr & MSR_RI));
-	BUG_ON(!(regs->msr & MSR_PR));
+		BUG_ON(!(msr & MSR_RI));
+	BUG_ON(!(msr & MSR_PR));
 	BUG_ON(arch_irq_disabled_regs(regs));
 
-	return __interrupt_exit_user_prepare(regs, 0, true);
+	return __interrupt_exit_user_prepare(regs, msr, 0, true);
 }
 
 void preempt_schedule_irq(void);
@@ -379,9 +379,9 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
 	unsigned long kuap;
 
 	if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x) &&
-	    unlikely(!(regs->msr & MSR_RI)))
+	    unlikely(!(msr & MSR_RI)))
 		unrecoverable_exception(regs);
-	BUG_ON(regs->msr & MSR_PR);
+	BUG_ON(msr & MSR_PR);
 	/*
 	 * CT_WARN_ON comes here via program_check_exception,
 	 * so avoid recursion.
@@ -400,7 +400,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
 
 	if (!arch_irq_disabled_regs(regs)) {
 		/* Returning to a kernel context with local irqs enabled. */
-		WARN_ON_ONCE(!(regs->msr & MSR_EE));
+		WARN_ON_ONCE(!(msr & MSR_EE));
 again:
 		if (IS_ENABLED(CONFIG_PREEMPT)) {
 			/* Return to preemptible kernel context */
@@ -416,14 +416,14 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
 		/* Returning to a kernel context with local irqs disabled. */
 		__hard_EE_RI_disable();
 #ifdef CONFIG_PPC64
-		if (regs->msr & MSR_EE)
+		if (msr & MSR_EE)
 			local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
 #endif
 	}
 
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	local_paca->tm_scratch = regs->msr;
+	local_paca->tm_scratch = msr;
 #endif
 
 	/*
-- 
2.25.0

Re: [PATCH 1/2] powerpc/interrupt: Refactor interrupt_exit_user_prepare() and syscall_exit_prepare()

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of May 14, 2021 6:28 pm:
> Last part of interrupt_exit_user_prepare() and syscall_exit_prepare()
> are identical.
> 
> Create a __interrupt_exit_user_prepare() function that is called by
> both.
> 
> Note that it replaces a local_irq_save(flags) by local_irq_disable().
> This is similar because the flags are never used. On ppc 8xx it is
> more efficient because it doesn't require reading MSR.

Can these cleanups go after my interrupt performance improvements?
I posted them for last series but were dropped due to crashes without
time to resubmit. I'm working on them again now.

Thanks,
Nick

Re: [PATCH 1/2] powerpc/interrupt: Refactor interrupt_exit_user_prepare() and syscall_exit_prepare()

[Christophe Leroy]

Le 17/05/2021 à 09:44, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of May 14, 2021 6:28 pm:
>> Last part of interrupt_exit_user_prepare() and syscall_exit_prepare()
>> are identical.
>>
>> Create a __interrupt_exit_user_prepare() function that is called by
>> both.
>>
>> Note that it replaces a local_irq_save(flags) by local_irq_disable().
>> This is similar because the flags are never used. On ppc 8xx it is
>> more efficient because it doesn't require reading MSR.
> 
> Can these cleanups go after my interrupt performance improvements?
> I posted them for last series but were dropped due to crashes without
> time to resubmit. I'm working on them again now.
> 

Euh ... ok why not, but at the time being interrupt_exit_user_prepare() and syscall_exit_prepare() 
are very similar. Which makes sense because both of them are returning from kernel to user so they 
are to do the same preparation.

If you are doing the same changes to both of them, maybe it is worst including this refactor at the 
begining of your series. Or are you making them diverge with that series ?

Christophe