[PATCH 1/2] ARC: define __ALIGN_STR and __ALIGN symbols for ARC

[PATCH 1/2] ARC: define __ALIGN_STR and __ALIGN symbols for ARC

[Eugeniy Paltsev]

As of today ARC uses generic __ALIGN_STR and __ALIGN symbol
definitions from "include/linux/linkage.h"
They are defined to ".align 4,0x90" which instructed the assembler
to use `0x90` as a fill byte when aligning functions declared with
ENTRY or similar macroses. This leads to generated weird instructions
in code (when alignment is used) like "ldh_s r12,[r0,0x20]" which is
encoded as 0x9090 for ARCv2.

Let's use ".align 4" which insert a "nop_s" instruction instead.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 arch/arc/include/asm/linkage.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arc/include/asm/linkage.h b/arch/arc/include/asm/linkage.h
index d9ee43c6b7db..fe19f1d412e7 100644
--- a/arch/arc/include/asm/linkage.h
+++ b/arch/arc/include/asm/linkage.h
@@ -29,6 +29,8 @@
 .endm
 
 #define ASM_NL		 `	/* use '`' to mark new line in macro */
+#define __ALIGN		.align 4
+#define __ALIGN_STR	__stringify(__ALIGN)
 
 /* annotation for data we want in DCCM - if enabled in .config */
 .macro ARCFP_DATA nm
-- 
2.21.1

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

[Eugeniy Paltsev]

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.

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.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 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
-
-- 
2.21.1

Re: [PATCH 1/2] ARC: define __ALIGN_STR and __ALIGN symbols for ARC

[Vineet Gupta]

On 3/11/20 9:26 AM, Eugeniy Paltsev wrote:
> As of today ARC uses generic __ALIGN_STR and __ALIGN symbol
> definitions from "include/linux/linkage.h"
> They are defined to ".align 4,0x90" which instructed the assembler
> to use `0x90` as a fill byte when aligning functions declared with
> ENTRY or similar macroses. This leads to generated weird instructions
> in code (when alignment is used) like "ldh_s r12,[r0,0x20]" which is
> encoded as 0x9090 for ARCv2.
> 
> Let's use ".align 4" which insert a "nop_s" instruction instead.
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>

Acked-by: Vineet Gupta <vgupta@synopsys.com>

-Vineet

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

[Vineet Gupta]

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.

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.

What's weird is I never hit the original problem you ran into - are you using a
toolchain with the branch relaxation stuff  ?
I faked it using a nop_s and the SYM_FUNC_START_NOALIGN( ) annotation and can see
all instructions getting unaligned.

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@synopsys.com>
> ---
>  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
> -

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

[Eugeniy Paltsev]

>From: Vineet Gupta <vgupta@synopsys.com>
>Sent: Wednesday, March 11, 2020 20:38
>To: Eugeniy Paltsev; linux-snps-arc@lists.infradead.org
>Cc: linux-kernel@vger.kernel.org; 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@synopsys.com>
>> ---
>>  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
>> -

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

[Vineet Gupta]

On 3/11/20 1:58 PM, Eugeniy Paltsev wrote:
>>
>> 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 =)

I don't remember the exact numbers either.

>> 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.

I knew you would argue hence I already copy/pasted the start and end of the
epilogue already in my prev reply which you didn't care to read thru.

If you start counting instructions from <ret_from_exception> all the way to end of
<debug_marker_ds> there are over 130 instructions all of which can be rendered
unaligned by your patch. What is worse is that this would be unpredictable: the
unaligned case will mostly NOT happen, but a new compiler or some subtle code
change could causing potentially side-effects we won't even know where to look.

>> 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?

Yes I know that already. It is fine in general as I explained earlier, but can
potentially NOT when 130 instructions are unaligned.

>> 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

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

[Eugeniy Paltsev]

>From: Vineet Gupta <vgupta@synopsys.com>
>Sent: Thursday, March 12, 2020 00:10
>To: Eugeniy Paltsev; linux-snps-arc@lists.infradead.org
>Cc: linux-kernel@vger.kernel.org; Alexey Brodkin
>Subject: Re: [PATCH 2/2] ARC: don't align ret_from_exception function
>
>On 3/11/20 1:58 PM, Eugeniy Paltsev wrote:
>>>
>>> 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 =)
>
>I don't remember the exact numbers either.
>
>>> 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.
>
>I knew you would argue hence I already copy/pasted the start and end of the
>epilogue already in my prev reply which you didn't care to read thru.

Well, I beg to differ :)

>If you start counting instructions from <ret_from_exception> all the way to end of
><debug_marker_ds> there are over 130 instructions all of which can be rendered
>unaligned by your patch. What is worse is that this would be unpredictable: the
>unaligned case will mostly NOT happen, but a new compiler or some subtle code
>change could causing potentially side-effects we won't even know where to look.

I'm definitely missing something. Let's forget for a moment about our code
written in ASM and check the the binaries compiled from C sources.
The function is aligned by 4 bytes. However the function body contain the mix of
16 bit, 32 bit, 48 bit and 64 bit instructions. So we have huge amount of
instructions with size > 16 bit which are aligned by 16 bit.

I briefly checked our RPM but I don't see any note that it is OK to have
dozen of unaligned instructions but it is NOT OK to have a hundred of them.

So, what are your concerns based on?

>>> 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?
>
>Yes I know that already. It is fine in general as I explained earlier, but can
>potentially NOT when 130 instructions are unaligned.
>
>>> 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