| Message ID | 1455816310-11308-1-git-send-email-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 18 Feb 2016, Chris Brandt wrote: > 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. There are simply too many people who don't know what they're doing. 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. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > arch/arm/Kconfig | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index cc95ff8..2e4a127 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -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 > -- > 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. 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. > 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 ? Chris
On Thu, 18 Feb 2016, Chris Brandt wrote: > > 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. > > > > 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 > ? I don't know all the Kconfig possibilities. But would something like this work? config FOO depends on !(XIP && CHOICE_MADE) select CHOICE_MADE ? Nicolas
On Thu, 18 Feb 2016, Nicolas Pitre wrote: > > 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" Chris
On Thu, Feb 18, 2016 at 07:42:20PM +0000, Chris Brandt wrote: > On Thu, 18 Feb 2016, Nicolas Pitre wrote: > > > > 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" It's missing a bool/tristate. Every config option needs to be typed.
On Thu, 18 Feb 2016, Russell King - ARM Linux wrote: > On Thu, Feb 18, 2016 at 07:42:20PM +0000, Chris Brandt wrote: > > On Thu, 18 Feb 2016, Nicolas Pitre wrote: > > > > > > 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" > > 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. Chris
On Friday 19 February 2016 03:39:38 Chris Brandt wrote: > On Thu, 18 Feb 2016, Russell King - ARM Linux wrote: > > > On Thu, Feb 18, 2016 at 07:42:20PM +0000, Chris Brandt wrote: > > > On Thu, 18 Feb 2016, Nicolas Pitre wrote: > > > > > > > > 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" > > > > 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. I think Kconfig interprets the above as a circular dependency (using depends and select on the same symbol), it will print a warning and misbehave randomly. 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
On 19 Feb 2016, Arnd Bergmann wrote: > 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. OK, I see that now (no idea how you guy weed through so many reflector emails) > 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) This method is the quickest and easiest (from a kbuild perspective), and I was going with the assumption that the less I modify, less likely I was to break something else. > - 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) > - 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. A plausible deniability clause. Funny. > - 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: arch/arm/mach-shmobile/Kconfig:51: unexpected 'endif' within choice block arch/arm/mach-shmobile/Kconfig:131: unexpected 'endchoice' within if block 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. Chris
On Friday 19 February 2016 13:49:38 Chris Brandt wrote: > On 19 Feb 2016, Arnd Bergmann wrote: > > - 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) Right, this seems fine to me, and we can combine that with a Kconfig "choice" statement for picking one of the addresses for RAM and ROM that are supported by the platforms (there should ideally only be one of each, making the choice automatic). > > - 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: We can have 'tristate' inside of 'choice', which lets you pick one or more the the inner options when they are all 'm', but only one if one of them is 'y'. I don't think that solves the problem at hand though, because that breaks the current per-platform submenus that are not allowed inside of a 'choice'. > 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. We already turn them off today, mainly any dynamic patching is broken, and so is the address randomization that Ard is working on, but those are easy enough to handle. > 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
On 19 Feb 2016, Arnd Bergmann wrote: > > 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. > 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 > Yup. As I said...car...vroom vroom...cliff. (to be humble, I've driven off that cliff before and spent hours trying to figure out what happened) Chris
On Friday 19 February 2016 14:46:36 Chris Brandt wrote: > On 19 Feb 2016, Arnd Bergmann wrote: > > > > 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. Not really: for DEBUG_LL, it doesn't work because DEBUG_LL is literally meant to work from the first instruction in the kernel, so there can't be any code to configure it. If you don't need to go that low-level, you should use "earlycon", which can parse the DT just fine. 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
On 19 Feb 2016, Arnd Bergmann wrote: > 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" 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) Chris
On Friday 19 February 2016 15:36:23 Chris Brandt wrote: > On 19 Feb 2016, Arnd Bergmann wrote: > > > 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" > > 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
On 19 Feb 2016, Arnd Bergmann wrote: > > > 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 Cool. If you come up with a patch, I'll give it a try. Overall, I'd rather side with "Let me enable stuff even though it might not work automatically" as opposed to "Don't let me do anything unless it's simple and safe" (which kind of reminds me of Apple products) Chris
On Fri, Feb 19, 2016 at 04:59:43PM +0100, Arnd Bergmann wrote: > On Friday 19 February 2016 15:36:23 Chris Brandt wrote: > > On 19 Feb 2016, Arnd Bergmann wrote: > > > > > 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" > > > > 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. The next job is to perf the resulting kernel - because you will have the additional overhead of loading the offset. Size is not everything, contary to popular beliefs.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index cc95ff8..2e4a127 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -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
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 <chris.brandt@renesas.com> --- arch/arm/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)