[PATCH 02/18] powerpc: remove arguments from fault handler functions
Nicholas Piggin
npiggin at gmail.com
Tue Nov 10 19:29:47 AEDT 2020
Excerpts from Christophe Leroy's message of November 6, 2020 5:59 pm:
>
>
> Le 05/11/2020 à 15:34, Nicholas Piggin a écrit :
>> 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 <npiggin at gmail.com>
>> ---
>> arch/powerpc/include/asm/asm-prototypes.h | 4 ++--
>> arch/powerpc/include/asm/bug.h | 4 ++--
>> 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 | 6 ++----
>> 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 | 16 +++++++++-------
>> 11 files changed, 38 insertions(+), 47 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/asm-prototypes.h b/arch/powerpc/include/asm/asm-prototypes.h
>> index d0b832cbbec8..22c9d08fa3a4 100644
>> --- a/arch/powerpc/include/asm/asm-prototypes.h
>> +++ b/arch/powerpc/include/asm/asm-prototypes.h
>> @@ -82,8 +82,8 @@ void kernel_bad_stack(struct pt_regs *regs);
>> void system_reset_exception(struct pt_regs *regs);
>> void machine_check_exception(struct pt_regs *regs);
>> void emulation_assist_interrupt(struct pt_regs *regs);
>> -long do_slb_fault(struct pt_regs *regs, unsigned long ea);
>> -void do_bad_slb_fault(struct pt_regs *regs, unsigned long ea, long err);
>> +long do_slb_fault(struct pt_regs *regs);
>> +void do_bad_slb_fault(struct pt_regs *regs);
>>
>> /* signals, syscalls and interrupts */
>> long sys_swapcontext(struct ucontext __user *old_ctx,
>> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
>> index d714d83bbc7c..2fa0cf6c6011 100644
>> --- a/arch/powerpc/include/asm/bug.h
>> +++ b/arch/powerpc/include/asm/bug.h
>> @@ -111,8 +111,8 @@
>> #ifndef __ASSEMBLY__
>>
>> struct pt_regs;
>> -extern int do_page_fault(struct pt_regs *, unsigned long, unsigned long);
>> -extern int hash__do_page_fault(struct pt_regs *, unsigned long, unsigned long);
>> +extern long do_page_fault(struct pt_regs *);
>> +extern long hash__do_page_fault(struct pt_regs *);
>
> extern is pointless
Thanks. Sorry I'll get it right one day.
>> @@ -191,9 +191,9 @@ _ENTRY(saved_ksp_limit)
>> */
>> START_EXCEPTION(0x0400, InstructionAccess)
>> EXCEPTION_PROLOG
>> - mr r4,r12 /* Pass SRR0 as arg2 */
>> - stw r4, _DEAR(r11)
>> - li r5,0 /* Pass zero as arg3 */
>> + li r5,0
>> + stw r5, _ESR(r11) /* Zero ESR */
>> + stw r12, _DEAR(r11) /* SRR0 as DEAR */
>
> I think we should avoid this, see below
>
>> @@ -356,14 +356,14 @@ DataStoreTLBMiss:
>> . = 0x1300
>> InstructionTLBError:
>> EXCEPTION_PROLOG
>> - mr r4,r12
>> andis. r5,r9,DSISR_SRR1_MATCH_32S at h /* Filter relevant SRR1 bits */
>
> Could avoid this, see below
>
>> andis. r10,r9,SRR1_ISI_NOPT at h
>> beq+ .Litlbie
>> - tlbie r4
>> + tlbie r12
>> /* 0x400 is InstructionAccess exception, needed by bad_page_fault() */
>> .Litlbie:
>> - stw r4, _DAR(r11)
>> + stw r12, _DAR(r11)
>> + stw r5, _DSISR(r11)
>
> And this
>> @@ -369,9 +369,9 @@ BEGIN_MMU_FTR_SECTION
>> bl hash_page
>> END_MMU_FTR_SECTION_IFSET(MMU_FTR_HPTE_TABLE)
>> #endif /* CONFIG_VMAP_STACK */
>> -1: mr r4,r12
>> andis. r5,r9,DSISR_SRR1_MATCH_32S at h /* Filter relevant SRR1 bits */
>> - stw r4, _DAR(r11)
>> + stw r5, _DSISR(r11)
>> + stw r12, _DAR(r11)
>
> And this including the andis.
>
>> @@ -477,9 +477,7 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_EMB_HV)
>> NORMAL_EXCEPTION_PROLOG(INST_STORAGE); \
>> mfspr r5,SPRN_ESR; /* Grab the ESR and save it */ \
>> stw r5,_ESR(r11); \
>> - mr r4,r12; /* Pass SRR0 as arg2 */ \
>> - stw r4, _DEAR(r11); \
>> - li r5,0; /* Pass zero as arg3 */ \
>> + stw r12, _DEAR(r11); /* Pass SRR0 as arg2 */ \
>
> And this
>
[...]
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index e65a49f246ef..390a296b16a3 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -549,11 +549,12 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>> }
>> NOKPROBE_SYMBOL(__do_page_fault);
>>
>> -int do_page_fault(struct pt_regs *regs, unsigned long address,
>> - unsigned long error_code)
>> +long do_page_fault(struct pt_regs *regs)
>> {
>> enum ctx_state prev_state = exception_enter();
>> - int err;
>> + unsigned long address = regs->dar;
>> + unsigned long error_code = regs->dsisr;
>> + long err;
>
> By doing something more or less like this (need to be tuned for bookE as well):
>
> + int is_exec = TRAP(regs) == 0x400;
> + unsigned long address = is_exec ? regs->ssr0 : regs->dar;
> + unsigned long error_code = is_exec ? (regs->ssr1 & DSISR_SRR1_MATCH_32S) : regs->dsisr;
Ah, I didn't see that you saved these in srr0/1 already. Hmm, not in
pt_regs though. thread_struct (VMAP_STACK only)? exception_regs (booke
only)? Doesn't seem so easy.
In general I don't have a problem with some processor specific things
like this in do page_fault though if it speeds things up. If it gets
a bit more complicated then we can have some accsssor function
get_fault_details(regs, &address, &error_code, &is_exec);
>> @@ -580,11 +581,12 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>> NOKPROBE_SYMBOL(do_page_fault);
>>
>> #ifdef CONFIG_PPC_BOOK3S_64
>> -/* Same as do_page_fault but interrupt entry has already run in do_hash_fault */
>> -int hash__do_page_fault(struct pt_regs *regs, unsigned long address,
>> - unsigned long error_code)
>> +/* Same as do_page_fault but no interrupt entry */
>> +long hash__do_page_fault(struct pt_regs *regs)
>> {
>> - int err;
>> + unsigned long address = regs->dar;
>> + unsigned long error_code = regs->dsisr;
>> + long err;
>>
>> err = __do_page_fault(regs, address, error_code);
>> if (unlikely(err)) {
>>
>
> There is probably also something we can simplify around get_and_save_dar_dsisr_on_stack() macro in
> head_32.h, no need to reload DAR, at least for 8xx. Maybe as a followup patch later.
Admittedly my 32-bit knowledge or test environments are not great :(
Thanks,
Nick
More information about the Linuxppc-dev
mailing list