Re: [PATCH v3 3/5] RISC-V: Align the .init.text section

[Palmer Dabbelt]

On Fri, 18 Dec 2020 00:19:09 PST (-0800), atishp@xxxxxxxxxxxxxx wrote:
> On Thu, Dec 17, 2020 at 12:33 AM Atish Patra <atishp@xxxxxxxxxxxxxx> wrote:
> > On Wed, Dec 16, 2020 at 10:51 PM Palmer Dabbelt <palmer@xxxxxxxxxxx> wrote:
> > >
> > > On Tue, 15 Dec 2020 22:02:54 PST (-0800), Palmer Dabbelt wrote:
> > > > On Wed, 04 Nov 2020 16:04:37 PST (-0800), Atish Patra wrote:
> > > >> In order to improve kernel text protection, we need separate .init.text/
> > > >> .init.data/.text in separate sections. However, RISC-V linker relaxation
> > > >> code is not aware of any alignment between sections. As a result, it may
> > > >> relax any RISCV_CALL relocations between sections to JAL without realizing
> > > >> that an inter section alignment may move the address farther. That may
> > > >> lead to a relocation truncated fit error. However, linker relaxation code
> > > >> is aware of the individual section alignments.
> > > >>
> > > >> The detailed discussion on this issue can be found here.
> > > >> https://github.com/riscv/riscv-gnu-toolchain/issues/738
> > > >>
> > > >> Keep the .init.text section aligned so that linker relaxation will take
> > > >> that as a hint while relaxing inter section calls.
> > > >> Here are the code size changes for each section because of this change.
> > > >>
> > > >> section         change in size (in bytes)
> > > >>   .head.text      +4
> > > >>   .text           +40
> > > >>   .init.text      +6530
> > > >>   .exit.text      +84
> > > >>
> > > >> The only significant increase in size happened for .init.text because
> > > >> all intra relocations also use 2MB alignment.
> > > >>
> > > >> Suggested-by: Jim Wilson <jimw@xxxxxxxxxx>
> > > >> Signed-off-by: Atish Patra <atish.patra@xxxxxxx>
> > > >> ---
> > > >>  arch/riscv/kernel/vmlinux.lds.S | 8 +++++++-
> > > >>  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> > > >> index 3ffbd6cbdb86..cacd7898ba7f 100644
> > > >> --- a/arch/riscv/kernel/vmlinux.lds.S
> > > >> +++ b/arch/riscv/kernel/vmlinux.lds.S
> > > >> @@ -30,7 +30,13 @@ SECTIONS
> > > >>      . = ALIGN(PAGE_SIZE);
> > > >>
> > > >>      __init_begin = .;
> > > >> -    INIT_TEXT_SECTION(PAGE_SIZE)
> > > >> +    __init_text_begin = .;
> > > >> +    .init.text : AT(ADDR(.init.text) - LOAD_OFFSET) ALIGN(SECTION_ALIGN) { \
> > > >> +            _sinittext = .;                                         \
> > > >> +            INIT_TEXT                                               \
> > > >> +            _einittext = .;                                         \
> > > >> +    }
> > > >> +
> > > >>      . = ALIGN(8);
> > > >>      __soc_early_init_table : {
> > > >>              __soc_early_init_table_start = .;
> > > >
> > > > Not sure what's going on here (or why I wasn't catching it earlier), but this
> > > > is breaking boot on one of my test configs.  I'm not getting any Linux boot
> > > > spew, so it's something fairly early.  I'm running defconfig with
> > > >
> > > >     CONFIG_PREEMPT=y
> > > >     CONFIG_DEBUG_PREEMPT=y
> > > >     CONFIG_PROVE_LOCKING=y
> > > >
> > > > It looks like that's been throwing a bunch of warnings for a while, but it did
> > > > at least used to boot.  No idea what PREEMPT would have to do with this, and
> > > > the other two don't generally trigger issues that early in boot (or at least,
> > > > trigger halts that early in boot).
> > > >
>
> I am able to reproduce this issue but with CONFIG_PROVE_LOCKING not
> CONFIG_PREEMPT.
> With CONFIG_PREEMPT, I see a bunch of warnings around smp_processor_id
> but it boots even with 5.0.
> If CONFIG_PROVE_LOCKING is enabled, I am not able to boot using 5.0.
> However, 5.2.0 works fine.
> I am going to take a look at the issue with 5.0 and PROVE_LOCKING.
>
> The config preempt warnings are resolved by the following patch. I
> have tested it in Qemu.
>
> https://patchwork.kernel.org/project/linux-riscv/patch/20201116081238.44223-1-wangkefeng.wang@xxxxxxxxxx/

Thanks!

> > > > There's a bunch of other stuff that depends on this that's on for-next so I
> > > > don't want to just drop it, but I also don't want to break something.  I'm just
> > > > running QEMU's virt board.
> > > >
> >
> > I just verified for-next on QEMU 5.2.0 for virt (RV32,64, nommu) and
> > sifive_u as well.
> > I will give it a try on unleashed tomorrow as well with the above
> > configs enabled.
> >
> > > > I'll take a look again tomorrow night, but if anyone has some time to look
> > > > that'd be great!
> > >
> > > Looks like this breaks on QEMU 5.0.0 but works on 5.2.0.
> >
> > I will take a look tomorrow to check the root cause.
> >
> > I guess technically
> > > that means could be considered a regression, but as we don't really have any
> > > scheme for which old versions of QEMU we support it's not absolute.  I'd
> > > usually err on the side of keeping support for older platforms, but in this
> > > case it's probably just not worth the time so I'm going to just ignore it.
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@xxxxxxxxxxxxxxxxxxx
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> >
> >
> > --
> > Regards,
> > Atish