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 <npig...@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@h /* Filter relevant SRR1 bits */ > > Could avoid this, see below > >> andis. r10,r9,SRR1_ISI_NOPT@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@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