diff mbox

[v3] ARM: xip: Use correct symbol for end of ROM marker

Message ID HK2PR06MB056196B2E169A504A16ED9738A1D0@HK2PR06MB0561.apcprd06.prod.outlook.com (mailing list archive)
State Changes Requested
Delegated to: Simon Horman
Headers show

Commit Message

Chris Brandt Nov. 17, 2015, 4:45 p.m. UTC
> The old method with conditional placement in the linker script is
> broken beyond repair.  There is simply way too much stuff these days
> to do this sanely.  I think the only way to fix it properly is to
> have a separate linker script for XIP usage with proper (and obvious)
> section placement.



Step 1: Change the Makefiles to select an alternative linker script.

What do you think about doing the following?








Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Nicolas Pitre Nov. 17, 2015, 4:57 p.m. UTC | #1
On Tue, 17 Nov 2015, Chris Brandt wrote:

> > The old method with conditional placement in the linker script is
> > broken beyond repair.  There is simply way too much stuff these days
> > to do this sanely.  I think the only way to fix it properly is to
> > have a separate linker script for XIP usage with proper (and obvious)
> > section placement.
> 
> 
> 
> Step 1: Change the Makefiles to select an alternative linker script.
> 
> What do you think about doing the following?




That's fine.  Obviously this is the first step when developing this, but 
logically this will have to be the last patch in the series when you're 
done.

> 
> 
> diff --git a/Makefile b/Makefile
> index fd46821..956ad21 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -898,7 +898,11 @@ libs-y		:= $(libs-y1) $(libs-y2)
>  # Externally visible symbols (used by link-vmlinux.sh)
>  export KBUILD_VMLINUX_INIT := $(head-y) $(init-y)
>  export KBUILD_VMLINUX_MAIN := $(core-y) $(libs-y) $(drivers-y) $(net-y)
> +ifdef CONFIG_XIP_KERNEL
> +export KBUILD_LDS          := arch/$(SRCARCH)/kernel/vmlinux-xip.lds
> +else
>  export KBUILD_LDS          := arch/$(SRCARCH)/kernel/vmlinux.lds
> +endif
>  export LDFLAGS_vmlinux
>  # used by scripts/pacmage/Makefile
>  export KBUILD_ALLDIRS := $(sort $(filter-out arch/%,$(vmlinux-alldirs)) arch Documentation include samples scripts tools virt)
>  
>  
> 
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index af9e59b..d97fc2e 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -3,6 +3,7 @@
>  #
>  
>  CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
> +CPPFLAGS_vmlinux-xip.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
>  AFLAGS_head.o        := -DTEXT_OFFSET=$(TEXT_OFFSET)
>  
>  ifdef CONFIG_FUNCTION_TRACER
> @@ -92,4 +93,8 @@ obj-y				+= psci-call.o
>  obj-$(CONFIG_SMP)		+= psci_smp.o
>  endif
>  
> +ifdef CONFIG_XIP_KERNEL
> +extra-y := $(head-y) vmlinux-xip.lds
> +else
>  extra-y := $(head-y) vmlinux.lds
> +endif
> 
> 
> 
> 
> 
> 
> Chris
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 18, 2015, 2:09 a.m. UTC | #2
So after looking back in history, I think I now understand how/when the XIP stuff broke.

It looks like originally, the init section came first, followed by text and data.

This is why all the XIP code used _etext as its marker for the end of what needed to be mapped.
Like you said, the initialized data values get copied from ROM to RAM early in boot so it's not an issue if the data section (that comes after text) gets blown away when the full MMU setup is done.


However, after the init section was moved from the first section to almost the last, all the _etext references scatter through the kernel were never updated.


So basically, even though I've now got a separate linker script for XIP builds, it really hasn't changed much because the real issue all along was that the XIP_KERNEL references to _etext should have been changed to _data_loc.

With that said, I still think the correct way to fix this issue is to export _data_loc and use that as the end marker. (notice I changed my mind and now say _data_loc instead of _edata_loc because there's no reason to map the full data section anymore).

The other option, if we really wanted to keep the references to _etext as they are, is to simply move the one line " _etext = .;" to the bottom of the file....right before _data_loc is declared.
But...that would be a bit misleading if you ask me.


It sounds like there would still be a request to split up the linker scripts in order to remove the 6 XIP_KERNEL references scattered  throughout vmlinux.lds.S. However, it would still be nice to keep the two versions looking as close as they can to each other with only the obvious changes (like removing XIP_KERNEL from the non-XIP version, and removing SMP_ON_UP from the XIP version)


Thoughts?


Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 18, 2015, 3:17 a.m. UTC | #3
On Wed, 18 Nov 2015, Chris Brandt wrote:

> It sounds like there would still be a request to split up the linker 
> scripts in order to remove the 6 XIP_KERNEL references scattered 
> throughout vmlinux.lds.S. However, it would still be nice to keep the 
> two versions looking as close as they can to each other with only the 
> obvious changes (like removing XIP_KERNEL from the non-XIP version, 
> and removing SMP_ON_UP from the XIP version)

I don't think so. XIP and non-XIP are fundamentally looking different 
these days.  Trying to keep them as similar as possible is rather an 
impediment to both cases and brings no real benefits.

Not only should .init.smpalt be gone from the XIP version, but it might 
be safer to put that into a special "should_be_empty" section and bail 
out with ASSERT() if that section size isn't zero. Same thing for 
pv_table. This way we'll be sure that nothing in the kernel config 
expects self-modified code to be present.

With XIP there is still an init section, but it is pointless to store 
.init.text stuff in there. That should instead stay in ROM and never be 
discarded (obviously). No need to page align it either at that point 
which should save some ROM space.

Things like .kprobes.text should be in ROM, but the entire executable in 
ROM should be surrounded by __kprobes_text_start and __kprobes_text_end 
to prevent any attempt to put kprobes there. This way, kprobes could 
still work for modules.

Etc. Etc.

There are simply too many things these days that require special 
considerations with XIP. And freeing the main script from XIP 
considerations will ease maintenance as well, more than the added 
maintenance from the duplicated parts.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 18, 2015, 8:30 a.m. UTC | #4
On Tuesday 17 November 2015 22:17:41 Nicolas Pitre wrote:
> On Wed, 18 Nov 2015, Chris Brandt wrote:
> 
> > It sounds like there would still be a request to split up the linker 
> > scripts in order to remove the 6 XIP_KERNEL references scattered 
> > throughout vmlinux.lds.S. However, it would still be nice to keep the 
> > two versions looking as close as they can to each other with only the 
> > obvious changes (like removing XIP_KERNEL from the non-XIP version, 
> > and removing SMP_ON_UP from the XIP version)
> 
> I don't think so. XIP and non-XIP are fundamentally looking different 
> these days.  Trying to keep them as similar as possible is rather an 
> impediment to both cases and brings no real benefits.
> 
> Not only should .init.smpalt be gone from the XIP version, but it might 
> be safer to put that into a special "should_be_empty" section and bail 
> out with ASSERT() if that section size isn't zero. Same thing for 
> pv_table. This way we'll be sure that nothing in the kernel config 
> expects self-modified code to be present.
> 
> With XIP there is still an init section, but it is pointless to store 
> .init.text stuff in there. That should instead stay in ROM and never be 
> discarded (obviously). No need to page align it either at that point 
> which should save some ROM space.
> 
> Things like .kprobes.text should be in ROM, but the entire executable in 
> ROM should be surrounded by __kprobes_text_start and __kprobes_text_end 
> to prevent any attempt to put kprobes there. This way, kprobes could 
> still work for modules.
> 
> Etc. Etc.
> 
> There are simply too many things these days that require special 
> considerations with XIP. And freeing the main script from XIP 
> considerations will ease maintenance as well, more than the added 
> maintenance from the duplicated parts.

[Adding Uwe and Maxime to Cc]

I believe Uwe has been running XIP_KERNEL on efm32 for a while now,
but he also mentioned that he needed some extra hacks in the linker
script and elsewhere to get it to work.

It would be good to coordinate this, he may have found additional
problems, or he can maybe help test the new approach on his hardware.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 18, 2015, 3:16 p.m. UTC | #5
> With XIP there is still an init section, [snip]
> No need to page align it either at that point which should save some
> ROM space.

Good point.

> Things like .kprobes.text should be in ROM, [snip]
>
> Etc. Etc.

Here's the thing...I'm probably out of my depth on most of this. But, I'd be happy to post up a RFC patch and then start collecting the feedback on what can be removed. I can at least test it on a real XIP system.


Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 18, 2015, 3:28 p.m. UTC | #6
> [Adding Uwe and Maxime to Cc]
> 
> I believe Uwe has been running XIP_KERNEL on efm32 for a while now,
> but he also mentioned that he needed some extra hacks in the linker
> script and elsewhere to get it to work.


Yes, Maxime got the XIP fix for scripts/link-vmlinux.sh into the kernel ([PATCH v6 01/15] scripts: link-vmlinux: Don't pass page offset to kallsyms if XIP Kernel), which of course made one less file I have to patch.


> It would be good to coordinate this, he may have found additional
> problems, or he can maybe help test the new approach on his hardware.

The part I'm using (RZ/A1H) is a Cortex A9 and the EFM32 is a Cortex M3, so we'd be able to test both MMU and non-MMU XIP systems.


Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 18, 2015, 5:07 p.m. UTC | #7
On Wed, 18 Nov 2015, Chris Brandt wrote:

> > With XIP there is still an init section, [snip]
> > No need to page align it either at that point which should save some
> > ROM space.
> 
> Good point.
> 
> > Things like .kprobes.text should be in ROM, [snip]
> >
> > Etc. Etc.
> 
> Here's the thing...I'm probably out of my depth on most of this. But, 
> I'd be happy to post up a RFC patch and then start collecting the 
> feedback on what can be removed. I can at least test it on a real XIP 
> system.

Sure.  You should start by splitting the XIP stuff out to a separate 
script so functionally it is the same (broken) end result.  And from 
there adding incremental fixes to the XIP script.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 18, 2015, 7:36 p.m. UTC | #8
> Sure.  You should start by splitting the XIP stuff out to a separate
> script so functionally it is the same (broken) end result.  And from
> there adding incremental fixes to the XIP script.

More or less that's what I'm doing now on my system. You can diff the 2 in order to see what I've removed.

But wait...you said 'the last step' was to add a 2nd linker file and change the Makefiles to use it.


Are you saying I should first submit a patch that simply adds the new file, but not hook it up to any Makefiles yet?
Then, once it has been updated a few times and is in better shape, I submit another patch that hooks it up with the Makefiles so I can then finally remove the XIP_KERNEL parts from the current vmlinux.lds.S?

Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 18, 2015, 7:44 p.m. UTC | #9
On Wed, 18 Nov 2015, Chris Brandt wrote:

> > Sure.  You should start by splitting the XIP stuff out to a separate
> > script so functionally it is the same (broken) end result.  And from
> > there adding incremental fixes to the XIP script.
> 
> More or less that's what I'm doing now on my system. You can diff the 2 in order to see what I've removed.
> 
> But wait...you said 'the last step' was to add a 2nd linker file and change the Makefiles to use it.
> 
> 
> Are you saying I should first submit a patch that simply adds the new file, but not hook it up to any Makefiles yet?
> Then, once it has been updated a few times and is in better shape, I submit another patch that hooks it up with the Makefiles so I can then finally remove the XIP_KERNEL parts from the current vmlinux.lds.S?

Well... I'm of the opinion now that the first patch should be the 
creation of the new script being a copy of the original with the XIP 
conditionals removed from both, plus the makefile changes. That should 
be easy to verify that nothing changed result wise, and then additional 
patches could bring XIP efinements on top without disturbing the non-XIP 
case.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 18, 2015, 8 p.m. UTC | #10
>> Are you saying I should first submit a patch that simply adds the
>> new file, but not hook it up to any Makefiles yet?
>> Then, once it has been updated a few times and is in better shape,
>> I submit another patch that hooks it up with the Makefiles so I can
>> then finally remove the XIP_KERNEL parts from the current
>> vmlinux.lds.S?
>
> Well... I'm of the opinion now that the first patch should be the
> creation of the new script being a copy of the original with the
> XIP conditionals removed from both, plus the makefile changes. That
> should be easy to verify that nothing changed result wise, and then
> additional patches could bring XIP efinements on top without
> disturbing the non-XIP case.
>
> Nicolas

OK. I'm good with that. I'll submit a patch and see what happens.

Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Makefile b/Makefile
index fd46821..956ad21 100644
--- a/Makefile
+++ b/Makefile
@@ -898,7 +898,11 @@  libs-y		:= $(libs-y1) $(libs-y2)
 # Externally visible symbols (used by link-vmlinux.sh)
 export KBUILD_VMLINUX_INIT := $(head-y) $(init-y)
 export KBUILD_VMLINUX_MAIN := $(core-y) $(libs-y) $(drivers-y) $(net-y)
+ifdef CONFIG_XIP_KERNEL
+export KBUILD_LDS          := arch/$(SRCARCH)/kernel/vmlinux-xip.lds
+else
 export KBUILD_LDS          := arch/$(SRCARCH)/kernel/vmlinux.lds
+endif
 export LDFLAGS_vmlinux
 # used by scripts/pacmage/Makefile
 export KBUILD_ALLDIRS := $(sort $(filter-out arch/%,$(vmlinux-alldirs)) arch Documentation include samples scripts tools virt)
 
 

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index af9e59b..d97fc2e 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -3,6 +3,7 @@ 
 #
 
 CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
+CPPFLAGS_vmlinux-xip.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
 AFLAGS_head.o        := -DTEXT_OFFSET=$(TEXT_OFFSET)
 
 ifdef CONFIG_FUNCTION_TRACER
@@ -92,4 +93,8 @@  obj-y				+= psci-call.o
 obj-$(CONFIG_SMP)		+= psci_smp.o
 endif
 
+ifdef CONFIG_XIP_KERNEL
+extra-y := $(head-y) vmlinux-xip.lds
+else
 extra-y := $(head-y) vmlinux.lds
+endif