Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions

Le 08/09/2020 à 10:29, Christophe Leroy a écrit :

Le 08/09/2020 à 09:48, Nicholas Piggin a écrit :

quoted

Excerpts from Christophe Leroy's message of September 7, 2020 9:34 pm:

quoted

On Mon, 2020-09-07 at 11:20 +0200, Christophe Leroy wrote:

quoted

Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :

quoted

Make interrupt handlers all just take the pt_regs * argument and load
DAR/DSISR etc from that. Make those that return a value return long.

I like this, it will likely simplify a bit the VMAP_STACK mess.

Not sure it is that easy. My board is stuck after the start of init.


On the 8xx, on Instruction TLB Error exception, we do

    andis.    r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 
bits */

On book3s/32, on ISI exception we do:
    andis.    r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 
bits */

On 40x and bookE, on ISI exception we do:
    li    r5,0            /* Pass zero as arg3 */


And regs->dsisr will just contain nothing

So it means we should at least write back r5 into regs->dsisr from 
there
? The performance impact should be minimal as we already write _DAR so
the cache line should already be in the cache.

A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
allthough we don't want to do it for both ISI and DSI at the end, so
you'll have to do it in every head_xxx.S

To get you series build and work, I did the following hacks:

Great, thanks for this.

quoted

diff --git a/arch/powerpc/include/asm/interrupt.h

b/arch/powerpc/include/asm/interrupt.h
index acfcc7d5779b..c11045d3113a 100644

--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h

@@ -93,7 +93,9 @@ static inline void interrupt_nmi_exit_prepare(struct

pt_regs *regs, struct inter
  {
      nmi_exit();
+#ifdef CONFIG_PPC64
      this_cpu_set_ftrace_enabled(state->ftrace_enabled);
+#endif

This seems okay, not a hack.

quoted

  #ifdef CONFIG_PPC_BOOK3S_64
      /* Check we didn't change the pending interrupt mask. */

diff --git a/arch/powerpc/kernel/entry_32.S

b/arch/powerpc/kernel/entry_32.S
index f4d0af8e1136..66f7adbe1076 100644

--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S

@@ -663,6 +663,7 @@ ppc_swapcontext:

   */
      .globl    handle_page_fault
  handle_page_fault:
+    stw    r5,_DSISR(r1)
      addi    r3,r1,STACK_FRAME_OVERHEAD
  #ifdef CONFIG_PPC_BOOK3S_32
      andis.  r0,r5,DSISR_DABRMATCH@h

Is this what you want to do for 32, or do you want to seperate
ISI and DSI sides?

No I think we want to separate ISI and DSI sides.

And I think the specific filtering done in ISI could be done in 
do_page_fault() in C. Ok, it would make a special handling for is_exec 
but there are already several places where the behaviour differs based 
on is_exec.
The only thing we need to keep at ASM level is the DABR stuff for 
calling do_break() in handle_page_fault(), because it is used to decide 
whether full regs are saved or not. But it could be a test done earlier 
in the prolog and the result being kept in one of the CR bits.

Looking at it once more, I'm wondering whether we really need a full regs.

Before commit 
https://github.com/linuxppc/linux/commit/d300627c6a53693fb01479b59b0cdd293761b1fa#diff-f9658f412252f3bb3093e0a95b37f3ac 
do_break() was called from do_page_fault() without a full regs set.

Christophe