powerpc: interrupt wrappers

The page fault handling still has some complex logic particularly around hash table handling, in asm. Implement this in C instead.

Signed-off-by Nicholas Piggin <[email protected]>

---
  arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 +
  arch/powerpc/kernel/exceptions-64s.S          | 131 +++---------------
  arch/powerpc/mm/book3s64/hash_utils.c         |  77 ++++++----
  arch/powerpc/mm/fault.c                       |  46 ++++--
  4 files changed, 107 insertions(+), 148 deletions(-)

arch/powerpc/include/asm/book3s/64/mmu-hash.h +1 -0

index 066b1d34c7bc..60a669379aa0 100644
@@ -454,6 +454,7 @@ static inline unsigned long hpt_hash(unsigned long vpn,
  #define HPTE_NOHPTE_UPDATE	0x2
  #define HPTE_USE_KERNEL_KEY	0x4
 
+int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr);
  extern int __hash_page_4K(unsigned long ea, unsigned long access,
  			  unsigned long vsid, pte_t *ptep, unsigned long trap,
  			  unsigned long flags, int ssize, int subpage_prot);

arch/powerpc/kernel/exceptions-64s.S +6 -5

index 6e53f7638737..bcb5e81d2088 100644
@@ -1401,14 +1401,15 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
   *
   * Handling:
   * - Hash MMU
- *   Go to do_hash_page first to see if the HPT can be filled from an entry in
- *   the Linux page table. Hash faults can hit in kernel mode in a fairly
+ *   Go to do_hash_fault, which attempts to fill the HPT from an entry in the
+ *   Linux page table. Hash faults can hit in kernel mode in a fairly
   *   arbitrary state (e.g., interrupts disabled, locks held) when accessing
   *   "non-bolted" regions, e.g., vmalloc space. However these should always be
- *   backed by Linux page tables.
+ *   backed by Linux page table entries.
   *
- *   If none is found, do a Linux page fault. Linux page faults can happen in
- *   kernel mode due to user copy operations of course.
+ *   If no entry is found the Linux page fault handler is invoked (by
+ *   do_hash_fault). Linux page faults can happen in kernel mode due to user
+ *   copy operations of course.
   *
   *   KVM: The KVM HDSI handler may perform a load with MSR[DR]=1 in guest
   *   MMU context, which may cause a DSI in the host, which must go to the

arch/powerpc/kernel/exceptions-64s.S +5 -0

+	beq+	interrupt_return
+	/* We need to restore NVGPRS */
+	REST_NVGPRS(r1)
+	b       interrupt_return
 
  	GEN_KVM data_access
 

@@ -1540,13 +1545,17 @@ EXC_COMMON_BEGIN(instruction_access_common)
  	GEN_COMMON instruction_access
  	ld	r4,_DAR(r1)
  	ld	r5,_DSISR(r1)
+	addi	r3,r1,STACK_FRAME_OVERHEAD

arch/powerpc/kernel/exceptions-64s.S +4 -96

+	beq+	interrupt_return
+	/* We need to restore NVGPRS */
+	REST_NVGPRS(r1)
+	b       interrupt_return
 
  	GEN_KVM instruction_access
 

@@ -3221,99 +3230,3 @@ disable_machine_check:
  	RFI_TO_KERNEL
  1:	mtlr	r0
  	blr
-
-/*
- * Hash table stuff
- */
-	.balign	IFETCH_ALIGN_BYTES
-do_hash_page:
-#ifdef CONFIG_PPC_BOOK3S_64
-	lis	r0,(DSISR_BAD_FAULT_64S | DSISR_DABRMATCH | DSISR_KEYFAULT)@h
-	ori	r0,r0,DSISR_BAD_FAULT_64S@l
-	and.	r0,r5,r0		/* weird error? */
-	bne-	handle_page_fault	/* if not, try to insert a HPTE */
-
-	/*
-	 * If we are in an "NMI" (e.g., an interrupt when soft-disabled), then
-	 * don't call hash_page, just fail the fault. This is required to
-	 * prevent re-entrancy problems in the hash code, namely perf
-	 * interrupts hitting while something holds H_PAGE_BUSY, and taking a
-	 * hash fault. See the comment in hash_preload().
-	 */
-	ld	r11, PACA_THREAD_INFO(r13)
-	lwz	r0,TI_PREEMPT(r11)
-	andis.	r0,r0,NMI_MASK@h
-	bne	77f
-
-	/*
-	 * r3 contains the trap number
-	 * r4 contains the faulting address
-	 * r5 contains dsisr
-	 * r6 msr
-	 *
-	 * at return r3 = 0 for success, 1 for page fault, negative for error
-	 */
-	bl	__hash_page		/* build HPTE if possible */
-        cmpdi	r3,0			/* see if __hash_page succeeded */
-
-	/* Success */
-	beq	interrupt_return	/* Return from exception on success */
-
-	/* Error */
-	blt-	13f
-
-	/* Reload DAR/DSISR into r4/r5 for the DABR check below */
-	ld	r4,_DAR(r1)
-	ld      r5,_DSISR(r1)
-#endif /* CONFIG_PPC_BOOK3S_64 */
-
-/* Here we have a page fault that hash_page can't handle. */
-handle_page_fault:
-11:	andis.  r0,r5,DSISR_DABRMATCH@h
-	bne-    handle_dabr_fault
-	addi	r3,r1,STACK_FRAME_OVERHEAD
-	bl	do_page_fault
-	cmpdi	r3,0
-	beq+	interrupt_return
-	mr	r5,r3
-	addi	r3,r1,STACK_FRAME_OVERHEAD
-	ld	r4,_DAR(r1)
-	bl	__bad_page_fault
-	b	interrupt_return
-
-/* We have a data breakpoint exception - handle it */
-handle_dabr_fault:
-	ld      r4,_DAR(r1)
-	ld      r5,_DSISR(r1)
-	addi    r3,r1,STACK_FRAME_OVERHEAD
-	bl      do_break
-	/*
-	 * do_break() may have changed the NV GPRS while handling a breakpoint.
-	 * If so, we need to restore them with their updated values.
-	 */
-	REST_NVGPRS(r1)
-	b       interrupt_return
-
-
-#ifdef CONFIG_PPC_BOOK3S_64
-/* We have a page fault that hash_page could handle but HV refused
- * the PTE insertion
- */
-13:	mr	r5,r3
-	addi	r3,r1,STACK_FRAME_OVERHEAD
-	ld	r4,_DAR(r1)
-	bl	low_hash_fault
-	b	interrupt_return
-#endif
-
-/*
- * We come here as a result of a DSI at a point where we don't want
- * to call hash_page, such as when we are accessing memory (possibly
- * user memory) inside a PMU interrupt that occurred while interrupts
- * were soft-disabled.  We want to invoke the exception handler for
- * the access, or panic if there isn't a handler.
- */
-77:	addi	r3,r1,STACK_FRAME_OVERHEAD
-	li	r5,SIGSEGV
-	bl	bad_page_fault
-	b	interrupt_return

arch/powerpc/mm/book3s64/hash_utils.c +49 -28

index 73b06adb6eeb..5a61182ddf75 100644
@@ -1512,16 +1512,40 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap,
  }
  EXPORT_SYMBOL_GPL(hash_page);
 
-int __hash_page(unsigned long trap, unsigned long ea, unsigned long dsisr,
-		unsigned long msr)
+int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr)
  {
  	unsigned long access = _PAGE_PRESENT | _PAGE_READ;
  	unsigned long flags = 0;
-	struct mm_struct *mm = current->mm;
-	unsigned int region_id = get_region_id(ea);
+	struct mm_struct *mm;
+	unsigned int region_id;
+	int err;
+
+	if (unlikely(dsisr & (DSISR_BAD_FAULT_64S | DSISR_DABRMATCH | DSISR_KEYFAULT)))
+		goto page_fault;
+
+	/*
+	 * If we are in an "NMI" (e.g., an interrupt when soft-disabled), then
+	 * don't call hash_page, just fail the fault. This is required to
+	 * prevent re-entrancy problems in the hash code, namely perf
+	 * interrupts hitting while something holds H_PAGE_BUSY, and taking a
+	 * hash fault. See the comment in hash_preload().
+	 *
+	 * We come here as a result of a DSI at a point where we don't want
+	 * to call hash_page, such as when we are accessing memory (possibly
+	 * user memory) inside a PMU interrupt that occurred while interrupts
+	 * were soft-disabled.  We want to invoke the exception handler for
+	 * the access, or panic if there isn't a handler.
+	 */
+	if (unlikely(in_nmi())) {
+		bad_page_fault(regs, ea, SIGSEGV);
+		return 0;
+	}
 
+	region_id = get_region_id(ea);
  	if ((region_id == VMALLOC_REGION_ID) || (region_id == IO_REGION_ID))
  		mm = &init_mm;
+	else
+		mm = current->mm;
 
  	if (dsisr & DSISR_NOHPTE)
  		flags |= HPTE_NOHPTE_UPDATE;

@@ -1537,13 +1561,31 @@ int __hash_page(unsigned long trap, unsigned long ea, unsigned long dsisr,
  	 * 2) user space access kernel space.
  	 */
  	access |= _PAGE_PRIVILEGED;
-	if ((msr & MSR_PR) || (region_id == USER_REGION_ID))
+	if (user_mode(regs) || (region_id == USER_REGION_ID))
  		access &= ~_PAGE_PRIVILEGED;
 
-	if (trap == 0x400)
+	if (regs->trap == 0x400)
  		access |= _PAGE_EXEC;
 
-	return hash_page_mm(mm, ea, access, trap, flags);
+	err = hash_page_mm(mm, ea, access, regs->trap, flags);
+	if (unlikely(err < 0)) {
+		// failed to instert a hash PTE due to an hypervisor error
+		if (user_mode(regs)) {
+			if (IS_ENABLED(CONFIG_PPC_SUBPAGE_PROT) && err == -2)
+				_exception(SIGSEGV, regs, SEGV_ACCERR, ea);
+			else
+				_exception(SIGBUS, regs, BUS_ADRERR, ea);
+		} else {
+			bad_page_fault(regs, ea, SIGBUS);
+		}
+		err = 0;
+
+	} else if (err) {
+page_fault:
+		err = do_page_fault(regs, ea, dsisr);
+	}
+
+	return err;
  }
 
  #ifdef CONFIG_PPC_MM_SLICES

@@ -1843,27 +1885,6 @@ void flush_hash_range(unsigned long number, int local)
  	}
  }
 
-/*
- * low_hash_fault is called when we the low level hash code failed
- * to instert a PTE due to an hypervisor error
- */
-void low_hash_fault(struct pt_regs *regs, unsigned long address, int rc)
-{
-	enum ctx_state prev_state = exception_enter();
-
-	if (user_mode(regs)) {
-#ifdef CONFIG_PPC_SUBPAGE_PROT
-		if (rc == -2)
-			_exception(SIGSEGV, regs, SEGV_ACCERR, address);
-		else
-#endif
-			_exception(SIGBUS, regs, BUS_ADRERR, address);
-	} else
-		bad_page_fault(regs, address, SIGBUS);
-
-	exception_exit(prev_state);
-}
-
  long hpte_insert_repeating(unsigned long hash, unsigned long vpn,
  			   unsigned long pa, unsigned long rflags,
  			   unsigned long vflags, int psize, int ssize)

arch/powerpc/mm/fault.c +10 -9

index 8961b44f350c..77a3155c77b6 100644
@@ -369,7 +369,9 @@ static void sanity_check_fault(bool is_write, bool is_user,
  #define page_fault_is_bad(__err)	(0)
  #elif defined(CONFIG_PPC_8xx)
  #define page_fault_is_bad(__err)	((__err) & DSISR_NOEXEC_OR_G)
-#elif defined(CONFIG_PPC64)
+#elif defined(CONFIG_PPC_BOOK3S_64)
+#define page_fault_is_bad(__err)	((__err) & (DSISR_BAD_FAULT_64S | DSISR_DABRMATCH))
+#elif defined(CONFIG_PPC_BOOK3E_64)
  #define page_fault_is_bad(__err)	((__err) & DSISR_BAD_FAULT_64S)
  #else
  #define page_fault_is_bad(__err)	((__err) & DSISR_BAD_FAULT_32S)

@@ -404,6 +406,9 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
  		return 0;
 
  	if (unlikely(page_fault_is_bad(error_code))) {
+		if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && (error_code & DSISR_DABRMATCH))
+			return -1;
+
  		if (is_user) {
  			_exception(SIGBUS, regs, BUS_OBJERR, address);
  			return 0;

@@ -545,20 +550,39 @@ NOKPROBE_SYMBOL(__do_page_fault);
  int do_page_fault(struct pt_regs *regs, unsigned long address,
  		  unsigned long error_code)
  {
-	const struct exception_table_entry *entry;
  	enum ctx_state prev_state = exception_enter();
-	int rc = __do_page_fault(regs, address, error_code);
-	exception_exit(prev_state);
-	if (likely(!rc))
-		return 0;
+	int err;
 
-	entry = search_exception_tables(regs->nip);
-	if (unlikely(!entry))
-		return rc;
+	err = __do_page_fault(regs, address, error_code);
+	if (unlikely(err)) {
+		const struct exception_table_entry *entry;
 

arch/powerpc/mm/fault.c +9 -0

+		if (err > 0) {
+			__bad_page_fault(regs, address, err);
+			err = 0;
+		} else {
+			/*
+			 * do_break() may change NV GPRS while handling the
+			 * breakpoint. Return -ve to caller to do that.
+			 */
+			do_break(regs, address, error_code);

Le 13/01/2021 à 08:31, Nicholas Piggin a écrit :

Quoted 47 lines

The page fault handling still has some complex logic particularly around hash table handling, in asm. Implement this in C instead.

Signed-off-by Nicholas Piggin <[email protected]>

---
  arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 +
  arch/powerpc/kernel/exceptions-64s.S          | 131 +++---------------
  arch/powerpc/mm/book3s64/hash_utils.c         |  77 ++++++----
  arch/powerpc/mm/fault.c                       |  46 ++++--
  4 files changed, 107 insertions(+), 148 deletions(-)

arch/powerpc/include/asm/book3s/64/mmu-hash.h +1 -0

index 066b1d34c7bc..60a669379aa0 100644
@@ -454,6 +454,7 @@ static inline unsigned long hpt_hash(unsigned long vpn,
  #define HPTE_NOHPTE_UPDATE	0x2
  #define HPTE_USE_KERNEL_KEY	0x4
 
+int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr);
  extern int __hash_page_4K(unsigned long ea, unsigned long access,
  			  unsigned long vsid, pte_t *ptep, unsigned long trap,
  			  unsigned long flags, int ssize, int subpage_prot);

arch/powerpc/kernel/exceptions-64s.S +6 -5

index 6e53f7638737..bcb5e81d2088 100644
@@ -1401,14 +1401,15 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
   *
   * Handling:
   * - Hash MMU
- *   Go to do_hash_page first to see if the HPT can be filled from an entry in
- *   the Linux page table. Hash faults can hit in kernel mode in a fairly
+ *   Go to do_hash_fault, which attempts to fill the HPT from an entry in the
+ *   Linux page table. Hash faults can hit in kernel mode in a fairly
   *   arbitrary state (e.g., interrupts disabled, locks held) when accessing
   *   "non-bolted" regions, e.g., vmalloc space. However these should always be
- *   backed by Linux page tables.
+ *   backed by Linux page table entries.
   *
- *   If none is found, do a Linux page fault. Linux page faults can happen in
- *   kernel mode due to user copy operations of course.
+ *   If no entry is found the Linux page fault handler is invoked (by
+ *   do_hash_fault). Linux page faults can happen in kernel mode due to user
+ *   copy operations of course.
   *
   *   KVM: The KVM HDSI handler may perform a load with MSR[DR]=1 in guest

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(data_access_common) · line 1442

Quoted 5 lines

arch/powerpc/kernel/exceptions-64s.S +0 -0

   *   MMU context, which may cause a DSI in the host, which must go to the

@@ -1439,13 +1440,17 @@ EXC_COMMON_BEGIN(data_access_common)
  	GEN_COMMON data_access
  	ld	r4,_DAR(r1)
  	ld	r5,_DSISR(r1)

We have DSISR here. I think the dispatch between page fault or do_break() should be done here:
- It would be more similar to other arches

- Would avoid doing it also in instruction fault

- Would avoid that -1 return which looks more like a hack.

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(data_access_common) · line 1443

Quoted 1 line

arch/powerpc/kernel/exceptions-64s.S +1 -0

+#ifdef CONFIG_PPC_BOOK3S_64

Seems like you are re-implementing handle_page_fault() inside do_page_fault(). Wouldn't it be possible to keep do_page_fault() as is for the moment and implement a C version of handle_page_fault() ?

Or just keep it in assembly ? It is not that big, keeping it in assembly would keep things more common with PPC32, and would still allow to save NV GPRS only when needed.

Excerpts from Christophe Leroy's message of January 14, 2021 12:12 am:

Quoted 50 lines

Le 13/01/2021 à 08:31, Nicholas Piggin a écrit :

Quoted 47 lines

The page fault handling still has some complex logic particularly around hash table handling, in asm. Implement this in C instead.

Signed-off-by Nicholas Piggin <[email protected]>

---
  arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 +
  arch/powerpc/kernel/exceptions-64s.S          | 131 +++---------------
  arch/powerpc/mm/book3s64/hash_utils.c         |  77 ++++++----
  arch/powerpc/mm/fault.c                       |  46 ++++--
  4 files changed, 107 insertions(+), 148 deletions(-)

arch/powerpc/include/asm/book3s/64/mmu-hash.h +1 -0

index 066b1d34c7bc..60a669379aa0 100644
@@ -454,6 +454,7 @@ static inline unsigned long hpt_hash(unsigned long vpn,
  #define HPTE_NOHPTE_UPDATE	0x2
  #define HPTE_USE_KERNEL_KEY	0x4
 
+int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr);
  extern int __hash_page_4K(unsigned long ea, unsigned long access,
  			  unsigned long vsid, pte_t *ptep, unsigned long trap,
  			  unsigned long flags, int ssize, int subpage_prot);

arch/powerpc/kernel/exceptions-64s.S +6 -5

index 6e53f7638737..bcb5e81d2088 100644
@@ -1401,14 +1401,15 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
   *
   * Handling:
   * - Hash MMU
- *   Go to do_hash_page first to see if the HPT can be filled from an entry in
- *   the Linux page table. Hash faults can hit in kernel mode in a fairly
+ *   Go to do_hash_fault, which attempts to fill the HPT from an entry in the
+ *   Linux page table. Hash faults can hit in kernel mode in a fairly
   *   arbitrary state (e.g., interrupts disabled, locks held) when accessing
   *   "non-bolted" regions, e.g., vmalloc space. However these should always be
- *   backed by Linux page tables.
+ *   backed by Linux page table entries.
   *
- *   If none is found, do a Linux page fault. Linux page faults can happen in
- *   kernel mode due to user copy operations of course.
+ *   If no entry is found the Linux page fault handler is invoked (by
+ *   do_hash_fault). Linux page faults can happen in kernel mode due to user
+ *   copy operations of course.
   *
   *   KVM: The KVM HDSI handler may perform a load with MSR[DR]=1 in guest

Quoted 8 lines

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(data_access_common) · line 1442

Quoted 5 lines

arch/powerpc/kernel/exceptions-64s.S +0 -0

   *   MMU context, which may cause a DSI in the host, which must go to the

@@ -1439,13 +1440,17 @@ EXC_COMMON_BEGIN(data_access_common)
  	GEN_COMMON data_access
  	ld	r4,_DAR(r1)
  	ld	r5,_DSISR(r1)

We have DSISR here. I think the dispatch between page fault or do_break() should be done here:
- It would be more similar to other arches

Other sub-archs?

Quoted 1 line

- Would avoid doing it also in instruction fault

True but it's hidden under an unlikely branch so won't really help instruction fault.

Quoted 1 line

- Would avoid that -1 return which looks more like a hack.

I don't really see it as a hack, we return a code to asm caller to direct whether to restore registers or not, we alrady have this pattern.

(I'm hoping all that might be go away one day by conrolling NV regs from C if we can get good code generation but even if not we still have it in the interrupt returns).

That said I will give it a try here. At very least it might be a better intermediate step.

Excerpts from Nicholas Piggin's message of January 14, 2021 1:24 pm:

Quoted 73 lines

Excerpts from Christophe Leroy's message of January 14, 2021 12:12 am:

Quoted 50 lines

Le 13/01/2021 à 08:31, Nicholas Piggin a écrit :

Quoted 47 lines

The page fault handling still has some complex logic particularly around hash table handling, in asm. Implement this in C instead.

Signed-off-by Nicholas Piggin <[email protected]>

---
   arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 +
   arch/powerpc/kernel/exceptions-64s.S          | 131 +++---------------
   arch/powerpc/mm/book3s64/hash_utils.c         |  77 ++++++----
   arch/powerpc/mm/fault.c                       |  46 ++++--
   4 files changed, 107 insertions(+), 148 deletions(-)

arch/powerpc/include/asm/book3s/64/mmu-hash.h +1 -0

index 066b1d34c7bc..60a669379aa0 100644
@@ -454,6 +454,7 @@ static inline unsigned long hpt_hash(unsigned long vpn,
   #define HPTE_NOHPTE_UPDATE	0x2
   #define HPTE_USE_KERNEL_KEY	0x4
 
+int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr);
   extern int __hash_page_4K(unsigned long ea, unsigned long access,
   			  unsigned long vsid, pte_t *ptep, unsigned long trap,
   			  unsigned long flags, int ssize, int subpage_prot);

arch/powerpc/kernel/exceptions-64s.S +6 -5

index 6e53f7638737..bcb5e81d2088 100644
@@ -1401,14 +1401,15 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
    *
    * Handling:
    * - Hash MMU
- *   Go to do_hash_page first to see if the HPT can be filled from an entry in
- *   the Linux page table. Hash faults can hit in kernel mode in a fairly
+ *   Go to do_hash_fault, which attempts to fill the HPT from an entry in the
+ *   Linux page table. Hash faults can hit in kernel mode in a fairly
    *   arbitrary state (e.g., interrupts disabled, locks held) when accessing
    *   "non-bolted" regions, e.g., vmalloc space. However these should always be
- *   backed by Linux page tables.
+ *   backed by Linux page table entries.
    *
- *   If none is found, do a Linux page fault. Linux page faults can happen in
- *   kernel mode due to user copy operations of course.
+ *   If no entry is found the Linux page fault handler is invoked (by
+ *   do_hash_fault). Linux page faults can happen in kernel mode due to user
+ *   copy operations of course.
    *
    *   KVM: The KVM HDSI handler may perform a load with MSR[DR]=1 in guest

Quoted 8 lines

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(data_access_common) · line 1442

Quoted 5 lines

arch/powerpc/kernel/exceptions-64s.S +0 -0

    *   MMU context, which may cause a DSI in the host, which must go to the

@@ -1439,13 +1440,17 @@ EXC_COMMON_BEGIN(data_access_common)
   	GEN_COMMON data_access
   	ld	r4,_DAR(r1)
   	ld	r5,_DSISR(r1)

We have DSISR here. I think the dispatch between page fault or do_break() should be done here:
- It would be more similar to other arches

Other sub-archs?

Quoted 1 line

- Would avoid doing it also in instruction fault

True but it's hidden under an unlikely branch so won't really help instruction fault.

Quoted 1 line

- Would avoid that -1 return which looks more like a hack.

I don't really see it as a hack, we return a code to asm caller to direct whether to restore registers or not, we alrady have this pattern.

Quoted 6 lines

(I'm hoping all that might be go away one day by conrolling NV regs from C if we can get good code generation but even if not we still have it in the interrupt returns).

That said I will give it a try here. At very least it might be a better intermediate step.

Ah yes, this way doesn't work well for later patches because you end e.g., with the do_break call having to call the interrupt handler wrappers again when they actually expect to be in the asm entry state (e.g., irq soft-mask state) when called, and return via interrupt_return after the exit wrapper runs (which 64s uses to implement better context tracking for example).

That could possibly be hacked up to deal with multiple interrupt wrappers per interrupt, but I'd rather not go backwards.

That does leave the other sub archs as having this issue, but they don't do so much in their handlers. 32 doesn't have soft-mask or context tracking to deal with for example. We will need to fix this up though and unify things more.

Le 14/01/2021 à 13:09, Nicholas Piggin a écrit :

Quoted 88 lines

Excerpts from Nicholas Piggin's message of January 14, 2021 1:24 pm:

Quoted 73 lines

Excerpts from Christophe Leroy's message of January 14, 2021 12:12 am:

Quoted 50 lines

Le 13/01/2021 à 08:31, Nicholas Piggin a écrit :

Quoted 47 lines

The page fault handling still has some complex logic particularly around hash table handling, in asm. Implement this in C instead.

Signed-off-by Nicholas Piggin <[email protected]>

---
   arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 +
   arch/powerpc/kernel/exceptions-64s.S          | 131 +++---------------
   arch/powerpc/mm/book3s64/hash_utils.c         |  77 ++++++----
   arch/powerpc/mm/fault.c                       |  46 ++++--
   4 files changed, 107 insertions(+), 148 deletions(-)

arch/powerpc/include/asm/book3s/64/mmu-hash.h +1 -0

index 066b1d34c7bc..60a669379aa0 100644
@@ -454,6 +454,7 @@ static inline unsigned long hpt_hash(unsigned long vpn,
   #define HPTE_NOHPTE_UPDATE	0x2
   #define HPTE_USE_KERNEL_KEY	0x4
 
+int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr);
   extern int __hash_page_4K(unsigned long ea, unsigned long access,
   			  unsigned long vsid, pte_t *ptep, unsigned long trap,
   			  unsigned long flags, int ssize, int subpage_prot);

arch/powerpc/kernel/exceptions-64s.S +6 -5

index 6e53f7638737..bcb5e81d2088 100644
@@ -1401,14 +1401,15 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
    *
    * Handling:
    * - Hash MMU
- *   Go to do_hash_page first to see if the HPT can be filled from an entry in
- *   the Linux page table. Hash faults can hit in kernel mode in a fairly
+ *   Go to do_hash_fault, which attempts to fill the HPT from an entry in the
+ *   Linux page table. Hash faults can hit in kernel mode in a fairly
    *   arbitrary state (e.g., interrupts disabled, locks held) when accessing
    *   "non-bolted" regions, e.g., vmalloc space. However these should always be
- *   backed by Linux page tables.
+ *   backed by Linux page table entries.
    *
- *   If none is found, do a Linux page fault. Linux page faults can happen in
- *   kernel mode due to user copy operations of course.
+ *   If no entry is found the Linux page fault handler is invoked (by
+ *   do_hash_fault). Linux page faults can happen in kernel mode due to user
+ *   copy operations of course.
    *
    *   KVM: The KVM HDSI handler may perform a load with MSR[DR]=1 in guest

Quoted 8 lines

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(data_access_common) · line 1442

Quoted 5 lines

arch/powerpc/kernel/exceptions-64s.S +0 -0

    *   MMU context, which may cause a DSI in the host, which must go to the

@@ -1439,13 +1440,17 @@ EXC_COMMON_BEGIN(data_access_common)
   	GEN_COMMON data_access
   	ld	r4,_DAR(r1)
   	ld	r5,_DSISR(r1)

We have DSISR here. I think the dispatch between page fault or do_break() should be done here:
- It would be more similar to other arches

Other sub-archs?

Quoted 1 line

- Would avoid doing it also in instruction fault

True but it's hidden under an unlikely branch so won't really help instruction fault.

Quoted 1 line

- Would avoid that -1 return which looks more like a hack.

I don't really see it as a hack, we return a code to asm caller to direct whether to restore registers or not, we alrady have this pattern.

Quoted 6 lines

(I'm hoping all that might be go away one day by conrolling NV regs from C if we can get good code generation but even if not we still have it in the interrupt returns).

That said I will give it a try here. At very least it might be a better intermediate step.

Ah yes, this way doesn't work well for later patches because you end e.g., with the do_break call having to call the interrupt handler wrappers again when they actually expect to be in the asm entry state (e.g., irq soft-mask state) when called, and return via interrupt_return after the exit wrapper runs (which 64s uses to implement better context tracking for example).

Quoted 8 lines

That could possibly be hacked up to deal with multiple interrupt wrappers per interrupt, but I'd rather not go backwards.

That does leave the other sub archs as having this issue, but they don't do so much in their handlers. 32 doesn't have soft-mask or context tracking to deal with for example. We will need to fix this up though and unify things more.

Not sure I understand what you mean exactly.

On the 8xx, do_break() is called by totally different exceptions:
- Exception 0x1c00 Data breakpoint ==> do_break()
- Exception 0x1300 Instruction TLB error ==> handle_page_fault()
- Exception 0x1400 Data TLB error ==> handle_page_fault()

On book3s/32, we now (after my patch ie patch 1 in your series ) have either do_break() or handle_page_fault() being called from very early in ASM.

If you do the same in book3s/64, then there is no issue with interrupt wrappers being called twice, is it ?

Excerpts from Christophe Leroy's message of January 14, 2021 10:25 pm:

Quoted 107 lines

Le 14/01/2021 à 13:09, Nicholas Piggin a écrit :

Quoted 88 lines

Excerpts from Nicholas Piggin's message of January 14, 2021 1:24 pm:

Quoted 73 lines

Excerpts from Christophe Leroy's message of January 14, 2021 12:12 am:

Quoted 50 lines

Le 13/01/2021 à 08:31, Nicholas Piggin a écrit :

Quoted 47 lines

The page fault handling still has some complex logic particularly around hash table handling, in asm. Implement this in C instead.

Signed-off-by Nicholas Piggin <[email protected]>

---
    arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 +
    arch/powerpc/kernel/exceptions-64s.S          | 131 +++---------------
    arch/powerpc/mm/book3s64/hash_utils.c         |  77 ++++++----
    arch/powerpc/mm/fault.c                       |  46 ++++--
    4 files changed, 107 insertions(+), 148 deletions(-)

arch/powerpc/include/asm/book3s/64/mmu-hash.h +1 -0

index 066b1d34c7bc..60a669379aa0 100644
@@ -454,6 +454,7 @@ static inline unsigned long hpt_hash(unsigned long vpn,
    #define HPTE_NOHPTE_UPDATE	0x2
    #define HPTE_USE_KERNEL_KEY	0x4
 
+int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr);
    extern int __hash_page_4K(unsigned long ea, unsigned long access,
    			  unsigned long vsid, pte_t *ptep, unsigned long trap,
    			  unsigned long flags, int ssize, int subpage_prot);

arch/powerpc/kernel/exceptions-64s.S +6 -5

index 6e53f7638737..bcb5e81d2088 100644
@@ -1401,14 +1401,15 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
     *
     * Handling:
     * - Hash MMU
- *   Go to do_hash_page first to see if the HPT can be filled from an entry in
- *   the Linux page table. Hash faults can hit in kernel mode in a fairly
+ *   Go to do_hash_fault, which attempts to fill the HPT from an entry in the
+ *   Linux page table. Hash faults can hit in kernel mode in a fairly
     *   arbitrary state (e.g., interrupts disabled, locks held) when accessing
     *   "non-bolted" regions, e.g., vmalloc space. However these should always be
- *   backed by Linux page tables.
+ *   backed by Linux page table entries.
     *
- *   If none is found, do a Linux page fault. Linux page faults can happen in
- *   kernel mode due to user copy operations of course.
+ *   If no entry is found the Linux page fault handler is invoked (by
+ *   do_hash_fault). Linux page faults can happen in kernel mode due to user
+ *   copy operations of course.
     *
     *   KVM: The KVM HDSI handler may perform a load with MSR[DR]=1 in guest

Quoted 8 lines

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(data_access_common) · line 1442

Quoted 5 lines

arch/powerpc/kernel/exceptions-64s.S +0 -0

     *   MMU context, which may cause a DSI in the host, which must go to the

@@ -1439,13 +1440,17 @@ EXC_COMMON_BEGIN(data_access_common)
    	GEN_COMMON data_access
    	ld	r4,_DAR(r1)
    	ld	r5,_DSISR(r1)

We have DSISR here. I think the dispatch between page fault or do_break() should be done here:
- It would be more similar to other arches

Other sub-archs?

Quoted 1 line

- Would avoid doing it also in instruction fault

True but it's hidden under an unlikely branch so won't really help instruction fault.

Quoted 1 line

- Would avoid that -1 return which looks more like a hack.

I don't really see it as a hack, we return a code to asm caller to direct whether to restore registers or not, we alrady have this pattern.

Quoted 6 lines

(I'm hoping all that might be go away one day by conrolling NV regs from C if we can get good code generation but even if not we still have it in the interrupt returns).

That said I will give it a try here. At very least it might be a better intermediate step.

Ah yes, this way doesn't work well for later patches because you end e.g., with the do_break call having to call the interrupt handler wrappers again when they actually expect to be in the asm entry state (e.g., irq soft-mask state) when called, and return via interrupt_return after the exit wrapper runs (which 64s uses to implement better context tracking for example).

Quoted 8 lines

That could possibly be hacked up to deal with multiple interrupt wrappers per interrupt, but I'd rather not go backwards.

That does leave the other sub archs as having this issue, but they don't do so much in their handlers. 32 doesn't have soft-mask or context tracking to deal with for example. We will need to fix this up though and unify things more.

Not sure I understand what you mean exactly.

On the 8xx, do_break() is called by totally different exceptions:
- Exception 0x1c00 Data breakpoint ==> do_break()
- Exception 0x1300 Instruction TLB error ==> handle_page_fault()
- Exception 0x1400 Data TLB error ==> handle_page_fault()

Quoted 5 lines

On book3s/32, we now (after my patch ie patch 1 in your series ) have either do_break() or handle_page_fault() being called from very early in ASM.

If you do the same in book3s/64, then there is no issue with interrupt wrappers being called twice, is it ?

bad_page_fault is the problem, it has to go afterwards.

Once we have the changed 64s behaviour of do_page_fault, I don't know if there is any point leaving do_break in asm is there? I guess it is neat to treat it quite separately, I might need to count fast path branches... I have done the split anyway already, so I will post your way first.

Le 14/01/2021 à 14:17, Nicholas Piggin a écrit :

Quoted 114 lines

Excerpts from Christophe Leroy's message of January 14, 2021 10:25 pm:

Quoted 107 lines

Le 14/01/2021 à 13:09, Nicholas Piggin a écrit :

Quoted 88 lines

Excerpts from Nicholas Piggin's message of January 14, 2021 1:24 pm:

Quoted 73 lines

Excerpts from Christophe Leroy's message of January 14, 2021 12:12 am:

Quoted 50 lines

Le 13/01/2021 à 08:31, Nicholas Piggin a écrit :

Quoted 47 lines

The page fault handling still has some complex logic particularly around hash table handling, in asm. Implement this in C instead.

Signed-off-by Nicholas Piggin <[email protected]>

---
    arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 +
    arch/powerpc/kernel/exceptions-64s.S          | 131 +++---------------
    arch/powerpc/mm/book3s64/hash_utils.c         |  77 ++++++----
    arch/powerpc/mm/fault.c                       |  46 ++++--
    4 files changed, 107 insertions(+), 148 deletions(-)

arch/powerpc/include/asm/book3s/64/mmu-hash.h +1 -0

index 066b1d34c7bc..60a669379aa0 100644
@@ -454,6 +454,7 @@ static inline unsigned long hpt_hash(unsigned long vpn,
    #define HPTE_NOHPTE_UPDATE	0x2
    #define HPTE_USE_KERNEL_KEY	0x4
 
+int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr);
    extern int __hash_page_4K(unsigned long ea, unsigned long access,
    			  unsigned long vsid, pte_t *ptep, unsigned long trap,
    			  unsigned long flags, int ssize, int subpage_prot);

arch/powerpc/kernel/exceptions-64s.S +6 -5

index 6e53f7638737..bcb5e81d2088 100644
@@ -1401,14 +1401,15 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
     *
     * Handling:
     * - Hash MMU
- *   Go to do_hash_page first to see if the HPT can be filled from an entry in
- *   the Linux page table. Hash faults can hit in kernel mode in a fairly
+ *   Go to do_hash_fault, which attempts to fill the HPT from an entry in the
+ *   Linux page table. Hash faults can hit in kernel mode in a fairly
     *   arbitrary state (e.g., interrupts disabled, locks held) when accessing
     *   "non-bolted" regions, e.g., vmalloc space. However these should always be
- *   backed by Linux page tables.
+ *   backed by Linux page table entries.
     *
- *   If none is found, do a Linux page fault. Linux page faults can happen in
- *   kernel mode due to user copy operations of course.
+ *   If no entry is found the Linux page fault handler is invoked (by
+ *   do_hash_fault). Linux page faults can happen in kernel mode due to user
+ *   copy operations of course.
     *
     *   KVM: The KVM HDSI handler may perform a load with MSR[DR]=1 in guest

Quoted 8 lines

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(data_access_common) · line 1442

Quoted 5 lines

arch/powerpc/kernel/exceptions-64s.S +0 -0

     *   MMU context, which may cause a DSI in the host, which must go to the

@@ -1439,13 +1440,17 @@ EXC_COMMON_BEGIN(data_access_common)
    	GEN_COMMON data_access
    	ld	r4,_DAR(r1)
    	ld	r5,_DSISR(r1)

We have DSISR here. I think the dispatch between page fault or do_break() should be done here:
- It would be more similar to other arches

Other sub-archs?

Quoted 1 line

- Would avoid doing it also in instruction fault

True but it's hidden under an unlikely branch so won't really help instruction fault.

Quoted 1 line

- Would avoid that -1 return which looks more like a hack.

I don't really see it as a hack, we return a code to asm caller to direct whether to restore registers or not, we alrady have this pattern.

Quoted 6 lines

(I'm hoping all that might be go away one day by conrolling NV regs from C if we can get good code generation but even if not we still have it in the interrupt returns).

That said I will give it a try here. At very least it might be a better intermediate step.

Ah yes, this way doesn't work well for later patches because you end e.g., with the do_break call having to call the interrupt handler wrappers again when they actually expect to be in the asm entry state (e.g., irq soft-mask state) when called, and return via interrupt_return after the exit wrapper runs (which 64s uses to implement better context tracking for example).

Quoted 8 lines

That could possibly be hacked up to deal with multiple interrupt wrappers per interrupt, but I'd rather not go backwards.

That does leave the other sub archs as having this issue, but they don't do so much in their handlers. 32 doesn't have soft-mask or context tracking to deal with for example. We will need to fix this up though and unify things more.

Not sure I understand what you mean exactly.

On the 8xx, do_break() is called by totally different exceptions:
- Exception 0x1c00 Data breakpoint ==> do_break()
- Exception 0x1300 Instruction TLB error ==> handle_page_fault()
- Exception 0x1400 Data TLB error ==> handle_page_fault()

Quoted 5 lines

On book3s/32, we now (after my patch ie patch 1 in your series ) have either do_break() or handle_page_fault() being called from very early in ASM.

If you do the same in book3s/64, then there is no issue with interrupt wrappers being called twice, is it ?

Quoted 7 lines

bad_page_fault is the problem, it has to go afterwards.

Once we have the changed 64s behaviour of do_page_fault, I don't know if there is any point leaving do_break in asm is there? I guess it is neat to treat it quite separately, I might need to count fast path branches... I have done the split anyway already, so I will post your way first.

As far as I understand, not taken unlikely branches are costless (at least on book3s/32), so you would only suffer the cost of the logical 'and.' on the value of DSISR that you already have in a register. Should be in the noise.

bad_page_fault() is not in the fast path anymore since we now handle the exception fixup at the end of do_page_fault(). So I think it shouldn't be a concern to call the wrapper again for bad_page_fault()

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

This is done to make the function signatures match other handlers, which will help with a future patch to add wrappers. Explicit arguments could be added for performance but that would require more wrapper macro variants.

Signed-off-by Nicholas Piggin <[email protected]>

---
  arch/powerpc/include/asm/asm-prototypes.h     |  4 ++--
  arch/powerpc/include/asm/book3s/64/mmu-hash.h |  2 +-
  arch/powerpc/include/asm/bug.h                |  2 +-
  arch/powerpc/kernel/entry_32.S                |  6 +-----
  arch/powerpc/kernel/exceptions-64e.S          |  2 --
  arch/powerpc/kernel/exceptions-64s.S          | 14 ++------------
  arch/powerpc/kernel/head_40x.S                | 10 +++++-----
  arch/powerpc/kernel/head_8xx.S                |  6 +++---
  arch/powerpc/kernel/head_book3s_32.S          |  5 ++---
  arch/powerpc/kernel/head_booke.h              |  4 +---
  arch/powerpc/mm/book3s64/hash_utils.c         |  8 +++++---
  arch/powerpc/mm/book3s64/slb.c                | 11 +++++++----
  arch/powerpc/mm/fault.c                       |  7 ++++---
  13 files changed, 34 insertions(+), 47 deletions(-)

arch/powerpc/kernel/entry_32.S +0 -0

index 238eacfda7b0..a32157ce0551 100644

Similar to the previous patch this makes interrupt handler function types more regular so they can be wrapped with the next patch.

bad_page_fault and do_break are not performance critical.

Signed-off-by Nicholas Piggin <[email protected]>

---
  arch/powerpc/include/asm/bug.h             |  4 ++--
  arch/powerpc/include/asm/debug.h           |  3 +--
  arch/powerpc/kernel/entry_32.S             |  3 +--
  arch/powerpc/kernel/exceptions-64e.S       |  3 +--
  arch/powerpc/kernel/exceptions-64s.S       |  3 +--
  arch/powerpc/kernel/head_8xx.S             |  5 ++---
  arch/powerpc/kernel/process.c              |  7 +++----
  arch/powerpc/kernel/traps.c                |  2 +-
  arch/powerpc/mm/book3s64/hash_utils.c      |  4 ++--
  arch/powerpc/mm/book3s64/slb.c             |  2 +-
  arch/powerpc/mm/fault.c                    | 10 +++++-----
  arch/powerpc/platforms/8xx/machine_check.c |  2 +-
  12 files changed, 21 insertions(+), 27 deletions(-)

arch/powerpc/include/asm/bug.h +2 -2

index f7827e993196..4220789b9a97 100644
@@ -112,8 +112,8 @@
 
  struct pt_regs;
  long do_page_fault(struct pt_regs *);
-extern void bad_page_fault(struct pt_regs *, unsigned long, int);
-void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig);
+void bad_page_fault(struct pt_regs *, int);
+void __bad_page_fault(struct pt_regs *regs, int sig);
  extern void _exception(int, struct pt_regs *, int, unsigned long);
  extern void _exception_pkey(struct pt_regs *, unsigned long, int);
  extern void die(const char *, struct pt_regs *, long);

arch/powerpc/include/asm/debug.h +1 -2

index ec57daf87f40..0550eceab3ca 100644
@@ -52,8 +52,7 @@ extern void do_send_trap(struct pt_regs *regs, unsigned long address,
  			 unsigned long error_code, int brkpt);
  #else
 
-extern void do_break(struct pt_regs *regs, unsigned long address,
-		     unsigned long error_code);
+void do_break(struct pt_regs *regs);
  #endif
 
  #endif /* _ASM_POWERPC_DEBUG_H */

arch/powerpc/kernel/entry_32.S +1 -2

index a32157ce0551..a94127eed56b 100644
@@ -673,9 +673,8 @@ handle_page_fault:
  	lwz	r0,_TRAP(r1)
  	clrrwi	r0,r0,1
  	stw	r0,_TRAP(r1)
-	mr	r5,r3
+	mr	r4,r3		/* err arg for bad_page_fault */
  	addi	r3,r1,STACK_FRAME_OVERHEAD
-	lwz	r4,_DAR(r1)
  	bl	__bad_page_fault
  	b	ret_from_except_full
 

arch/powerpc/kernel/exceptions-64e.S +1 -2

index 43e71d86dcbf..52421042a020 100644
@@ -1018,9 +1018,8 @@ storage_fault_common:
  	bne-	1f
  	b	ret_from_except_lite
  1:	bl	save_nvgprs
-	mr	r5,r3
+	mr	r4,r3
  	addi	r3,r1,STACK_FRAME_OVERHEAD
-	ld	r4,_DAR(r1)
  	bl	__bad_page_fault
  	b	ret_from_except
 

arch/powerpc/kernel/exceptions-64s.S +1 -2

index 814cff2c649e..36dea2020ec5 100644
@@ -2136,8 +2136,7 @@ EXC_COMMON_BEGIN(h_data_storage_common)
  	GEN_COMMON h_data_storage
  	addi    r3,r1,STACK_FRAME_OVERHEAD
  BEGIN_MMU_FTR_SECTION
-	ld	r4,_DAR(r1)
-	li	r5,SIGSEGV
+	li	r4,SIGSEGV
  	bl      bad_page_fault
  MMU_FTR_SECTION_ELSE
  	bl      unknown_exception

arch/powerpc/kernel/head_8xx.S +2 -3

index 0b2c247cfdff..7869db974185 100644
@@ -364,10 +364,9 @@ do_databreakpoint:
  	addi	r3,r1,STACK_FRAME_OVERHEAD
  	mfspr	r4,SPRN_BAR
  	stw	r4,_DAR(r11)
-#ifdef CONFIG_VMAP_STACK
-	lwz	r5,_DSISR(r11)
-#else
+#ifndef CONFIG_VMAP_STACK
  	mfspr	r5,SPRN_DSISR
+	stw	r5,_DSISR(r11)
  #endif
  	EXC_XFER_STD(0x1c00, do_break)
 

arch/powerpc/kernel/process.c +3 -4

index a66f435dabbf..4f0f81e9420b 100644
@@ -659,11 +659,10 @@ static void do_break_handler(struct pt_regs *regs)
  	}
  }
 
-void do_break (struct pt_regs *regs, unsigned long address,
-		    unsigned long error_code)
+void do_break(struct pt_regs *regs)
  {
  	current->thread.trap_nr = TRAP_HWBKPT;
-	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
+	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, regs->dsisr,
  			11, SIGSEGV) == NOTIFY_STOP)
  		return;
 

@@ -681,7 +680,7 @@ void do_break (struct pt_regs *regs, unsigned long address,
  		do_break_handler(regs);
 
  	/* Deliver the signal to userspace */
-	force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
+	force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)regs->dar);
  }
  #endif	/* CONFIG_PPC_ADV_DEBUG_REGS */
 

arch/powerpc/kernel/traps.c +1 -1

index 3ec7b443fe6b..f3f6af3141ee 100644
@@ -1612,7 +1612,7 @@ void alignment_exception(struct pt_regs *regs)
  	if (user_mode(regs))
  		_exception(sig, regs, code, regs->dar);
  	else
-		bad_page_fault(regs, regs->dar, sig);
+		bad_page_fault(regs, sig);
 
  bail:
  	exception_exit(prev_state);

arch/powerpc/mm/book3s64/hash_utils.c +2 -2

index 8d014924ee0d..77073a256cff 100644
@@ -1539,7 +1539,7 @@ long do_hash_fault(struct pt_regs *regs)
  	 * the access, or panic if there isn't a handler.
  	 */
  	if (unlikely(in_nmi())) {
-		bad_page_fault(regs, ea, SIGSEGV);
+		bad_page_fault(regs, SIGSEGV);
  		return 0;
  	}
 

@@ -1578,7 +1578,7 @@ long do_hash_fault(struct pt_regs *regs)
  			else
  				_exception(SIGBUS, regs, BUS_ADRERR, ea);
  		} else {
-			bad_page_fault(regs, ea, SIGBUS);
+			bad_page_fault(regs, SIGBUS);
  		}
  		err = 0;
 

arch/powerpc/mm/book3s64/slb.c +1 -1

index 985902ce0272..c581548b533f 100644
@@ -874,7 +874,7 @@ void do_bad_slb_fault(struct pt_regs *regs)
  		if (user_mode(regs))
  			_exception(SIGSEGV, regs, SEGV_BNDERR, regs->dar);
  		else
-			bad_page_fault(regs, regs->dar, SIGSEGV);
+			bad_page_fault(regs, SIGSEGV);
  	} else if (err == -EINVAL) {
  		unrecoverable_exception(regs);
  	} else {

arch/powerpc/mm/fault.c +5 -5

index e170501081a7..36604ff8b3ec 100644
@@ -569,14 +569,14 @@ long do_page_fault(struct pt_regs *regs)
  	/* 32 and 64e handle these errors in asm */
  	if (unlikely(err)) {
  		if (err > 0) {
-			__bad_page_fault(regs, address, err);
+			__bad_page_fault(regs, err);
  			err = 0;
  		} else {
  			/*
  			 * do_break() may change NV GPRS while handling the
  			 * breakpoint. Return -ve to caller to do that.
  			 */
-			do_break(regs, address, error_code);
+			do_break(regs);
  		}
  	}
  #endif

@@ -592,7 +592,7 @@ NOKPROBE_SYMBOL(do_page_fault);
   * It is called from the DSI and ISI handlers in head.S and from some
   * of the procedures in traps.c.
   */
-void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
+void __bad_page_fault(struct pt_regs *regs, int sig)
  {
  	int is_write = page_fault_is_write(regs->dsisr);
 

@@ -630,7 +630,7 @@ void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
  	die("Kernel access of bad area", regs, sig);
  }
 
-void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
+void bad_page_fault(struct pt_regs *regs, int sig)
  {
  	const struct exception_table_entry *entry;
 

@@ -639,5 +639,5 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
  	if (entry)
  		instruction_pointer_set(regs, extable_fixup(entry));
  	else
-		__bad_page_fault(regs, address, sig);
+		__bad_page_fault(regs, sig);
  }

arch/powerpc/platforms/8xx/machine_check.c +1 -1

index 88dedf38eccd..656365975895 100644
@@ -26,7 +26,7 @@ int machine_check_8xx(struct pt_regs *regs)
  	 * to deal with that than having a wart in the mcheck handler.
  	 * -- BenH
  	 */
-	bad_page_fault(regs, regs->dar, SIGBUS);
+	bad_page_fault(regs, SIGBUS);
  	return 1;
  #else
  	return 0;

Le 13/01/2021 à 08:31, Nicholas Piggin a écrit :

Quoted 3 lines

Similar to the previous patch this makes interrupt handler function types more regular so they can be wrapped with the next patch.

Quoted 1 line

bad_page_fault and do_break are not performance critical.

It's a bit different between do_break() and bad_page_fault():
- do_break() is not performance critical for sure
- bad_page_fault(), it doesn't matter, because bad_page_fault() was not using the address param so it doesn't get anything from regs at the end.

Maybe it would be worth splitting in two patches, one for bad_page_fault() and one for do_break()

Add wrapper functions (derived from x86 macros) for interrupt handler functions. This allows interrupt entry code to be written in C.

Signed-off-by Nicholas Piggin <[email protected]>

---
  arch/powerpc/include/asm/asm-prototypes.h     |  29 ---
  arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 -
  arch/powerpc/include/asm/hw_irq.h             |   9 -
  arch/powerpc/include/asm/interrupt.h          | 218 ++++++++++++++++++
  arch/powerpc/include/asm/time.h               |   2 +
  arch/powerpc/kernel/dbell.c                   |  12 +-
  arch/powerpc/kernel/exceptions-64s.S          |   7 +-
  arch/powerpc/kernel/head_book3s_32.S          |   6 +-
  arch/powerpc/kernel/irq.c                     |   3 +-
  arch/powerpc/kernel/mce.c                     |   5 +-
  arch/powerpc/kernel/syscall_64.c              |   1 +
  arch/powerpc/kernel/tau_6xx.c                 |   2 +-
  arch/powerpc/kernel/time.c                    |   3 +-
  arch/powerpc/kernel/traps.c                   |  90 +++++---
  arch/powerpc/kernel/watchdog.c                |   7 +-
  arch/powerpc/kvm/book3s_hv.c                  |   1 +
  arch/powerpc/kvm/book3s_hv_builtin.c          |   1 +
  arch/powerpc/kvm/booke.c                      |   1 +
  arch/powerpc/mm/book3s64/hash_utils.c         |  57 +++--
  arch/powerpc/mm/book3s64/slb.c                |  29 +--
  arch/powerpc/mm/fault.c                       |  15 +-
  arch/powerpc/platforms/powernv/idle.c         |   1 +
  22 files changed, 374 insertions(+), 126 deletions(-)

create mode 100644 arch/powerpc/include/asm/interrupt.h

arch/powerpc/include/asm/interrupt.h new file +0 -0

new file mode 100644
index 000000000000..60363e5eeffa

arch/powerpc/include/asm/interrupt.h new file +26 -0

+/* Interrupt handlers */
+DECLARE_INTERRUPT_HANDLER_NMI(machine_check_early);
+DECLARE_INTERRUPT_HANDLER_NMI(hmi_exception_realmode);
+DECLARE_INTERRUPT_HANDLER(SMIException);
+DECLARE_INTERRUPT_HANDLER(handle_hmi_exception);
+DECLARE_INTERRUPT_HANDLER(instruction_breakpoint_exception);
+DECLARE_INTERRUPT_HANDLER(RunModeException);
+DECLARE_INTERRUPT_HANDLER(single_step_exception);
+DECLARE_INTERRUPT_HANDLER(program_check_exception);
+DECLARE_INTERRUPT_HANDLER(alignment_exception);
+DECLARE_INTERRUPT_HANDLER(StackOverflow);
+DECLARE_INTERRUPT_HANDLER(stack_overflow_exception);
+DECLARE_INTERRUPT_HANDLER(kernel_fp_unavailable_exception);
+DECLARE_INTERRUPT_HANDLER(altivec_unavailable_exception);
+DECLARE_INTERRUPT_HANDLER(vsx_unavailable_exception);
+DECLARE_INTERRUPT_HANDLER(fp_unavailable_tm);
+DECLARE_INTERRUPT_HANDLER(altivec_unavailable_tm);
+DECLARE_INTERRUPT_HANDLER(vsx_unavailable_tm);
+DECLARE_INTERRUPT_HANDLER(facility_unavailable_exception);
+DECLARE_INTERRUPT_HANDLER_ASYNC(TAUException);
+DECLARE_INTERRUPT_HANDLER(altivec_assist_exception);
+DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
+DECLARE_INTERRUPT_HANDLER(kernel_bad_stack);
+DECLARE_INTERRUPT_HANDLER_NMI(system_reset_exception);
+#ifdef CONFIG_PPC_BOOK3S_64
+DECLARE_INTERRUPT_HANDLER_ASYNC(machine_check_exception);

arch/powerpc/include/asm/interrupt.h new file +13 -0

+
+DECLARE_INTERRUPT_HANDLER_ASYNC(timer_interrupt);
+DECLARE_INTERRUPT_HANDLER_NMI(performance_monitor_exception_nmi);
+DECLARE_INTERRUPT_HANDLER_ASYNC(performance_monitor_exception_async);
+DECLARE_INTERRUPT_HANDLER_RAW(performance_monitor_exception);
+DECLARE_INTERRUPT_HANDLER(WatchdogException);
+DECLARE_INTERRUPT_HANDLER(unknown_exception);
+DECLARE_INTERRUPT_HANDLER_ASYNC(unknown_async_exception);
+
+void replay_system_reset(void);
+void replay_soft_interrupts(void);
+
+#endif /* _ASM_POWERPC_INTERRUPT_H */

arch/powerpc/include/asm/time.h +0 -0

index 8f789b597bae..8dd3cdb25338 100644

arch/powerpc/include/asm/time.h +1 -0

+
  /* SPLPAR */
  void accumulate_stolen_time(void);
 

arch/powerpc/kernel/dbell.c +0 -0

index 52680cf07c9d..c0f99f8ffa7d 100644

arch/powerpc/kernel/dbell.c +3 -6

-void doorbell_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_ASYNC(doorbell_exception)
  {
+#ifdef CONFIG_SMP
  	struct pt_regs *old_regs = set_irq_regs(regs);
 
  	irq_enter();

@@ -37,11 +37,7 @@ void doorbell_exception(struct pt_regs *regs)
  	trace_doorbell_exit(regs);
  	irq_exit();
  	set_irq_regs(old_regs);
-}
  #else /* CONFIG_SMP */
-void doorbell_exception(struct pt_regs *regs)
-{
  	printk(KERN_WARNING "Received doorbell on non-smp system\n");
-}
  #endif /* CONFIG_SMP */
-
+}

arch/powerpc/kernel/exceptions-64s.S +0 -0

index 36dea2020ec5..8b0db807974c 100644

arch/powerpc/kernel/exceptions-64s.S +1 -1

  MMU_FTR_SECTION_ELSE
  	bl      unknown_exception
  ALT_MMU_FTR_SECTION_END_IFSET(MMU_FTR_TYPE_RADIX)

@@ -2310,7 +2309,7 @@ EXC_COMMON_BEGIN(h_doorbell_common)
  #ifdef CONFIG_PPC_DOORBELL
  	bl	doorbell_exception
  #else
-	bl	unknown_exception
+	bl	unknown_async_exception
  #endif
  	b	interrupt_return
 

arch/powerpc/kernel/head_book3s_32.S +3 -3

index 94ad1372c490..9b4d5432e2db 100644
@@ -238,8 +238,8 @@ __secondary_hold_acknowledge:
 
  /* System reset */
  /* core99 pmac starts the seconary here by changing the vector, and
-   putting it back to what it was (unknown_exception) when done.  */
-	EXCEPTION(0x100, Reset, unknown_exception, EXC_XFER_STD)
+   putting it back to what it was (unknown_async_exception) when done.  */
+	EXCEPTION(0x100, Reset, unknown_async_exception, EXC_XFER_STD)
 
  /* Machine check */
  /*

@@ -631,7 +631,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_NEED_DTLB_SW_LRU)
  #endif
 
  #ifndef CONFIG_TAU_INT
-#define TAUException	unknown_exception
+#define TAUException	unknown_async_exception
  #endif
 
  	EXCEPTION(0x1300, Trap_13, instruction_breakpoint_exception, EXC_XFER_STD)

arch/powerpc/kernel/irq.c +2 -1

index 6b1eca53e36c..2055d204d08e 100644
@@ -54,6 +54,7 @@
  #include <linux/pgtable.h>
 
  #include <linux/uaccess.h>
+#include <asm/interrupt.h>
  #include <asm/io.h>
  #include <asm/irq.h>
  #include <asm/cache.h>

@@ -665,7 +666,7 @@ void __do_irq(struct pt_regs *regs)
  	irq_exit();
  }
 
-void do_IRQ(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_ASYNC(do_IRQ)
  {
  	struct pt_regs *old_regs = set_irq_regs(regs);
  	void *cursp, *irqsp, *sirqsp;

arch/powerpc/kernel/mce.c +3 -2

index 9f3e133b57b7..54269947113d 100644
@@ -18,6 +18,7 @@
  #include <linux/extable.h>
  #include <linux/ftrace.h>
 
+#include <asm/interrupt.h>
  #include <asm/machdep.h>
  #include <asm/mce.h>
  #include <asm/nmi.h>

@@ -588,7 +589,7 @@ EXPORT_SYMBOL_GPL(machine_check_print_event_info);
   *
   * regs->nip and regs->msr contains srr0 and ssr1.
   */
-long notrace machine_check_early(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_NMI(machine_check_early)
  {
  	long handled = 0;
  	u8 ftrace_enabled = this_cpu_get_ftrace_enabled();

@@ -722,7 +723,7 @@ long hmi_handle_debugtrig(struct pt_regs *regs)
  /*
   * Return values:
   */
-long hmi_exception_realmode(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_NMI(hmi_exception_realmode)
  {	
  	int ret;
 

arch/powerpc/kernel/syscall_64.c +1 -0

index 7c85ed04a164..dd87b2118620 100644
@@ -5,6 +5,7 @@
  #include <asm/kup.h>
  #include <asm/cputime.h>
  #include <asm/hw_irq.h>
+#include <asm/interrupt.h>
  #include <asm/kprobes.h>
  #include <asm/paca.h>
  #include <asm/ptrace.h>

arch/powerpc/kernel/tau_6xx.c +1 -1

index 0b4694b8d248..46b2e5de4ef5 100644
@@ -100,7 +100,7 @@ static void TAUupdate(int cpu)
   * with interrupts disabled
   */
 
-void TAUException(struct pt_regs * regs)
+DEFINE_INTERRUPT_HANDLER_ASYNC(TAUException)
  {
  	int cpu = smp_processor_id();
 

arch/powerpc/kernel/time.c +2 -1

index 67feb3524460..435a251247ed 100644
@@ -56,6 +56,7 @@
  #include <linux/processor.h>
  #include <asm/trace.h>
 
+#include <asm/interrupt.h>
  #include <asm/io.h>
  #include <asm/nvram.h>
  #include <asm/cache.h>

@@ -570,7 +571,7 @@ void arch_irq_work_raise(void)
   * timer_interrupt - gets called when the decrementer overflows,
   * with interrupts disabled.
   */
-void timer_interrupt(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt)
  {
  	struct clock_event_device *evt = this_cpu_ptr(&decrementers);
  	u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);

arch/powerpc/kernel/traps.c +2 -2

index 9b5298c016c7..f4462b481248 100644
@@ -41,6 +41,7 @@
  #include <asm/emulated_ops.h>
  #include <linux/uaccess.h>
  #include <asm/debugfs.h>
+#include <asm/interrupt.h>
  #include <asm/io.h>
  #include <asm/machdep.h>
  #include <asm/rtas.h>

@@ -430,8 +431,7 @@ void hv_nmi_check_nonrecoverable(struct pt_regs *regs)
  	regs->msr &= ~MSR_RI;
  #endif
  }
-
-void system_reset_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_NMI(system_reset_exception)
  {
  	unsigned long hsrr0, hsrr1;
  	bool saved_hsrrs = false;

arch/powerpc/kernel/traps.c +7 -1

  /*
   * I/O accesses can cause machine checks on powermacs.

@@ -788,7 +791,12 @@ int machine_check_generic(struct pt_regs *regs)
  }
  #endif /* everything else */
 
-void machine_check_exception(struct pt_regs *regs)
+
+#ifdef CONFIG_PPC_BOOK3S_64
+DEFINE_INTERRUPT_HANDLER_ASYNC(machine_check_exception)
+#else
+DEFINE_INTERRUPT_HANDLER_NMI(machine_check_exception)
+#endif
  {
  	int recover = 0;
 

@@ -838,13 +846,21 @@ void machine_check_exception(struct pt_regs *regs)
  	if (!(regs->msr & MSR_RI))
  		die("Unrecoverable Machine check", regs, SIGBUS);
 
+#ifdef CONFIG_PPC_BOOK3S_64

arch/powerpc/kernel/traps.c +3 -3

-void SMIException(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(SMIException) /* async? */
  {
  	die("System Management Interrupt", regs, SIGABRT);
  }

@@ -1030,7 +1046,7 @@ static void p9_hmi_special_emu(struct pt_regs *regs)
  }
  #endif /* CONFIG_VSX */
 
-void handle_hmi_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_ASYNC(handle_hmi_exception)
  {
  	struct pt_regs *old_regs;
 

@@ -1059,7 +1075,7 @@ void handle_hmi_exception(struct pt_regs *regs)
  	set_irq_regs(old_regs);
  }
 
-void unknown_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(unknown_exception)
  {
  	enum ctx_state prev_state = exception_enter();
 

arch/powerpc/kernel/traps.c +40 -21

+DEFINE_INTERRUPT_HANDLER_ASYNC(unknown_async_exception)
+{
+	enum ctx_state prev_state = exception_enter();
+
+	printk("Bad trap at PC: %lx, SR: %lx, vector=%lx\n",
+	       regs->nip, regs->msr, regs->trap);
+
+	_exception(SIGTRAP, regs, TRAP_UNK, 0);
+
+	exception_exit(prev_state);
+}
+
+DEFINE_INTERRUPT_HANDLER(instruction_breakpoint_exception)
  {
  	enum ctx_state prev_state = exception_enter();
 

@@ -1086,12 +1114,12 @@ void instruction_breakpoint_exception(struct pt_regs *regs)
  	exception_exit(prev_state);
  }
 
-void RunModeException(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(RunModeException)
  {
  	_exception(SIGTRAP, regs, TRAP_UNK, 0);
  }
 
-void single_step_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(single_step_exception)
  {
  	enum ctx_state prev_state = exception_enter();
 

@@ -1436,7 +1464,7 @@ static int emulate_math(struct pt_regs *regs)
  static inline int emulate_math(struct pt_regs *regs) { return -1; }
  #endif
 
-void program_check_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(program_check_exception)
  {
  	enum ctx_state prev_state = exception_enter();
  	unsigned int reason = get_reason(regs);

@@ -1561,14 +1589,14 @@ NOKPROBE_SYMBOL(program_check_exception);
   * This occurs when running in hypervisor mode on POWER6 or later
   * and an illegal instruction is encountered.
   */
-void emulation_assist_interrupt(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(emulation_assist_interrupt)
  {
  	regs->msr |= REASON_ILLEGAL;
  	program_check_exception(regs);
  }
  NOKPROBE_SYMBOL(emulation_assist_interrupt);
 
-void alignment_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(alignment_exception)
  {
  	enum ctx_state prev_state = exception_enter();
  	int sig, code, fixed = 0;

@@ -1618,7 +1646,7 @@ void alignment_exception(struct pt_regs *regs)
  	exception_exit(prev_state);
  }
 
-void StackOverflow(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(StackOverflow)
  {
  	pr_crit("Kernel stack overflow in process %s[%d], r1=%lx\n",
  		current->comm, task_pid_nr(current), regs->gpr[1]);

@@ -1627,7 +1655,7 @@ void StackOverflow(struct pt_regs *regs)
  	panic("kernel stack overflow");
  }
 
-void stack_overflow_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(stack_overflow_exception)
  {
  	enum ctx_state prev_state = exception_enter();
 

@@ -1636,7 +1664,7 @@ void stack_overflow_exception(struct pt_regs *regs)
  	exception_exit(prev_state);
  }
 
-void kernel_fp_unavailable_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(kernel_fp_unavailable_exception)
  {
  	enum ctx_state prev_state = exception_enter();
 

@@ -1647,7 +1675,7 @@ void kernel_fp_unavailable_exception(struct pt_regs *regs)
  	exception_exit(prev_state);
  }
 
-void altivec_unavailable_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(altivec_unavailable_exception)
  {
  	enum ctx_state prev_state = exception_enter();
 

@@ -1666,7 +1694,7 @@ void altivec_unavailable_exception(struct pt_regs *regs)
  	exception_exit(prev_state);
  }
 
-void vsx_unavailable_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(vsx_unavailable_exception)
  {
  	if (user_mode(regs)) {
  		/* A user program has executed an vsx instruction,

@@ -1697,7 +1725,7 @@ static void tm_unavailable(struct pt_regs *regs)
  	die("Unrecoverable TM Unavailable Exception", regs, SIGABRT);
  }
 
-void facility_unavailable_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(facility_unavailable_exception)
  {
  	static char *facility_strings[] = {
  		[FSCR_FP_LG] = "FPU",

@@ -1817,7 +1845,7 @@ void facility_unavailable_exception(struct pt_regs *regs)
 
  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 
-void fp_unavailable_tm(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(fp_unavailable_tm)
  {
  	/* Note:  This does not handle any kind of FP laziness. */
 

@@ -1850,7 +1878,7 @@ void fp_unavailable_tm(struct pt_regs *regs)
  	tm_recheckpoint(&current->thread);
  }
 
-void altivec_unavailable_tm(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(altivec_unavailable_tm)
  {
  	/* See the comments in fp_unavailable_tm().  This function operates
  	 * the same way.

@@ -1865,7 +1893,7 @@ void altivec_unavailable_tm(struct pt_regs *regs)
  	current->thread.used_vr = 1;
  }
 
-void vsx_unavailable_tm(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(vsx_unavailable_tm)
  {
  	/* See the comments in fp_unavailable_tm().  This works similarly,
  	 * though we're loading both FP and VEC registers in here.

@@ -1890,7 +1918,8 @@ void vsx_unavailable_tm(struct pt_regs *regs)
  }
  #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
 
-static void performance_monitor_exception_nmi(struct pt_regs *regs)
+#ifdef CONFIG_PPC64
+DEFINE_INTERRUPT_HANDLER_NMI(performance_monitor_exception_nmi)
  {
  	nmi_enter();
 

@@ -1899,9 +1928,12 @@ static void performance_monitor_exception_nmi(struct pt_regs *regs)
  	perf_irq(regs);
 
  	nmi_exit();
+
+	return 0;
  }
+#endif
 
-static void performance_monitor_exception_async(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_ASYNC(performance_monitor_exception_async)
  {
  	irq_enter();
 

@@ -1912,7 +1944,7 @@ static void performance_monitor_exception_async(struct pt_regs *regs)
  	irq_exit();
  }
 
-void performance_monitor_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_RAW(performance_monitor_exception)
  {
  	/*
  	 * On 64-bit, if perf interrupts hit in a local_irq_disable

@@ -1924,6 +1956,8 @@ void performance_monitor_exception(struct pt_regs *regs)
  		performance_monitor_exception_nmi(regs);
  	else
  		performance_monitor_exception_async(regs);
+
+	return 0;
  }
 
  #ifdef CONFIG_PPC_ADV_DEBUG_REGS

@@ -2057,7 +2091,7 @@ NOKPROBE_SYMBOL(DebugException);
  #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
 
  #ifdef CONFIG_ALTIVEC
-void altivec_assist_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(altivec_assist_exception)
  {
  	int err;
 

@@ -2199,7 +2233,7 @@ void SPEFloatingPointRoundException(struct pt_regs *regs)
   * in the MSR is 0.  This indicates that SRR0/1 are live, and that
   * we therefore lost state by taking this exception.
   */
-void unrecoverable_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
  {
  	pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
  		 regs->trap, regs->nip, regs->msr);

@@ -2219,7 +2253,7 @@ void __attribute__ ((weak)) WatchdogHandler(struct pt_regs *regs)
  	return;
  }
 
-void WatchdogException(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(WatchdogException) /* XXX NMI? async? */
  {
  	printk (KERN_EMERG "PowerPC Book-E Watchdog Exception\n");
  	WatchdogHandler(regs);

@@ -2230,7 +2264,7 @@ void WatchdogException(struct pt_regs *regs)
   * We enter here if we discover during exception entry that we are
   * running in supervisor mode with a userspace value in the stack pointer.
   */
-void kernel_bad_stack(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(kernel_bad_stack)
  {
  	printk(KERN_EMERG "Bad kernel stack pointer %lx at %lx\n",
  	       regs->gpr[1], regs->nip);

arch/powerpc/kernel/watchdog.c +5 -2

index af3c15a1d41e..824b9376ac35 100644
@@ -26,6 +26,7 @@
  #include <linux/delay.h>
  #include <linux/smp.h>
 
+#include <asm/interrupt.h>
  #include <asm/paca.h>
 
  /*

@@ -247,14 +248,14 @@ static void watchdog_timer_interrupt(int cpu)
  		watchdog_smp_panic(cpu, tb);
  }
 
-void soft_nmi_interrupt(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_NMI(soft_nmi_interrupt)
  {
  	unsigned long flags;
  	int cpu = raw_smp_processor_id();
  	u64 tb;
 
  	if (!cpumask_test_cpu(cpu, &wd_cpus_enabled))
-		return;
+		return 0;
 
  	nmi_enter();
 

@@ -291,6 +292,8 @@ void soft_nmi_interrupt(struct pt_regs *regs)
 
  out:
  	nmi_exit();
+
+	return 0;
  }
 
  static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)

arch/powerpc/kvm/book3s_hv.c +1 -0

index 6f612d240392..3f9a229f82a2 100644
@@ -53,6 +53,7 @@
  #include <asm/cputable.h>
  #include <asm/cacheflush.h>
  #include <linux/uaccess.h>
+#include <asm/interrupt.h>
  #include <asm/io.h>
  #include <asm/kvm_ppc.h>
  #include <asm/kvm_book3s.h>

arch/powerpc/kvm/book3s_hv_builtin.c +1 -0

index 8053efdf7ea7..10fc274bea65 100644
@@ -17,6 +17,7 @@
 
  #include <asm/asm-prototypes.h>
  #include <asm/cputable.h>
+#include <asm/interrupt.h>
  #include <asm/kvm_ppc.h>
  #include <asm/kvm_book3s.h>
  #include <asm/archrandom.h>

arch/powerpc/kvm/booke.c +1 -0

index 288a9820ec01..bd2bb73021d8 100644
@@ -20,6 +20,7 @@
 
  #include <asm/cputable.h>
  #include <linux/uaccess.h>
+#include <asm/interrupt.h>
  #include <asm/kvm_ppc.h>
  #include <asm/cacheflush.h>
  #include <asm/dbell.h>

arch/powerpc/mm/book3s64/hash_utils.c +2 -23

index 77073a256cff..453afb9ae9b4 100644
@@ -38,6 +38,7 @@
  #include <linux/pgtable.h>
 
  #include <asm/debugfs.h>
+#include <asm/interrupt.h>
  #include <asm/processor.h>
  #include <asm/mmu.h>
  #include <asm/mmu_context.h>

@@ -1512,7 +1513,7 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap,
  }
  EXPORT_SYMBOL_GPL(hash_page);
 
-long do_hash_fault(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_RET(__do_hash_fault)
  {
  	unsigned long ea = regs->dar;
  	unsigned long dsisr = regs->dsisr;

@@ -1522,27 +1523,6 @@ long do_hash_fault(struct pt_regs *regs)
  	unsigned int region_id;
  	long err;
 
-	if (unlikely(dsisr & (DSISR_BAD_FAULT_64S | DSISR_DABRMATCH | DSISR_KEYFAULT)))
-		goto page_fault;
-
-	/*
-	 * If we are in an "NMI" (e.g., an interrupt when soft-disabled), then
-	 * don't call hash_page, just fail the fault. This is required to
-	 * prevent re-entrancy problems in the hash code, namely perf
-	 * interrupts hitting while something holds H_PAGE_BUSY, and taking a
-	 * hash fault. See the comment in hash_preload().
-	 *
-	 * We come here as a result of a DSI at a point where we don't want
-	 * to call hash_page, such as when we are accessing memory (possibly
-	 * user memory) inside a PMU interrupt that occurred while interrupts
-	 * were soft-disabled.  We want to invoke the exception handler for
-	 * the access, or panic if there isn't a handler.
-	 */
-	if (unlikely(in_nmi())) {
-		bad_page_fault(regs, SIGSEGV);
-		return 0;
-	}
-
  	region_id = get_region_id(ea);
  	if ((region_id == VMALLOC_REGION_ID) || (region_id == IO_REGION_ID))
  		mm = &init_mm;

@@ -1583,13 +1563,44 @@ long do_hash_fault(struct pt_regs *regs)
  		err = 0;
 
  	} else if (err) {
-page_fault:
  		err = do_page_fault(regs);

arch/powerpc/mm/book3s64/hash_utils.c +27 -0

+{
+	unsigned long dsisr = regs->dsisr;
+
+	if (unlikely(dsisr & (DSISR_BAD_FAULT_64S | DSISR_DABRMATCH | DSISR_KEYFAULT)))
+		return do_page_fault(regs);
+
+	/*
+	 * If we are in an "NMI" (e.g., an interrupt when soft-disabled), then
+	 * don't call hash_page, just fail the fault. This is required to
+	 * prevent re-entrancy problems in the hash code, namely perf
+	 * interrupts hitting while something holds H_PAGE_BUSY, and taking a
+	 * hash fault. See the comment in hash_preload().
+	 *
+	 * We come here as a result of a DSI at a point where we don't want
+	 * to call hash_page, such as when we are accessing memory (possibly
+	 * user memory) inside a PMU interrupt that occurred while interrupts
+	 * were soft-disabled.  We want to invoke the exception handler for
+	 * the access, or panic if there isn't a handler.
+	 */
+	if (unlikely(in_nmi())) {
+		do_bad_page_fault(regs);
+		return 0;
+	}
+
+	return __do_hash_fault(regs);
+}
+
  #ifdef CONFIG_PPC_MM_SLICES
  static bool should_hash_preload(struct mm_struct *mm, unsigned long ea)
  {

arch/powerpc/mm/book3s64/slb.c +5 -12

index c581548b533f..0ae10adae203 100644
@@ -10,6 +10,7 @@
   */
 
  #include <asm/asm-prototypes.h>
+#include <asm/interrupt.h>
  #include <asm/mmu.h>
  #include <asm/mmu_context.h>
  #include <asm/paca.h>

@@ -813,7 +814,7 @@ static long slb_allocate_user(struct mm_struct *mm, unsigned long ea)
  	return slb_insert_entry(ea, context, flags, ssize, false);
  }
 
-long do_slb_fault(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_RAW(do_slb_fault)
  {
  	unsigned long ea = regs->dar;
  	unsigned long id = get_region_id(ea);

@@ -827,17 +828,19 @@ long do_slb_fault(struct pt_regs *regs)
  	/*
  	 * SLB kernel faults must be very careful not to touch anything
  	 * that is not bolted. E.g., PACA and global variables are okay,
-	 * mm->context stuff is not.
-	 *
-	 * SLB user faults can access all of kernel memory, but must be
-	 * careful not to touch things like IRQ state because it is not
-	 * "reconciled" here. The difficulty is that we must use
-	 * fast_exception_return to return from kernel SLB faults without
-	 * looking at possible non-bolted memory. We could test user vs
-	 * kernel faults in the interrupt handler asm and do a full fault,
-	 * reconcile, ret_from_except for user faults which would make them
-	 * first class kernel code. But for performance it's probably nicer
-	 * if they go via fast_exception_return too.
+	 * mm->context stuff is not. SLB user faults may access all of
+	 * memory (and induce one recursive SLB kernel fault), so the
+	 * kernel fault must not trample on the user fault state at those

arch/powerpc/mm/book3s64/slb.c +1 -1

  	 */
  	if (id >= LINEAR_MAP_REGION_ID) {
  		long err;

@@ -866,7 +869,7 @@ long do_slb_fault(struct pt_regs *regs)
  	}
  }
 
-void do_bad_slb_fault(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER(do_bad_slb_fault)
  {
  	int err = regs->result;
 

arch/powerpc/mm/fault.c +14 -1

index 36604ff8b3ec..9e1cd74ebb13 100644
@@ -34,6 +34,7 @@
  #include <linux/uaccess.h>
 
  #include <asm/firmware.h>
+#include <asm/interrupt.h>
  #include <asm/page.h>
  #include <asm/mmu.h>
  #include <asm/mmu_context.h>

@@ -547,7 +548,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
  }
  NOKPROBE_SYMBOL(__do_page_fault);
 
-long do_page_fault(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_RET(do_page_fault)
  {
  	enum ctx_state prev_state = exception_enter();
  	unsigned long address = regs->dar;

@@ -641,3 +642,15 @@ void bad_page_fault(struct pt_regs *regs, int sig)
  	else
  		__bad_page_fault(regs, sig);
  }
+
+#ifdef CONFIG_PPC_BOOK3S_64
+DEFINE_INTERRUPT_HANDLER(__do_bad_page_fault)
+{
+	__bad_page_fault(regs, SIGSEGV);
+}
+
+DEFINE_INTERRUPT_HANDLER(do_bad_page_fault)
+{
+	bad_page_fault(regs, SIGSEGV);
+}
+#endif

arch/powerpc/platforms/powernv/idle.c +1 -0

index e6f461812856..999997d9e9a9 100644
@@ -14,6 +14,7 @@
 
  #include <asm/asm-prototypes.h>
  #include <asm/firmware.h>
+#include <asm/interrupt.h>
  #include <asm/machdep.h>
  #include <asm/opal.h>
  #include <asm/cputhreads.h>

Le 13/01/2021 à 08:32, Nicholas Piggin a écrit :

Quoted 2 lines

Add wrapper functions (derived from x86 macros) for interrupt handler functions. This allows interrupt entry code to be written in C.

Looks like you are doing more than just that is this patch. WOuld be worth splitting in several patches I think,

I'd suggest:
- Other patches for unrelated changes, see below for details
- One patch that brings the wrapper macros
- One patch that uses those macros

Quoted 28 lines

Signed-off-by Nicholas Piggin <[email protected]>

---
  arch/powerpc/include/asm/asm-prototypes.h     |  29 ---
  arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 -
  arch/powerpc/include/asm/hw_irq.h             |   9 -
  arch/powerpc/include/asm/interrupt.h          | 218 ++++++++++++++++++
  arch/powerpc/include/asm/time.h               |   2 +
  arch/powerpc/kernel/dbell.c                   |  12 +-
  arch/powerpc/kernel/exceptions-64s.S          |   7 +-
  arch/powerpc/kernel/head_book3s_32.S          |   6 +-
  arch/powerpc/kernel/irq.c                     |   3 +-
  arch/powerpc/kernel/mce.c                     |   5 +-
  arch/powerpc/kernel/syscall_64.c              |   1 +
  arch/powerpc/kernel/tau_6xx.c                 |   2 +-
  arch/powerpc/kernel/time.c                    |   3 +-
  arch/powerpc/kernel/traps.c                   |  90 +++++---
  arch/powerpc/kernel/watchdog.c                |   7 +-
  arch/powerpc/kvm/book3s_hv.c                  |   1 +
  arch/powerpc/kvm/book3s_hv_builtin.c          |   1 +
  arch/powerpc/kvm/booke.c                      |   1 +
  arch/powerpc/mm/book3s64/hash_utils.c         |  57 +++--
  arch/powerpc/mm/book3s64/slb.c                |  29 +--
  arch/powerpc/mm/fault.c                       |  15 +-
  arch/powerpc/platforms/powernv/idle.c         |   1 +
  22 files changed, 374 insertions(+), 126 deletions(-)

create mode 100644 arch/powerpc/include/asm/interrupt.h

...

Quoted 5 lines

arch/powerpc/include/asm/interrupt.h new file +0 -0

new file mode 100644
index 000000000000..60363e5eeffa

...

Quoted 26 lines

arch/powerpc/include/asm/interrupt.h new file +26 -0

+/* Interrupt handlers */
+DECLARE_INTERRUPT_HANDLER_NMI(machine_check_early);
+DECLARE_INTERRUPT_HANDLER_NMI(hmi_exception_realmode);
+DECLARE_INTERRUPT_HANDLER(SMIException);
+DECLARE_INTERRUPT_HANDLER(handle_hmi_exception);
+DECLARE_INTERRUPT_HANDLER(instruction_breakpoint_exception);
+DECLARE_INTERRUPT_HANDLER(RunModeException);
+DECLARE_INTERRUPT_HANDLER(single_step_exception);
+DECLARE_INTERRUPT_HANDLER(program_check_exception);
+DECLARE_INTERRUPT_HANDLER(alignment_exception);
+DECLARE_INTERRUPT_HANDLER(StackOverflow);
+DECLARE_INTERRUPT_HANDLER(stack_overflow_exception);
+DECLARE_INTERRUPT_HANDLER(kernel_fp_unavailable_exception);
+DECLARE_INTERRUPT_HANDLER(altivec_unavailable_exception);
+DECLARE_INTERRUPT_HANDLER(vsx_unavailable_exception);
+DECLARE_INTERRUPT_HANDLER(fp_unavailable_tm);
+DECLARE_INTERRUPT_HANDLER(altivec_unavailable_tm);
+DECLARE_INTERRUPT_HANDLER(vsx_unavailable_tm);
+DECLARE_INTERRUPT_HANDLER(facility_unavailable_exception);
+DECLARE_INTERRUPT_HANDLER_ASYNC(TAUException);
+DECLARE_INTERRUPT_HANDLER(altivec_assist_exception);
+DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
+DECLARE_INTERRUPT_HANDLER(kernel_bad_stack);
+DECLARE_INTERRUPT_HANDLER_NMI(system_reset_exception);
+#ifdef CONFIG_PPC_BOOK3S_64
+DECLARE_INTERRUPT_HANDLER_ASYNC(machine_check_exception);

arch/powerpc/include/asm/interrupt.h line 205

Quoted 10 lines

arch/powerpc/include/asm/interrupt.h new file +10 -0

+#else
+DECLARE_INTERRUPT_HANDLER_NMI(machine_check_exception);
+#endif
+DECLARE_INTERRUPT_HANDLER(emulation_assist_interrupt);
+DECLARE_INTERRUPT_HANDLER_RAW(do_slb_fault);
+DECLARE_INTERRUPT_HANDLER(do_bad_slb_fault);
+DECLARE_INTERRUPT_HANDLER_RAW(do_hash_fault);
+DECLARE_INTERRUPT_HANDLER_RET(do_page_fault);
+DECLARE_INTERRUPT_HANDLER(__do_bad_page_fault);
+DECLARE_INTERRUPT_HANDLER(do_bad_page_fault);

Missing DECLARE_INTERRUPT_HANDLER(do_break)

Quoted 21 lines

arch/powerpc/include/asm/interrupt.h new file +13 -0

+
+DECLARE_INTERRUPT_HANDLER_ASYNC(timer_interrupt);
+DECLARE_INTERRUPT_HANDLER_NMI(performance_monitor_exception_nmi);
+DECLARE_INTERRUPT_HANDLER_ASYNC(performance_monitor_exception_async);
+DECLARE_INTERRUPT_HANDLER_RAW(performance_monitor_exception);
+DECLARE_INTERRUPT_HANDLER(WatchdogException);
+DECLARE_INTERRUPT_HANDLER(unknown_exception);
+DECLARE_INTERRUPT_HANDLER_ASYNC(unknown_async_exception);
+
+void replay_system_reset(void);
+void replay_soft_interrupts(void);
+
+#endif /* _ASM_POWERPC_INTERRUPT_H */

arch/powerpc/include/asm/time.h +0 -0

index 8f789b597bae..8dd3cdb25338 100644
@@ -102,6 +102,8 @@ DECLARE_PER_CPU(u64, decrementers_next_tb);
  /* Convert timebase ticks to nanoseconds */
  unsigned long long tb_to_ns(unsigned long long tb_ticks);
 

arch/powerpc/include/asm/time.h DECLARE_PER_CPU(u64, decrementers_next_tb); · line 105

Quoted 1 line

arch/powerpc/include/asm/time.h +1 -0

+void timer_broadcast_interrupt(void);

This seems unrelated. I think a separate patch woud be better for moving prototypes without making them wrappers.

Quoted 17 lines

arch/powerpc/include/asm/time.h +1 -0

+
  /* SPLPAR */
  void accumulate_stolen_time(void);
 

arch/powerpc/kernel/dbell.c +1 -0

index 52680cf07c9d..c0f99f8ffa7d 100644
@@ -12,14 +12,14 @@
  #include <linux/hardirq.h>
 
  #include <asm/dbell.h>
+#include <asm/interrupt.h>
  #include <asm/irq_regs.h>
  #include <asm/kvm_ppc.h>
  #include <asm/trace.h>
 

arch/powerpc/kernel/dbell.c line 20

Quoted 2 lines

arch/powerpc/kernel/dbell.c +0 -2

-#ifdef CONFIG_SMP
-

This seems unrelated, is that needed ? What's the problem with having to full versions of doorbell_exception() ?

Quoted 25 lines

arch/powerpc/kernel/dbell.c +3 -6

-void doorbell_exception(struct pt_regs *regs)
+DEFINE_INTERRUPT_HANDLER_ASYNC(doorbell_exception)
  {
+#ifdef CONFIG_SMP
  	struct pt_regs *old_regs = set_irq_regs(regs);
 
  	irq_enter();

@@ -37,11 +37,7 @@ void doorbell_exception(struct pt_regs *regs)
  	trace_doorbell_exit(regs);
  	irq_exit();
  	set_irq_regs(old_regs);
-}
  #else /* CONFIG_SMP */
-void doorbell_exception(struct pt_regs *regs)
-{
  	printk(KERN_WARNING "Received doorbell on non-smp system\n");
-}
  #endif /* CONFIG_SMP */
-
+}

arch/powerpc/kernel/exceptions-64s.S +0 -0

index 36dea2020ec5..8b0db807974c 100644

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(doorbell_super_common) · line 1926

Quoted 6 lines

arch/powerpc/kernel/exceptions-64s.S +1 -1

@@ -1923,7 +1923,7 @@ EXC_COMMON_BEGIN(doorbell_super_common)
  #ifdef CONFIG_PPC_DOORBELL
  	bl	doorbell_exception
  #else
-	bl	unknown_exception
+	bl	unknown_async_exception

Unrelated to wrappers ?

arch/powerpc/kernel/exceptions-64s.S EXC_COMMON_BEGIN(h_data_storage_common) · line 2139

Quoted 10 lines

arch/powerpc/kernel/exceptions-64s.S +1 -2

  #endif
  	b	interrupt_return
 

@@ -2136,8 +2136,7 @@ EXC_COMMON_BEGIN(h_data_storage_common)
  	GEN_COMMON h_data_storage
  	addi    r3,r1,STACK_FRAME_OVERHEAD
  BEGIN_MMU_FTR_SECTION
-	li	r4,SIGSEGV
-	bl      bad_page_fault
+	bl      do_bad_page_fault

Is this name change related ?