LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] powerpc/64s: power4 nap fixup in C
@ 2021-03-12  1:20 Nicholas Piggin
  2021-03-16  7:16 ` Christophe Leroy
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Nicholas Piggin @ 2021-03-12  1:20 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Nicholas Piggin

There is no need for this to be in asm, use the new intrrupt entry wrapper.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Hopefully this works on a real G5 now, but I couldn't reproduce the
problem with QEMU.

Thanks,
Nick

 arch/powerpc/include/asm/interrupt.h   | 19 +++++++++++
 arch/powerpc/include/asm/processor.h   |  1 +
 arch/powerpc/include/asm/thread_info.h |  6 ++++
 arch/powerpc/kernel/exceptions-64s.S   | 45 --------------------------
 arch/powerpc/kernel/idle_book3s.S      |  4 +++
 5 files changed, 30 insertions(+), 45 deletions(-)

diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index aedfba29e43a..ef015d3b5e39 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -9,6 +9,17 @@
 #include <asm/kprobes.h>
 #include <asm/runlatch.h>
 
+static inline void nap_adjust_return(struct pt_regs *regs)
+{
+#ifdef CONFIG_PPC_970_NAP
+	if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
+		/* Can avoid a test-and-clear because NMIs do not call this */
+		clear_thread_local_flags(_TLF_NAPPING);
+		regs->nip = (unsigned long)power4_idle_nap_return;
+	}
+#endif
+}
+
 struct interrupt_state {
 #ifdef CONFIG_PPC_BOOK3E_64
 	enum ctx_state ctx_state;
@@ -111,6 +122,9 @@ static inline void interrupt_async_exit_prepare(struct pt_regs *regs, struct int
 {
 	irq_exit();
 	interrupt_exit_prepare(regs, state);
+
+	/* Adjust at exit so the main handler sees the true NIA */
+	nap_adjust_return(regs);
 }
 
 struct interrupt_nmi_state {
@@ -164,6 +178,11 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
 			radix_enabled() || (mfmsr() & MSR_DR))
 		nmi_exit();
 
+	/*
+	 * nmi does not call nap_adjust_return because nmi should not create
+	 * new work to do (must use irq_work for that).
+	 */
+
 #ifdef CONFIG_PPC64
 	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
 		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 8acc3590c971..eedc3c775141 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -393,6 +393,7 @@ extern unsigned long isa300_idle_stop_mayloss(unsigned long psscr_val);
 extern unsigned long isa206_idle_insn_mayloss(unsigned long type);
 #ifdef CONFIG_PPC_970_NAP
 extern void power4_idle_nap(void);
+void power4_idle_nap_return(void);
 #endif
 
 extern unsigned long cpuidle_disable;
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 386d576673a1..bf137151100b 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -152,6 +152,12 @@ void arch_setup_new_exec(void);
 
 #ifndef __ASSEMBLY__
 
+static inline void clear_thread_local_flags(unsigned int flags)
+{
+	struct thread_info *ti = current_thread_info();
+	ti->local_flags &= ~flags;
+}
+
 static inline bool test_thread_local_flags(unsigned int flags)
 {
 	struct thread_info *ti = current_thread_info();
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 60d3051a8bc8..ea7a443488d2 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -692,25 +692,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
 	ld	r1,GPR1(r1)
 .endm
 
-/*
- * When the idle code in power4_idle puts the CPU into NAP mode,
- * it has to do so in a loop, and relies on the external interrupt
- * and decrementer interrupt entry code to get it out of the loop.
- * It sets the _TLF_NAPPING bit in current_thread_info()->local_flags
- * to signal that it is in the loop and needs help to get out.
- */
-#ifdef CONFIG_PPC_970_NAP
-#define FINISH_NAP				\
-BEGIN_FTR_SECTION				\
-	ld	r11, PACA_THREAD_INFO(r13);	\
-	ld	r9,TI_LOCAL_FLAGS(r11);		\
-	andi.	r10,r9,_TLF_NAPPING;		\
-	bnel	power4_fixup_nap;		\
-END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
-#else
-#define FINISH_NAP
-#endif
-
 /*
  * There are a few constraints to be concerned with.
  * - Real mode exceptions code/data must be located at their physical location.
@@ -1248,7 +1229,6 @@ EXC_COMMON_BEGIN(machine_check_common)
 	 */
 	GEN_COMMON machine_check
 
-	FINISH_NAP
 	/* Enable MSR_RI when finished with PACA_EXMC */
 	li	r10,MSR_RI
 	mtmsrd 	r10,1
@@ -1571,7 +1551,6 @@ EXC_VIRT_BEGIN(hardware_interrupt, 0x4500, 0x100)
 EXC_VIRT_END(hardware_interrupt, 0x4500, 0x100)
 EXC_COMMON_BEGIN(hardware_interrupt_common)
 	GEN_COMMON hardware_interrupt
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_IRQ
 	b	interrupt_return
@@ -1801,7 +1780,6 @@ EXC_VIRT_BEGIN(decrementer, 0x4900, 0x80)
 EXC_VIRT_END(decrementer, 0x4900, 0x80)
 EXC_COMMON_BEGIN(decrementer_common)
 	GEN_COMMON decrementer
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	timer_interrupt
 	b	interrupt_return
@@ -1886,7 +1864,6 @@ EXC_VIRT_BEGIN(doorbell_super, 0x4a00, 0x100)
 EXC_VIRT_END(doorbell_super, 0x4a00, 0x100)
 EXC_COMMON_BEGIN(doorbell_super_common)
 	GEN_COMMON doorbell_super
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 #ifdef CONFIG_PPC_DOORBELL
 	bl	doorbell_exception
@@ -2237,7 +2214,6 @@ EXC_COMMON_BEGIN(hmi_exception_early_common)
 
 EXC_COMMON_BEGIN(hmi_exception_common)
 	GEN_COMMON hmi_exception
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	handle_hmi_exception
 	b	interrupt_return
@@ -2266,7 +2242,6 @@ EXC_VIRT_BEGIN(h_doorbell, 0x4e80, 0x20)
 EXC_VIRT_END(h_doorbell, 0x4e80, 0x20)
 EXC_COMMON_BEGIN(h_doorbell_common)
 	GEN_COMMON h_doorbell
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 #ifdef CONFIG_PPC_DOORBELL
 	bl	doorbell_exception
@@ -2299,7 +2274,6 @@ EXC_VIRT_BEGIN(h_virt_irq, 0x4ea0, 0x20)
 EXC_VIRT_END(h_virt_irq, 0x4ea0, 0x20)
 EXC_COMMON_BEGIN(h_virt_irq_common)
 	GEN_COMMON h_virt_irq
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_IRQ
 	b	interrupt_return
@@ -2345,7 +2319,6 @@ EXC_VIRT_BEGIN(performance_monitor, 0x4f00, 0x20)
 EXC_VIRT_END(performance_monitor, 0x4f00, 0x20)
 EXC_COMMON_BEGIN(performance_monitor_common)
 	GEN_COMMON performance_monitor
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	performance_monitor_exception
 	b	interrupt_return
@@ -3096,24 +3069,6 @@ USE_FIXED_SECTION(virt_trampolines)
 __end_interrupts:
 DEFINE_FIXED_SYMBOL(__end_interrupts)
 
-#ifdef CONFIG_PPC_970_NAP
-	/*
-	 * Called by exception entry code if _TLF_NAPPING was set, this clears
-	 * the NAPPING flag, and redirects the exception exit to
-	 * power4_fixup_nap_return.
-	 */
-	.globl power4_fixup_nap
-EXC_COMMON_BEGIN(power4_fixup_nap)
-	andc	r9,r9,r10
-	std	r9,TI_LOCAL_FLAGS(r11)
-	LOAD_REG_ADDR(r10, power4_idle_nap_return)
-	std	r10,_NIP(r1)
-	blr
-
-power4_idle_nap_return:
-	blr
-#endif
-
 CLOSE_FIXED_SECTION(real_vectors);
 CLOSE_FIXED_SECTION(real_trampolines);
 CLOSE_FIXED_SECTION(virt_vectors);
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index f9e6d83e6720..abb719b21cae 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
 	mtmsrd	r7
 	isync
 	b	1b
+
+	.globl power4_idle_nap_return
+power4_idle_nap_return:
+	blr
 #endif
-- 
2.23.0


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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-03-12  1:20 [PATCH] powerpc/64s: power4 nap fixup in C Nicholas Piggin
@ 2021-03-16  7:16 ` Christophe Leroy
  2021-03-16  8:11   ` Nicholas Piggin
  2021-03-29  4:40 ` Michael Ellerman
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Christophe Leroy @ 2021-03-16  7:16 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev



Le 12/03/2021 à 02:20, Nicholas Piggin a écrit :
> There is no need for this to be in asm, use the new intrrupt entry wrapper.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Hopefully this works on a real G5 now, but I couldn't reproduce the
> problem with QEMU.
> 
> Thanks,
> Nick
> 
>   arch/powerpc/include/asm/interrupt.h   | 19 +++++++++++
>   arch/powerpc/include/asm/processor.h   |  1 +
>   arch/powerpc/include/asm/thread_info.h |  6 ++++
>   arch/powerpc/kernel/exceptions-64s.S   | 45 --------------------------
>   arch/powerpc/kernel/idle_book3s.S      |  4 +++
>   5 files changed, 30 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index aedfba29e43a..ef015d3b5e39 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -9,6 +9,17 @@
>   #include <asm/kprobes.h>
>   #include <asm/runlatch.h>
>   
> +static inline void nap_adjust_return(struct pt_regs *regs)
> +{
> +#ifdef CONFIG_PPC_970_NAP
> +	if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
> +		/* Can avoid a test-and-clear because NMIs do not call this */
> +		clear_thread_local_flags(_TLF_NAPPING);
> +		regs->nip = (unsigned long)power4_idle_nap_return;

Why don't you do regs->nip = regs->link like PPC32 instead of going via an intermediate symbol that 
does nothing else than branching to LR ?

> +	}
> +#endif
> +}
> +
>   struct interrupt_state {
>   #ifdef CONFIG_PPC_BOOK3E_64
>   	enum ctx_state ctx_state;
> @@ -111,6 +122,9 @@ static inline void interrupt_async_exit_prepare(struct pt_regs *regs, struct int
>   {
>   	irq_exit();
>   	interrupt_exit_prepare(regs, state);
> +
> +	/* Adjust at exit so the main handler sees the true NIA */
> +	nap_adjust_return(regs);
>   }
>   
>   struct interrupt_nmi_state {
> @@ -164,6 +178,11 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
>   			radix_enabled() || (mfmsr() & MSR_DR))
>   		nmi_exit();
>   
> +	/*
> +	 * nmi does not call nap_adjust_return because nmi should not create
> +	 * new work to do (must use irq_work for that).
> +	 */
> +
>   #ifdef CONFIG_PPC64
>   	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
>   		this_cpu_set_ftrace_enabled(state->ftrace_enabled);

...

> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index f9e6d83e6720..abb719b21cae 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
>   	mtmsrd	r7
>   	isync
>   	b	1b
> +
> +	.globl power4_idle_nap_return
> +power4_idle_nap_return:
> +	blr
>   #endif
> 

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-03-16  7:16 ` Christophe Leroy
@ 2021-03-16  8:11   ` Nicholas Piggin
  0 siblings, 0 replies; 16+ messages in thread
From: Nicholas Piggin @ 2021-03-16  8:11 UTC (permalink / raw)
  To: Christophe Leroy, linuxppc-dev

Excerpts from Christophe Leroy's message of March 16, 2021 5:16 pm:
> 
> 
> Le 12/03/2021 à 02:20, Nicholas Piggin a écrit :
>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> Hopefully this works on a real G5 now, but I couldn't reproduce the
>> problem with QEMU.
>> 
>> Thanks,
>> Nick
>> 
>>   arch/powerpc/include/asm/interrupt.h   | 19 +++++++++++
>>   arch/powerpc/include/asm/processor.h   |  1 +
>>   arch/powerpc/include/asm/thread_info.h |  6 ++++
>>   arch/powerpc/kernel/exceptions-64s.S   | 45 --------------------------
>>   arch/powerpc/kernel/idle_book3s.S      |  4 +++
>>   5 files changed, 30 insertions(+), 45 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index aedfba29e43a..ef015d3b5e39 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -9,6 +9,17 @@
>>   #include <asm/kprobes.h>
>>   #include <asm/runlatch.h>
>>   
>> +static inline void nap_adjust_return(struct pt_regs *regs)
>> +{
>> +#ifdef CONFIG_PPC_970_NAP
>> +	if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
>> +		/* Can avoid a test-and-clear because NMIs do not call this */
>> +		clear_thread_local_flags(_TLF_NAPPING);
>> +		regs->nip = (unsigned long)power4_idle_nap_return;
> 
> Why don't you do regs->nip = regs->link like PPC32 instead of going via an intermediate symbol that 
> does nothing else than branching to LR ?

It is supposed to keep the return branch predictor balanced.

I don't know if these CPUs have one, if it gets lost during nap, or if 
nap latency is so high it really doesn't matter. But I think it is good
practice to make a habit of keeping things balanced.

Thanks,
Nick

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-03-12  1:20 [PATCH] powerpc/64s: power4 nap fixup in C Nicholas Piggin
  2021-03-16  7:16 ` Christophe Leroy
@ 2021-03-29  4:40 ` Michael Ellerman
  2021-03-29  8:33 ` Benjamin Herrenschmidt
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Michael Ellerman @ 2021-03-29  4:40 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin

Nicholas Piggin <npiggin@gmail.com> writes:
> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Hopefully this works on a real G5 now, but I couldn't reproduce the
> problem with QEMU.

It still prevents my G5 from booting.

Next time someone is in the office I'll ask them to check the display to
see if there's an oops.

cheers

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-03-12  1:20 [PATCH] powerpc/64s: power4 nap fixup in C Nicholas Piggin
  2021-03-16  7:16 ` Christophe Leroy
  2021-03-29  4:40 ` Michael Ellerman
@ 2021-03-29  8:33 ` Benjamin Herrenschmidt
  2021-03-29  9:04   ` Christophe Leroy
       [not found]   ` <236a67a4-1609-5fec-3c68-41db02cd1a4c__18973.8760514714$1617008745$gmane$org@csgroup.eu>
       [not found] ` <87y2e6fu7v.fsf__9754.75274478725$1616992871$gmane$org@mpe.ellerman.id.au>
  2021-04-10 14:28 ` Michael Ellerman
  4 siblings, 2 replies; 16+ messages in thread
From: Benjamin Herrenschmidt @ 2021-03-29  8:33 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev

On Fri, 2021-03-12 at 11:20 +1000, Nicholas Piggin wrote:
> 
> +static inline void nap_adjust_return(struct pt_regs *regs)
> 
> +{
> 
> +#ifdef CONFIG_PPC_970_NAP
> 
> +       if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
> +               /* Can avoid a test-and-clear because NMIs do not call this */
> +               clear_thread_local_flags(_TLF_NAPPING);
> +               regs->nip = (unsigned long)power4_idle_nap_return;
> +       }

Is this a pointer to a function descriptor or the actual code ?

Cheers,
Ben.

> +#endif
> 
> +}
> 
> +
> 
>  struct interrupt_state {
> 
>  #ifdef CONFIG_PPC_BOOK3E_64
> 
>         enum ctx_state ctx_state;
> 
> @@ -111,6 +122,9 @@ static inline void interrupt_async_exit_prepare(struct pt_regs *regs, struct int
> 
>  {
> 
>         irq_exit();
> 
>         interrupt_exit_prepare(regs, state);
> 
> +
> 
> +       /* Adjust at exit so the main handler sees the true NIA */
> 
> +       nap_adjust_return(regs);
> 
>  }
> 
>  
> 
>  struct interrupt_nmi_state {
> 
> @@ -164,6 +178,11 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
> 
>                         radix_enabled() || (mfmsr() & MSR_DR))
> 
>                 nmi_exit();
> 
>  
> 
> +       /*
> 
> +        * nmi does not call nap_adjust_return because nmi should not create
> 
> +        * new work to do (must use irq_work for that).
> 
> +        */
> 
> +
> 
>  #ifdef CONFIG_PPC64
> 
>         if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
> 
>                 this_cpu_set_ftrace_enabled(state->ftrace_enabled);
> 
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> 
> index 8acc3590c971..eedc3c775141 100644
> 
> --- a/arch/powerpc/include/asm/processor.h
> 
> +++ b/arch/powerpc/include/asm/processor.h
> 
> @@ -393,6 +393,7 @@ extern unsigned long isa300_idle_stop_mayloss(unsigned long psscr_val);
> 
>  extern unsigned long isa206_idle_insn_mayloss(unsigned long type);
> 
>  #ifdef CONFIG_PPC_970_NAP
> 
>  extern void power4_idle_nap(void);
> 
> +void power4_idle_nap_return(void);
> 
>  #endif
> 
>  
> 
>  extern unsigned long cpuidle_disable;
> 
> diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
> 
> index 386d576673a1..bf137151100b 100644
> 
> --- a/arch/powerpc/include/asm/thread_info.h
> 
> +++ b/arch/powerpc/include/asm/thread_info.h
> 
> @@ -152,6 +152,12 @@ void arch_setup_new_exec(void);
> 
>  
> 
>  #ifndef __ASSEMBLY__
> 
>  
> 
> +static inline void clear_thread_local_flags(unsigned int flags)
> 
> +{
> 
> +       struct thread_info *ti = current_thread_info();
> 
> +       ti->local_flags &= ~flags;
> 
> +}
> 
> +
> 
>  static inline bool test_thread_local_flags(unsigned int flags)
> 
>  {
> 
>         struct thread_info *ti = current_thread_info();
> 
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> 
> index 60d3051a8bc8..ea7a443488d2 100644
> 
> --- a/arch/powerpc/kernel/exceptions-64s.S
> 
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> 
> @@ -692,25 +692,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
> 
>         ld      r1,GPR1(r1)
> 
>  .endm
> 
>  
> 
> -/*
> 
> - * When the idle code in power4_idle puts the CPU into NAP mode,
> 
> - * it has to do so in a loop, and relies on the external interrupt
> 
> - * and decrementer interrupt entry code to get it out of the loop.
> 
> - * It sets the _TLF_NAPPING bit in current_thread_info()->local_flags
> 
> - * to signal that it is in the loop and needs help to get out.
> 
> - */
> 
> -#ifdef CONFIG_PPC_970_NAP
> 
> -#define FINISH_NAP                             \
> 
> -BEGIN_FTR_SECTION                              \
> 
> -       ld      r11, PACA_THREAD_INFO(r13);     \
> 
> -       ld      r9,TI_LOCAL_FLAGS(r11);         \
> 
> -       andi.   r10,r9,_TLF_NAPPING;            \
> 
> -       bnel    power4_fixup_nap;               \
> 
> -END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
> 
> -#else
> 
> -#define FINISH_NAP
> 
> -#endif
> 
> -
> 
>  /*
> 
>   * There are a few constraints to be concerned with.
> 
>   * - Real mode exceptions code/data must be located at their physical location.
> 
> @@ -1248,7 +1229,6 @@ EXC_COMMON_BEGIN(machine_check_common)
> 
>          */
> 
>         GEN_COMMON machine_check
> 
>  
> 
> -       FINISH_NAP
> 
>         /* Enable MSR_RI when finished with PACA_EXMC */
> 
>         li      r10,MSR_RI
> 
>         mtmsrd  r10,1
> 
> @@ -1571,7 +1551,6 @@ EXC_VIRT_BEGIN(hardware_interrupt, 0x4500, 0x100)
> 
>  EXC_VIRT_END(hardware_interrupt, 0x4500, 0x100)
> 
>  EXC_COMMON_BEGIN(hardware_interrupt_common)
> 
>         GEN_COMMON hardware_interrupt
> 
> -       FINISH_NAP
> 
>         addi    r3,r1,STACK_FRAME_OVERHEAD
> 
>         bl      do_IRQ
> 
>         b       interrupt_return
> 
> @@ -1801,7 +1780,6 @@ EXC_VIRT_BEGIN(decrementer, 0x4900, 0x80)
> 
>  EXC_VIRT_END(decrementer, 0x4900, 0x80)
> 
>  EXC_COMMON_BEGIN(decrementer_common)
> 
>         GEN_COMMON decrementer
> 
> -       FINISH_NAP
> 
>         addi    r3,r1,STACK_FRAME_OVERHEAD
> 
>         bl      timer_interrupt
> 
>         b       interrupt_return
> 
> @@ -1886,7 +1864,6 @@ EXC_VIRT_BEGIN(doorbell_super, 0x4a00, 0x100)
> 
>  EXC_VIRT_END(doorbell_super, 0x4a00, 0x100)
> 
>  EXC_COMMON_BEGIN(doorbell_super_common)
> 
>         GEN_COMMON doorbell_super
> 
> -       FINISH_NAP
> 
>         addi    r3,r1,STACK_FRAME_OVERHEAD
> 
>  #ifdef CONFIG_PPC_DOORBELL
> 
>         bl      doorbell_exception
> 
> @@ -2237,7 +2214,6 @@ EXC_COMMON_BEGIN(hmi_exception_early_common)
> 
>  
> 
>  EXC_COMMON_BEGIN(hmi_exception_common)
> 
>         GEN_COMMON hmi_exception
> 
> -       FINISH_NAP
> 
>         addi    r3,r1,STACK_FRAME_OVERHEAD
> 
>         bl      handle_hmi_exception
> 
>         b       interrupt_return
> 
> @@ -2266,7 +2242,6 @@ EXC_VIRT_BEGIN(h_doorbell, 0x4e80, 0x20)
> 
>  EXC_VIRT_END(h_doorbell, 0x4e80, 0x20)
> 
>  EXC_COMMON_BEGIN(h_doorbell_common)
> 
>         GEN_COMMON h_doorbell
> 
> -       FINISH_NAP
> 
>         addi    r3,r1,STACK_FRAME_OVERHEAD
> 
>  #ifdef CONFIG_PPC_DOORBELL
> 
>         bl      doorbell_exception
> 
> @@ -2299,7 +2274,6 @@ EXC_VIRT_BEGIN(h_virt_irq, 0x4ea0, 0x20)
> 
>  EXC_VIRT_END(h_virt_irq, 0x4ea0, 0x20)
> 
>  EXC_COMMON_BEGIN(h_virt_irq_common)
> 
>         GEN_COMMON h_virt_irq
> 
> -       FINISH_NAP
> 
>         addi    r3,r1,STACK_FRAME_OVERHEAD
> 
>         bl      do_IRQ
> 
>         b       interrupt_return
> 
> @@ -2345,7 +2319,6 @@ EXC_VIRT_BEGIN(performance_monitor, 0x4f00, 0x20)
> 
>  EXC_VIRT_END(performance_monitor, 0x4f00, 0x20)
> 
>  EXC_COMMON_BEGIN(performance_monitor_common)
> 
>         GEN_COMMON performance_monitor
> 
> -       FINISH_NAP
> 
>         addi    r3,r1,STACK_FRAME_OVERHEAD
> 
>         bl      performance_monitor_exception
> 
>         b       interrupt_return
> 
> @@ -3096,24 +3069,6 @@ USE_FIXED_SECTION(virt_trampolines)
> 
>  __end_interrupts:
> 
>  DEFINE_FIXED_SYMBOL(__end_interrupts)
> 
>  
> 
> -#ifdef CONFIG_PPC_970_NAP
> 
> -       /*
> 
> -        * Called by exception entry code if _TLF_NAPPING was set, this clears
> 
> -        * the NAPPING flag, and redirects the exception exit to
> 
> -        * power4_fixup_nap_return.
> 
> -        */
> 
> -       .globl power4_fixup_nap
> 
> -EXC_COMMON_BEGIN(power4_fixup_nap)
> 
> -       andc    r9,r9,r10
> 
> -       std     r9,TI_LOCAL_FLAGS(r11)
> 
> -       LOAD_REG_ADDR(r10, power4_idle_nap_return)
> 
> -       std     r10,_NIP(r1)
> 
> -       blr
> 
> -
> 
> -power4_idle_nap_return:
> 
> -       blr
> 
> -#endif
> 
> -
> 
>  CLOSE_FIXED_SECTION(real_vectors);
> 
>  CLOSE_FIXED_SECTION(real_trampolines);
> 
>  CLOSE_FIXED_SECTION(virt_vectors);
> 
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> 
> index f9e6d83e6720..abb719b21cae 100644
> 
> --- a/arch/powerpc/kernel/idle_book3s.S
> 
> +++ b/arch/powerpc/kernel/idle_book3s.S
> 
> @@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
> 
>         mtmsrd  r7
> 
>         isync
> 
>         b       1b
> 
> +
> 
> +       .globl power4_idle_nap_return
> 
> +power4_idle_nap_return:
> 
> +       blr
> 
>  #endif
> 
> --
> 
> 2.23.0


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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-03-29  8:33 ` Benjamin Herrenschmidt
@ 2021-03-29  9:04   ` Christophe Leroy
       [not found]   ` <236a67a4-1609-5fec-3c68-41db02cd1a4c__18973.8760514714$1617008745$gmane$org@csgroup.eu>
  1 sibling, 0 replies; 16+ messages in thread
From: Christophe Leroy @ 2021-03-29  9:04 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Nicholas Piggin, linuxppc-dev



Le 29/03/2021 à 10:33, Benjamin Herrenschmidt a écrit :
> On Fri, 2021-03-12 at 11:20 +1000, Nicholas Piggin wrote:
>>
>> +static inline void nap_adjust_return(struct pt_regs *regs)
>>
>> +{
>>
>> +#ifdef CONFIG_PPC_970_NAP
>>
>> +       if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
>> +               /* Can avoid a test-and-clear because NMIs do not call this */
>> +               clear_thread_local_flags(_TLF_NAPPING);
>> +               regs->nip = (unsigned long)power4_idle_nap_return;
>> +       }
> 
> Is this a pointer to a function descriptor or the actual code ?
> 

--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
  	mtmsrd	r7
  	isync
  	b	1b
+
+	.globl power4_idle_nap_return
+power4_idle_nap_return:
+	blr
  #endif


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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
       [not found]   ` <236a67a4-1609-5fec-3c68-41db02cd1a4c__18973.8760514714$1617008745$gmane$org@csgroup.eu>
@ 2021-03-29 11:56     ` Andreas Schwab
       [not found]     ` <87czvikwb7.fsf__13924.9042653077$1617019017$gmane$org@igel.home>
  1 sibling, 0 replies; 16+ messages in thread
From: Andreas Schwab @ 2021-03-29 11:56 UTC (permalink / raw)
  To: Christophe Leroy; +Cc: linuxppc-dev, Nicholas Piggin

On Mär 29 2021, Christophe Leroy wrote:

> Le 29/03/2021 à 10:33, Benjamin Herrenschmidt a écrit :
>> On Fri, 2021-03-12 at 11:20 +1000, Nicholas Piggin wrote:
>>>
>>> +static inline void nap_adjust_return(struct pt_regs *regs)
>>>
>>> +{
>>>
>>> +#ifdef CONFIG_PPC_970_NAP
>>>
>>> +       if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
>>> +               /* Can avoid a test-and-clear because NMIs do not call this */
>>> +               clear_thread_local_flags(_TLF_NAPPING);
>>> +               regs->nip = (unsigned long)power4_idle_nap_return;
>>> +       }
>> Is this a pointer to a function descriptor or the actual code ?
>> 
>
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
>  	mtmsrd	r7
>  	isync
>  	b	1b
> +
> +	.globl power4_idle_nap_return
> +power4_idle_nap_return:
> +	blr
>  #endif

The problem is not the definition, it is the reference.  In C, a
function symbol always resolves to the address of the descriptor.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
       [not found] ` <87y2e6fu7v.fsf__9754.75274478725$1616992871$gmane$org@mpe.ellerman.id.au>
@ 2021-03-29 15:30   ` Andreas Schwab
       [not found]   ` <87v99aj7tr.fsf__47134.2879392736$1617031867$gmane$org@igel.home>
  1 sibling, 0 replies; 16+ messages in thread
From: Andreas Schwab @ 2021-03-29 15:30 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, Nicholas Piggin

On Mär 29 2021, Michael Ellerman wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> Hopefully this works on a real G5 now, but I couldn't reproduce the
>> problem with QEMU.
>
> It still prevents my G5 from booting.

I see differing failures.  What's common is that there is a pause of
about 60 seconds before the crash occurs.  It looks like the crash
occurs in power4_idle_nap+0x30/0x34.  Unfortuately, the BootX console is
too small to see enough.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
       [not found]   ` <87v99aj7tr.fsf__47134.2879392736$1617031867$gmane$org@igel.home>
@ 2021-03-29 16:23     ` Andreas Schwab
  2021-04-01  7:36       ` Nicholas Piggin
  0 siblings, 1 reply; 16+ messages in thread
From: Andreas Schwab @ 2021-03-29 16:23 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, Nicholas Piggin

On Mär 29 2021, Andreas Schwab wrote:

> On Mär 29 2021, Michael Ellerman wrote:
>
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> Hopefully this works on a real G5 now, but I couldn't reproduce the
>>> problem with QEMU.
>>
>> It still prevents my G5 from booting.
>
> I see differing failures.  What's common is that there is a pause of
> about 60 seconds before the crash occurs.  It looks like the crash
> occurs in power4_idle_nap+0x30/0x34.  Unfortuately, the BootX console is
> too small to see enough.

I was now able to see the messages on the VGA console, and the problem
is actually that the cpus are starting to stall.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
       [not found]     ` <87czvikwb7.fsf__13924.9042653077$1617019017$gmane$org@igel.home>
@ 2021-03-29 16:51       ` Andreas Schwab
  0 siblings, 0 replies; 16+ messages in thread
From: Andreas Schwab @ 2021-03-29 16:51 UTC (permalink / raw)
  To: Christophe Leroy; +Cc: linuxppc-dev, Nicholas Piggin

On Mär 29 2021, Andreas Schwab wrote:

> On Mär 29 2021, Christophe Leroy wrote:
>
>> Le 29/03/2021 à 10:33, Benjamin Herrenschmidt a écrit :
>>> On Fri, 2021-03-12 at 11:20 +1000, Nicholas Piggin wrote:
>>>>
>>>> +static inline void nap_adjust_return(struct pt_regs *regs)
>>>>
>>>> +{
>>>>
>>>> +#ifdef CONFIG_PPC_970_NAP
>>>>
>>>> +       if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
>>>> +               /* Can avoid a test-and-clear because NMIs do not call this */
>>>> +               clear_thread_local_flags(_TLF_NAPPING);
>>>> +               regs->nip = (unsigned long)power4_idle_nap_return;
>>>> +       }
>>> Is this a pointer to a function descriptor or the actual code ?
>>> 
>>
>> --- a/arch/powerpc/kernel/idle_book3s.S
>> +++ b/arch/powerpc/kernel/idle_book3s.S
>> @@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
>>  	mtmsrd	r7
>>  	isync
>>  	b	1b
>> +
>> +	.globl power4_idle_nap_return
>> +power4_idle_nap_return:
>> +	blr
>>  #endif
>
> The problem is not the definition, it is the reference.  In C, a
> function symbol always resolves to the address of the descriptor.

Sorry, this is wrong, I have misremembered how function descriptors work
on ppc64.  The address is really pointing to the actual code.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-03-29 16:23     ` Andreas Schwab
@ 2021-04-01  7:36       ` Nicholas Piggin
  2021-04-05 12:56         ` Nicholas Piggin
  0 siblings, 1 reply; 16+ messages in thread
From: Nicholas Piggin @ 2021-04-01  7:36 UTC (permalink / raw)
  To: Michael Ellerman, Andreas Schwab; +Cc: linuxppc-dev

Excerpts from Andreas Schwab's message of March 30, 2021 2:23 am:
> On Mär 29 2021, Andreas Schwab wrote:
> 
>> On Mär 29 2021, Michael Ellerman wrote:
>>
>>> Nicholas Piggin <npiggin@gmail.com> writes:
>>>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> ---
>>>> Hopefully this works on a real G5 now, but I couldn't reproduce the
>>>> problem with QEMU.
>>>
>>> It still prevents my G5 from booting.
>>
>> I see differing failures.  What's common is that there is a pause of
>> about 60 seconds before the crash occurs.  It looks like the crash
>> occurs in power4_idle_nap+0x30/0x34.  Unfortuately, the BootX console is
>> too small to see enough.
> 
> I was now able to see the messages on the VGA console, and the problem
> is actually that the cpus are starting to stall.

This is strange, I haven't been able to figure out what is wrong.

I've been looking at QEMU code and now I'll have to try find a POWER4/5
or PPC970 manual to see what exactly this MSR[POW] thing does and make 
sure QEMU matches it.

Progress might be slow. Worst case I guess we revert if it can't
be fixed before next merge window.

Thanks,
Nick

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-04-01  7:36       ` Nicholas Piggin
@ 2021-04-05 12:56         ` Nicholas Piggin
  2021-04-05 16:40           ` Andreas Schwab
  0 siblings, 1 reply; 16+ messages in thread
From: Nicholas Piggin @ 2021-04-05 12:56 UTC (permalink / raw)
  To: Michael Ellerman, Andreas Schwab; +Cc: linuxppc-dev

Excerpts from Nicholas Piggin's message of April 1, 2021 5:36 pm:
> Excerpts from Andreas Schwab's message of March 30, 2021 2:23 am:
>> On Mär 29 2021, Andreas Schwab wrote:
>> 
>>> On Mär 29 2021, Michael Ellerman wrote:
>>>
>>>> Nicholas Piggin <npiggin@gmail.com> writes:
>>>>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>>>>>
>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>> ---
>>>>> Hopefully this works on a real G5 now, but I couldn't reproduce the
>>>>> problem with QEMU.
>>>>
>>>> It still prevents my G5 from booting.
>>>
>>> I see differing failures.  What's common is that there is a pause of
>>> about 60 seconds before the crash occurs.  It looks like the crash
>>> occurs in power4_idle_nap+0x30/0x34.  Unfortuately, the BootX console is
>>> too small to see enough.
>> 
>> I was now able to see the messages on the VGA console, and the problem
>> is actually that the cpus are starting to stall.
> 
> This is strange, I haven't been able to figure out what is wrong.
> 
> I've been looking at QEMU code and now I'll have to try find a POWER4/5
> or PPC970 manual to see what exactly this MSR[POW] thing does and make 
> sure QEMU matches it.

I worked it out. There was a window where it could take another 
interrupt before the first one adjusts the nip.

I managed to trigger it in qemu and this version fixed it.

---
 arch/powerpc/include/asm/interrupt.h   | 24 ++++++++++++++
 arch/powerpc/include/asm/processor.h   |  1 +
 arch/powerpc/include/asm/thread_info.h |  6 ++++
 arch/powerpc/kernel/exceptions-64s.S   | 45 --------------------------
 arch/powerpc/kernel/idle_book3s.S      |  4 +++
 5 files changed, 35 insertions(+), 45 deletions(-)

diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index e8d09a841373..2643028cf67a 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -9,6 +9,17 @@
 #include <asm/kprobes.h>
 #include <asm/runlatch.h>
 
+static inline void nap_adjust_return(struct pt_regs *regs)
+{
+#ifdef CONFIG_PPC_970_NAP
+	if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
+		/* Can avoid a test-and-clear because NMIs do not call this */
+		clear_thread_local_flags(_TLF_NAPPING);
+		regs->nip = (unsigned long)power4_idle_nap_return;
+	}
+#endif
+}
+
 struct interrupt_state {
 #ifdef CONFIG_PPC_BOOK3E_64
 	enum ctx_state ctx_state;
@@ -109,6 +120,14 @@ static inline void interrupt_async_enter_prepare(struct pt_regs *regs, struct in
 
 static inline void interrupt_async_exit_prepare(struct pt_regs *regs, struct interrupt_state *state)
 {
+	/*
+	 * Adjust at exit so the main handler sees the true NIA. This must
+	 * come before irq_exit() because irq_exit can enable interrupts, and
+	 * if another interrupt is taken before nap_adjust_return has run
+	 * here, then that interrupt would return directly to idle nap return.
+	 */
+	nap_adjust_return(regs);
+
 	irq_exit();
 	interrupt_exit_prepare(regs, state);
 }
@@ -164,6 +183,11 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
 			radix_enabled() || (mfmsr() & MSR_DR))
 		nmi_exit();
 
+	/*
+	 * nmi does not call nap_adjust_return because nmi should not create
+	 * new work to do (must use irq_work for that).
+	 */
+
 #ifdef CONFIG_PPC64
 	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
 		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 8acc3590c971..eedc3c775141 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -393,6 +393,7 @@ extern unsigned long isa300_idle_stop_mayloss(unsigned long psscr_val);
 extern unsigned long isa206_idle_insn_mayloss(unsigned long type);
 #ifdef CONFIG_PPC_970_NAP
 extern void power4_idle_nap(void);
+void power4_idle_nap_return(void);
 #endif
 
 extern unsigned long cpuidle_disable;
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 386d576673a1..bf137151100b 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -152,6 +152,12 @@ void arch_setup_new_exec(void);
 
 #ifndef __ASSEMBLY__
 
+static inline void clear_thread_local_flags(unsigned int flags)
+{
+	struct thread_info *ti = current_thread_info();
+	ti->local_flags &= ~flags;
+}
+
 static inline bool test_thread_local_flags(unsigned int flags)
 {
 	struct thread_info *ti = current_thread_info();
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 8082b690e874..0cdb59e8b577 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -692,25 +692,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
 	ld	r1,GPR1(r1)
 .endm
 
-/*
- * When the idle code in power4_idle puts the CPU into NAP mode,
- * it has to do so in a loop, and relies on the external interrupt
- * and decrementer interrupt entry code to get it out of the loop.
- * It sets the _TLF_NAPPING bit in current_thread_info()->local_flags
- * to signal that it is in the loop and needs help to get out.
- */
-#ifdef CONFIG_PPC_970_NAP
-#define FINISH_NAP				\
-BEGIN_FTR_SECTION				\
-	ld	r11, PACA_THREAD_INFO(r13);	\
-	ld	r9,TI_LOCAL_FLAGS(r11);		\
-	andi.	r10,r9,_TLF_NAPPING;		\
-	bnel	power4_fixup_nap;		\
-END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
-#else
-#define FINISH_NAP
-#endif
-
 /*
  * There are a few constraints to be concerned with.
  * - Real mode exceptions code/data must be located at their physical location.
@@ -1248,7 +1229,6 @@ EXC_COMMON_BEGIN(machine_check_common)
 	 */
 	GEN_COMMON machine_check
 
-	FINISH_NAP
 	/* Enable MSR_RI when finished with PACA_EXMC */
 	li	r10,MSR_RI
 	mtmsrd 	r10,1
@@ -1571,7 +1551,6 @@ EXC_VIRT_BEGIN(hardware_interrupt, 0x4500, 0x100)
 EXC_VIRT_END(hardware_interrupt, 0x4500, 0x100)
 EXC_COMMON_BEGIN(hardware_interrupt_common)
 	GEN_COMMON hardware_interrupt
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_IRQ
 	b	interrupt_return
@@ -1801,7 +1780,6 @@ EXC_VIRT_BEGIN(decrementer, 0x4900, 0x80)
 EXC_VIRT_END(decrementer, 0x4900, 0x80)
 EXC_COMMON_BEGIN(decrementer_common)
 	GEN_COMMON decrementer
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	timer_interrupt
 	b	interrupt_return
@@ -1886,7 +1864,6 @@ EXC_VIRT_BEGIN(doorbell_super, 0x4a00, 0x100)
 EXC_VIRT_END(doorbell_super, 0x4a00, 0x100)
 EXC_COMMON_BEGIN(doorbell_super_common)
 	GEN_COMMON doorbell_super
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 #ifdef CONFIG_PPC_DOORBELL
 	bl	doorbell_exception
@@ -2237,7 +2214,6 @@ EXC_COMMON_BEGIN(hmi_exception_early_common)
 
 EXC_COMMON_BEGIN(hmi_exception_common)
 	GEN_COMMON hmi_exception
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	handle_hmi_exception
 	b	interrupt_return
@@ -2266,7 +2242,6 @@ EXC_VIRT_BEGIN(h_doorbell, 0x4e80, 0x20)
 EXC_VIRT_END(h_doorbell, 0x4e80, 0x20)
 EXC_COMMON_BEGIN(h_doorbell_common)
 	GEN_COMMON h_doorbell
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 #ifdef CONFIG_PPC_DOORBELL
 	bl	doorbell_exception
@@ -2299,7 +2274,6 @@ EXC_VIRT_BEGIN(h_virt_irq, 0x4ea0, 0x20)
 EXC_VIRT_END(h_virt_irq, 0x4ea0, 0x20)
 EXC_COMMON_BEGIN(h_virt_irq_common)
 	GEN_COMMON h_virt_irq
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_IRQ
 	b	interrupt_return
@@ -2345,7 +2319,6 @@ EXC_VIRT_BEGIN(performance_monitor, 0x4f00, 0x20)
 EXC_VIRT_END(performance_monitor, 0x4f00, 0x20)
 EXC_COMMON_BEGIN(performance_monitor_common)
 	GEN_COMMON performance_monitor
-	FINISH_NAP
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	performance_monitor_exception
 	b	interrupt_return
@@ -3096,24 +3069,6 @@ USE_FIXED_SECTION(virt_trampolines)
 __end_interrupts:
 DEFINE_FIXED_SYMBOL(__end_interrupts)
 
-#ifdef CONFIG_PPC_970_NAP
-	/*
-	 * Called by exception entry code if _TLF_NAPPING was set, this clears
-	 * the NAPPING flag, and redirects the exception exit to
-	 * power4_fixup_nap_return.
-	 */
-	.globl power4_fixup_nap
-EXC_COMMON_BEGIN(power4_fixup_nap)
-	andc	r9,r9,r10
-	std	r9,TI_LOCAL_FLAGS(r11)
-	LOAD_REG_ADDR(r10, power4_idle_nap_return)
-	std	r10,_NIP(r1)
-	blr
-
-power4_idle_nap_return:
-	blr
-#endif
-
 CLOSE_FIXED_SECTION(real_vectors);
 CLOSE_FIXED_SECTION(real_trampolines);
 CLOSE_FIXED_SECTION(virt_vectors);
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index f9e6d83e6720..abb719b21cae 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
 	mtmsrd	r7
 	isync
 	b	1b
+
+	.globl power4_idle_nap_return
+power4_idle_nap_return:
+	blr
 #endif
-- 
2.23.0


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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-04-05 12:56         ` Nicholas Piggin
@ 2021-04-05 16:40           ` Andreas Schwab
  2021-04-06  2:45             ` Nicholas Piggin
  0 siblings, 1 reply; 16+ messages in thread
From: Andreas Schwab @ 2021-04-05 16:40 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: linuxppc-dev

On Apr 05 2021, Nicholas Piggin wrote:

> I worked it out. There was a window where it could take another 
> interrupt before the first one adjusts the nip.
>
> I managed to trigger it in qemu and this version fixed it.

Works for me as well.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-04-05 16:40           ` Andreas Schwab
@ 2021-04-06  2:45             ` Nicholas Piggin
  0 siblings, 0 replies; 16+ messages in thread
From: Nicholas Piggin @ 2021-04-06  2:45 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: linuxppc-dev

Excerpts from Andreas Schwab's message of April 6, 2021 2:40 am:
> On Apr 05 2021, Nicholas Piggin wrote:
> 
>> I worked it out. There was a window where it could take another 
>> interrupt before the first one adjusts the nip.
>>
>> I managed to trigger it in qemu and this version fixed it.
> 
> Works for me as well.

Aha, thank you for testing it. Finally fixed it. I'll resubmit the
patch.

Thanks,
Nick

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-03-12  1:20 [PATCH] powerpc/64s: power4 nap fixup in C Nicholas Piggin
                   ` (3 preceding siblings ...)
       [not found] ` <87y2e6fu7v.fsf__9754.75274478725$1616992871$gmane$org@mpe.ellerman.id.au>
@ 2021-04-10 14:28 ` Michael Ellerman
  2021-04-19  5:16   ` Michael Ellerman
  4 siblings, 1 reply; 16+ messages in thread
From: Michael Ellerman @ 2021-04-10 14:28 UTC (permalink / raw)
  To: linuxppc-dev, Nicholas Piggin

On Fri, 12 Mar 2021 11:20:44 +1000, Nicholas Piggin wrote:
> There is no need for this to be in asm, use the new intrrupt entry wrapper.

Applied to powerpc/next.

[1/1] powerpc/64s: power4 nap fixup in C
      https://git.kernel.org/powerpc/c/98db179a78dd8379e9d2cbfc3f00224168a9344c

cheers

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

* Re: [PATCH] powerpc/64s: power4 nap fixup in C
  2021-04-10 14:28 ` Michael Ellerman
@ 2021-04-19  5:16   ` Michael Ellerman
  0 siblings, 0 replies; 16+ messages in thread
From: Michael Ellerman @ 2021-04-19  5:16 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev, Nicholas Piggin

Michael Ellerman <patch-notifications@ellerman.id.au> writes:
> On Fri, 12 Mar 2021 11:20:44 +1000, Nicholas Piggin wrote:
>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>
> Applied to powerpc/next.
>
> [1/1] powerpc/64s: power4 nap fixup in C
>       https://git.kernel.org/powerpc/c/98db179a78dd8379e9d2cbfc3f00224168a9344c

Script is drunk again, v2 was applied, not this one.

cheers

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

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12  1:20 [PATCH] powerpc/64s: power4 nap fixup in C Nicholas Piggin
2021-03-16  7:16 ` Christophe Leroy
2021-03-16  8:11   ` Nicholas Piggin
2021-03-29  4:40 ` Michael Ellerman
2021-03-29  8:33 ` Benjamin Herrenschmidt
2021-03-29  9:04   ` Christophe Leroy
     [not found]   ` <236a67a4-1609-5fec-3c68-41db02cd1a4c__18973.8760514714$1617008745$gmane$org@csgroup.eu>
2021-03-29 11:56     ` Andreas Schwab
     [not found]     ` <87czvikwb7.fsf__13924.9042653077$1617019017$gmane$org@igel.home>
2021-03-29 16:51       ` Andreas Schwab
     [not found] ` <87y2e6fu7v.fsf__9754.75274478725$1616992871$gmane$org@mpe.ellerman.id.au>
2021-03-29 15:30   ` Andreas Schwab
     [not found]   ` <87v99aj7tr.fsf__47134.2879392736$1617031867$gmane$org@igel.home>
2021-03-29 16:23     ` Andreas Schwab
2021-04-01  7:36       ` Nicholas Piggin
2021-04-05 12:56         ` Nicholas Piggin
2021-04-05 16:40           ` Andreas Schwab
2021-04-06  2:45             ` Nicholas Piggin
2021-04-10 14:28 ` Michael Ellerman
2021-04-19  5:16   ` Michael Ellerman

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git