[BUG, RFC] MTD Execute in Place on ARM breaks build

[BUG, RFC] MTD Execute in Place on ARM breaks build

[Petr Cvek]

Hello,

Configuration:

	CONFIG_MTD_XIP=y

on PXA2xx architecture causes request of undefined macros from 

	drivers/mtd/chipscfi_cmdset_0001.c

These macros (using ICIP and ICMR) are defined here:

	http://lxr.free-electrons.com/source/arch/arm/mach-pxa/include/mach/mtd-xip.h#L20

register definitions for ICIP and ICMR were removed by commit:
	
	5d284e353eb11ab2e8b1c5671ba06489b0bd1e0c

Similar is for macros with OSCR (lines 23 and 24), which have different type.
Re-adding ICIP and ICMR definition and explicit type conversion will fix the build
and the kernel boots, but I'm not yet able to test XIP functionality (too many different
bugs to flash over windows mobile image).

My temporal fix (ugly):

#define ICIP __REG(0x40D00000)  /* Interrupt Controller IRQ Pending Register */
#define ICMR __REG(0x40D00004)  /* Interrupt Controller Mask Register */
...
#define xip_currtime()		((unsigned long) OSCR)
#define xip_elapsed_since(x)	(signed)((((unsigned long) OSCR) - (x)) / 4)


P.S. Same macros are used on omap1 and sa1100 too.

Petr Cvek

[BUG, RFC] MTD Execute in Place on ARM breaks build

[Petr Cvek]

Hello,

Configuration:

	CONFIG_MTD_XIP=y

on PXA2xx architecture causes request of undefined macros from 

	drivers/mtd/chipscfi_cmdset_0001.c

These macros (using ICIP and ICMR) are defined here:

	http://lxr.free-electrons.com/source/arch/arm/mach-pxa/include/mach/mtd-xip.h#L20

register definitions for ICIP and ICMR were removed by commit:
	
	5d284e353eb11ab2e8b1c5671ba06489b0bd1e0c

Similar is for macros with OSCR (lines 23 and 24), which have different type.
Re-adding ICIP and ICMR definition and explicit type conversion will fix the build
and the kernel boots, but I'm not yet able to test XIP functionality (too many different
bugs to flash over windows mobile image).

My temporal fix (ugly):

#define ICIP __REG(0x40D00000)  /* Interrupt Controller IRQ Pending Register */
#define ICMR __REG(0x40D00004)  /* Interrupt Controller Mask Register */
...
#define xip_currtime()		((unsigned long) OSCR)
#define xip_elapsed_since(x)	(signed)((((unsigned long) OSCR) - (x)) / 4)


P.S. Same macros are used on omap1 and sa1100 too.

Petr Cvek

Re: [BUG, RFC] MTD Execute in Place on ARM breaks build

[Arnd Bergmann]

On Friday 07 August 2015 22:20:41 Petr Cvek wrote:
> Hello,
> 
> Configuration:
> 
> 	CONFIG_MTD_XIP=y
> 
> on PXA2xx architecture causes request of undefined macros from 
> 
> 	drivers/mtd/chipscfi_cmdset_0001.c
> 
> These macros (using ICIP and ICMR) are defined here:
> 
> 	http://lxr.free-electrons.com/source/arch/arm/mach-pxa/include/mach/mtd-xip.h#L20
> 
> register definitions for ICIP and ICMR were removed by commit:
> 	
> 	5d284e353eb11ab2e8b1c5671ba06489b0bd1e0c
> 
> Similar is for macros with OSCR (lines 23 and 24), which have different type.
> Re-adding ICIP and ICMR definition and explicit type conversion will fix the build
> and the kernel boots, but I'm not yet able to test XIP functionality (too many different
> bugs to flash over windows mobile image).
>
> My temporal fix (ugly):
> 
> #define ICIP __REG(0x40D00000)  /* Interrupt Controller IRQ Pending Register */
> #define ICMR __REG(0x40D00004)  /* Interrupt Controller Mask Register */
> ...
> #define xip_currtime()		((unsigned long) OSCR)
> #define xip_elapsed_since(x)	(signed)((((unsigned long) OSCR) - (x)) / 4)


Macros that do implicit pointer dereferences are discouraged, a better way to
write them is

	#define xip_currtime()	readl_relaxed(OSCR)

with the OSCR definition from arch/arm/mach-pxa/include/mach/regs-ost.h.

> P.S. Same macros are used on omap1 and sa1100 too.

There is a (slow) effort to move both omap1 and pxa into ARCH_MULTIPLATFORM
in the long run, and at that point we have to come up with something
better. The easiest way out would be to replace the functions with
function pointers that can be set by platform specific code.

A cleaner approach might be to replace xip_currtime() with
ktime_get_ns(), and find some other generic interface to replace
xip_irqpending. That would let us eliminate the mach/mtd-xip.h
header entirely and make mtd-xip portablel to a lot of other
platforms.

For a real multiplatform kernel, you'd also need to replace the
#ifdef CONFIG_MTD_XIP instances with a runtime conditional, but that
is less urgent for real world use cases.

	Arnd

[BUG, RFC] MTD Execute in Place on ARM breaks build

[Arnd Bergmann]

On Friday 07 August 2015 22:20:41 Petr Cvek wrote:
> Hello,
> 
> Configuration:
> 
> 	CONFIG_MTD_XIP=y
> 
> on PXA2xx architecture causes request of undefined macros from 
> 
> 	drivers/mtd/chipscfi_cmdset_0001.c
> 
> These macros (using ICIP and ICMR) are defined here:
> 
> 	http://lxr.free-electrons.com/source/arch/arm/mach-pxa/include/mach/mtd-xip.h#L20
> 
> register definitions for ICIP and ICMR were removed by commit:
> 	
> 	5d284e353eb11ab2e8b1c5671ba06489b0bd1e0c
> 
> Similar is for macros with OSCR (lines 23 and 24), which have different type.
> Re-adding ICIP and ICMR definition and explicit type conversion will fix the build
> and the kernel boots, but I'm not yet able to test XIP functionality (too many different
> bugs to flash over windows mobile image).
>
> My temporal fix (ugly):
> 
> #define ICIP __REG(0x40D00000)  /* Interrupt Controller IRQ Pending Register */
> #define ICMR __REG(0x40D00004)  /* Interrupt Controller Mask Register */
> ...
> #define xip_currtime()		((unsigned long) OSCR)
> #define xip_elapsed_since(x)	(signed)((((unsigned long) OSCR) - (x)) / 4)


Macros that do implicit pointer dereferences are discouraged, a better way to
write them is

	#define xip_currtime()	readl_relaxed(OSCR)

with the OSCR definition from arch/arm/mach-pxa/include/mach/regs-ost.h.

> P.S. Same macros are used on omap1 and sa1100 too.

There is a (slow) effort to move both omap1 and pxa into ARCH_MULTIPLATFORM
in the long run, and at that point we have to come up with something
better. The easiest way out would be to replace the functions with
function pointers that can be set by platform specific code.

A cleaner approach might be to replace xip_currtime() with
ktime_get_ns(), and find some other generic interface to replace
xip_irqpending. That would let us eliminate the mach/mtd-xip.h
header entirely and make mtd-xip portablel to a lot of other
platforms.

For a real multiplatform kernel, you'd also need to replace the
#ifdef CONFIG_MTD_XIP instances with a runtime conditional, but that
is less urgent for real world use cases.

	Arnd

Re: [BUG, RFC] MTD Execute in Place on ARM breaks build

[Russell King - ARM Linux]

On Sat, Aug 08, 2015 at 01:42:50PM +0200, Arnd Bergmann wrote:
> A cleaner approach might be to replace xip_currtime() with
> ktime_get_ns(), and find some other generic interface to replace
> xip_irqpending.

Definitely not.

The issue here is that the kernel sits in flash.  When we want to write
to flash, we need to switch the flash into a mode where the data
contained in the flash becomes unreadable - reading from it results in
status bytes being returned.  Status bytes are not executable.

To get around that problem, we have a small amount of code in RAM which
does the flash erasing and/or programming.  This code, however, needs
to have access to the interrupt controller and timer.

Calling generic functions that would be part of the paged-out kernel
image is just not possible; doing so will immediately crash the kernel
and break what's being achieved here.  You'd need to mark these functions
and any functions that they then call (including spinlocks, probably
the entire lockdep infrastructure, etc) with __xipram.  I don't think
that's feasible.

In any case, I don't think XIP multiplatform makes any sense what so
ever.  Needless to say, it _could_ be made to work, but you're likely
need some complexity, and given that it has very few users, I don't
think there's much to be gained from putting that work in.  We've even
talked a few times about removing XIP support altogether.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[BUG, RFC] MTD Execute in Place on ARM breaks build

[Russell King - ARM Linux]

On Sat, Aug 08, 2015 at 01:42:50PM +0200, Arnd Bergmann wrote:
> A cleaner approach might be to replace xip_currtime() with
> ktime_get_ns(), and find some other generic interface to replace
> xip_irqpending.

Definitely not.

The issue here is that the kernel sits in flash.  When we want to write
to flash, we need to switch the flash into a mode where the data
contained in the flash becomes unreadable - reading from it results in
status bytes being returned.  Status bytes are not executable.

To get around that problem, we have a small amount of code in RAM which
does the flash erasing and/or programming.  This code, however, needs
to have access to the interrupt controller and timer.

Calling generic functions that would be part of the paged-out kernel
image is just not possible; doing so will immediately crash the kernel
and break what's being achieved here.  You'd need to mark these functions
and any functions that they then call (including spinlocks, probably
the entire lockdep infrastructure, etc) with __xipram.  I don't think
that's feasible.

In any case, I don't think XIP multiplatform makes any sense what so
ever.  Needless to say, it _could_ be made to work, but you're likely
need some complexity, and given that it has very few users, I don't
think there's much to be gained from putting that work in.  We've even
talked a few times about removing XIP support altogether.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [BUG, RFC] MTD Execute in Place on ARM breaks build

[Petr Cvek]

On 8.8.2015 15:47, Russell King - ARM Linux wrote:
> On Sat, Aug 08, 2015 at 01:42:50PM +0200, Arnd Bergmann wrote:
>> A cleaner approach might be to replace xip_currtime() with
>> ktime_get_ns(), and find some other generic interface to replace
>> xip_irqpending.
> 
> Definitely not.
> 
> The issue here is that the kernel sits in flash.  When we want to write
> to flash, we need to switch the flash into a mode where the data
> contained in the flash becomes unreadable - reading from it results in
> status bytes being returned.  Status bytes are not executable.
> 
> To get around that problem, we have a small amount of code in RAM which
> does the flash erasing and/or programming.  This code, however, needs
> to have access to the interrupt controller and timer.
> 
> Calling generic functions that would be part of the paged-out kernel
> image is just not possible; doing so will immediately crash the kernel
> and break what's being achieved here.  You'd need to mark these functions
> and any functions that they then call (including spinlocks, probably
> the entire lockdep infrastructure, etc) with __xipram.  I don't think
> that's feasible.
> 
> In any case, I don't think XIP multiplatform makes any sense what so
> ever.  Needless to say, it _could_ be made to work, but you're likely
> need some complexity, and given that it has very few users, I don't
> think there's much to be gained from putting that work in.  We've even
> talked a few times about removing XIP support altogether.
> 

How often are these chips erased/written? If it is only for system image update,
it could be done with special code (something like BIOS flashing). Reading chip
in XIP mode is more useful IMO.

Anyway, is my patch proposal for build fixing OK?

Cheers,
Petr

[BUG, RFC] MTD Execute in Place on ARM breaks build

[Petr Cvek]

On 8.8.2015 15:47, Russell King - ARM Linux wrote:
> On Sat, Aug 08, 2015 at 01:42:50PM +0200, Arnd Bergmann wrote:
>> A cleaner approach might be to replace xip_currtime() with
>> ktime_get_ns(), and find some other generic interface to replace
>> xip_irqpending.
> 
> Definitely not.
> 
> The issue here is that the kernel sits in flash.  When we want to write
> to flash, we need to switch the flash into a mode where the data
> contained in the flash becomes unreadable - reading from it results in
> status bytes being returned.  Status bytes are not executable.
> 
> To get around that problem, we have a small amount of code in RAM which
> does the flash erasing and/or programming.  This code, however, needs
> to have access to the interrupt controller and timer.
> 
> Calling generic functions that would be part of the paged-out kernel
> image is just not possible; doing so will immediately crash the kernel
> and break what's being achieved here.  You'd need to mark these functions
> and any functions that they then call (including spinlocks, probably
> the entire lockdep infrastructure, etc) with __xipram.  I don't think
> that's feasible.
> 
> In any case, I don't think XIP multiplatform makes any sense what so
> ever.  Needless to say, it _could_ be made to work, but you're likely
> need some complexity, and given that it has very few users, I don't
> think there's much to be gained from putting that work in.  We've even
> talked a few times about removing XIP support altogether.
> 

How often are these chips erased/written? If it is only for system image update,
it could be done with special code (something like BIOS flashing). Reading chip
in XIP mode is more useful IMO.

Anyway, is my patch proposal for build fixing OK?

Cheers,
Petr

Re: [BUG, RFC] MTD Execute in Place on ARM breaks build

[Russell King - ARM Linux]

On Sat, Aug 08, 2015 at 08:52:22PM +0200, Petr Cvek wrote:
> How often are these chips erased/written? If it is only for system
> image update, it could be done with special code (something like
> BIOS flashing).

I have no idea what the use case is for this.  Nicolas did all the
development work while working for Montavista...  It's entirely
possible that people use jffs2 or some other writable filesystem.

> Anyway, is my patch proposal for build fixing OK?

I don't think so - it effectively hides the broken nature of it,
which really isn't what kernel maintanence is about.  If something
is wrong, either it needs to be fixed properly, or it needs to stay
as a reportable bug so it reminds people that it needs to be fixed.
Papering over a bug with something that doesn't work isn't a way
forward.

Arnd suggested getting around some of the issue with:

	#define xip_currtime()  readl_relaxed(OSCR)

which would be a step in the right direction - that should issue
correct code on PXA to read the OSCR register from the __xipram
marked code.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[BUG, RFC] MTD Execute in Place on ARM breaks build

[Russell King - ARM Linux]

On Sat, Aug 08, 2015 at 08:52:22PM +0200, Petr Cvek wrote:
> How often are these chips erased/written? If it is only for system
> image update, it could be done with special code (something like
> BIOS flashing).

I have no idea what the use case is for this.  Nicolas did all the
development work while working for Montavista...  It's entirely
possible that people use jffs2 or some other writable filesystem.

> Anyway, is my patch proposal for build fixing OK?

I don't think so - it effectively hides the broken nature of it,
which really isn't what kernel maintanence is about.  If something
is wrong, either it needs to be fixed properly, or it needs to stay
as a reportable bug so it reminds people that it needs to be fixed.
Papering over a bug with something that doesn't work isn't a way
forward.

Arnd suggested getting around some of the issue with:

	#define xip_currtime()  readl_relaxed(OSCR)

which would be a step in the right direction - that should issue
correct code on PXA to read the OSCR register from the __xipram
marked code.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.