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

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

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