| Message ID | 20150130153414.GM26493@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 30 Jan 2015, Russell King - ARM Linux wrote: > Here's a potential patch for issue 2b - to cause the link to fail if > the resulting kernel would be broken by trying to run it. > > arch/arm/kernel/vmlinux.lds.S | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S > index b31aa73e8076..9351f7fbdfb1 100644 > --- a/arch/arm/kernel/vmlinux.lds.S > +++ b/arch/arm/kernel/vmlinux.lds.S > @@ -351,3 +351,13 @@ ASSERT((__arch_info_end - __arch_info_begin), "no machine record defined") > * The above comment applies as well. > */ > ASSERT(((__hyp_idmap_text_end - __hyp_idmap_text_start) <= PAGE_SIZE), "HYP init code too big") > + > +#ifdef CONFIG_XIP_KERNEL > +/* > + * __data_loc is not only the LMA of the data section, but also the VMA of > + * the end of the .rodata section. This must not overlap the VMA of the > + * data section. Since the .text section starts in module space, and that > + * is always below the .data section, this should be sufficient. > + */ > +ASSERT((_data >= __data_loc), "Text section oversize") > +#endif I agree with this patch. Acked-by: Nicolas Pitre <nico@linaro.org> This might not prevent a config leading to this from happening, but at least it makes the issue much clearer. XIP kernel was created for systems where the total amount of RAM is often smaller than the imposed size limit here. Nicolas
On Fri, Jan 30, 2015 at 12:29:30PM -0500, Nicolas Pitre wrote: > On Fri, 30 Jan 2015, Russell King - ARM Linux wrote: > > +#ifdef CONFIG_XIP_KERNEL > > +/* > > + * __data_loc is not only the LMA of the data section, but also the VMA of > > + * the end of the .rodata section. This must not overlap the VMA of the > > + * data section. Since the .text section starts in module space, and that > > + * is always below the .data section, this should be sufficient. > > + */ > > +ASSERT((_data >= __data_loc), "Text section oversize") > > +#endif > > I agree with this patch. > > Acked-by: Nicolas Pitre <nico@linaro.org> > > This might not prevent a config leading to this from happening, but at > least it makes the issue much clearer. XIP kernel was created for > systems where the total amount of RAM is often smaller than the imposed > size limit here. Yes, I expect more of Arnd's randconfigs to fail with this patch applied. I did also notice that we still have swapper_pg_dir at _data - 0x4000 for XIP kernels - so the above check is slightly too lenient. A better threshold for __data_loc might be MODULES_END, since we can't allow the XIP part to overlap into RAM. #ifdef CONFIG_HIGHMEM #define MODULES_END (PAGE_OFFSET - PMD_SIZE) #else #define MODULES_END (PAGE_OFFSET) #endif
On Sat, Jan 31, 2015 at 12:22:53AM +0000, Russell King - ARM Linux wrote: > On Fri, Jan 30, 2015 at 12:29:30PM -0500, Nicolas Pitre wrote: > > On Fri, 30 Jan 2015, Russell King - ARM Linux wrote: > > > +#ifdef CONFIG_XIP_KERNEL > > > +/* > > > + * __data_loc is not only the LMA of the data section, but also the VMA of > > > + * the end of the .rodata section. This must not overlap the VMA of the > > > + * data section. Since the .text section starts in module space, and that > > > + * is always below the .data section, this should be sufficient. > > > + */ > > > +ASSERT((_data >= __data_loc), "Text section oversize") > > > +#endif > > > > I agree with this patch. > > > > Acked-by: Nicolas Pitre <nico@linaro.org> > > > > This might not prevent a config leading to this from happening, but at > > least it makes the issue much clearer. XIP kernel was created for > > systems where the total amount of RAM is often smaller than the imposed > > size limit here. > > Yes, I expect more of Arnd's randconfigs to fail with this patch applied. > > I did also notice that we still have swapper_pg_dir at _data - 0x4000 > for XIP kernels - so the above check is slightly too lenient. A better > threshold for __data_loc might be MODULES_END, since we can't allow the > XIP part to overlap into RAM. > > #ifdef CONFIG_HIGHMEM > #define MODULES_END (PAGE_OFFSET - PMD_SIZE) > #else > #define MODULES_END (PAGE_OFFSET) > #endif It looks like we have cases where this falsely triggers. Consider EFM32: CONFIG_DRAM_BASE=0x88000000 CONFIG_DRAM_SIZE=0x00400000 CONFIG_FLASH_MEM_BASE=0x8c000000 CONFIG_FLASH_SIZE=0x01000000 This means that we quite legally end up with the .data section below the .text section, which makes: ASSERT((_data >= __data_loc), "Text section oversize") falsely trigger. The linker has the capacity to specify regions of ROM and RAM in the linker file, I wonder if we should be using those for XIP. Merely adding the MEMORY table to the linker file is not good enough - we also need to explicitly tell the linker which memory regions to place the output sections, otherwise the linker ends up making assumptions. What that means is... asm-generic/vmlinux.lds.h breaks for us. Any ideas? I think using the MEMORY table would be the best approach, because that should allow us to properly verify that the resulting binary should fit in the memory regions.
On Wed, 4 Feb 2015, Russell King - ARM Linux wrote: > It looks like we have cases where this falsely triggers. Consider EFM32: > > CONFIG_DRAM_BASE=0x88000000 > CONFIG_DRAM_SIZE=0x00400000 > CONFIG_FLASH_MEM_BASE=0x8c000000 > CONFIG_FLASH_SIZE=0x01000000 > > This means that we quite legally end up with the .data section below the > .text section, which makes: > > ASSERT((_data >= __data_loc), "Text section oversize") > > falsely trigger. > > The linker has the capacity to specify regions of ROM and RAM in the > linker file, I wonder if we should be using those for XIP. Merely > adding the MEMORY table to the linker file is not good enough - we > also need to explicitly tell the linker which memory regions to place > the output sections, otherwise the linker ends up making assumptions. > > What that means is... asm-generic/vmlinux.lds.h breaks for us. > > Any ideas? I think using the MEMORY table would be the best approach, > because that should allow us to properly verify that the resulting > binary should fit in the memory regions. Maybe simply having an assert() on the size of the .text section could be all that is needed. We already know the maximum size in advance. Nicolas
On Tue, Feb 03, 2015 at 08:59:15PM -0500, Nicolas Pitre wrote: > On Wed, 4 Feb 2015, Russell King - ARM Linux wrote: > > > It looks like we have cases where this falsely triggers. Consider EFM32: > > > > CONFIG_DRAM_BASE=0x88000000 > > CONFIG_DRAM_SIZE=0x00400000 > > CONFIG_FLASH_MEM_BASE=0x8c000000 > > CONFIG_FLASH_SIZE=0x01000000 > > > > This means that we quite legally end up with the .data section below the > > .text section, which makes: > > > > ASSERT((_data >= __data_loc), "Text section oversize") > > > > falsely trigger. > > > > The linker has the capacity to specify regions of ROM and RAM in the > > linker file, I wonder if we should be using those for XIP. Merely > > adding the MEMORY table to the linker file is not good enough - we > > also need to explicitly tell the linker which memory regions to place > > the output sections, otherwise the linker ends up making assumptions. > > > > What that means is... asm-generic/vmlinux.lds.h breaks for us. > > > > Any ideas? I think using the MEMORY table would be the best approach, > > because that should allow us to properly verify that the resulting > > binary should fit in the memory regions. > > Maybe simply having an assert() on the size of the .text section could > be all that is needed. We already know the maximum size in advance. That doesn't work, it's not just the .text section but also .rodata, __bug_table, __ksymtab, __ksymtab_gpl, __kcrctab, __kcrctab_gpl, __ksymtab_strings, __param, __modver, __ex_table, .notes, .vectors, .stubs, .init.text, maybe .exit.text, .init.arch.info, .init.tagtable, .init.smpalt, .init.pv_table, and apparently .init.data (which is surely wrong?) The following is from Arnd's failing configuration: 18 .init.tagtable 00000040 80073a9c 80073a9c 0100ba9c 2**2 CONTENTS, ALLOC, LOAD, READONLY, DATA 19 .init.data 000010e8 80073adc 80073adc 0100badc 2**2 CONTENTS, ALLOC, LOAD, DATA 20 .data 003552c4 80008000 80074bc4 01010000 2**8 CONTENTS, ALLOC, LOAD, DATA Hmm, if .init.data is contained in the flash section (which it seemingly is), it seems that XIP support is currently broken - that section is definitely a read/write section. No one has seemingly noticed that it's broken and it's been broken for a long time, so maybe the simple answer then is just to rip XIP support out? How does EFM32 work? Does it work?
On Wed, 4 Feb 2015, Russell King - ARM Linux wrote: > On Tue, Feb 03, 2015 at 08:59:15PM -0500, Nicolas Pitre wrote: > > On Wed, 4 Feb 2015, Russell King - ARM Linux wrote: > > > > > It looks like we have cases where this falsely triggers. Consider EFM32: > > > > > > CONFIG_DRAM_BASE=0x88000000 > > > CONFIG_DRAM_SIZE=0x00400000 > > > CONFIG_FLASH_MEM_BASE=0x8c000000 > > > CONFIG_FLASH_SIZE=0x01000000 > > > > > > This means that we quite legally end up with the .data section below the > > > .text section, which makes: > > > > > > ASSERT((_data >= __data_loc), "Text section oversize") > > > > > > falsely trigger. > > > > > > The linker has the capacity to specify regions of ROM and RAM in the > > > linker file, I wonder if we should be using those for XIP. Merely > > > adding the MEMORY table to the linker file is not good enough - we > > > also need to explicitly tell the linker which memory regions to place > > > the output sections, otherwise the linker ends up making assumptions. > > > > > > What that means is... asm-generic/vmlinux.lds.h breaks for us. > > > > > > Any ideas? I think using the MEMORY table would be the best approach, > > > because that should allow us to properly verify that the resulting > > > binary should fit in the memory regions. > > > > Maybe simply having an assert() on the size of the .text section could > > be all that is needed. We already know the maximum size in advance. > > That doesn't work, it's not just the .text section but also .rodata, > __bug_table, __ksymtab, __ksymtab_gpl, __kcrctab, __kcrctab_gpl, > __ksymtab_strings, __param, __modver, __ex_table, .notes, .vectors, > .stubs, .init.text, maybe .exit.text, .init.arch.info, .init.tagtable, > .init.smpalt, .init.pv_table, My suggestion would be to put a symbol at the end of it all to size the lot. > and apparently .init.data (which is > surely wrong?) The following is from Arnd's failing configuration: > > 18 .init.tagtable 00000040 80073a9c 80073a9c 0100ba9c 2**2 > CONTENTS, ALLOC, LOAD, READONLY, DATA > 19 .init.data 000010e8 80073adc 80073adc 0100badc 2**2 > CONTENTS, ALLOC, LOAD, DATA > 20 .data 003552c4 80008000 80074bc4 01010000 2**8 > CONTENTS, ALLOC, LOAD, DATA > > Hmm, if .init.data is contained in the flash section (which it seemingly > is), it seems that XIP support is currently broken - that section is > definitely a read/write section. No one has seemingly noticed that it's > broken and it's been broken for a long time, so maybe the simple answer > then is just to rip XIP support out? That's what I proposed a couple years ago but some people were apparently still using it. Now with all the buzz around IOT it is well possible that XIP will gain more traction again. Nicolas
On Wednesday 04 February 2015 09:44:14 Russell King - ARM Linux wrote: > On Tue, Feb 03, 2015 at 08:59:15PM -0500, Nicolas Pitre wrote: > > On Wed, 4 Feb 2015, Russell King - ARM Linux wrote: > > > > > It looks like we have cases where this falsely triggers. Consider EFM32: > > > > > > CONFIG_DRAM_BASE=0x88000000 > > > CONFIG_DRAM_SIZE=0x00400000 > > > CONFIG_FLASH_MEM_BASE=0x8c000000 > > > CONFIG_FLASH_SIZE=0x01000000 > > > > > > This means that we quite legally end up with the .data section below the > > > .text section, which makes: > > > > > > ASSERT((_data >= __data_loc), "Text section oversize") > > > > > > falsely trigger. > > > > > > The linker has the capacity to specify regions of ROM and RAM in the > > > linker file, I wonder if we should be using those for XIP. Merely > > > adding the MEMORY table to the linker file is not good enough - we > > > also need to explicitly tell the linker which memory regions to place > > > the output sections, otherwise the linker ends up making assumptions. > > > > > > What that means is... asm-generic/vmlinux.lds.h breaks for us. > > > > > > Any ideas? I think using the MEMORY table would be the best approach, > > > because that should allow us to properly verify that the resulting > > > binary should fit in the memory regions. > > > > Maybe simply having an assert() on the size of the .text section could > > be all that is needed. We already know the maximum size in advance. > > That doesn't work, it's not just the .text section but also .rodata, > __bug_table, __ksymtab, __ksymtab_gpl, __kcrctab, __kcrctab_gpl, > __ksymtab_strings, __param, __modver, __ex_table, .notes, .vectors, > .stubs, .init.text, maybe .exit.text, .init.arch.info, .init.tagtable, > .init.smpalt, .init.pv_table, and apparently .init.data (which is > surely wrong?) The following is from Arnd's failing configuration: > > 18 .init.tagtable 00000040 80073a9c 80073a9c 0100ba9c 2**2 > CONTENTS, ALLOC, LOAD, READONLY, DATA > 19 .init.data 000010e8 80073adc 80073adc 0100badc 2**2 > CONTENTS, ALLOC, LOAD, DATA > 20 .data 003552c4 80008000 80074bc4 01010000 2**8 > CONTENTS, ALLOC, LOAD, DATA > > Hmm, if .init.data is contained in the flash section (which it seemingly > is), it seems that XIP support is currently broken - that section is > definitely a read/write section. No one has seemingly noticed that it's > broken and it's been broken for a long time, so maybe the simple answer > then is just to rip XIP support out? > > How does EFM32 work? Does it work? I believe that Uwe has a patch series on top of mainline that he still requires for using the platform. The part I'm sure about is that it does not work without XIP_KERNEL, given the tight memory constraints (2MB RAM?). Arnd
On Sat, Jan 31, 2015 at 12:22:53AM +0000, Russell King - ARM Linux wrote: > On Fri, Jan 30, 2015 at 12:29:30PM -0500, Nicolas Pitre wrote: > > On Fri, 30 Jan 2015, Russell King - ARM Linux wrote: > > > +#ifdef CONFIG_XIP_KERNEL > > > +/* > > > + * __data_loc is not only the LMA of the data section, but also the VMA of > > > + * the end of the .rodata section. This must not overlap the VMA of the > > > + * data section. Since the .text section starts in module space, and that > > > + * is always below the .data section, this should be sufficient. > > > + */ > > > +ASSERT((_data >= __data_loc), "Text section oversize") > > > +#endif > > > > I agree with this patch. > > > > Acked-by: Nicolas Pitre <nico@linaro.org> > > > > This might not prevent a config leading to this from happening, but at > > least it makes the issue much clearer. XIP kernel was created for > > systems where the total amount of RAM is often smaller than the imposed > > size limit here. > > Yes, I expect more of Arnd's randconfigs to fail with this patch applied. > > I did also notice that we still have swapper_pg_dir at _data - 0x4000 > for XIP kernels - so the above check is slightly too lenient. A better > threshold for __data_loc might be MODULES_END, since we can't allow the > XIP part to overlap into RAM. > > #ifdef CONFIG_HIGHMEM > #define MODULES_END (PAGE_OFFSET - PMD_SIZE) > #else > #define MODULES_END (PAGE_OFFSET) > #endif BTW, on no-MMU MODULES_END is defined as #define END_MEM (UL(CONFIG_DRAM_BASE) + CONFIG_DRAM_SIZE) #define MODULES_END (END_MEM) I'd like to get rid of CONFIG_DRAM_SIZE because it's stupid to have that fixed in .config. MODULES_END is hardly used on no-MMU apart from printing the (IIRC wrong) memory layout during boot. If we define END_MEM to 0xffffffff on no-MMU this should effectively nop the assertion which at least prevents false positives?! Best regards Uwe
On 2015-02-04 10:44, Russell King - ARM Linux wrote: > On Tue, Feb 03, 2015 at 08:59:15PM -0500, Nicolas Pitre wrote: >> On Wed, 4 Feb 2015, Russell King - ARM Linux wrote: >> >> > It looks like we have cases where this falsely triggers. Consider EFM32: >> > >> > CONFIG_DRAM_BASE=0x88000000 >> > CONFIG_DRAM_SIZE=0x00400000 >> > CONFIG_FLASH_MEM_BASE=0x8c000000 >> > CONFIG_FLASH_SIZE=0x01000000 >> > >> > This means that we quite legally end up with the .data section below the >> > .text section, which makes: >> > >> > ASSERT((_data >= __data_loc), "Text section oversize") >> > >> > falsely trigger. >> > >> > The linker has the capacity to specify regions of ROM and RAM in the >> > linker file, I wonder if we should be using those for XIP. Merely >> > adding the MEMORY table to the linker file is not good enough - we >> > also need to explicitly tell the linker which memory regions to place >> > the output sections, otherwise the linker ends up making assumptions. >> > >> > What that means is... asm-generic/vmlinux.lds.h breaks for us. >> > >> > Any ideas? I think using the MEMORY table would be the best approach, >> > because that should allow us to properly verify that the resulting >> > binary should fit in the memory regions. >> >> Maybe simply having an assert() on the size of the .text section could >> be all that is needed. We already know the maximum size in advance. > > That doesn't work, it's not just the .text section but also .rodata, > __bug_table, __ksymtab, __ksymtab_gpl, __kcrctab, __kcrctab_gpl, > __ksymtab_strings, __param, __modver, __ex_table, .notes, .vectors, > .stubs, .init.text, maybe .exit.text, .init.arch.info, .init.tagtable, > .init.smpalt, .init.pv_table, and apparently .init.data (which is > surely wrong?) The following is from Arnd's failing configuration: > > 18 .init.tagtable 00000040 80073a9c 80073a9c 0100ba9c 2**2 > CONTENTS, ALLOC, LOAD, READONLY, DATA > 19 .init.data 000010e8 80073adc 80073adc 0100badc 2**2 > CONTENTS, ALLOC, LOAD, DATA > 20 .data 003552c4 80008000 80074bc4 01010000 2**8 > CONTENTS, ALLOC, LOAD, DATA > > Hmm, if .init.data is contained in the flash section (which it seemingly > is), it seems that XIP support is currently broken - that section is > definitely a read/write section. No one has seemingly noticed that it's > broken and it's been broken for a long time, so maybe the simple answer > then is just to rip XIP support out? Hi Russell, I noticed that kallsyms resolution fails with XIP kernel in my patchset to support Vybrids Cortex-M4, see http://article.gmane.org/gmane.linux.drivers.devicetree/101196 Fwiw, although that platform has enough memory to use a normal kernel too, the Cortex-M4 architecture has a dedicated code bus. I used the XIP kernel to make use of the code bus for the kernel code at least. However, there might be better ways to archive this. -- Stefan
diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S index b31aa73e8076..9351f7fbdfb1 100644 --- a/arch/arm/kernel/vmlinux.lds.S +++ b/arch/arm/kernel/vmlinux.lds.S @@ -351,3 +351,13 @@ ASSERT((__arch_info_end - __arch_info_begin), "no machine record defined") * The above comment applies as well. */ ASSERT(((__hyp_idmap_text_end - __hyp_idmap_text_start) <= PAGE_SIZE), "HYP init code too big") + +#ifdef CONFIG_XIP_KERNEL +/* + * __data_loc is not only the LMA of the data section, but also the VMA of + * the end of the .rodata section. This must not overlap the VMA of the + * data section. Since the .text section starts in module space, and that + * is always below the .data section, this should be sufficient. + */ +ASSERT((_data >= __data_loc), "Text section oversize") +#endif