linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit()
@ 2021-06-04 14:56 Christophe Leroy
  2021-06-04 14:56 ` [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit() Christophe Leroy
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Christophe Leroy @ 2021-06-04 14:56 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
  Cc: linux-kernel, linuxppc-dev

prep_irq_for_user_exit() is a superset of
prep_irq_for_kernel_enabled_exit(). In order to allow refactoring in
following patch, interchange the two as prep_irq_for_user_exit() will
call prep_irq_for_kernel_enabled_exit().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
This series applies on top of Nic's series to speed up interrupt return on 64s

 arch/powerpc/kernel/interrupt.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index 74c995a42399..539455c62c5b 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -40,33 +40,27 @@ static inline bool exit_must_hard_disable(void)
 #endif
 
 /*
- * local irqs must be disabled. Returns false if the caller must re-enable
- * them, check for new work, and try again.
- *
- * This should be called with local irqs disabled, but if they were previously
- * enabled when the interrupt handler returns (indicating a process-context /
- * synchronous interrupt) then irqs_enabled should be true.
+ * restartable is true then EE/RI can be left on because interrupts are handled
+ * with a restart sequence.
  */
-static notrace __always_inline bool prep_irq_for_user_exit(void)
+static notrace __always_inline bool prep_irq_for_kernel_enabled_exit(bool restartable)
 {
-	user_enter_irqoff();
 	/* This must be done with RI=1 because tracing may touch vmaps */
 	trace_hardirqs_on();
 
 #ifdef CONFIG_PPC32
 	__hard_EE_RI_disable();
 #else
-	if (exit_must_hard_disable())
+	if (exit_must_hard_disable() || !restartable)
 		__hard_EE_RI_disable();
 
 	/* This pattern matches prep_irq_for_idle */
 	if (unlikely(lazy_irq_pending_nocheck())) {
-		if (exit_must_hard_disable()) {
+		if (exit_must_hard_disable() || !restartable) {
 			local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
 			__hard_RI_enable();
 		}
 		trace_hardirqs_off();
-		user_exit_irqoff();
 
 		return false;
 	}
@@ -75,27 +69,33 @@ static notrace __always_inline bool prep_irq_for_user_exit(void)
 }
 
 /*
- * restartable is true then EE/RI can be left on because interrupts are handled
- * with a restart sequence.
+ * local irqs must be disabled. Returns false if the caller must re-enable
+ * them, check for new work, and try again.
+ *
+ * This should be called with local irqs disabled, but if they were previously
+ * enabled when the interrupt handler returns (indicating a process-context /
+ * synchronous interrupt) then irqs_enabled should be true.
  */
-static notrace __always_inline bool prep_irq_for_kernel_enabled_exit(bool restartable)
+static notrace __always_inline bool prep_irq_for_user_exit(void)
 {
+	user_enter_irqoff();
 	/* This must be done with RI=1 because tracing may touch vmaps */
 	trace_hardirqs_on();
 
 #ifdef CONFIG_PPC32
 	__hard_EE_RI_disable();
 #else
-	if (exit_must_hard_disable() || !restartable)
+	if (exit_must_hard_disable())
 		__hard_EE_RI_disable();
 
 	/* This pattern matches prep_irq_for_idle */
 	if (unlikely(lazy_irq_pending_nocheck())) {
-		if (exit_must_hard_disable() || !restartable) {
+		if (exit_must_hard_disable()) {
 			local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
 			__hard_RI_enable();
 		}
 		trace_hardirqs_off();
+		user_exit_irqoff();
 
 		return false;
 	}
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit()
  2021-06-04 14:56 [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit() Christophe Leroy
@ 2021-06-04 14:56 ` Christophe Leroy
  2021-06-11  2:30   ` Nicholas Piggin
  2021-06-04 14:56 ` [PATCH v2 3/4] powerpc/interrupt: Rename and lightly change syscall_exit_prepare_main() Christophe Leroy
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Christophe Leroy @ 2021-06-04 14:56 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
  Cc: linux-kernel, linuxppc-dev

prep_irq_for_user_exit() is a superset of
prep_irq_for_kernel_enabled_exit().

Refactor it.

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

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index 539455c62c5b..b6aa80930733 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -78,29 +78,14 @@ static notrace __always_inline bool prep_irq_for_kernel_enabled_exit(bool restar
  */
 static notrace __always_inline bool prep_irq_for_user_exit(void)
 {
-	user_enter_irqoff();
-	/* This must be done with RI=1 because tracing may touch vmaps */
-	trace_hardirqs_on();
-
-#ifdef CONFIG_PPC32
-	__hard_EE_RI_disable();
-#else
-	if (exit_must_hard_disable())
-		__hard_EE_RI_disable();
+	bool ret;
 
-	/* This pattern matches prep_irq_for_idle */
-	if (unlikely(lazy_irq_pending_nocheck())) {
-		if (exit_must_hard_disable()) {
-			local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
-			__hard_RI_enable();
-		}
-		trace_hardirqs_off();
+	user_enter_irqoff();
+	ret = prep_irq_for_kernel_enabled_exit(true);
+	if (!ret)
 		user_exit_irqoff();
 
-		return false;
-	}
-#endif
-	return true;
+	return ret;
 }
 
 /* Has to run notrace because it is entered not completely "reconciled" */
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/4] powerpc/interrupt: Rename and lightly change syscall_exit_prepare_main()
  2021-06-04 14:56 [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit() Christophe Leroy
  2021-06-04 14:56 ` [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit() Christophe Leroy
@ 2021-06-04 14:56 ` Christophe Leroy
  2021-06-11  2:32   ` Nicholas Piggin
  2021-06-04 14:56 ` [PATCH v2 4/4] powerpc/interrupt: Refactor interrupt_exit_user_prepare() Christophe Leroy
  2021-06-11  2:26 ` [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit() Nicholas Piggin
  3 siblings, 1 reply; 9+ messages in thread
From: Christophe Leroy @ 2021-06-04 14:56 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
  Cc: linux-kernel, linuxppc-dev

Rename syscall_exit_prepare_main() into interrupt_exit_prepare_main()

Make it static as it is not used anywhere else.

Pass it the 'ret' so that it can 'or' it directly instead of
oring twice, once inside the function and once outside.

And remove 'r3' parameter which is not used.

Also fix a typo where CONFIG_PPC_BOOK3S should be CONFIG_PPC_BOOK3S_64.

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

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index b6aa80930733..bc3c1892ed80 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -228,11 +228,10 @@ static notrace void booke_load_dbcr0(void)
 #endif
 }
 
-notrace unsigned long syscall_exit_prepare_main(unsigned long r3,
-						struct pt_regs *regs)
+static notrace unsigned long
+interrupt_exit_user_prepare_main(struct pt_regs *regs, unsigned long ret)
 {
 	unsigned long ti_flags;
-	unsigned long ret = 0;
 
 again:
 	ti_flags = READ_ONCE(current_thread_info()->flags);
@@ -254,7 +253,7 @@ notrace unsigned long syscall_exit_prepare_main(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);
@@ -350,7 +349,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 	}
 
 	local_irq_disable();
-	ret |= syscall_exit_prepare_main(r3, regs);
+	ret = interrupt_exit_user_prepare_main(regs, ret);
 
 #ifdef CONFIG_PPC64
 	regs->exit_result = ret;
@@ -378,7 +377,7 @@ notrace unsigned long syscall_exit_restart(unsigned long r3, struct pt_regs *reg
 
 	BUG_ON(!user_mode(regs));
 
-	regs->exit_result |= syscall_exit_prepare_main(r3, regs);
+	regs->exit_result = interrupt_exit_user_prepare_main(regs, regs->exit_result);
 
 	return regs->exit_result;
 }
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 4/4] powerpc/interrupt: Refactor interrupt_exit_user_prepare()
  2021-06-04 14:56 [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit() Christophe Leroy
  2021-06-04 14:56 ` [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit() Christophe Leroy
  2021-06-04 14:56 ` [PATCH v2 3/4] powerpc/interrupt: Rename and lightly change syscall_exit_prepare_main() Christophe Leroy
@ 2021-06-04 14:56 ` Christophe Leroy
  2021-06-11  2:26 ` [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit() Nicholas Piggin
  3 siblings, 0 replies; 9+ messages in thread
From: Christophe Leroy @ 2021-06-04 14:56 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
  Cc: linux-kernel, linuxppc-dev

interrupt_exit_user_prepare() is a superset of
interrupt_exit_user_prepare_main().

Refactor to avoid code duplication.

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

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index bc3c1892ed80..f5d30323028e 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -385,9 +385,7 @@ notrace unsigned long syscall_exit_restart(unsigned long r3, struct pt_regs *reg
 
 notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs)
 {
-	unsigned long ti_flags;
-	unsigned long flags;
-	unsigned long ret = 0;
+	unsigned long ret;
 
 	if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x))
 		BUG_ON(!(regs->msr & MSR_RI));
@@ -401,63 +399,14 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs)
 	 */
 	kuap_assert_locked();
 
-	local_irq_save(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);
-		}
-		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 (!prep_irq_for_user_exit()) {
-		local_irq_enable();
-		local_irq_disable();
-		goto again;
-	}
-
-	booke_load_dbcr0();
-
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	local_paca->tm_scratch = regs->msr;
-#endif
+	local_irq_disable();
 
-	account_cpu_user_exit();
+	ret = interrupt_exit_user_prepare_main(regs, 0);
 
 #ifdef CONFIG_PPC64
 	regs->exit_result = ret;
 #endif
 
-	/* Restore user access locks last */
-	kuap_user_restore(regs);
-	kuep_unlock();
-
 	return ret;
 }
 
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit()
  2021-06-04 14:56 [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit() Christophe Leroy
                   ` (2 preceding siblings ...)
  2021-06-04 14:56 ` [PATCH v2 4/4] powerpc/interrupt: Refactor interrupt_exit_user_prepare() Christophe Leroy
@ 2021-06-11  2:26 ` Nicholas Piggin
  3 siblings, 0 replies; 9+ messages in thread
From: Nicholas Piggin @ 2021-06-11  2:26 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
	Paul Mackerras
  Cc: linux-kernel, linuxppc-dev

Excerpts from Christophe Leroy's message of June 5, 2021 12:56 am:
> prep_irq_for_user_exit() is a superset of
> prep_irq_for_kernel_enabled_exit(). In order to allow refactoring in
> following patch, interchange the two as prep_irq_for_user_exit() will
> call prep_irq_for_kernel_enabled_exit().
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> This series applies on top of Nic's series to speed up interrupt return on 64s

Thanks for rebasing it.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> 
>  arch/powerpc/kernel/interrupt.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index 74c995a42399..539455c62c5b 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -40,33 +40,27 @@ static inline bool exit_must_hard_disable(void)
>  #endif
>  
>  /*
> - * local irqs must be disabled. Returns false if the caller must re-enable
> - * them, check for new work, and try again.
> - *
> - * This should be called with local irqs disabled, but if they were previously
> - * enabled when the interrupt handler returns (indicating a process-context /
> - * synchronous interrupt) then irqs_enabled should be true.
> + * restartable is true then EE/RI can be left on because interrupts are handled
> + * with a restart sequence.
>   */
> -static notrace __always_inline bool prep_irq_for_user_exit(void)
> +static notrace __always_inline bool prep_irq_for_kernel_enabled_exit(bool restartable)
>  {
> -	user_enter_irqoff();
>  	/* This must be done with RI=1 because tracing may touch vmaps */
>  	trace_hardirqs_on();
>  
>  #ifdef CONFIG_PPC32
>  	__hard_EE_RI_disable();
>  #else
> -	if (exit_must_hard_disable())
> +	if (exit_must_hard_disable() || !restartable)
>  		__hard_EE_RI_disable();
>  
>  	/* This pattern matches prep_irq_for_idle */
>  	if (unlikely(lazy_irq_pending_nocheck())) {
> -		if (exit_must_hard_disable()) {
> +		if (exit_must_hard_disable() || !restartable) {
>  			local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
>  			__hard_RI_enable();
>  		}
>  		trace_hardirqs_off();
> -		user_exit_irqoff();
>  
>  		return false;
>  	}
> @@ -75,27 +69,33 @@ static notrace __always_inline bool prep_irq_for_user_exit(void)
>  }
>  
>  /*
> - * restartable is true then EE/RI can be left on because interrupts are handled
> - * with a restart sequence.
> + * local irqs must be disabled. Returns false if the caller must re-enable
> + * them, check for new work, and try again.
> + *
> + * This should be called with local irqs disabled, but if they were previously
> + * enabled when the interrupt handler returns (indicating a process-context /
> + * synchronous interrupt) then irqs_enabled should be true.
>   */
> -static notrace __always_inline bool prep_irq_for_kernel_enabled_exit(bool restartable)
> +static notrace __always_inline bool prep_irq_for_user_exit(void)
>  {
> +	user_enter_irqoff();
>  	/* This must be done with RI=1 because tracing may touch vmaps */
>  	trace_hardirqs_on();
>  
>  #ifdef CONFIG_PPC32
>  	__hard_EE_RI_disable();
>  #else
> -	if (exit_must_hard_disable() || !restartable)
> +	if (exit_must_hard_disable())
>  		__hard_EE_RI_disable();
>  
>  	/* This pattern matches prep_irq_for_idle */
>  	if (unlikely(lazy_irq_pending_nocheck())) {
> -		if (exit_must_hard_disable() || !restartable) {
> +		if (exit_must_hard_disable()) {
>  			local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
>  			__hard_RI_enable();
>  		}
>  		trace_hardirqs_off();
> +		user_exit_irqoff();
>  
>  		return false;
>  	}
> -- 
> 2.25.0
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit()
  2021-06-04 14:56 ` [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit() Christophe Leroy
@ 2021-06-11  2:30   ` Nicholas Piggin
  2021-06-15  8:37     ` Christophe Leroy
  0 siblings, 1 reply; 9+ messages in thread
From: Nicholas Piggin @ 2021-06-11  2:30 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
	Paul Mackerras
  Cc: linux-kernel, linuxppc-dev

Excerpts from Christophe Leroy's message of June 5, 2021 12:56 am:
> prep_irq_for_user_exit() is a superset of
> prep_irq_for_kernel_enabled_exit().
> 
> Refactor it.

I like the refactoring, but now prep_irq_for_user_exit() is calling 
prep_irq_for_kernel_enabled_exit(), which seems like the wrong naming.

You could re-name prep_irq_for_kernel_enabled_exit() to
prep_irq_for_enabled_exit() maybe? Or it could be 
__prep_irq_for_enabled_exit() then prep_irq_for_kernel_enabled_exit()
and prep_irq_for_user_exit() would both call it.

Otherwise

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/interrupt.c | 25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index 539455c62c5b..b6aa80930733 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -78,29 +78,14 @@ static notrace __always_inline bool prep_irq_for_kernel_enabled_exit(bool restar
>   */
>  static notrace __always_inline bool prep_irq_for_user_exit(void)
>  {
> -	user_enter_irqoff();
> -	/* This must be done with RI=1 because tracing may touch vmaps */
> -	trace_hardirqs_on();
> -
> -#ifdef CONFIG_PPC32
> -	__hard_EE_RI_disable();
> -#else
> -	if (exit_must_hard_disable())
> -		__hard_EE_RI_disable();
> +	bool ret;
>  
> -	/* This pattern matches prep_irq_for_idle */
> -	if (unlikely(lazy_irq_pending_nocheck())) {
> -		if (exit_must_hard_disable()) {
> -			local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
> -			__hard_RI_enable();
> -		}
> -		trace_hardirqs_off();
> +	user_enter_irqoff();
> +	ret = prep_irq_for_kernel_enabled_exit(true);
> +	if (!ret)
>  		user_exit_irqoff();
>  
> -		return false;
> -	}
> -#endif
> -	return true;
> +	return ret;
>  }
>  
>  /* Has to run notrace because it is entered not completely "reconciled" */
> -- 
> 2.25.0
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/4] powerpc/interrupt: Rename and lightly change syscall_exit_prepare_main()
  2021-06-04 14:56 ` [PATCH v2 3/4] powerpc/interrupt: Rename and lightly change syscall_exit_prepare_main() Christophe Leroy
@ 2021-06-11  2:32   ` Nicholas Piggin
  0 siblings, 0 replies; 9+ messages in thread
From: Nicholas Piggin @ 2021-06-11  2:32 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
	Paul Mackerras
  Cc: linux-kernel, linuxppc-dev

Excerpts from Christophe Leroy's message of June 5, 2021 12:56 am:
> Rename syscall_exit_prepare_main() into interrupt_exit_prepare_main()
> 
> Make it static as it is not used anywhere else.
> 
> Pass it the 'ret' so that it can 'or' it directly instead of
> oring twice, once inside the function and once outside.
> 
> And remove 'r3' parameter which is not used.
> 
> Also fix a typo where CONFIG_PPC_BOOK3S should be CONFIG_PPC_BOOK3S_64.

This all looks good I think. I need to grab this fix from your series.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/interrupt.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index b6aa80930733..bc3c1892ed80 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -228,11 +228,10 @@ static notrace void booke_load_dbcr0(void)
>  #endif
>  }
>  
> -notrace unsigned long syscall_exit_prepare_main(unsigned long r3,
> -						struct pt_regs *regs)
> +static notrace unsigned long
> +interrupt_exit_user_prepare_main(struct pt_regs *regs, unsigned long ret)
>  {
>  	unsigned long ti_flags;
> -	unsigned long ret = 0;
>  
>  again:
>  	ti_flags = READ_ONCE(current_thread_info()->flags);
> @@ -254,7 +253,7 @@ notrace unsigned long syscall_exit_prepare_main(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);
> @@ -350,7 +349,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>  	}
>  
>  	local_irq_disable();
> -	ret |= syscall_exit_prepare_main(r3, regs);
> +	ret = interrupt_exit_user_prepare_main(regs, ret);
>  
>  #ifdef CONFIG_PPC64
>  	regs->exit_result = ret;
> @@ -378,7 +377,7 @@ notrace unsigned long syscall_exit_restart(unsigned long r3, struct pt_regs *reg
>  
>  	BUG_ON(!user_mode(regs));
>  
> -	regs->exit_result |= syscall_exit_prepare_main(r3, regs);
> +	regs->exit_result = interrupt_exit_user_prepare_main(regs, regs->exit_result);
>  
>  	return regs->exit_result;
>  }
> -- 
> 2.25.0
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit()
  2021-06-11  2:30   ` Nicholas Piggin
@ 2021-06-15  8:37     ` Christophe Leroy
  2021-06-17  3:33       ` Nicholas Piggin
  0 siblings, 1 reply; 9+ messages in thread
From: Christophe Leroy @ 2021-06-15  8:37 UTC (permalink / raw)
  To: Nicholas Piggin, Benjamin Herrenschmidt, Michael Ellerman,
	Paul Mackerras
  Cc: linux-kernel, linuxppc-dev



Le 11/06/2021 à 04:30, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of June 5, 2021 12:56 am:
>> prep_irq_for_user_exit() is a superset of
>> prep_irq_for_kernel_enabled_exit().
>>
>> Refactor it.
> 
> I like the refactoring, but now prep_irq_for_user_exit() is calling
> prep_irq_for_kernel_enabled_exit(), which seems like the wrong naming.
> 
> You could re-name prep_irq_for_kernel_enabled_exit() to
> prep_irq_for_enabled_exit() maybe? Or it could be
> __prep_irq_for_enabled_exit() then prep_irq_for_kernel_enabled_exit()
> and prep_irq_for_user_exit() would both call it.

I renamed it prep_irq_for_enabled_exit().

And I realised that after patch 4, prep_irq_for_enabled_exit() has become a trivial function used 
only once.

So I swapped patches 1/2 with patches 3/4 and added a 5th one to squash prep_irq_for_enabled_exit() 
into its caller.

You didn't have any comment on patch 4 (that is now patch 2) ?

Thanks for the review
Christophe

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit()
  2021-06-15  8:37     ` Christophe Leroy
@ 2021-06-17  3:33       ` Nicholas Piggin
  0 siblings, 0 replies; 9+ messages in thread
From: Nicholas Piggin @ 2021-06-17  3:33 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
	Paul Mackerras
  Cc: linux-kernel, linuxppc-dev

Excerpts from Christophe Leroy's message of June 15, 2021 6:37 pm:
> 
> 
> Le 11/06/2021 à 04:30, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of June 5, 2021 12:56 am:
>>> prep_irq_for_user_exit() is a superset of
>>> prep_irq_for_kernel_enabled_exit().
>>>
>>> Refactor it.
>> 
>> I like the refactoring, but now prep_irq_for_user_exit() is calling
>> prep_irq_for_kernel_enabled_exit(), which seems like the wrong naming.
>> 
>> You could re-name prep_irq_for_kernel_enabled_exit() to
>> prep_irq_for_enabled_exit() maybe? Or it could be
>> __prep_irq_for_enabled_exit() then prep_irq_for_kernel_enabled_exit()
>> and prep_irq_for_user_exit() would both call it.
> 
> I renamed it prep_irq_for_enabled_exit().
> 
> And I realised that after patch 4, prep_irq_for_enabled_exit() has become a trivial function used 
> only once.
> 
> So I swapped patches 1/2 with patches 3/4 and added a 5th one to squash prep_irq_for_enabled_exit() 
> into its caller.
> 
> You didn't have any comment on patch 4 (that is now patch 2) ?

I think it's okay, just trying to hunt down some apparent big-endian bug 
with my series. I can't see any problems with yours though, thanks for
rebasing them, I'll take a better look when I can.

Thanks,
Nick

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-17  3:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-04 14:56 [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit() Christophe Leroy
2021-06-04 14:56 ` [PATCH v2 2/4] powerpc/interrupt: Refactor prep_irq_for_user_exit() Christophe Leroy
2021-06-11  2:30   ` Nicholas Piggin
2021-06-15  8:37     ` Christophe Leroy
2021-06-17  3:33       ` Nicholas Piggin
2021-06-04 14:56 ` [PATCH v2 3/4] powerpc/interrupt: Rename and lightly change syscall_exit_prepare_main() Christophe Leroy
2021-06-11  2:32   ` Nicholas Piggin
2021-06-04 14:56 ` [PATCH v2 4/4] powerpc/interrupt: Refactor interrupt_exit_user_prepare() Christophe Leroy
2021-06-11  2:26 ` [PATCH v2 1/4] powerpc/interrupt: Interchange prep_irq_for_{kernel_enabled/user}_exit() Nicholas Piggin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).