Re: [PATCH 2/2] ARC: don't align ret_from_exception function

[Eugeniy Paltsev]

>From: Vineet Gupta <vgupta@xxxxxxxxxxxx>
>Sent: Wednesday, March 11, 2020 20:38
>To: Eugeniy Paltsev; linux-snps-arc@xxxxxxxxxxxxxxxxxxx
>Cc: linux-kernel@xxxxxxxxxxxxxxx; Alexey Brodkin
>Subject: Re: [PATCH 2/2] ARC: don't align ret_from_exception function
>
>On 3/11/20 9:26 AM, Eugeniy Paltsev wrote:
>> ARC have a tricky implemented ret_from_exception function.
>> It is written on ASM and can be called like regular function.
>> However it has another 'entry point' as it can be called as a
>> continuation of EV_Trap function.
>
>It is not really intended / needed to be called form outside ASM - but has to be
>spread across 2 asm files as it is mostly target isa independent, while the
>callers are in separate target isa specific files.
>The ENTRY/END annotations are simply for the dwarf unwinder to stop gracefully.
>
>> As we declare "ret_from_exception" using ENTRY macro it may align
>> "ret_from_exception" by 4 bytes by adding padding before it.
>> "ret_from_exception" doesn't require alignment by 4 bytes
>> (as it doesn't go to vector table, etc...) so let's avoid aligning
>> it by switch from ENTRY (which is alias for SYM_FUNC_START) to
>> SYM_FUNC_START_NOALIGN macro.
>
>I would like to keep it aligned.
>
>ARC700 definitely has penalty for unaligned branch targets, so it will definitely
>suffer there.

Do you know some exact numbers? I'm not an expert in ARC700 (fortunately =)

>
>For HS, unaligned branch targets have no penalty (for the general case atleast),
>but if it does get unaligned, the entire entry prologue code will be - i.e. each
>one of the subsequent 130 or so instructions. That doesn't seem like a good idea
>in general to me.

I really don't insist about applying this patch but I don't understand your
argumentation about ARC HS like at all. Could you please explain in more detail what
130 instructions you are talking about?

>What's weird is I never hit the original problem you ran into - are you using a
>toolchain with the branch relaxation stuff  ?

Looks like we just were lucky enough and didn't change this code a lot.
I faced with this issue when I was developing DSP-related stuff. Initially I
triggered it when I replaced 'mov r10, 0' instruction with 'mov_s r10, 0' and got
weird kernel crush.

It can be reproduced with any toolchain (it's not related to branch relaxation stuff).

>I faked it using a nop_s and the SYM_FUNC_START_NOALIGN( ) annotation and can see
>all instructions getting unaligned.

What is the problem with it? It's totally valid and fine for ARC HS to have instructions
aligned by 2 byte. Or are you talking about ARC700 again?

>Before;
>
>921238e4 <ret_from_exception>:
>921238e4:    ld    r8,[sp,124]
>921238e8:    bbit0.nt    r8,0x7,212
>...
>92123ac8:    rtie
>92123acc <debug_marker_ds>:
>92123acc:    ld    r2,[0x927d81d8]
>92123ad4:    add    r2,r2,0x1
>92123ad8:    st    r2,[0x927d81d8]
>92123ae0:    bmskn    r11,r10,0xf
>92123ae4:    sr    r11,[aux_irq_act]
>92123ae8:    b    -140    ;92123a5c
>
>After:
>
>9212393c:    nop_s
>9212393e <ret_from_exception>:
>9212393e:    ld    r8,[sp,124]
>92123942:    bbit0.nt    r8,0x7,214
>...
>92123b22:    rtie
>92123b26 <debug_marker_ds>:
>92123b26:    ld    r2,[0x927d81d8]
>92123b2e:    add    r2,r2,0x1
>92123b32:    st    r2,[0x927d81d8]
>92123b3a:    bmskn    r11,r10,0xf
>92123b3e:    sr    r11,[aux_irq_act]
>92123b42:    b    -138    ;92123ab6 <debug_marker_syscall>
>92123b46:    nop_s
>
>> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@xxxxxxxxxxxx>
>> ---
>>  arch/arc/kernel/entry-arcv2.S | 2 +-
>>  arch/arc/kernel/entry.S       | 3 +--
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arc/kernel/entry-arcv2.S b/arch/arc/kernel/entry-arcv2.S
>> index 12d5f12d10d2..d482e1507f56 100644
>> --- a/arch/arc/kernel/entry-arcv2.S
>> +++ b/arch/arc/kernel/entry-arcv2.S
>> @@ -260,4 +260,4 @@ debug_marker_ds:
>>       sr      r11, [AUX_IRQ_ACT]
>>       b       .Lexcept_ret
>>
>> -END(ret_from_exception)
>> +SYM_FUNC_END(ret_from_exception)
>> diff --git a/arch/arc/kernel/entry.S b/arch/arc/kernel/entry.S
>> index 60406ec62eb8..79409b518a09 100644
>> --- a/arch/arc/kernel/entry.S
>> +++ b/arch/arc/kernel/entry.S
>> @@ -280,7 +280,7 @@ END(EV_Trap)
>>  ;
>>  ; If ret to user mode do we need to handle signals, schedule() et al.
>>
>> -ENTRY(ret_from_exception)
>> +SYM_FUNC_START_NOALIGN(ret_from_exception)
>>
>>       ; Pre-{IRQ,Trap,Exception} K/U mode from pt_regs->status32
>>       ld  r8, [sp, PT_status32]   ; returning to User/Kernel Mode
>> @@ -373,4 +373,3 @@ resume_kernel_mode:
>>       b       .Lrestore_regs
>>
>>  ##### DONT ADD CODE HERE - .Lrestore_regs actually follows in entry-<isa>.S
>> -