ARM: Allow MULTIPLATFORM to select XIP

As Multiplatform seems to be the way of the future, you should not restrict from selecting XIP. Whether it makes sense or not depends on how you configure the rest of the kernel (as in, removing individual platforms). Also, it stands to reason that if you select MULTIPLATFORM and XIP_KERNEL, you either know what you are doing...or have NO idea what you are doing.

Signed-off-by Chris Brandt <[email protected]>

---
 arch/arm/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm/Kconfig +2 -2

index cc95ff8..2e4a127 100644
@@ -327,7 +327,7 @@ config ARCH_MULTIPLATFORM
 	depends on MMU
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select ARM_HAS_SG_CHAIN
-	select ARM_PATCH_PHYS_VIRT
+	select ARM_PATCH_PHYS_VIRT if !XIP_KERNEL
 	select AUTO_ZRELADDR
 	select CLKSRC_OF
 	select COMMON_CLK

@@ -1919,7 +1919,7 @@ endchoice
 
 config XIP_KERNEL
 	bool "Kernel Execute-In-Place from ROM"
-	depends on !ARM_LPAE && !ARCH_MULTIPLATFORM
+	depends on !ARM_LPAE
 	help
 	  Execute-In-Place allows the kernel to run from non-volatile storage
 	  directly addressable by the CPU, such as NOR flash. This saves RAM

Signature · 1 line

1.9.1

And realistically it makes no sense to use XIP (surely because you need to save space) and have multi-platform support in your kernel.

IMHO it should be clear that with XIP there is only one platform that can be supported.

Quoted 2 lines

And realistically it makes no sense to use XIP (surely because you need to save space) and have multi-platform support in your kernel.

The problem is that you don't have a choice: Multi-platform is your only choice for many devices (MMU devices that is).

So for XIP, it comes down to either:

A) Go back to making individual configs for each MPU

B) Let everything keep getting grouped together into multi-platform, and then just de-selecting the MPU you don't want.

I can tell you when I build my upstream XIP kernel for testing for the RZ/A1, it's ARCH_MULTIPLATFORM (all Renesas parts are multiplatform) and then remove everything I don't want/need, my kernel size is 3.4MB.

Quoted 2 lines

IMHO it should be clear that with XIP there is only one platform that can be supported.

I would be fine if there was some way you could only select XIP_KERNEL if only 1 of the MPUs in the multiplatform group were selected...but I can't figure out how to do that in the kbuild system (without adding lots of chunky configs all over the place...which is not what I want)

Is there some way you can do a PLATFORM_COUNTER++ for each device that is selected, and then XIP_KERNEL would depend on PLATFORM_COUNTER == 1 ?

Quoted 3 lines

Is there some way you can do a PLATFORM_COUNTER++ for each device that is selected, and then XIP_KERNEL would depend on PLATFORM_COUNTER == 1 ?

I don't know all the Kconfig possibilities.

But would something like this work?

config FOO

	depends on !(XIP && CHOICE_MADE)
	select CHOICE_MADE

Quoted 11 lines · Nicolas Pitre

Quoted 3 lines

Is there some way you can do a PLATFORM_COUNTER++ for each device that is selected, and then XIP_KERNEL would depend on PLATFORM_COUNTER == 1 ?

I don't know all the Kconfig possibilities.

But would something like this work?

config FOO

	depends on !(XIP && CHOICE_MADE)
	select CHOICE_MADE

I follow your logic and that might work: If you made multiple choices, it would just take the first one.

However...I just tried it and it crashes.

scripts/kconfig/mconf Kconfig
make[2]: *** [menuconfig] Segmentation fault (core dumped) make[1]: *** [menuconfig] Error 2

It looks like you can't depend on something not being set that you are going to set yourself. Basically..."do as I say, not as I do"

Quoted 24 lines · Chris Brandt

Quoted 11 lines · Nicolas Pitre

Quoted 3 lines

Is there some way you can do a PLATFORM_COUNTER++ for each device that is selected, and then XIP_KERNEL would depend on PLATFORM_COUNTER == 1 ?

I don't know all the Kconfig possibilities.

But would something like this work?

config FOO

	depends on !(XIP && CHOICE_MADE)
	select CHOICE_MADE

I follow your logic and that might work: If you made multiple choices, it would just take the first one.

However...I just tried it and it crashes.

scripts/kconfig/mconf Kconfig
make[2]: *** [menuconfig] Segmentation fault (core dumped) make[1]: *** [menuconfig] Error 2

Quoted 2 lines

It looks like you can't depend on something not being set that you are going to set yourself. Basically..."do as I say, not as I do"

It's missing a bool/tristate. Every config option needs to be typed.

Quoted 25 lines · Russell King - ARM Linux

Quoted 24 lines · Chris Brandt

Quoted 11 lines · Nicolas Pitre

Quoted 3 lines

Is there some way you can do a PLATFORM_COUNTER++ for each device that is selected, and then XIP_KERNEL would depend on PLATFORM_COUNTER == 1 ?

I don't know all the Kconfig possibilities.

But would something like this work?

config FOO
depends on !(XIP && CHOICE_MADE)
select CHOICE_MADE

I follow your logic and that might work: If you made multiple choices, it would just take the first one.

However...I just tried it and it crashes.

scripts/kconfig/mconf Kconfig
make[2]: *** [menuconfig] Segmentation fault (core dumped) make[1]: *** [menuconfig] Error 2

Quoted 4 lines

Quoted 2 lines

It looks like you can't depend on something not being set that you are going to set yourself. Basically..."do as I say, not as I do"

It's missing a bool/tristate. Every config option needs to be typed.

Thank you! That makes things better.

Technically, it seems to work (you can only select 1), but the menu system is a little flakey when you try jumping back and forth between what device to select. Somehow when 2 get selected, I need to get kbuild to go back and do a full refresh or something.

I'm going to play with it some more before I start shouting victory.

In a different thread today, I was trying to come up with a way to reliably pick a PHYS_OFFSET value, and I think it can be done but it quickly gets ugly without extending the Kconfig language.

Also, the same problem exists in three areas:

a) PHYS_OFFSET / DRAM_BASE
b) XIP_PHYS_ADDR
c) DEBUG_UART_VIRT/DEBUG_UART_PHYS

The last one is already really ugly and by nature causes problems whenever someone enables DEBUG_LL and tries to boot a compressed kernel on a platform other than the one that was configured.

XIP_PHYS_ADDR is probably the worst here, because the number does not just depend on the SoC family but the specific board configuration and could even change when you rearrange the partitions on your NOR flash (unless you enforce that the kernel has to start at the beginning of the ROM or some other fixed location).

It would be nice to handle all three of the above in a similar manner, and I'm definitely open to a range of solutions for this, like

- always allow turning XIP_KERNEL on for all configurations, and expect the user to know what they are doing (as your current patch)

- allow turning on XIP_KERNEL as long as Kconfig can figure out that this has a chance of working on at least one platform (easy enough to implement, and similar to how we handle DEBUG_LL).

- make XIP_KERNEL and DEBUG_LL depend on either CONFIG_EXPERT or a new "I know this breaks other platforms and I won't complain about that" flag.

- implement a Kconfig method to only allow XIP_KERNEL if exactly one platform is enable that supports it, or if all platforms use the exact same PHYS_OFFSET and XIP_PHYS_ADDR settings. Whether we want to enforce the same thing for DEBUG_LL is a separate matter once that infrastructure exists.

Arnd

Quoted 3 lines · Arnd Bergmann

- allow turning on XIP_KERNEL as long as Kconfig can figure out that this has a chance of working on at least one platform (easy enough to implement, and similar to how we handle DEBUG_LL).

In the patch I was working on, if ARCH_MULTIPLATFORM was selected
(which is the problem I'm trying to solve), you can only select
XIP_KERNEL if 'MULTIPLATFORM_XIP_CAPABLE' is selected. At least then we could restrict the multiplatform platforms to ones that stand a chance of XIP booting (or at least have been known to boot at one point or another)

Quoted 7 lines

- implement a Kconfig method to only allow XIP_KERNEL if exactly one platform is enable that supports it, or if all platforms use the exact same PHYS_OFFSET and XIP_PHYS_ADDR settings. Whether we want to enforce the same thing for DEBUG_LL is a separate matter once that infrastructure exists.

Arnd

This last one would be my favorite.

It would be nice if you could turn a section of "config ARCH_xxx bool" options into "choice" single select with some magical dynamic #ifdef.

if XIP_KERNEL
choice
endif

config DEVICE_A
bool "Device A"

config DEVICE_B
bool "Device B"

config DEVICE_C
bool "Device C"

if XIP_KERNEL
endchoice
endif

...but...this doesn't work:

I need a:

__INLINE__ if XIP_KERNEL
choice
endif

Although...at some point I'd argue that you are trying to cater to a level of safety that is above and beyond XIP_KERNEL. There are probably multiple CONFIG options in the kernel that would break an XIP anyway.

A car with the best seat belts and airbags still won't help you if you drive it off a cliff.

Quoted 2 lines

A car with the best seat belts and airbags still won't help you if you drive it off a cliff.

That was my point about DEBUG_LL: We already allow picking a DEBUG_LL option that immediately breaks booting on any other platform, and (worse) breaks even printing any useful output about that fact.

Even allowing XIP_KERNEL unconditionally doesn't make it much worse.

Other options that are able to ruin your day are CONFIG_CPU_BIG_ENDIAN (which breaks all user space when changed) and CONFIG_ARM_LPAE (which breaks booting on Cortex-A8/A9/A5 systems), or simply disabling essential drivers like your console or the block device for your rootfs.

Arnd

Quoted 6 lines · Arnd Bergmann

Quoted 2 lines

A car with the best seat belts and airbags still won't help you if you drive it off a cliff.

That was my point about DEBUG_LL: We already allow picking a DEBUG_LL option that immediately breaks booting on any other platform, and (worse) breaks even printing any useful output about that fact.

Ya, the DEBUG_LL issue seems like it could be an endless hole of options all throughout the Kconfigs.

DT was supposed to allow everything to be configured at boot time, but it looks like we still need an early-Device-Tree for settings before DT is available. That would solve your DEBUG_LL and PHYS_OFFSET issue.

For PHYS_OFFSET, parsing the DT doesn't help at all because we only need it for XIP_KERNEL (more or less at least) which cannot patch the kernel image at boot time, so knowing that the address is wrong also doesn't help you.

Arnd

Quoted 5 lines · Arnd Bergmann

For PHYS_OFFSET, parsing the DT doesn't help at all because we only need it for XIP_KERNEL (more or less at least) which cannot patch the kernel image at boot time, so knowing that the address is wrong also doesn't help you.

Quoted 1 line

Arnd

OK, at first I was thinking that PHYS_OFFSET was only used early in boot as a pointer to valid RAM before the DT was read (in head.S and head-nommu.S). Hence, you could pass in the pointer in via another CPU register like the DT pointer is passed in.

But, now I see that PHYS_OFFSET is used all over the place as a hard coded #define, hence your comment "which cannot patch the kernel image at boot time"

So, I retract my thought...it has to be configured at build-time (unless of course you turn it into a global variable everywhere...which might be an even bigger mess)

Quoted 9 lines

But, now I see that PHYS_OFFSET is used all over the place as a hard coded #define, hence your comment "which cannot patch the kernel image at boot time"

So, I retract my thought...it has to be configured at build-time (unless of course you turn it into a global variable everywhere...which
might be an even bigger mess)

BTW, I've tried removing the patching in CONFIG_PHYS_VIRT and replaced it with references to __pv_phys_pfn_offset, which surprisingly only grew .text from 4901692 to 4904300 bytes, so the size overhead of doing this would be close to zero.

Arnd

Quoted 10 lines · Arnd Bergmann

Quoted 9 lines

But, now I see that PHYS_OFFSET is used all over the place as a hard coded #define, hence your comment "which cannot patch the kernel image at boot time"

So, I retract my thought...it has to be configured at build-time (unless of course you turn it into a global variable everywhere...which
might be an even bigger mess)

Quoted 6 lines

BTW, I've tried removing the patching in CONFIG_PHYS_VIRT and replaced it with references to __pv_phys_pfn_offset, which surprisingly only grew .text from 4901692 to 4904300 bytes, so the size overhead of doing this would be close to zero.

Arnd

Cool. If you come up with a patch, I'll give it a try.

Quoted 13 lines · On Friday 19 February 2016 15:36:23 Chris Brandt wrote:

Quoted 6 lines · Arnd Bergmann

For PHYS_OFFSET, parsing the DT doesn't help at all because we only need it for XIP_KERNEL (more or less at least) which cannot patch the kernel image at boot time, so knowing that the address is wrong also doesn't help you.

Arnd

OK, at first I was thinking that PHYS_OFFSET was only used early in boot as a pointer to valid RAM before the DT was read (in head.S and head-nommu.S). Hence, you could pass in the pointer in via another CPU register like the DT pointer is passed in.

But, now I see that PHYS_OFFSET is used all over the place as a hard coded #define, hence your comment "which cannot patch the kernel image at boot time"

Quoted 2 lines

So, I retract my thought...it has to be configured at build-time (unless of course you turn it into a global variable everywhere...which might be an even bigger mess)

BTW, I've tried removing the patching in CONFIG_PHYS_VIRT and replaced it with references to __pv_phys_pfn_offset, which surprisingly only grew .text from 4901692 to 4904300 bytes, so the size overhead of doing this would be close to zero.