c6x linker issue on linux-next-20160808 + some linker table work

c6x linker issue on linux-next-20160808 + some linker table work

[Luis R. Rodriguez]

Mark, Aurelien,

I've run into a linker (ld) issue caused by the linker table work I've
been working on [0]. I looked into this and for the life of me, I
cannot comprehend what the problem is, so was hoping you folks might
be able to chime in.

A snapshot you can use is available here:

https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160808-linker-table-v4

The commit that causes the issue is "firmware: port built-in section
to linker table":

https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/commit/?h=20160808-linker-table-v4&id=2f4516be8c89b3ca008962798d44b54751e62844

its the first use of linker tables in the kernel, but I can't figure
out why this error comes up, nor how to fix it. Any ideas?

[0] lkml.kernel.org/r/1469222687-1600-1-git-send-email-mcgrof@kernel.org

  Luis

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Guenter Roeck]

On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> Mark, Aurelien,
>
> I've run into a linker (ld) issue caused by the linker table work I've
> been working on [0]. I looked into this and for the life of me, I
> cannot comprehend what the problem is, so was hoping you folks might
> be able to chime in.
>

For reference, the error is

c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined

Toolchain used is:

c6x-elf-gcc (GCC) 5.2.0
GNU ld (GNU Binutils) 2.25

Guenter

> A snapshot you can use is available here:
>
> https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160808-linker-table-v4
>
> The commit that causes the issue is "firmware: port built-in section
> to linker table":
>
> https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/commit/?h=20160808-linker-table-v4&id=2f4516be8c89b3ca008962798d44b54751e62844
>
> its the first use of linker tables in the kernel, but I can't figure
> out why this error comes up, nor how to fix it. Any ideas?
>
> [0] lkml.kernel.org/r/1469222687-1600-1-git-send-email-mcgrof@kernel.org
>
>   Luis
>

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > 
> > Mark, Aurelien,
> > 
> > I've run into a linker (ld) issue caused by the linker table work I've
> > been working on [0]. I looked into this and for the life of me, I
> > cannot comprehend what the problem is, so was hoping you folks might
> > be able to chime in.
> > 
> For reference, the error is
> 
> c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined

DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.

> 
> Toolchain used is:
> 
> c6x-elf-gcc (GCC) 5.2.0
> GNU ld (GNU Binutils) 2.25
> 
> Guenter
> 
> > 
> > A snapshot you can use is available here:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160808-linker-table-v4
> > 
> > The commit that causes the issue is "firmware: port built-in section
> > to linker table":
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/commit/?h=20160808-linker-table-v4&id=2f4516be8c89b3ca008962798d44b54751e62844
> > 
> > its the first use of linker tables in the kernel, but I can't figure
> > out why this error comes up, nor how to fix it. Any ideas?
> > 
> > [0] lkml.kernel.org/r/1469222687-1600-1-git-send-email-mcgrof@kernel.org
> > 
> >   Luis
> > 

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Luis R. Rodriguez]

On Tue, Aug 09, 2016 at 01:04:00PM -0400, Mark Salter wrote:
> On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> > On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > > 
> > > Mark, Aurelien,
> > > 
> > > I've run into a linker (ld) issue caused by the linker table work I've
> > > been working on [0]. I looked into this and for the life of me, I
> > > cannot comprehend what the problem is, so was hoping you folks might
> > > be able to chime in.
> > > 
> > For reference, the error is
> > 
> > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> 
> DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
> be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.

I see -mno-dsbt on arch/c6x/Makefile already -- however at link time this is
an issue if linker tables are used it seems. Do you have any other recommendation?

I will note that it would seem that even i386 and x86-64 compiler/binutils seem
to have relocation issues on older compiler/binutils, for instance:

gcc-4.7.2
binutils-2.22 

Yields:

x86_64:allyesconfig || x86_64:allmodconfig
Invalid absolute R_X86_64_64 relocation: __per_cpu_load 

i386:defconfig
Invalid absolute R_386_32 relocation: __vvar_page

This issue on x86 is not observed as so far as of gcc 5.2.1
and binutils-2.26.1.

hpa -- if you can think of a work around for this for older compilers/linkers
let me know... unless we are OK to increase the requirements for x86.

  Luis

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Tue, 2016-08-09 at 20:40 +0200, Luis R. Rodriguez wrote:
> On Tue, Aug 09, 2016 at 01:04:00PM -0400, Mark Salter wrote:
> > 
> > On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> > > 
> > > On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > > > 
> > > > 
> > > > Mark, Aurelien,
> > > > 
> > > > I've run into a linker (ld) issue caused by the linker table work I've
> > > > been working on [0]. I looked into this and for the life of me, I
> > > > cannot comprehend what the problem is, so was hoping you folks might
> > > > be able to chime in.
> > > > 
> > > For reference, the error is
> > > 
> > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
> > be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.
> I see -mno-dsbt on arch/c6x/Makefile already -- however at link time this is
> an issue if linker tables are used it seems. Do you have any other recommendation?
> 

I'm looking at it now...

> I will note that it would seem that even i386 and x86-64 compiler/binutils seem
> to have relocation issues on older compiler/binutils, for instance:
> 
> gcc-4.7.2
> binutils-2.22 
> 
> Yields:
> 
> x86_64:allyesconfig || x86_64:allmodconfig
> Invalid absolute R_X86_64_64 relocation: __per_cpu_load 
> 
> i386:defconfig
> Invalid absolute R_386_32 relocation: __vvar_page
> 
> This issue on x86 is not observed as so far as of gcc 5.2.1
> and binutils-2.26.1.
> 
> hpa -- if you can think of a work around for this for older compilers/linkers
> let me know... unless we are OK to increase the requirements for x86.
> 
>   Luis

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Tue, 2016-08-09 at 20:40 +0200, Luis R. Rodriguez wrote:
> On Tue, Aug 09, 2016 at 01:04:00PM -0400, Mark Salter wrote:
> > 
> > On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> > > 
> > > On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > > > 
> > > > 
> > > > Mark, Aurelien,
> > > > 
> > > > I've run into a linker (ld) issue caused by the linker table work I've
> > > > been working on [0]. I looked into this and for the life of me, I
> > > > cannot comprehend what the problem is, so was hoping you folks might
> > > > be able to chime in.
> > > > 
> > > For reference, the error is
> > > 
> > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
> > be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.
> I see -mno-dsbt on arch/c6x/Makefile already -- however at link time this is
> an issue if linker tables are used it seems. Do you have any other recommendation?
> 
> I will note that it would seem that even i386 and x86-64 compiler/binutils seem
> to have relocation issues on older compiler/binutils, for instance:

I see the problem with gcc 6 as well.

So there appears to be some toolchain issues at play here. We build the kernel with two
c6x-specific options: -mno-dsbt and -msdata=none. I already mentioned dsbt. The sdata
option may be one of:

-msdata=default
     Put small global and static data in the .neardata section, which is pointed to by
     register B14. Put small uninitialized global and static data in the .bss section,
     which is adjacent to the .neardata section. Put small read-only data into the 
     .rodata section. The corresponding sections used for large pieces of data are
     .fardata, .far and .const.

-msdata=all
    Put all data, not just small objects, into the sections reserved for small data,
    and use addressing relative to the B14 register to access them.

-msdata=none
    Make no use of the sections reserved for small data, and use absolute addresses
    to access all data. Put all initialized global and static data in the .fardata
    section, and all uninitialized data in the .far section. Put all constant data
    into the .const section.


Both small data and DSBT make use of base register + 15-bit offset to access data
and thus the SB-relative reloc in the above error message.

I think that gcc sees the .rodata section from DEFINE_LINKTABLE_RO() for builtin_fw
and thinks it needs an SB-relative reloc. When the linker sees that reloc, it thinks
it needs the dsbt base register and thus the error. Interestingly, weak data is
never put in the small data section so if gcc sees that data is weak, it doesn't
check the section name to see if it is a small data section. So SB-relative only
gets used for builtin_fw__end, but not the weak builtin_fw even though they both
are in the .rodata section.

I suspect gcc should avoid being fooled by .rodata if -msdata=none is used.
Regardless, I think this could all be avoided if the RO tables used .const
instead of .rodata for c6x.


> gcc-4.7.2
> binutils-2.22 
> 
> Yields:
> 
> x86_64:allyesconfig || x86_64:allmodconfig
> Invalid absolute R_X86_64_64 relocation: __per_cpu_load 
> 
> i386:defconfig
> Invalid absolute R_386_32 relocation: __vvar_page
> 
> This issue on x86 is not observed as so far as of gcc 5.2.1
> and binutils-2.26.1.
> 
> hpa -- if you can think of a work around for this for older compilers/linkers
> let me know... unless we are OK to increase the requirements for x86.
> 
>   Luis

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Tue, 2016-08-09 at 19:09 -0700, Luis R. Rodriguez wrote:
> On Aug 9, 2016 6:50 PM, "Mark Salter" <msalter@redhat.com> wrote:
> >
> > On Tue, 2016-08-09 at 20:40 +0200, Luis R. Rodriguez wrote:
> > > On Tue, Aug 09, 2016 at 01:04:00PM -0400, Mark Salter wrote:
> > > >
> > > > On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> > > > >
> > > > > On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > > > > >
> > > > > >
> > > > > > Mark, Aurelien,
> > > > > >
> > > > > > I've run into a linker (ld) issue caused by the linker table work I've
> > > > > > been working on [0]. I looked into this and for the life of me, I
> > > > > > cannot comprehend what the problem is, so was hoping you folks might
> > > > > > be able to chime in.
> > > > > >
> > > > > For reference, the error is
> > > > >
> > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
> > > > be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.
> > > I see -mno-dsbt on arch/c6x/Makefile already -- however at link time this is
> > > an issue if linker tables are used it seems. Do you have any other recommendation?
> > >
> > > I will note that it would seem that even i386 and x86-64 compiler/binutils seem
> > > to have relocation issues on older compiler/binutils, for instance:
> >
> > I see the problem with gcc 6 as well.
> >
> > So there appears to be some toolchain issues at play here. We build the kernel with two
> > c6x-specific options: -mno-dsbt and -msdata=none. I already mentioned dsbt. The sdata
> > option may be one of:
> >
> > -msdata=default
> >      Put small global and static data in the .neardata section, which is pointed to by
> >      register B14. Put small uninitialized global and static data in the .bss section,
> >      which is adjacent to the .neardata section. Put small read-only data into the 
> >      .rodata section. The corresponding sections used for large pieces of data are
> >      .fardata, .far and .const.
> >
> > -msdata=all
> >     Put all data, not just small objects, into the sections reserved for small data,
> >     and use addressing relative to the B14 register to access them.
> >
> > -msdata=none
> >     Make no use of the sections reserved for small data, and use absolute addresses
> >     to access all data. Put all initialized global and static data in the .fardata
> >     section, and all uninitialized data in the .far section. Put all constant data
> >     into the .const section.
> >
> >
> > Both small data and DSBT make use of base register + 15-bit offset to access data
> > and thus the SB-relative reloc in the above error message.
> >
> > I think that gcc sees the .rodata section from DEFINE_LINKTABLE_RO() for builtin_fw
> > and thinks it needs an SB-relative reloc. When the linker sees that reloc, it thinks
> > it needs the dsbt base register and thus the error. Interestingly, weak data is
> > never put in the small data section so if gcc sees that data is weak, it doesn't
> > check the section name to see if it is a small data section. So SB-relative only
> > gets used for builtin_fw__end, but not the weak builtin_fw even though they both
> > are in the .rodata section.
> >
> > I suspect gcc should avoid being fooled by .rodata if -msdata=none is used.
> > Regardless, I think this could all be avoided if the RO tables used .const
> > instead of .rodata for c6x.
> Thanks for the thorough analysis, would you be OK for c6x to use .const for all read only linker tables or section ranges ?
> I had not added #ifndef around the core-sections.h main ELF definitons but could add one as its needed. In this case perhals that is needed and fine by you
> for SECTION_RODATA.
> We can also override any of the core section setter helpers for archs but in this case based on what you say it seems this is needed. Unless of course just
> -msdata=none is fine and that's not yet used and you prefer that.
>   Luis

We're already using -msdata=none for kernel builds. From the gcc docs, one would think
all const data goes into .const with -msdata=none, but the kernel forces a lot of weak
const kallsyms data ,rodata so c6x vmlinux.lds still needs to have a .rodata section. I
think we need to use .const for the c6x read-only linker tables and keep .rodata for
RO_DATA_SECTION in vmlinux.lds.h.

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Luis R. Rodriguez]

On Tue, Aug 09, 2016 at 11:04:07PM -0400, Mark Salter wrote:
> On Tue, 2016-08-09 at 19:09 -0700, Luis R. Rodriguez wrote:
> > On Aug 9, 2016 6:50 PM, "Mark Salter" <msalter@redhat.com> wrote:
> > >
> > > On Tue, 2016-08-09 at 20:40 +0200, Luis R. Rodriguez wrote:
> > > > On Tue, Aug 09, 2016 at 01:04:00PM -0400, Mark Salter wrote:
> > > > >
> > > > > On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> > > > > >
> > > > > > On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > > > > > >
> > > > > > >
> > > > > > > Mark, Aurelien,
> > > > > > >
> > > > > > > I've run into a linker (ld) issue caused by the linker table work I've
> > > > > > > been working on [0]. I looked into this and for the life of me, I
> > > > > > > cannot comprehend what the problem is, so was hoping you folks might
> > > > > > > be able to chime in.
> > > > > > >
> > > > > > For reference, the error is
> > > > > >
> > > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
> > > > > be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.
> > > > I see -mno-dsbt on arch/c6x/Makefile already -- however at link time this is
> > > > an issue if linker tables are used it seems. Do you have any other recommendation?
> > > >
> > > > I will note that it would seem that even i386 and x86-64 compiler/binutils seem
> > > > to have relocation issues on older compiler/binutils, for instance:
> > >
> > > I see the problem with gcc 6 as well.
> > >
> > > So there appears to be some toolchain issues at play here. We build the kernel with two
> > > c6x-specific options: -mno-dsbt and -msdata=none. I already mentioned dsbt. The sdata
> > > option may be one of:
> > >
> > > -msdata=default
> > >      Put small global and static data in the .neardata section, which is pointed to by
> > >      register B14. Put small uninitialized global and static data in the .bss section,
> > >      which is adjacent to the .neardata section. Put small read-only data into the 
> > >      .rodata section. The corresponding sections used for large pieces of data are
> > >      .fardata, .far and .const.
> > >
> > > -msdata=all
> > >     Put all data, not just small objects, into the sections reserved for small data,
> > >     and use addressing relative to the B14 register to access them.
> > >
> > > -msdata=none
> > >     Make no use of the sections reserved for small data, and use absolute addresses
> > >     to access all data. Put all initialized global and static data in the .fardata
> > >     section, and all uninitialized data in the .far section. Put all constant data
> > >     into the .const section.
> > >
> > >
> > > Both small data and DSBT make use of base register + 15-bit offset to access data
> > > and thus the SB-relative reloc in the above error message.
> > >
> > > I think that gcc sees the .rodata section from DEFINE_LINKTABLE_RO() for builtin_fw
> > > and thinks it needs an SB-relative reloc. When the linker sees that reloc, it thinks
> > > it needs the dsbt base register and thus the error. Interestingly, weak data is
> > > never put in the small data section so if gcc sees that data is weak, it doesn't
> > > check the section name to see if it is a small data section. So SB-relative only
> > > gets used for builtin_fw__end, but not the weak builtin_fw even though they both
> > > are in the .rodata section.
> > >
> > > I suspect gcc should avoid being fooled by .rodata if -msdata=none is used.
> > > Regardless, I think this could all be avoided if the RO tables used .const
> > > instead of .rodata for c6x.
> > Thanks for the thorough analysis, would you be OK for c6x to use .const for all read only linker tables or section ranges ?
> > I had not added #ifndef around the core-sections.h main ELF definitons but could add one as its needed. In this case perhals that is needed and fine by you
> > for SECTION_RODATA.
> > We can also override any of the core section setter helpers for archs but in this case based on what you say it seems this is needed. Unless of course just
> > -msdata=none is fine and that's not yet used and you prefer that.
> >   Luis
> 
> We're already using -msdata=none for kernel builds. From the gcc docs, one would think
> all const data goes into .const with -msdata=none, but the kernel forces a lot of weak
> const kallsyms data ,rodata so c6x vmlinux.lds still needs to have a .rodata section. I
> think we need to use .const for the c6x read-only linker tables and keep .rodata for
> RO_DATA_SECTION in vmlinux.lds.h.

OK thanks I've found a clean solution minimal solution to this as follows. This now
builds fine. Is this a fine work around for now ?

diff --git a/arch/c6x/include/asm/Kbuild b/arch/c6x/include/asm/Kbuild
index c62f0fac6226..c54f7cc1f63e 100644
--- a/arch/c6x/include/asm/Kbuild
+++ b/arch/c6x/include/asm/Kbuild
@@ -64,5 +64,4 @@ generic-y += word-at-a-time.h
 generic-y += xor.h
 generic-y += section-core.h
 generic-y += ranges.h
-generic-y += tables.h
 generic-y += kprobes.h
diff --git a/arch/c6x/include/asm/tables.h b/arch/c6x/include/asm/tables.h
new file mode 100644
index 000000000000..7a9e31575f44
--- /dev/null
+++ b/arch/c6x/include/asm/tables.h
@@ -0,0 +1,26 @@
+#ifndef _ASM_C6X_ASM_TABLES_H
+#define _ASM_C6X_ASM_TABLES_H
+/*
+ * Copyright (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of copyleft-next (version 0.3.1 or later) as published
+ * at http://copyleft-next.org/.
+ */
+
+/*
+ * The c6x toolchain has a bug present even on gcc-6 when non-weak attributes
+ * are used and send them to .rodata even though waek attributes are put in
+ * .const, this forces the linker to believe the address is relative relative
+ * to the a base + offset and you end up with SB-relative reloc error upon
+ * linking. Wor around this by by forcing the ending RO non-waek linker
+ * tables to be weak as well to fix this * for now.
+ *
+ * [0] https://lkml.kernel.org/r/1470798247.3551.94.camel@redhat.com
+ */
+
+#define SECTION_TBL_RO		.const
+
+#include <asm-generic/tables.h>
+
+#endif /* _ASM_C6X_ASM_TABLES_H */
diff --git a/include/asm-generic/tables.h b/include/asm-generic/tables.h
index f9c169ef06b4..50b62616075c 100644
--- a/include/asm-generic/tables.h
+++ b/include/asm-generic/tables.h
@@ -17,6 +17,11 @@
 #define SECTION_TBL_ALL(section)					\
 	SECTION_CORE_ALL(section,tbl)
 
+/* Some toolchains are buggy, let them override */
+#ifndef SECTION_TBL_RO
+#define SECTION_TBL_RO	SECTION_RODATA
+#endif
+
 #ifndef set_section_tbl
 # define set_section_tbl(section, name, level, flags)			\
 	 set_section_core(section, tbl, name, level, flags)
diff --git a/include/linux/tables.h b/include/linux/tables.h
index 639d0144871d..a39ab03751bc 100644
--- a/include/linux/tables.h
+++ b/include/linux/tables.h
@@ -404,13 +404,17 @@
  * @name: linker table name
  * @level: order level
  *
- * Declares a linker table which only requires read-only access.
+ * Declares a linker table which only requires read-only access. Contrary
+ * to LINKTABLE_RO_WEAK() which uses SECTION_RODATA this helper uses the
+ * section SECTION_TBL_RO here due to possible toolchains bug on some
+ * architectures, for instance the c6x architicture stuffs non-weak data
+ * into different sections other than the one intended.
  */
 #define LINKTABLE_RO(name, level)					\
 	const __typeof__(VMLINUX_SYMBOL(name)[0])			\
 	      __attribute__((used,					\
 			     __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
-			     section(SECTION_TBL(SECTION_RODATA,	\
+			     section(SECTION_TBL(SECTION_TBL_RO,	\
 						 name, level))))
 
 /**

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Wed, 2016-08-10 at 23:30 +0200, Luis R. Rodriguez wrote:
> On Tue, Aug 09, 2016 at 11:04:07PM -0400, Mark Salter wrote:
> > 
> > On Tue, 2016-08-09 at 19:09 -0700, Luis R. Rodriguez wrote:
> > > 
> > > On Aug 9, 2016 6:50 PM, "Mark Salter" <msalter@redhat.com> wrote:
> > > > 
> > > > 
> > > > On Tue, 2016-08-09 at 20:40 +0200, Luis R. Rodriguez wrote:
> > > > > 
> > > > > On Tue, Aug 09, 2016 at 01:04:00PM -0400, Mark Salter wrote:
> > > > > > 
> > > > > > 
> > > > > > On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Mark, Aurelien,
> > > > > > > > 
> > > > > > > > I've run into a linker (ld) issue caused by the linker table work I've
> > > > > > > > been working on [0]. I looked into this and for the life of me, I
> > > > > > > > cannot comprehend what the problem is, so was hoping you folks might
> > > > > > > > be able to chime in.
> > > > > > > > 
> > > > > > > For reference, the error is
> > > > > > > 
> > > > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > > DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
> > > > > > be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.
> > > > > I see -mno-dsbt on arch/c6x/Makefile already -- however at link time this is
> > > > > an issue if linker tables are used it seems. Do you have any other recommendation?
> > > > > 
> > > > > I will note that it would seem that even i386 and x86-64 compiler/binutils seem
> > > > > to have relocation issues on older compiler/binutils, for instance:
> > > > I see the problem with gcc 6 as well.
> > > > 
> > > > So there appears to be some toolchain issues at play here. We build the kernel with two
> > > > c6x-specific options: -mno-dsbt and -msdata=none. I already mentioned dsbt. The sdata
> > > > option may be one of:
> > > > 
> > > > -msdata=default
> > > >      Put small global and static data in the .neardata section, which is pointed to by
> > > >      register B14. Put small uninitialized global and static data in the .bss section,
> > > >      which is adjacent to the .neardata section. Put small read-only data into the 
> > > >      .rodata section. The corresponding sections used for large pieces of data are
> > > >      .fardata, .far and .const.
> > > > 
> > > > -msdata=all
> > > >     Put all data, not just small objects, into the sections reserved for small data,
> > > >     and use addressing relative to the B14 register to access them.
> > > > 
> > > > -msdata=none
> > > >     Make no use of the sections reserved for small data, and use absolute addresses
> > > >     to access all data. Put all initialized global and static data in the .fardata
> > > >     section, and all uninitialized data in the .far section. Put all constant data
> > > >     into the .const section.
> > > > 
> > > > 
> > > > Both small data and DSBT make use of base register + 15-bit offset to access data
> > > > and thus the SB-relative reloc in the above error message.
> > > > 
> > > > I think that gcc sees the .rodata section from DEFINE_LINKTABLE_RO() for builtin_fw
> > > > and thinks it needs an SB-relative reloc. When the linker sees that reloc, it thinks
> > > > it needs the dsbt base register and thus the error. Interestingly, weak data is
> > > > never put in the small data section so if gcc sees that data is weak, it doesn't
> > > > check the section name to see if it is a small data section. So SB-relative only
> > > > gets used for builtin_fw__end, but not the weak builtin_fw even though they both
> > > > are in the .rodata section.
> > > > 
> > > > I suspect gcc should avoid being fooled by .rodata if -msdata=none is used.
> > > > Regardless, I think this could all be avoided if the RO tables used .const
> > > > instead of .rodata for c6x.
> > > Thanks for the thorough analysis, would you be OK for c6x to use .const for all read only linker tables or section ranges ?
> > > I had not added #ifndef around the core-sections.h main ELF definitons but could add one as its needed. In this case perhals that is needed and fine by
> > > you
> > > for SECTION_RODATA.
> > > We can also override any of the core section setter helpers for archs but in this case based on what you say it seems this is needed. Unless of course
> > > just
> > > -msdata=none is fine and that's not yet used and you prefer that.
> > >   Luis
> > We're already using -msdata=none for kernel builds. From the gcc docs, one would think
> > all const data goes into .const with -msdata=none, but the kernel forces a lot of weak
> > const kallsyms data ,rodata so c6x vmlinux.lds still needs to have a .rodata section. I
> > think we need to use .const for the c6x read-only linker tables and keep .rodata for
> > RO_DATA_SECTION in vmlinux.lds.h.
> OK thanks I've found a clean solution minimal solution to this as follows. This now
> builds fine. Is this a fine work around for now ?

Almost. You also need:

diff --git a/include/linux/tables.h b/include/linux/tables.h
index a39ab03..3fa8d4d 100644
--- a/include/linux/tables.h
+++ b/include/linux/tables.h
@@ -325,7 +325,7 @@
              __attribute__((used,                                      \
                             weak,                                      \
                             __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
-                            section(SECTION_TBL(SECTION_RODATA,        \
+                            section(SECTION_TBL(SECTION_TBL_RO,        \
                                                 name, level))))
 
 /**

Otherwise, start and end RO table markers end up in different sections.

> 
> diff --git a/arch/c6x/include/asm/Kbuild b/arch/c6x/include/asm/Kbuild
> index c62f0fac6226..c54f7cc1f63e 100644
> --- a/arch/c6x/include/asm/Kbuild
> +++ b/arch/c6x/include/asm/Kbuild
> @@ -64,5 +64,4 @@ generic-y += word-at-a-time.h
>  generic-y += xor.h
>  generic-y += section-core.h
>  generic-y += ranges.h
> -generic-y += tables.h
>  generic-y += kprobes.h
> diff --git a/arch/c6x/include/asm/tables.h b/arch/c6x/include/asm/tables.h
> new file mode 100644
> index 000000000000..7a9e31575f44
> --- /dev/null
> +++ b/arch/c6x/include/asm/tables.h
> @@ -0,0 +1,26 @@
> +#ifndef _ASM_C6X_ASM_TABLES_H
> +#define _ASM_C6X_ASM_TABLES_H
> +/*
> + * Copyright (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of copyleft-next (version 0.3.1 or later) as published
> + * at http://copyleft-next.org/.
> + */
> +
> +/*
> + * The c6x toolchain has a bug present even on gcc-6 when non-weak attributes
> + * are used and send them to .rodata even though waek attributes are put in
> + * .const, this forces the linker to believe the address is relative relative
> + * to the a base + offset and you end up with SB-relative reloc error upon
> + * linking. Wor around this by by forcing the ending RO non-waek linker
> + * tables to be weak as well to fix this * for now.
> + *
> + * [0] https://lkml.kernel.org/r/1470798247.3551.94.camel@redhat.com
> + */
> +
> +#define SECTION_TBL_RO		.const
> +
> +#include <asm-generic/tables.h>
> +
> +#endif /* _ASM_C6X_ASM_TABLES_H */
> diff --git a/include/asm-generic/tables.h b/include/asm-generic/tables.h
> index f9c169ef06b4..50b62616075c 100644
> --- a/include/asm-generic/tables.h
> +++ b/include/asm-generic/tables.h
> @@ -17,6 +17,11 @@
>  #define SECTION_TBL_ALL(section)					\
>  	SECTION_CORE_ALL(section,tbl)
>  
> +/* Some toolchains are buggy, let them override */
> +#ifndef SECTION_TBL_RO
> +#define SECTION_TBL_RO	SECTION_RODATA
> +#endif
> +
>  #ifndef set_section_tbl
>  # define set_section_tbl(section, name, level, flags)			\
>  	 set_section_core(section, tbl, name, level, flags)
> diff --git a/include/linux/tables.h b/include/linux/tables.h
> index 639d0144871d..a39ab03751bc 100644
> --- a/include/linux/tables.h
> +++ b/include/linux/tables.h
> @@ -404,13 +404,17 @@
>   * @name: linker table name
>   * @level: order level
>   *
> - * Declares a linker table which only requires read-only access.
> + * Declares a linker table which only requires read-only access. Contrary
> + * to LINKTABLE_RO_WEAK() which uses SECTION_RODATA this helper uses the
> + * section SECTION_TBL_RO here due to possible toolchains bug on some
> + * architectures, for instance the c6x architicture stuffs non-weak data
> + * into different sections other than the one intended.
>   */
>  #define LINKTABLE_RO(name, level)					\
>  	const __typeof__(VMLINUX_SYMBOL(name)[0])			\
>  	      __attribute__((used,					\
>  			     __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
> -			     section(SECTION_TBL(SECTION_RODATA,	\
> +			     section(SECTION_TBL(SECTION_TBL_RO,	\
>  						 name, level))))
>  
>  /**

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Luis R. Rodriguez]

On Wed, Aug 10, 2016 at 07:04:09PM -0400, Mark Salter wrote:
> On Wed, 2016-08-10 at 23:30 +0200, Luis R. Rodriguez wrote:
> > On Tue, Aug 09, 2016 at 11:04:07PM -0400, Mark Salter wrote:
> > > 
> > > On Tue, 2016-08-09 at 19:09 -0700, Luis R. Rodriguez wrote:
> > > > 
> > > > On Aug 9, 2016 6:50 PM, "Mark Salter" <msalter@redhat.com> wrote:
> > > > > 
> > > > > 
> > > > > On Tue, 2016-08-09 at 20:40 +0200, Luis R. Rodriguez wrote:
> > > > > > 
> > > > > > On Tue, Aug 09, 2016 at 01:04:00PM -0400, Mark Salter wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Mark, Aurelien,
> > > > > > > > > 
> > > > > > > > > I've run into a linker (ld) issue caused by the linker table work I've
> > > > > > > > > been working on [0]. I looked into this and for the life of me, I
> > > > > > > > > cannot comprehend what the problem is, so was hoping you folks might
> > > > > > > > > be able to chime in.
> > > > > > > > > 
> > > > > > > > For reference, the error is
> > > > > > > > 
> > > > > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > > > DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
> > > > > > > be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.
> > > > > > I see -mno-dsbt on arch/c6x/Makefile already -- however at link time this is
> > > > > > an issue if linker tables are used it seems. Do you have any other recommendation?
> > > > > > 
> > > > > > I will note that it would seem that even i386 and x86-64 compiler/binutils seem
> > > > > > to have relocation issues on older compiler/binutils, for instance:
> > > > > I see the problem with gcc 6 as well.
> > > > > 
> > > > > So there appears to be some toolchain issues at play here. We build the kernel with two
> > > > > c6x-specific options: -mno-dsbt and -msdata=none. I already mentioned dsbt. The sdata
> > > > > option may be one of:
> > > > > 
> > > > > -msdata=default
> > > > >      Put small global and static data in the .neardata section, which is pointed to by
> > > > >      register B14. Put small uninitialized global and static data in the .bss section,
> > > > >      which is adjacent to the .neardata section. Put small read-only data into the 
> > > > >      .rodata section. The corresponding sections used for large pieces of data are
> > > > >      .fardata, .far and .const.
> > > > > 
> > > > > -msdata=all
> > > > >     Put all data, not just small objects, into the sections reserved for small data,
> > > > >     and use addressing relative to the B14 register to access them.
> > > > > 
> > > > > -msdata=none
> > > > >     Make no use of the sections reserved for small data, and use absolute addresses
> > > > >     to access all data. Put all initialized global and static data in the .fardata
> > > > >     section, and all uninitialized data in the .far section. Put all constant data
> > > > >     into the .const section.
> > > > > 
> > > > > 
> > > > > Both small data and DSBT make use of base register + 15-bit offset to access data
> > > > > and thus the SB-relative reloc in the above error message.
> > > > > 
> > > > > I think that gcc sees the .rodata section from DEFINE_LINKTABLE_RO() for builtin_fw
> > > > > and thinks it needs an SB-relative reloc. When the linker sees that reloc, it thinks
> > > > > it needs the dsbt base register and thus the error. Interestingly, weak data is
> > > > > never put in the small data section so if gcc sees that data is weak, it doesn't
> > > > > check the section name to see if it is a small data section. So SB-relative only
> > > > > gets used for builtin_fw__end, but not the weak builtin_fw even though they both
> > > > > are in the .rodata section.
> > > > > 
> > > > > I suspect gcc should avoid being fooled by .rodata if -msdata=none is used.
> > > > > Regardless, I think this could all be avoided if the RO tables used .const
> > > > > instead of .rodata for c6x.
> > > > Thanks for the thorough analysis, would you be OK for c6x to use .const for all read only linker tables or section ranges ?
> > > > I had not added #ifndef around the core-sections.h main ELF definitons but could add one as its needed. In this case perhals that is needed and fine by
> > > > you
> > > > for SECTION_RODATA.
> > > > We can also override any of the core section setter helpers for archs but in this case based on what you say it seems this is needed. Unless of course
> > > > just
> > > > -msdata=none is fine and that's not yet used and you prefer that.
> > > >   Luis
> > > We're already using -msdata=none for kernel builds. From the gcc docs, one would think
> > > all const data goes into .const with -msdata=none, but the kernel forces a lot of weak
> > > const kallsyms data ,rodata so c6x vmlinux.lds still needs to have a .rodata section. I
> > > think we need to use .const for the c6x read-only linker tables and keep .rodata for
> > > RO_DATA_SECTION in vmlinux.lds.h.
> > OK thanks I've found a clean solution minimal solution to this as follows. This now
> > builds fine. Is this a fine work around for now ?
> 
> Almost. You also need:
> 
> diff --git a/include/linux/tables.h b/include/linux/tables.h
> index a39ab03..3fa8d4d 100644
> --- a/include/linux/tables.h
> +++ b/include/linux/tables.h
> @@ -325,7 +325,7 @@
>               __attribute__((used,                                      \
>                              weak,                                      \
>                              __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
> -                            section(SECTION_TBL(SECTION_RODATA,        \
> +                            section(SECTION_TBL(SECTION_TBL_RO,        \
>                                                  name, level))))
>  
>  /**
> 
> Otherwise, start and end RO table markers end up in different sections.

I thought that was not needed as weak attributes already force it to go to
.const ? Anyway I've added this as well. Thanks!

  Luis

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Thu, 2016-08-11 at 07:56 +0200, Luis R. Rodriguez wrote:
> On Wed, Aug 10, 2016 at 07:04:09PM -0400, Mark Salter wrote:
> > 
> > On Wed, 2016-08-10 at 23:30 +0200, Luis R. Rodriguez wrote:
> > > 
> > > On Tue, Aug 09, 2016 at 11:04:07PM -0400, Mark Salter wrote:
> > > > 
> > > > 
> > > > On Tue, 2016-08-09 at 19:09 -0700, Luis R. Rodriguez wrote:
> > > > > 
> > > > > 
> > > > > On Aug 9, 2016 6:50 PM, "Mark Salter" <msalter@redhat.com> wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Tue, 2016-08-09 at 20:40 +0200, Luis R. Rodriguez wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On Tue, Aug 09, 2016 at 01:04:00PM -0400, Mark Salter wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Tue, 2016-08-09 at 06:37 -0700, Guenter Roeck wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 08/09/2016 01:11 AM, Luis R. Rodriguez wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Mark, Aurelien,
> > > > > > > > > > 
> > > > > > > > > > I've run into a linker (ld) issue caused by the linker table work I've
> > > > > > > > > > been working on [0]. I looked into this and for the life of me, I
> > > > > > > > > > cannot comprehend what the problem is, so was hoping you folks might
> > > > > > > > > > be able to chime in.
> > > > > > > > > > 
> > > > > > > > > For reference, the error is
> > > > > > > > > 
> > > > > > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > > > > > c6x-elf-ld: drivers/built-in.o: SB-relative relocation but __c6xabi_DSBT_BASE not defined
> > > > > > > > DSBT is a reference to the no-MMU userspace ABI used by c6x. The kernel shouldn't
> > > > > > > > be referencing DSBT base. The -mno-dsbt gcc flag should prevent it.
> > > > > > > I see -mno-dsbt on arch/c6x/Makefile already -- however at link time this is
> > > > > > > an issue if linker tables are used it seems. Do you have any other recommendation?
> > > > > > > 
> > > > > > > I will note that it would seem that even i386 and x86-64 compiler/binutils seem
> > > > > > > to have relocation issues on older compiler/binutils, for instance:
> > > > > > I see the problem with gcc 6 as well.
> > > > > > 
> > > > > > So there appears to be some toolchain issues at play here. We build the kernel with two
> > > > > > c6x-specific options: -mno-dsbt and -msdata=none. I already mentioned dsbt. The sdata
> > > > > > option may be one of:
> > > > > > 
> > > > > > -msdata=default
> > > > > >      Put small global and static data in the .neardata section, which is pointed to by
> > > > > >      register B14. Put small uninitialized global and static data in the .bss section,
> > > > > >      which is adjacent to the .neardata section. Put small read-only data into the 
> > > > > >      .rodata section. The corresponding sections used for large pieces of data are
> > > > > >      .fardata, .far and .const.
> > > > > > 
> > > > > > -msdata=all
> > > > > >     Put all data, not just small objects, into the sections reserved for small data,
> > > > > >     and use addressing relative to the B14 register to access them.
> > > > > > 
> > > > > > -msdata=none
> > > > > >     Make no use of the sections reserved for small data, and use absolute addresses
> > > > > >     to access all data. Put all initialized global and static data in the .fardata
> > > > > >     section, and all uninitialized data in the .far section. Put all constant data
> > > > > >     into the .const section.
> > > > > > 
> > > > > > 
> > > > > > Both small data and DSBT make use of base register + 15-bit offset to access data
> > > > > > and thus the SB-relative reloc in the above error message.
> > > > > > 
> > > > > > I think that gcc sees the .rodata section from DEFINE_LINKTABLE_RO() for builtin_fw
> > > > > > and thinks it needs an SB-relative reloc. When the linker sees that reloc, it thinks
> > > > > > it needs the dsbt base register and thus the error. Interestingly, weak data is
> > > > > > never put in the small data section so if gcc sees that data is weak, it doesn't
> > > > > > check the section name to see if it is a small data section. So SB-relative only
> > > > > > gets used for builtin_fw__end, but not the weak builtin_fw even though they both
> > > > > > are in the .rodata section.
> > > > > > 
> > > > > > I suspect gcc should avoid being fooled by .rodata if -msdata=none is used.
> > > > > > Regardless, I think this could all be avoided if the RO tables used .const
> > > > > > instead of .rodata for c6x.
> > > > > Thanks for the thorough analysis, would you be OK for c6x to use .const for all read only linker tables or section ranges ?
> > > > > I had not added #ifndef around the core-sections.h main ELF definitons but could add one as its needed. In this case perhals that is needed and fine
> > > > > by
> > > > > you
> > > > > for SECTION_RODATA.
> > > > > We can also override any of the core section setter helpers for archs but in this case based on what you say it seems this is needed. Unless of
> > > > > course
> > > > > just
> > > > > -msdata=none is fine and that's not yet used and you prefer that.
> > > > >   Luis
> > > > We're already using -msdata=none for kernel builds. From the gcc docs, one would think
> > > > all const data goes into .const with -msdata=none, but the kernel forces a lot of weak
> > > > const kallsyms data ,rodata so c6x vmlinux.lds still needs to have a .rodata section. I
> > > > think we need to use .const for the c6x read-only linker tables and keep .rodata for
> > > > RO_DATA_SECTION in vmlinux.lds.h.
> > > OK thanks I've found a clean solution minimal solution to this as follows. This now
> > > builds fine. Is this a fine work around for now ?
> > Almost. You also need:
> > 
> > diff --git a/include/linux/tables.h b/include/linux/tables.h
> > index a39ab03..3fa8d4d 100644
> > --- a/include/linux/tables.h
> > +++ b/include/linux/tables.h
> > @@ -325,7 +325,7 @@
> >               __attribute__((used,                                      \
> >                              weak,                                      \
> >                              __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
> > -                            section(SECTION_TBL(SECTION_RODATA,        \
> > +                            section(SECTION_TBL(SECTION_TBL_RO,        \
> >                                                  name, level))))
> >  
> >  /**
> > 
> > Otherwise, start and end RO table markers end up in different sections.
> I thought that was not needed as weak attributes already force it to go to
> .const ? Anyway I've added this as well. Thanks!

The section attribute forced both variables into .rodata but the weak
attribute prevented accesses from using the SB-relative reloc. The
non-weak variable is the one that led to the link error.

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Luis R. Rodriguez]

On Thu, Aug 11, 2016 at 07:32:42AM -0400, Mark Salter wrote:
> On Thu, 2016-08-11 at 07:56 +0200, Luis R. Rodriguez wrote:
> > On Wed, Aug 10, 2016 at 07:04:09PM -0400, Mark Salter wrote:
> > > 
> > > On Wed, 2016-08-10 at 23:30 +0200, Luis R. Rodriguez wrote:
> > > > OK thanks I've found a clean solution minimal solution to this as follows. This now
> > > > builds fine. Is this a fine work around for now ?
> > > Almost. You also need:
> > > 
> > > diff --git a/include/linux/tables.h b/include/linux/tables.h
> > > index a39ab03..3fa8d4d 100644
> > > --- a/include/linux/tables.h
> > > +++ b/include/linux/tables.h
> > > @@ -325,7 +325,7 @@
> > >               __attribute__((used,                                      \
> > >                              weak,                                      \
> > >                              __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
> > > -                            section(SECTION_TBL(SECTION_RODATA,        \
> > > +                            section(SECTION_TBL(SECTION_TBL_RO,        \
> > >                                                  name, level))))
> > >  
> > >  /**
> > > 
> > > Otherwise, start and end RO table markers end up in different sections.
> > I thought that was not needed as weak attributes already force it to go to
> > .const ? Anyway I've added this as well. Thanks!
> 
> The section attribute forced both variables into .rodata but the weak
> attribute prevented accesses from using the SB-relative reloc. The
> non-weak variable is the one that led to the link error.

I ask as set_section_tbl_type() was not patched for instance, so firmware/Makefile
still uses SECTION_RODATA, and it compiles and links fine. Should that also be
using then SECTION_TBL_RO ? Or do we only need this for the C constructors ?

  Luis

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Thu, 2016-08-11 at 17:59 +0200, Luis R. Rodriguez wrote:
> On Thu, Aug 11, 2016 at 07:32:42AM -0400, Mark Salter wrote:
> > 
> > On Thu, 2016-08-11 at 07:56 +0200, Luis R. Rodriguez wrote:
> > > 
> > > On Wed, Aug 10, 2016 at 07:04:09PM -0400, Mark Salter wrote:
> > > > 
> > > > 
> > > > On Wed, 2016-08-10 at 23:30 +0200, Luis R. Rodriguez wrote:
> > > > > 
> > > > > OK thanks I've found a clean solution minimal solution to this as follows. This now
> > > > > builds fine. Is this a fine work around for now ?
> > > > Almost. You also need:
> > > > 
> > > > diff --git a/include/linux/tables.h b/include/linux/tables.h
> > > > index a39ab03..3fa8d4d 100644
> > > > --- a/include/linux/tables.h
> > > > +++ b/include/linux/tables.h
> > > > @@ -325,7 +325,7 @@
> > > >               __attribute__((used,                                      \
> > > >                              weak,                                      \
> > > >                              __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
> > > > -                            section(SECTION_TBL(SECTION_RODATA,        \
> > > > +                            section(SECTION_TBL(SECTION_TBL_RO,        \
> > > >                                                  name, level))))
> > > >  
> > > >  /**
> > > > 
> > > > Otherwise, start and end RO table markers end up in different sections.
> > > I thought that was not needed as weak attributes already force it to go to
> > > .const ? Anyway I've added this as well. Thanks!
> > The section attribute forced both variables into .rodata but the weak
> > attribute prevented accesses from using the SB-relative reloc. The
> > non-weak variable is the one that led to the link error.
> I ask as set_section_tbl_type() was not patched for instance, so firmware/Makefile
> still uses SECTION_RODATA, and it compiles and links fine. Should that also be
> using then SECTION_TBL_RO ? Or do we only need this for the C constructors ?
> 
>   Luis

Yuck. You need SECTION_TBL_RO and s/.rodata/.const/ in that Makefile.
C6X doesn't support any of the devices with firmware, so I just added:

fw-shipped-y += ti_3410.fw

to firmware/Makefile for testing.

Leaving in .rodata and SECTION_RODATA, I got:

% readelf --syms vmlinux | grep -e _fw_ -e builtin_fw 
  8445: e01d4000     0 NOTYPE  LOCAL  DEFAULT    7 _fw_ti_3410_fw_bin
  8446: e01d75c5     0 NOTYPE  LOCAL  DEFAULT    7 _fw_end
  8447: e020a7cc     0 NOTYPE  LOCAL  DEFAULT    7 _fw_ti_3410_fw_name
 11063: e023d688     0 OBJECT  GLOBAL DEFAULT   13 builtin_fw__end
 15867: e023d688     0 OBJECT  WEAK   DEFAULT   13 builtin_fw

>From the above addresses, the _fw symbols are in .rodata and the builtin_fw symbols
are in .const.

Changing the Makefile to use .rodata and SECTION_TBL_RO, I see:

  8445: e0239688     0 NOTYPE  LOCAL  DEFAULT   13 _fw_ti_3410_fw_bin
  8446: e023cc4d     0 NOTYPE  LOCAL  DEFAULT   13 _fw_end
  8447: e023cc50     0 NOTYPE  LOCAL  DEFAULT   13 _fw_ti_3410_fw_name
 11063: e0239688     0 OBJECT  GLOBAL DEFAULT   13 builtin_fw__end
 15867: e0239688     0 OBJECT  WEAK   DEFAULT   13 builtin_fw

which has everything in .const as it should be. But still builtin_fw and
builtin_fw__end are at same address which seems wrong.

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Thu, 2016-08-11 at 17:59 +0200, Luis R. Rodriguez wrote:
> On Thu, Aug 11, 2016 at 07:32:42AM -0400, Mark Salter wrote:
> > 
> > On Thu, 2016-08-11 at 07:56 +0200, Luis R. Rodriguez wrote:
> > > 
> > > On Wed, Aug 10, 2016 at 07:04:09PM -0400, Mark Salter wrote:
> > > > 
> > > > 
> > > > On Wed, 2016-08-10 at 23:30 +0200, Luis R. Rodriguez wrote:
> > > > > 
> > > > > OK thanks I've found a clean solution minimal solution to this as follows. This now
> > > > > builds fine. Is this a fine work around for now ?
> > > > Almost. You also need:
> > > > 
> > > > diff --git a/include/linux/tables.h b/include/linux/tables.h
> > > > index a39ab03..3fa8d4d 100644
> > > > --- a/include/linux/tables.h
> > > > +++ b/include/linux/tables.h
> > > > @@ -325,7 +325,7 @@
> > > >               __attribute__((used,                                      \
> > > >                              weak,                                      \
> > > >                              __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
> > > > -                            section(SECTION_TBL(SECTION_RODATA,        \
> > > > +                            section(SECTION_TBL(SECTION_TBL_RO,        \
> > > >                                                  name, level))))
> > > >  
> > > >  /**
> > > > 
> > > > Otherwise, start and end RO table markers end up in different sections.
> > > I thought that was not needed as weak attributes already force it to go to
> > > .const ? Anyway I've added this as well. Thanks!
> > The section attribute forced both variables into .rodata but the weak
> > attribute prevented accesses from using the SB-relative reloc. The
> > non-weak variable is the one that led to the link error.
> I ask as set_section_tbl_type() was not patched for instance, so firmware/Makefile
> still uses SECTION_RODATA, and it compiles and links fine. Should that also be
> using then SECTION_TBL_RO ? Or do we only need this for the C constructors ?
> 
>   Luis

Yuck. You need SECTION_TBL_RO and s/.rodata/.const/ in that Makefile.
C6X doesn't support any of the devices with firmware, so I just added:

fw-shipped-y += ti_3410.fw

to firmware/Makefile for testing.

Leaving in .rodata and SECTION_RODATA, I got:

% readelf --syms vmlinux | grep -e _fw_ -e builtin_fw 
  8445: e01d4000     0 NOTYPE  LOCAL  DEFAULT    7 _fw_ti_3410_fw_bin
  8446: e01d75c5     0 NOTYPE  LOCAL  DEFAULT    7 _fw_end
  8447: e020a7cc     0 NOTYPE  LOCAL  DEFAULT    7 _fw_ti_3410_fw_name
 11063: e023d688     0 OBJECT  GLOBAL DEFAULT   13 builtin_fw__end
 15867: e023d688     0 OBJECT  WEAK   DEFAULT   13 builtin_fw

From the above addresses, the _fw symbols are in .rodata and the builtin_fw symbols
are in .const.

Changing the Makefile to use .rodata and SECTION_TBL_RO, I see:

  8445: e0239688     0 NOTYPE  LOCAL  DEFAULT   13 _fw_ti_3410_fw_bin
  8446: e023cc4d     0 NOTYPE  LOCAL  DEFAULT   13 _fw_end
  8447: e023cc50     0 NOTYPE  LOCAL  DEFAULT   13 _fw_ti_3410_fw_name
 11063: e0239688     0 OBJECT  GLOBAL DEFAULT   13 builtin_fw__end
 15867: e0239688     0 OBJECT  WEAK   DEFAULT   13 builtin_fw

which has everything in .const as it should be. But still builtin_fw and
builtin_fw__end are at same address which seems wrong.

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Mark Salter]

On Thu, 2016-08-11 at 17:59 +0200, Luis R. Rodriguez wrote:
> On Thu, Aug 11, 2016 at 07:32:42AM -0400, Mark Salter wrote:
> > 
> > On Thu, 2016-08-11 at 07:56 +0200, Luis R. Rodriguez wrote:
> > > 
> > > On Wed, Aug 10, 2016 at 07:04:09PM -0400, Mark Salter wrote:
> > > > 
> > > > 
> > > > On Wed, 2016-08-10 at 23:30 +0200, Luis R. Rodriguez wrote:
> > > > > 
> > > > > OK thanks I've found a clean solution minimal solution to this as follows. This now
> > > > > builds fine. Is this a fine work around for now ?
> > > > Almost. You also need:
> > > > 
> > > > diff --git a/include/linux/tables.h b/include/linux/tables.h
> > > > index a39ab03..3fa8d4d 100644
> > > > --- a/include/linux/tables.h
> > > > +++ b/include/linux/tables.h
> > > > @@ -325,7 +325,7 @@
> > > >               __attribute__((used,                                      \
> > > >                              weak,                                      \
> > > >                              __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
> > > > -                            section(SECTION_TBL(SECTION_RODATA,        \
> > > > +                            section(SECTION_TBL(SECTION_TBL_RO,        \
> > > >                                                  name, level))))
> > > >  
> > > >  /**
> > > > 
> > > > Otherwise, start and end RO table markers end up in different sections.
> > > I thought that was not needed as weak attributes already force it to go to
> > > .const ? Anyway I've added this as well. Thanks!
> > The section attribute forced both variables into .rodata but the weak
> > attribute prevented accesses from using the SB-relative reloc. The
> > non-weak variable is the one that led to the link error.
> I ask as set_section_tbl_type() was not patched for instance, so firmware/Makefile
> still uses SECTION_RODATA, and it compiles and links fine. Should that also be
> using then SECTION_TBL_RO ? Or do we only need this for the C constructors ?
> 
>   Luis

Yuck. You need SECTION_TBL_RO and s/.rodata/.const/ in that Makefile.
C6X doesn't support any of the devices with firmware, so I just added:

fw-shipped-y += ti_3410.fw

to firmware/Makefile for testing.

Leaving in .rodata and SECTION_RODATA, I got:

% readelf --syms vmlinux | grep -e _fw_ -e builtin_fw 
  8445: e01d4000     0 NOTYPE  LOCAL  DEFAULT    7 _fw_ti_3410_fw_bin
  8446: e01d75c5     0 NOTYPE  LOCAL  DEFAULT    7 _fw_end
  8447: e020a7cc     0 NOTYPE  LOCAL  DEFAULT    7 _fw_ti_3410_fw_name
 11063: e023d688     0 OBJECT  GLOBAL DEFAULT   13 builtin_fw__end
 15867: e023d688     0 OBJECT  WEAK   DEFAULT   13 builtin_fw

Re: c6x linker issue on linux-next-20160808 + some linker table work

[Luis R. Rodriguez]

On Thu, Aug 11, 2016 at 10:14 AM, Mark Salter <msalter@redhat.com> wrote:
> On Thu, 2016-08-11 at 17:59 +0200, Luis R. Rodriguez wrote:
>> On Thu, Aug 11, 2016 at 07:32:42AM -0400, Mark Salter wrote:
>> >
>> > On Thu, 2016-08-11 at 07:56 +0200, Luis R. Rodriguez wrote:
>> > >
>> > > On Wed, Aug 10, 2016 at 07:04:09PM -0400, Mark Salter wrote:
>> > > >
>> > > >
>> > > > On Wed, 2016-08-10 at 23:30 +0200, Luis R. Rodriguez wrote:
>> > > > >
>> > > > > OK thanks I've found a clean solution minimal solution to this as follows. This now
>> > > > > builds fine. Is this a fine work around for now ?
>> > > > Almost. You also need:
>> > > >
>> > > > diff --git a/include/linux/tables.h b/include/linux/tables.h
>> > > > index a39ab03..3fa8d4d 100644
>> > > > --- a/include/linux/tables.h
>> > > > +++ b/include/linux/tables.h
>> > > > @@ -325,7 +325,7 @@
>> > > >               __attribute__((used,                                      \
>> > > >                              weak,                                      \
>> > > >                              __aligned__(LINUX_SECTION_ALIGNMENT(name)),\
>> > > > -                            section(SECTION_TBL(SECTION_RODATA,        \
>> > > > +                            section(SECTION_TBL(SECTION_TBL_RO,        \
>> > > >                                                  name, level))))
>> > > >
>> > > >  /**
>> > > >
>> > > > Otherwise, start and end RO table markers end up in different sections.
>> > > I thought that was not needed as weak attributes already force it to go to
>> > > .const ? Anyway I've added this as well. Thanks!
>> > The section attribute forced both variables into .rodata but the weak
>> > attribute prevented accesses from using the SB-relative reloc. The
>> > non-weak variable is the one that led to the link error.
>> I ask as set_section_tbl_type() was not patched for instance, so firmware/Makefile
>> still uses SECTION_RODATA, and it compiles and links fine. Should that also be
>> using then SECTION_TBL_RO ? Or do we only need this for the C constructors ?
>>
>>   Luis
>
> Yuck. You need SECTION_TBL_RO and s/.rodata/.const/ in that Makefile.

OK as I suspected then... So what I will have to do is add into
asm-generic/tables.h a special section_tbl_ro_type() / push_*() for RO
tables, so that then they all use  SECTION_TBL_RO.

> C6X doesn't support any of the devices with firmware, so I just added:

I figured, but this will be more important as more linker RO tables
become prevalent so better we get this right now.

> fw-shipped-y += ti_3410.fw
>
> to firmware/Makefile for testing.
>
> Leaving in .rodata and SECTION_RODATA, I got:
>
> % readelf --syms vmlinux | grep -e _fw_ -e builtin_fw
>   8445: e01d4000     0 NOTYPE  LOCAL  DEFAULT    7 _fw_ti_3410_fw_bin
>   8446: e01d75c5     0 NOTYPE  LOCAL  DEFAULT    7 _fw_end
>   8447: e020a7cc     0 NOTYPE  LOCAL  DEFAULT    7 _fw_ti_3410_fw_name
>  11063: e023d688     0 OBJECT  GLOBAL DEFAULT   13 builtin_fw__end
>  15867: e023d688     0 OBJECT  WEAK   DEFAULT   13 builtin_fw
>
> From the above addresses, the _fw symbols are in .rodata and the builtin_fw symbols
> are in .const.
>
> Changing the Makefile to use .rodata and SECTION_TBL_RO, I see:
>
>   8445: e0239688     0 NOTYPE  LOCAL  DEFAULT   13 _fw_ti_3410_fw_bin
>   8446: e023cc4d     0 NOTYPE  LOCAL  DEFAULT   13 _fw_end
>   8447: e023cc50     0 NOTYPE  LOCAL  DEFAULT   13 _fw_ti_3410_fw_name
>  11063: e0239688     0 OBJECT  GLOBAL DEFAULT   13 builtin_fw__end
>  15867: e0239688     0 OBJECT  WEAK   DEFAULT   13 builtin_fw
>
> which has everything in .const as it should be. But still builtin_fw and
> builtin_fw__end are at same address which seems wrong.

The address order seems odd, I've seen something similar but it works,
you can add debugging code to fw_get_builtin_firmware() in
drivers/base/firmware_class.c to debug while iterating, and/or also
use LINUX_SECTION_SIZE(builtin_fw) to verify the size.

  Luis