LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
To: Vineet Gupta <Vineet.Gupta1@synopsys.com>,
	"linux-snps-arc@lists.infradead.org" 
	<linux-snps-arc@lists.infradead.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Alexey Brodkin" <Alexey.Brodkin@synopsys.com>
Subject: Re: [PATCH 2/2] ARC: don't align ret_from_exception function
Date: Wed, 11 Mar 2020 20:58:14 +0000	[thread overview]
Message-ID: <BY5PR12MB40348292DA303489C1DA2B66DEFC0@BY5PR12MB4034.namprd12.prod.outlook.com> (raw)
In-Reply-To: <448ba208-56a5-d8b2-9675-7be03b872c23@synopsys.com>

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

  reply	other threads:[~2020-03-11 20:58 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-11 16:26 [PATCH 1/2] ARC: define __ALIGN_STR and __ALIGN symbols for ARC Eugeniy Paltsev
2020-03-11 16:26 ` [PATCH 2/2] ARC: don't align ret_from_exception function Eugeniy Paltsev
2020-03-11 17:38   ` Vineet Gupta
2020-03-11 20:58     ` Eugeniy Paltsev [this message]
2020-03-11 21:10       ` Vineet Gupta
2020-03-11 22:28         ` Eugeniy Paltsev
2020-03-11 16:35 ` [PATCH 1/2] ARC: define __ALIGN_STR and __ALIGN symbols for ARC Vineet Gupta

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BY5PR12MB40348292DA303489C1DA2B66DEFC0@BY5PR12MB4034.namprd12.prod.outlook.com \
    --to=eugeniy.paltsev@synopsys.com \
    --cc=Alexey.Brodkin@synopsys.com \
    --cc=Vineet.Gupta1@synopsys.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-snps-arc@lists.infradead.org \
    --subject='Re: [PATCH 2/2] ARC: don'\''t align ret_from_exception function' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).