[PATCH] 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 <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

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.

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:

quoted

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

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:

quoted

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:

quoted

quoted

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.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

On Thu, 18 Feb 2016, Russell King - ARM Linux wrote:

On Thu, Feb 18, 2016 at 07:42:20PM +0000, Chris Brandt wrote:

quoted

On Thu, 18 Feb 2016, Nicolas Pitre wrote:

quoted

quoted

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:

quoted

On Thu, Feb 18, 2016 at 07:42:20PM +0000, Chris Brandt wrote:

quoted

On Thu, 18 Feb 2016, Nicolas Pitre wrote:

quoted

quoted

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:

quoted

- 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).

quoted

- 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:

quoted

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:

quoted

quoted

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:

quoted

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:

quoted

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 Friday 19 February 2016 16:28:06 Chris Brandt wrote:

On 19 Feb 2016, Arnd Bergmann wrote:

quoted

quoted

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.

It was really just a hack, deleting code that we normally need,  but
you can work on top of that. This is still not useful unless you
also find a way to guess the __pv_phys_pfn_offset value. Maybe
there is even a way to derive that from the stack pointer if we
can safely assume that it points into RAM ?

	Arnd

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 9427fd632552..3f797bc5d73f 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h

@@ -175,10 +175,11 @@
 #define __PV_BITS_7_0	0x81
 
 extern unsigned long __pv_phys_pfn_offset;
-extern u64 __pv_offset;
+extern phys_addr_t __pv_offset;
 extern void fixup_pv_table(const void *, unsigned long);
 extern const void *__pv_table_begin, *__pv_table_end;
 
+#define PAGE_SHIFT 12
 #define PHYS_OFFSET	((phys_addr_t)__pv_phys_pfn_offset << PAGE_SHIFT)
 #define PHYS_PFN_OFFSET	(__pv_phys_pfn_offset)
 

@@ -186,75 +187,14 @@ extern const void *__pv_table_begin, *__pv_table_end;
 	((((unsigned long)(kaddr) - PAGE_OFFSET) >> PAGE_SHIFT) + \
 	 PHYS_PFN_OFFSET)
 
-#define __pv_stub(from,to,instr,type)			\
-	__asm__("@ __pv_stub\n"				\
-	"1:	" instr "	%0, %1, %2\n"		\
-	"	.pushsection .pv_table,\"a\"\n"		\
-	"	.long	1b\n"				\
-	"	.popsection\n"				\
-	: "=r" (to)					\
-	: "r" (from), "I" (type))
-
-#define __pv_stub_mov_hi(t)				\
-	__asm__ volatile("@ __pv_stub_mov\n"		\
-	"1:	mov	%R0, %1\n"			\
-	"	.pushsection .pv_table,\"a\"\n"		\
-	"	.long	1b\n"				\
-	"	.popsection\n"				\
-	: "=r" (t)					\
-	: "I" (__PV_BITS_7_0))
-
-#define __pv_add_carry_stub(x, y)			\
-	__asm__ volatile("@ __pv_add_carry_stub\n"	\
-	"1:	adds	%Q0, %1, %2\n"			\
-	"	adc	%R0, %R0, #0\n"			\
-	"	.pushsection .pv_table,\"a\"\n"		\
-	"	.long	1b\n"				\
-	"	.popsection\n"				\
-	: "+r" (y)					\
-	: "r" (x), "I" (__PV_BITS_31_24)		\
-	: "cc")
-
-static inline phys_addr_t __virt_to_phys(unsigned long x)
-{
-	phys_addr_t t;
-
-	if (sizeof(phys_addr_t) == 4) {
-		__pv_stub(x, t, "add", __PV_BITS_31_24);
-	} else {
-		__pv_stub_mov_hi(t);
-		__pv_add_carry_stub(x, t);
-	}
-	return t;
-}
-
-static inline unsigned long __phys_to_virt(phys_addr_t x)
-{
-	unsigned long t;
-
-	/*
-	 * 'unsigned long' cast discard upper word when
-	 * phys_addr_t is 64 bit, and makes sure that inline
-	 * assembler expression receives 32 bit argument
-	 * in place where 'r' 32 bit operand is expected.
-	 */
-	__pv_stub((unsigned long) x, t, "sub", __PV_BITS_31_24);
-	return t;
-}
-
-#else
-
-#define PHYS_OFFSET	PLAT_PHYS_OFFSET
-#define PHYS_PFN_OFFSET	((unsigned long)(PHYS_OFFSET >> PAGE_SHIFT))
-
 static inline phys_addr_t __virt_to_phys(unsigned long x)
 {
-	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
+	return (phys_addr_t)x - PAGE_OFFSET + __pv_offset;
 }
 
 static inline unsigned long __phys_to_virt(phys_addr_t x)
 {
-	return x - PHYS_OFFSET + PAGE_OFFSET;
+	return x - __pv_offset + PAGE_OFFSET;
 }
 
 #define virt_to_pfn(kaddr) \

On Fri, Feb 19, 2016 at 04:59:43PM +0100, Arnd Bergmann wrote:

On Friday 19 February 2016 15:36:23 Chris Brandt wrote:

quoted

On 19 Feb 2016, Arnd Bergmann wrote:

quoted

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.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

As devices move to multiplatform only, you should not be restricted from
selecting XIP. Whether it makes sense or not depends on how you configure
the rest of the kernel (as in, removing individual platforms).
While there is no easy way to guarantee success when XIP is selected,
the MULTIPLATFORM_XIP_CAPABLE attribute at least provides a means to
show what devices have been known to boot (when configured correctly).

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v2:
* Added MULTIPLATFORM_XIP_CAPABLE because there is no good way to
restrict that only 1 device can be selected at one time, so this at
least narrows down the the exposure of XIP in multiplatform.
---
 arch/arm/Kconfig               | 7 +++++--
 arch/arm/mach-shmobile/Kconfig | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d67f1aa..0d838ed 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig

@@ -226,6 +226,9 @@ config NEED_RET_TO_USER
 config ARCH_MTD_XIP
 	bool
 
+config MULTIPLATFORM_XIP_CAPABLE
+	bool
+
 config VECTORS_BASE
 	hex
 	default 0xffff0000 if MMU || CPU_HIGH_VECTOR

@@ -327,7 +330,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

@@ -1918,7 +1921,7 @@ endchoice
 
 config XIP_KERNEL
 	bool "Kernel Execute-In-Place from ROM"
-	depends on !ARM_LPAE && !ARCH_MULTIPLATFORM
+	depends on (!ARM_LPAE && !ARCH_MULTIPLATFORM) || (!ARM_LPAE && MULTIPLATFORM_XIP_CAPABLE)
 	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

diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
index cd5f171..b27661e 100644
--- a/arch/arm/mach-shmobile/Kconfig
+++ b/arch/arm/mach-shmobile/Kconfig

@@ -57,6 +57,7 @@ config ARCH_R7S72100
 	bool "RZ/A1H (R7S72100)"
 	select PM_GENERIC_DOMAINS if PM
 	select SYS_SUPPORTS_SH_MTU2
+	select MULTIPLATFORM_XIP_CAPABLE
 
 config ARCH_R8A73A4
 	bool "R-Mobile APE6 (R8A73A40)"

-- 
1.9.1