Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
Le 08/09/2020 à 10:29, Christophe Leroy a écrit :
Le 08/09/2020 à 09:48, Nicholas Piggin a écrit :
Excerpts from Christophe Leroy's message of September 7, 2020 9:34 pm:
On Mon, 2020-09-07 at 11:20 +0200, Christophe Leroy wrote:
Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
Make interrupt handlers all just take the pt_regs * argument and load
DAR/DSISR etc from that. Make those that return a value return long.
I like this, it will likely simplify a bit the VMAP_STACK mess.
Not sure it is that easy. My board is stuck after the start of init.
On the 8xx, on Instruction TLB Error exception, we do
andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1
bits */
On book3s/32, on ISI exception we do:
andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1
bits */
On 40x and bookE, on ISI exception we do:
li r5,0 /* Pass zero as arg3 */
And regs->dsisr will just contain nothing
So it means we should at least write back r5 into regs->dsisr from
there
? The performance impact should be minimal as we already write _DAR so
the cache line should already be in the cache.
A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
allthough we don't want to do it for both ISI and DSI at the end, so
you'll have to do it in every head_xxx.S
To get you series build and work, I did the following hacks:
Great, thanks for this.
diff --git a/arch/powerpc/include/asm/interrupt.h
b/arch/powerpc/include/asm/interrupt.h
index acfcc7d5779b..c11045d3113a 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -93,7 +93,9 @@ static inline void interrupt_nmi_exit_prepare(struct
pt_regs *regs, struct inter
{
nmi_exit();
+#ifdef CONFIG_PPC64
this_cpu_set_ftrace_enabled(state->ftrace_enabled);
+#endif
This seems okay, not a hack.
#ifdef CONFIG_PPC_BOOK3S_64
/* Check we didn't change the pending interrupt mask. */
diff --git a/arch/powerpc/kernel/entry_32.S
b/arch/powerpc/kernel/entry_32.S
index f4d0af8e1136..66f7adbe1076 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -663,6 +663,7 @@ ppc_swapcontext:
*/
.globl handle_page_fault
handle_page_fault:
+ stw r5,_DSISR(r1)
addi r3,r1,STACK_FRAME_OVERHEAD
#ifdef CONFIG_PPC_BOOK3S_32
andis. r0,r5,DSISR_DABRMATCH@h
Is this what you want to do for 32, or do you want to seperate
ISI and DSI sides?
No I think we want to separate ISI and DSI sides.
And I think the specific filtering done in ISI could be done in
do_page_fault() in C. Ok, it would make a special handling for is_exec
but there are already several places where the behaviour differs based
on is_exec.
The only thing we need to keep at ASM level is the DABR stuff for
calling do_break() in handle_page_fault(), because it is used to decide
whether full regs are saved or not. But it could be a test done earlier
in the prolog and the result being kept in one of the CR bits.
Looking at it once more, I'm wondering whether we really need a full regs.
Before commit
https://github.com/linuxppc/linux/commit/d300627c6a53693fb01479b59b0cdd293761b1fa#diff-f9658f412252f3bb3093e0a95b37f3ac
do_break() was called from do_page_fault() without a full regs set.
Christophe
Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
Le 08/09/2020 à 09:48, Nicholas Piggin a écrit :
Excerpts from Christophe Leroy's message of September 7, 2020 9:34 pm:
On Mon, 2020-09-07 at 11:20 +0200, Christophe Leroy wrote:
Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
Make interrupt handlers all just take the pt_regs * argument and load
DAR/DSISR etc from that. Make those that return a value return long.
I like this, it will likely simplify a bit the VMAP_STACK mess.
Not sure it is that easy. My board is stuck after the start of init.
On the 8xx, on Instruction TLB Error exception, we do
andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
On book3s/32, on ISI exception we do:
andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
On 40x and bookE, on ISI exception we do:
li r5,0/* Pass zero as arg3 */
And regs->dsisr will just contain nothing
So it means we should at least write back r5 into regs->dsisr from there
? The performance impact should be minimal as we already write _DAR so
the cache line should already be in the cache.
A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
allthough we don't want to do it for both ISI and DSI at the end, so
you'll have to do it in every head_xxx.S
To get you series build and work, I did the following hacks:
Great, thanks for this.
diff --git a/arch/powerpc/include/asm/interrupt.h
b/arch/powerpc/include/asm/interrupt.h
index acfcc7d5779b..c11045d3113a 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -93,7 +93,9 @@ static inline void interrupt_nmi_exit_prepare(struct
pt_regs *regs, struct inter
{
nmi_exit();
+#ifdef CONFIG_PPC64
this_cpu_set_ftrace_enabled(state->ftrace_enabled);
+#endif
This seems okay, not a hack.
#ifdef CONFIG_PPC_BOOK3S_64
/* Check we didn't change the pending interrupt mask. */
diff --git a/arch/powerpc/kernel/entry_32.S
b/arch/powerpc/kernel/entry_32.S
index f4d0af8e1136..66f7adbe1076 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -663,6 +663,7 @@ ppc_swapcontext:
*/
.globl handle_page_fault
handle_page_fault:
+ stw r5,_DSISR(r1)
addir3,r1,STACK_FRAME_OVERHEAD
#ifdef CONFIG_PPC_BOOK3S_32
andis. r0,r5,DSISR_DABRMATCH@h
Is this what you want to do for 32, or do you want to seperate
ISI and DSI sides?
No I think we want to separate ISI and DSI sides.
And I think the specific filtering done in ISI could be done in
do_page_fault() in C. Ok, it would make a special handling for is_exec
but there are already several places where the behaviour differs based
on is_exec.
The only thing we need to keep at ASM level is the DABR stuff for
calling do_break() in handle_page_fault(), because it is used to decide
whether full regs are saved or not. But it could be a test done earlier
in the prolog and the result being kept in one of the CR bits.
That way we can avoid reloading dsisr and dar from the stack, especially
for VMAP stack as they are saved quite early in the prolog.
Or we can take them out of the thread struct and save them in the stack
a little latter in the prolog, but then we have to keep the RI bit a bit
cleared longer.
While we are playing with do_page_fault(), did you have a look at the
series I sent
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7baae4086cbb9ffb08c933b065ff7d29dbc03dd6.1596734104.git.christophe.le...@csgroup.eu/
Christophe
Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
Excerpts from Christophe Leroy's message of September 7, 2020 9:34 pm:
> On Mon, 2020-09-07 at 11:20 +0200, Christophe Leroy wrote:
>>
>> Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
>> > Make interrupt handlers all just take the pt_regs * argument and load
>> > DAR/DSISR etc from that. Make those that return a value return long.
>>
>> I like this, it will likely simplify a bit the VMAP_STACK mess.
>>
>> Not sure it is that easy. My board is stuck after the start of init.
>>
>>
>> On the 8xx, on Instruction TLB Error exception, we do
>>
>> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>>
>> On book3s/32, on ISI exception we do:
>> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>>
>> On 40x and bookE, on ISI exception we do:
>> li r5,0/* Pass zero as arg3 */
>>
>>
>> And regs->dsisr will just contain nothing
>>
>> So it means we should at least write back r5 into regs->dsisr from there
>> ? The performance impact should be minimal as we already write _DAR so
>> the cache line should already be in the cache.
>>
>> A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
>> allthough we don't want to do it for both ISI and DSI at the end, so
>> you'll have to do it in every head_xxx.S
>
> To get you series build and work, I did the following hacks:
Great, thanks for this.
> diff --git a/arch/powerpc/include/asm/interrupt.h
> b/arch/powerpc/include/asm/interrupt.h
> index acfcc7d5779b..c11045d3113a 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -93,7 +93,9 @@ static inline void interrupt_nmi_exit_prepare(struct
> pt_regs *regs, struct inter
> {
> nmi_exit();
>
> +#ifdef CONFIG_PPC64
> this_cpu_set_ftrace_enabled(state->ftrace_enabled);
> +#endif
This seems okay, not a hack.
> #ifdef CONFIG_PPC_BOOK3S_64
> /* Check we didn't change the pending interrupt mask. */
> diff --git a/arch/powerpc/kernel/entry_32.S
> b/arch/powerpc/kernel/entry_32.S
> index f4d0af8e1136..66f7adbe1076 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -663,6 +663,7 @@ ppc_swapcontext:
> */
> .globl handle_page_fault
> handle_page_fault:
> + stw r5,_DSISR(r1)
> addir3,r1,STACK_FRAME_OVERHEAD
> #ifdef CONFIG_PPC_BOOK3S_32
> andis. r0,r5,DSISR_DABRMATCH@h
Is this what you want to do for 32, or do you want to seperate
ISI and DSI sides?
Thanks,
Nick
Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
Excerpts from Christophe Leroy's message of September 7, 2020 7:20 pm: > > > Le 05/09/2020 à 19:43, Nicholas Piggin a écrit : >> Make interrupt handlers all just take the pt_regs * argument and load >> DAR/DSISR etc from that. Make those that return a value return long. > > I like this, it will likely simplify a bit the VMAP_STACK mess. > > Not sure it is that easy. My board is stuck after the start of init. > > > On the 8xx, on Instruction TLB Error exception, we do > > andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */ > > On book3s/32, on ISI exception we do: > andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */ > > On 40x and bookE, on ISI exception we do: > li r5,0/* Pass zero as arg3 */ > > > And regs->dsisr will just contain nothing > > So it means we should at least write back r5 into regs->dsisr from there > ? The performance impact should be minimal as we already write _DAR so > the cache line should already be in the cache. Yes, I think that would be required. Sorry I didn't look closely at 32 bit. > A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick, > allthough we don't want to do it for both ISI and DSI at the end, so > you'll have to do it in every head_xxx.S > > > While you are at it, it would probably also make sense to do remove the > address param of bad_page_fault(), there is no point in loading back > regs->dar in handle_page_fault() and machine_check_8xx() and > alignment_exception(), just read regs->dar in bad_page_fault() > > The case of do_break() should also be looked at. Yeah that's valid, I didn't do that because bad_page_fault was also being called from asm, but an incremental patch should be quite easy. > Why changing return code from int to long ? Oh it's to make the next patch work without any changes to function prototypes. Some handlers are returning int, others long. There is no reason not to just return long AFAIKS so that's what I changed to. Thanks, Nick
Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
On Mon, 2020-09-07 at 11:20 +0200, Christophe Leroy wrote:
>
> Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
> > Make interrupt handlers all just take the pt_regs * argument and load
> > DAR/DSISR etc from that. Make those that return a value return long.
>
> I like this, it will likely simplify a bit the VMAP_STACK mess.
>
> Not sure it is that easy. My board is stuck after the start of init.
>
>
> On the 8xx, on Instruction TLB Error exception, we do
>
> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>
> On book3s/32, on ISI exception we do:
> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>
> On 40x and bookE, on ISI exception we do:
> li r5,0/* Pass zero as arg3 */
>
>
> And regs->dsisr will just contain nothing
>
> So it means we should at least write back r5 into regs->dsisr from there
> ? The performance impact should be minimal as we already write _DAR so
> the cache line should already be in the cache.
>
> A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
> allthough we don't want to do it for both ISI and DSI at the end, so
> you'll have to do it in every head_xxx.S
To get you series build and work, I did the following hacks:
diff --git a/arch/powerpc/include/asm/interrupt.h
b/arch/powerpc/include/asm/interrupt.h
index acfcc7d5779b..c11045d3113a 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -93,7 +93,9 @@ static inline void interrupt_nmi_exit_prepare(struct
pt_regs *regs, struct inter
{
nmi_exit();
+#ifdef CONFIG_PPC64
this_cpu_set_ftrace_enabled(state->ftrace_enabled);
+#endif
#ifdef CONFIG_PPC_BOOK3S_64
/* Check we didn't change the pending interrupt mask. */
diff --git a/arch/powerpc/kernel/entry_32.S
b/arch/powerpc/kernel/entry_32.S
index f4d0af8e1136..66f7adbe1076 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -663,6 +663,7 @@ ppc_swapcontext:
*/
.globl handle_page_fault
handle_page_fault:
+ stw r5,_DSISR(r1)
addir3,r1,STACK_FRAME_OVERHEAD
#ifdef CONFIG_PPC_BOOK3S_32
andis. r0,r5,DSISR_DABRMATCH@h
---
Christophe
Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
Le 05/09/2020 à 19:43, Nicholas Piggin a écrit : Make interrupt handlers all just take the pt_regs * argument and load DAR/DSISR etc from that. Make those that return a value return long. I like this, it will likely simplify a bit the VMAP_STACK mess. Not sure it is that easy. My board is stuck after the start of init. On the 8xx, on Instruction TLB Error exception, we do andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */ On book3s/32, on ISI exception we do: andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */ On 40x and bookE, on ISI exception we do: li r5,0/* Pass zero as arg3 */ And regs->dsisr will just contain nothing So it means we should at least write back r5 into regs->dsisr from there ? The performance impact should be minimal as we already write _DAR so the cache line should already be in the cache. A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick, allthough we don't want to do it for both ISI and DSI at the end, so you'll have to do it in every head_xxx.S While you are at it, it would probably also make sense to do remove the address param of bad_page_fault(), there is no point in loading back regs->dar in handle_page_fault() and machine_check_8xx() and alignment_exception(), just read regs->dar in bad_page_fault() The case of do_break() should also be looked at. Why changing return code from int to long ? Christophe This is done to make the function signatures match more closely, which will help with a future patch to add wrappers. Explicit arguments could be re-added for performance in future but that would require more complex wrapper macros. Signed-off-by: Nicholas Piggin --- 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/mm/book3s64/hash_utils.c | 8 +--- arch/powerpc/mm/book3s64/slb.c| 11 +++ arch/powerpc/mm/fault.c | 16 +--- 7 files changed, 27 insertions(+), 32 deletions(-) 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 *); no extern extern void bad_page_fault(struct pt_regs *, unsigned long, int); extern void _exception(int, struct pt_regs *, int, unsigned long); extern void _exception_pkey(struct pt_regs *, unsigned long, int); Christophe
[RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
Make interrupt handlers all just take the pt_regs * argument and load DAR/DSISR etc from that. Make those that return a value return long. This is done to make the function signatures match more closely, which will help with a future patch to add wrappers. Explicit arguments could be re-added for performance in future but that would require more complex wrapper macros. Signed-off-by: Nicholas Piggin --- 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/mm/book3s64/hash_utils.c | 8 +--- arch/powerpc/mm/book3s64/slb.c| 11 +++ arch/powerpc/mm/fault.c | 16 +--- 7 files changed, 27 insertions(+), 32 deletions(-) diff --git a/arch/powerpc/include/asm/asm-prototypes.h b/arch/powerpc/include/asm/asm-prototypes.h index de14b1a34d56..fffac9de2922 100644 --- a/arch/powerpc/include/asm/asm-prototypes.h +++ b/arch/powerpc/include/asm/asm-prototypes.h @@ -81,8 +81,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 void bad_page_fault(struct pt_regs *, unsigned long, int); extern void _exception(int, struct pt_regs *, int, unsigned long); extern void _exception_pkey(struct pt_regs *, unsigned long, int); diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S index d9ed79415100..5988d61783b5 100644 --- a/arch/powerpc/kernel/exceptions-64e.S +++ b/arch/powerpc/kernel/exceptions-64e.S @@ -1012,8 +1012,6 @@ storage_fault_common: std r14,_DAR(r1) std r15,_DSISR(r1) addir3,r1,STACK_FRAME_OVERHEAD - mr r4,r14 - mr r5,r15 ld r14,PACA_EXGEN+EX_R14(r13) ld r15,PACA_EXGEN+EX_R15(r13) bl do_page_fault diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S index f830b893fe03..1f34cfd1887c 100644 --- a/arch/powerpc/kernel/exceptions-64s.S +++ b/arch/powerpc/kernel/exceptions-64s.S @@ -1437,8 +1437,6 @@ EXC_VIRT_BEGIN(data_access, 0x4300, 0x80) EXC_VIRT_END(data_access, 0x4300, 0x80) EXC_COMMON_BEGIN(data_access_common) GEN_COMMON data_access - ld r4,_DAR(r1) - ld r5,_DSISR(r1) addir3,r1,STACK_FRAME_OVERHEAD BEGIN_MMU_FTR_SECTION bl do_hash_fault @@ -1491,10 +1489,9 @@ EXC_VIRT_BEGIN(data_access_slb, 0x4380, 0x80) EXC_VIRT_END(data_access_slb, 0x4380, 0x80) EXC_COMMON_BEGIN(data_access_slb_common) GEN_COMMON data_access_slb - ld r4,_DAR(r1) - addir3,r1,STACK_FRAME_OVERHEAD BEGIN_MMU_FTR_SECTION /* HPT case, do SLB fault */ + addir3,r1,STACK_FRAME_OVERHEAD bl do_slb_fault cmpdi r3,0 bne-1f @@ -1506,8 +1503,6 @@ MMU_FTR_SECTION_ELSE ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX) std r3,RESULT(r1) RECONCILE_IRQ_STATE(r10, r11) - ld r4,_DAR(r1) - ld r5,RESULT(r1) addir3,r1,STACK_FRAME_OVERHEAD bl do_bad_slb_fault b interrupt_return @@ -1542,8 +1537,6 @@ EXC_VIRT_BEGIN(instruction_access, 0x4400, 0x80) EXC_VIRT_END(instruction_access, 0x4400, 0x80) EXC_COMMON_BEGIN(instruction_access_common) GEN_COMMON instruction_access - ld r4,_DAR(r1) - ld r5,_DSISR(r1) addir3,r1,STACK_FRAME_OVERHEAD BEGIN_MMU_FTR_SECTION bl do_hash_fault @@ -1587,10 +1580,9 @@ EXC_VIRT_BEGIN(instruction_access_slb, 0x4480, 0x80) EXC_VIRT_END(instruction_access_slb, 0x4480, 0x80) EXC_COMMON_BEGIN(instruction_access_slb_common) GEN_COMMON instruction_access_slb - ld r4,_DAR(r1) - addir3,r1,STACK_FRAME_OVERHEAD BEGIN_MMU_FTR_SECTION /* HPT case, do SLB fault */ + addir3,r1,STACK_FRAME_OVERHEAD bl do_slb_fault cmpdi r3,0 bne-
