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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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.

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.leroy@csgroup.eu/

Christophe