[PATCH 0/6] MMU disabling code and kexec fixes

Hello,

There's been some discussion on the lists recently concerning MMU-off
code and how best to do it safely. Since I believe this code should
exist in one place, where it can be accessed by the relevant subsystems
that need it, I had a crack at a patch series.

After applying this lot, I can successfully kexec a kernel on the
Realview-PBX board (although using SMP causes some issues in the new
kernel because the secondaries need throwing back to the bootloader).

Anyway, I'm posting this in the hope that it can be used by others who
are trying to disable the MMU for one reason or another.

All feedback welcome!

Will


Will Deacon (6):
  ARM: l2x0: fix disabling function to avoid livelock
  ARM: l2x0: fix invalidate-all function to avoid livelock
  ARM: proc-v7: add definition of cpu_reset for ARMv7 cores
  ARM: reset: add reset functionality for jumping to a physical address
  ARM: kexec: use arm_machine_reset for branching to the reboot buffer
  ARM: stop: execute platform callback from cpu_stop code

 arch/arm/Kconfig                |    2 +-
 arch/arm/include/asm/system.h   |    1 +
 arch/arm/kernel/machine_kexec.c |   14 +-----------
 arch/arm/kernel/process.c       |   42 ++++++++++++++++++++++++++++++++++----
 arch/arm/kernel/smp.c           |    4 +++
 arch/arm/mm/cache-l2x0.c        |   13 ++++++-----
 arch/arm/mm/proc-v7.S           |    7 ++++++
 7 files changed, 59 insertions(+), 24 deletions(-)

The l2x0_disable function attempts to writel with the l2x0_lock held.
This results in livelock when the writel contains an outer_sync call
for the platform.

This patch replaces the writel with a call to writel_relaxed in the
disabling code, preventing livelock from occurring.

Signed-off-by: Will Deacon <redacted>
---
 arch/arm/mm/cache-l2x0.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index ef59099..2bce3be 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c

@@ -266,7 +266,7 @@ static void l2x0_disable(void)
 	unsigned long flags;
 
 	spin_lock_irqsave(&l2x0_lock, flags);
-	writel(0, l2x0_base + L2X0_CTRL);
+	writel_relaxed(0, l2x0_base + L2X0_CTRL);
 	spin_unlock_irqrestore(&l2x0_lock, flags);
 }
 

-- 
1.7.0.4

With the L2 cache disabled, exclusive memory access instructions may
cease to function correctly, leading to livelock when trying to acquire
a spinlock.

The l2x0 invalidate-all routine *must* run with the cache disabled and so
needs to take extra care not to take any locks along the way.

This patch modifies the invalidation routine to avoid locking. Since
the cache is disabled, we make the assumption that other CPUs are not
executing background maintenance tasks on the L2 cache whilst we are
invalidating it.

Signed-off-by: Will Deacon <redacted>
---
 arch/arm/mm/cache-l2x0.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 2bce3be..fe5630f 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c

@@ -148,16 +148,17 @@ static void l2x0_clean_all(void)
 
 static void l2x0_inv_all(void)
 {
-	unsigned long flags;
-
-	/* invalidate all ways */
-	spin_lock_irqsave(&l2x0_lock, flags);
 	/* Invalidating when L2 is enabled is a nono */
 	BUG_ON(readl(l2x0_base + L2X0_CTRL) & 1);
+
+	/*
+	 * invalidate all ways
+	 * Since the L2 is disabled, exclusive accessors may not be
+	 * available to us, so avoid taking any locks.
+	 */
 	writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_INV_WAY);
 	cache_wait_way(l2x0_base + L2X0_INV_WAY, l2x0_way_mask);
 	cache_sync();
-	spin_unlock_irqrestore(&l2x0_lock, flags);
 }
 
 static void l2x0_inv_range(unsigned long start, unsigned long end)

-- 
1.7.0.4

This patch adds a simple definition of cpu_reset for ARMv7 cores, which
disables the MMU via the SCTLR.

Signed-off-by: Will Deacon <redacted>
---
 arch/arm/mm/proc-v7.S |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index b3b566e..776443a 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S

@@ -58,9 +58,16 @@ ENDPROC(cpu_v7_proc_fin)
  *	to what would be the reset vector.
  *
  *	- loc   - location to jump to for soft reset
+ *
+ *	This code must be executed using a flat identity mapping with
+ *      caches disabled.
  */
 	.align	5
 ENTRY(cpu_v7_reset)
+	mrc	p15, 0, r1, c1, c0, 0		@ ctrl register
+	bic	r1, r1, #0x1			@ ...............m
+	mcr	p15, 0, r1, c1, c0, 0		@ disable MMU
+	isb
 	mov	pc, r0
 ENDPROC(cpu_v7_reset)
 

-- 
1.7.0.4

Tools such as kexec and CPU hotplug require a way to reset the processor
and branch to some code in physical space. This requires various bits of
jiggery pokery with the caches and MMU which, when it goes wrong, tends
to lock up the system.

This patch implements a new function, arm_machine_reset, for
consolidating this code in one place where it can be used by multiple
subsystems.

Signed-off-by: Will Deacon <redacted>
---
 arch/arm/include/asm/system.h |    1 +
 arch/arm/kernel/process.c     |   42 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 832888d..cd2a3cd 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h

@@ -108,6 +108,7 @@ extern int cpu_architecture(void);
 extern void cpu_init(void);
 
 void arm_machine_restart(char mode, const char *cmd);
+void arm_machine_reset(unsigned long reset_code_phys);
 extern void (*arm_pm_restart)(char str, const char *cmd);
 
 #define UDBG_UNDEFINED	(1 << 0)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 5e1e541..0b46e9f 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c

@@ -91,12 +91,8 @@ static int __init hlt_setup(char *__unused)
 __setup("nohlt", nohlt_setup);
 __setup("hlt", hlt_setup);
 
-void arm_machine_restart(char mode, const char *cmd)
+static void prepare_for_reboot(char mode)
 {
-	/* Disable interrupts first */
-	local_irq_disable();
-	local_fiq_disable();
-
 	/*
 	 * Tell the mm system that we are going to reboot -
 	 * we may need it to insert some 1:1 mappings so that

@@ -112,6 +108,15 @@ void arm_machine_restart(char mode, const char *cmd)
 
 	/* Push out any further dirty data, and ensure cache is empty */
 	flush_cache_all();
+}
+
+void arm_machine_restart(char mode, const char *cmd)
+{
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
+
+	prepare_for_reboot(mode);
 
 	/*
 	 * Now call the architecture specific reboot code.

@@ -127,6 +132,33 @@ void arm_machine_restart(char mode, const char *cmd)
 	while (1);
 }
 
+void arm_machine_reset(unsigned long reset_code_phys)
+{
+	unsigned long cpu_reset_end = PAGE_ALIGN((unsigned long)cpu_reset);
+	/* This is stricter than necessary but better to be safe than sorry. */
+	BUG_ON(virt_to_phys((void *)cpu_reset_end) >= TASK_SIZE);
+
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
+
+	/*
+	 * Clean and invalidate L2.
+	 * This is racy, so we must be the last guy left.
+	 */
+	WARN_ON(num_online_cpus() > 1);
+	/* Flush while we still have locking available to us. */
+	outer_flush_all();
+	outer_disable();
+	/* Data destroyed here will only be speculative. */
+	outer_inv_all();
+
+	prepare_for_reboot(0);
+
+	/* Switch to the identity mapping. */
+	((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys);
+}
+
 /*
  * Function pointers to optional machine specific functions
  */

-- 
1.7.0.4

Now that there is a common way to reset the machine, let's use it
instead of reinventing the wheel in the kexec backend.

Signed-off-by: Will Deacon <redacted>
---
 arch/arm/kernel/machine_kexec.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index e59bbd4..f84567c 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c

@@ -16,8 +16,6 @@
 extern const unsigned char relocate_new_kernel[];
 extern const unsigned int relocate_new_kernel_size;
 
-extern void setup_mm_for_reboot(char mode);
-
 extern unsigned long kexec_start_address;
 extern unsigned long kexec_indirection_page;
 extern unsigned long kexec_mach_type;

@@ -111,14 +109,6 @@ void machine_kexec(struct kimage *image)
 
 	if (kexec_reinit)
 		kexec_reinit();
-	local_irq_disable();
-	local_fiq_disable();
-	setup_mm_for_reboot(0); /* mode is not used, so just pass 0*/
-	flush_cache_all();
-	outer_flush_all();
-	outer_disable();
-	cpu_proc_fin();
-	outer_inv_all();
-	flush_cache_all();
-	cpu_reset(reboot_code_buffer_phys);
+
+	arm_machine_reset(reboot_code_buffer_phys);
 }

-- 
1.7.0.4

Sending IPI_CPU_STOP to a CPU causes it to execute a busy cpu_relax
loop forever. This makes it impossible to kexec successfully on an SMP
system since the secondary CPUs do not reset.

This patch adds a callback to platform_cpu_kill, defined when
CONFIG_HOTPLUG_CPU=y, from the ipi_cpu_stop handling code. This function
currently just returns 1 on all platforms that define it but allows them
to do something more sophisticated in the future.

Signed-off-by: Will Deacon <redacted>
---
 arch/arm/Kconfig      |    2 +-
 arch/arm/kernel/smp.c |    4 ++++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9adc278..c26d8f6 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig

@@ -1795,7 +1795,7 @@ config XIP_PHYS_ADDR
 
 config KEXEC
 	bool "Kexec system call (EXPERIMENTAL)"
-	depends on EXPERIMENTAL
+	depends on EXPERIMENTAL && (!SMP || HOTPLUG_CPU)
 	help
 	  kexec is a system call that implements the ability to shutdown your
 	  current kernel, and to start another kernel.  It is like a reboot

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 344e52b..2c48a5f 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c

@@ -547,6 +547,10 @@ static void ipi_cpu_stop(unsigned int cpu)
 	local_fiq_disable();
 	local_irq_disable();
 
+#ifdef CONFIG_HOTPLUG_CPU
+	platform_cpu_kill(cpu);
+#endif
+
 	while (1)
 		cpu_relax();
 }

-- 
1.7.0.4

On Mon, 6 Jun 2011, Will Deacon wrote:

Tools such as kexec and CPU hotplug require a way to reset the processor
and branch to some code in physical space. This requires various bits of
jiggery pokery with the caches and MMU which, when it goes wrong, tends
to lock up the system.

This patch implements a new function, arm_machine_reset, for
consolidating this code in one place where it can be used by multiple
subsystems.

[ ... ]

+void arm_machine_reset(unsigned long reset_code_phys)
+{
+	unsigned long cpu_reset_end = PAGE_ALIGN((unsigned long)cpu_reset);
+	/* This is stricter than necessary but better to be safe than sorry. */
+	BUG_ON(virt_to_phys((void *)cpu_reset_end) >= TASK_SIZE);
+
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
+
+	/*
+	 * Clean and invalidate L2.
+	 * This is racy, so we must be the last guy left.
+	 */
+	WARN_ON(num_online_cpus() > 1);
+	/* Flush while we still have locking available to us. */
+	outer_flush_all();
+	outer_disable();
+	/* Data destroyed here will only be speculative. */
+	outer_inv_all();
+
+	prepare_for_reboot(0);
+
+	/* Switch to the identity mapping. */
+	((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys);

void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset)

+}
+
/*
 * Function pointers to optional machine specific functions
 */
-- 
1.7.0.4

On Tue, 7 Jun 2011, Frank Hofmann wrote:

On Mon, 6 Jun 2011, Will Deacon wrote:

quoted

Tools such as kexec and CPU hotplug require a way to reset the processor
and branch to some code in physical space. This requires various bits of
jiggery pokery with the caches and MMU which, when it goes wrong, tends
to lock up the system.

This patch implements a new function, arm_machine_reset, for
consolidating this code in one place where it can be used by multiple
subsystems.

[ ... ]

quoted

+void arm_machine_reset(unsigned long reset_code_phys)
+{
+	unsigned long cpu_reset_end = PAGE_ALIGN((unsigned long)cpu_reset);
+	/* This is stricter than necessary but better to be safe than sorry. 
*/
+	BUG_ON(virt_to_phys((void *)cpu_reset_end) >= TASK_SIZE);
+
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
+
+	/*
+	 * Clean and invalidate L2.
+	 * This is racy, so we must be the last guy left.
+	 */
+	WARN_ON(num_online_cpus() > 1);
+	/* Flush while we still have locking available to us. */
+	outer_flush_all();
+	outer_disable();
+	/* Data destroyed here will only be speculative. */
+	outer_inv_all();
+
+	prepare_for_reboot(0);
+
+	/* Switch to the identity mapping. */
+	((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys);

void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset)

Sorry, pressed wrong key ... posted too early.

I meant to say this line is magic, why not decouple the declaration bit 
from the invocation so that at least the function call looks "normal" ?


Regarding the use of local_irq/fiq_disable - is it safe to call these 
multiple times, i.e. is it safe to invoke the reset function from a 
context where IRQ/FIQ are already off ?


Another question regarding the MMU tables.

prepare_for_reboot / setup_mm_for_reboot assume that current->mm is 
available _and_ lowmem / user area there can be blotted over.

Wrt. to hibernation, that's a stretch unless some way is found to fully 
"fake a context". With that I mean to create a threadinfo and pagedir 
that's guaranteed to sit outside the target kernel heap.

Currently, the hibernation code switches to swapper_pg_dir and puts the 
1:1 mappings in there; I'm starting to think that is safe if for no other 
reason than swapper_pg_dir having _nothing_ in the user area ?

Is it convention, design or accident that swapper_pg_dir doesn't map 
anything in the low range ?

FrankH.

quoted

+}
+
/*
 * Function pointers to optional machine specific functions
 */
-- 
1.7.0.4

Hi Frank,

Thanks for looking at this.

On Tue, Jun 07, 2011 at 12:37:23PM +0100, Frank Hofmann wrote:

quoted

quoted

+void arm_machine_reset(unsigned long reset_code_phys)
+{
+	unsigned long cpu_reset_end = PAGE_ALIGN((unsigned long)cpu_reset);
+	/* This is stricter than necessary but better to be safe than sorry. 
*/
+	BUG_ON(virt_to_phys((void *)cpu_reset_end) >= TASK_SIZE);
+
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
+
+	/*
+	 * Clean and invalidate L2.
+	 * This is racy, so we must be the last guy left.
+	 */
+	WARN_ON(num_online_cpus() > 1);
+	/* Flush while we still have locking available to us. */
+	outer_flush_all();
+	outer_disable();
+	/* Data destroyed here will only be speculative. */
+	outer_inv_all();
+
+	prepare_for_reboot(0);
+
+	/* Switch to the identity mapping. */
+	((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys);

void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset)

Sorry, pressed wrong key ... posted too early.

No problem.

I meant to say this line is magic, why not decouple the declaration bit 
from the invocation so that at least the function call looks "normal" ?

You mean you don't like the LISP-ish look of it?! I take your point, I'll rework
that.

Regarding the use of local_irq/fiq_disable - is it safe to call these 
multiple times, i.e. is it safe to invoke the reset function from a 
context where IRQ/FIQ are already off ?

Yes, they just mask at the CPSR level.

Another question regarding the MMU tables.

prepare_for_reboot / setup_mm_for_reboot assume that current->mm is 
available _and_ lowmem / user area there can be blotted over.

Wrt. to hibernation, that's a stretch unless some way is found to fully 
"fake a context". With that I mean to create a threadinfo and pagedir 
that's guaranteed to sit outside the target kernel heap.

Ouch, ok.

Currently, the hibernation code switches to swapper_pg_dir and puts the 
1:1 mappings in there; I'm starting to think that is safe if for no other 
reason than swapper_pg_dir having _nothing_ in the user area ?

That sounds ok providing that, when you come out of hibernate, the 1:1 mapping
doesn't persist in swapper_pg_dir.

However, stepping back a bit here, there is a bigger problem to tackle. If the
physical address of cpu_reset is bigger than TASK_SIZE, then we fail to map it
using the current identity mapping code [that piggy backs on the current task].
If PHYS_OFFSET > TASK_SIZE, this will *always* be the case and is probably quite
common for a 2:2 split.

I can think of two ways to get around this:

(1) Use /another/ set of page tables for mapping the cpu_reset code only [and
    nothing else]. This is tricky since it will need to be set extremely late-on
    where we don't care about mapping in the rest of the kernel, data, stack etc.

(2) Make the assumption that the physical address of cpu_reset will not conflict
    with a virtual address that points at the kernel text via the linear mapping.
    This is probably a reasonable thing to assume, given that the kernel lives
    near the start of memory on most platforms. We could then take out the ID map
    but leaving the kernel text alone. We'd probably have to change to a new stack,
    which we could place immediately after the kernel text.

What do you reckon?

Is it convention, design or accident that swapper_pg_dir doesn't map 
anything in the low range ?

With the new switch_mm stuff, it's absolutely critical that we don't have
user mappings in there during normal execution (going down for hibernate
is probably ok since we'll clobber the caches and TLBs anyway).

Will

On Tue, 7 Jun 2011, Will Deacon wrote:

Hi Frank,

Thanks for looking at this.

Thanks for posting it ;-)

[ ... ]

quoted

quoted

quoted

+	/* Switch to the identity mapping. */
+	((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys);

void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset)

Sorry, pressed wrong key ... posted too early.

No problem.

quoted

I meant to say this line is magic, why not decouple the declaration bit
from the invocation so that at least the function call looks "normal" ?

You mean you don't like the LISP-ish look of it?! I take your point, I'll rework
that.

 	typeof(cpu_reset)(*phys_reset) =
 		(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

 	reset_func(reset_code_phys);

Works for me. Was experimenting with this when I pressed the wrong key.

quoted

Regarding the use of local_irq/fiq_disable - is it safe to call these
multiple times, i.e. is it safe to invoke the reset function from a
context where IRQ/FIQ are already off ?

Yes, they just mask at the CPSR level.

Good, thx for the confirmation.

quoted

Another question regarding the MMU tables.

prepare_for_reboot / setup_mm_for_reboot assume that current->mm is
available _and_ lowmem / user area there can be blotted over.

Wrt. to hibernation, that's a stretch unless some way is found to fully
"fake a context". With that I mean to create a threadinfo and pagedir
that's guaranteed to sit outside the target kernel heap.

Ouch, ok.

quoted

Currently, the hibernation code switches to swapper_pg_dir and puts the
1:1 mappings in there; I'm starting to think that is safe if for no other
reason than swapper_pg_dir having _nothing_ in the user area ?

That sounds ok providing that, when you come out of hibernate, the 1:1 mapping
doesn't persist in swapper_pg_dir.

I can delete those mappings - as they're created by identity_mapping_add() 
they can be ditched with identity_mapping_del() - which might be 
sufficient, provided, and there's the critical bit why I ask, that 
swapper_pg_dir _normally_ has nothing in the range anyway.

Thing is, just because something appears to work doesn't mean it has no 
unexpected/undesired side effects ... and I'm no ARM VM guru.

However, stepping back a bit here, there is a bigger problem to tackle. If the
physical address of cpu_reset is bigger than TASK_SIZE, then we fail to map it
using the current identity mapping code [that piggy backs on the current task].
If PHYS_OFFSET > TASK_SIZE, this will *always* be the case and is probably quite
common for a 2:2 split.

I can think of two ways to get around this:

(1) Use /another/ set of page tables for mapping the cpu_reset code only [and
   nothing else]. This is tricky since it will need to be set extremely late-on
   where we don't care about mapping in the rest of the kernel, data, stack etc.

(2) Make the assumption that the physical address of cpu_reset will not conflict
   with a virtual address that points at the kernel text via the linear mapping.
   This is probably a reasonable thing to assume, given that the kernel lives
   near the start of memory on most platforms. We could then take out the ID map
   but leaving the kernel text alone. We'd probably have to change to a new stack,
   which we could place immediately after the kernel text.

What do you reckon?

cpu_reset itself is relocatable, isn't it ? Maybe one could do the same 
thing with/for it as for the reset code itself. I.e. relocate to a 
"suitable" page if the 1:1 mapping for all of lowmem isn't possible. The 
catch with that is of course somehow, such a "1:1 candidate page" must be 
found.

quoted

Is it convention, design or accident that swapper_pg_dir doesn't map
anything in the low range ?

With the new switch_mm stuff, it's absolutely critical that we don't have
user mappings in there during normal execution (going down for hibernate
is probably ok since we'll clobber the caches and TLBs anyway).

I'm using swapper_pg_dir for hibernate (restore) for the simple reason 
that it maps all of kernel text/data writeable.

When invoking resume via uswsusp and/or tuxonice, it's triggered from a 
user context which is insufficient to _rewrite_ "universe".

So when the time comes to do the cpu_reset / cpu_resume dance, the active 
mm context is swapper_pg_dir; hence creating the identity mappings in 
there.

It's not hard to zap them afterwards; it'd be far more difficult to 
restore previous contents if there had been any.

Short: Good news that the user pagedir section in swapper_pg_dir is empty 
by design.


Thanks,
FrankH.

On Tue, Jun 07, 2011 at 02:54:53PM +0100, Frank Hofmann wrote:

On Tue, 7 Jun 2011, Will Deacon wrote:

quoted

Hi Frank,

Thanks for looking at this.

Thanks for posting it ;-)

[ ... ]

quoted

quoted

quoted

quoted

+	/* Switch to the identity mapping. */
+	((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys);

void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset)

Sorry, pressed wrong key ... posted too early.

No problem.

quoted

I meant to say this line is magic, why not decouple the declaration bit
from the invocation so that at least the function call looks "normal" ?

You mean you don't like the LISP-ish look of it?! I take your point, I'll rework
that.

	typeof(cpu_reset)(*phys_reset) =
		(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

This is a declaration, but the extra parentheses confuse me.

How about:

	typeof(cpu_reset) *phys_reset =
		(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

or even:

	typedef typeof(cpu_reset) *phys_reset_t;
	phys_reset_t phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);

	reset_func(reset_code_phys);

Do you mean reset_func(phys_reset) ?

Works for me. Was experimenting with this when I pressed the wrong key.

quoted

quoted

Regarding the use of local_irq/fiq_disable - is it safe to call these
multiple times, i.e. is it safe to invoke the reset function from a
context where IRQ/FIQ are already off ?

Yes, they just mask at the CPSR level.

Good, thx for the confirmation.

quoted

quoted

Another question regarding the MMU tables.

prepare_for_reboot / setup_mm_for_reboot assume that current->mm is
available _and_ lowmem / user area there can be blotted over.

Wrt. to hibernation, that's a stretch unless some way is found to fully
"fake a context". With that I mean to create a threadinfo and pagedir
that's guaranteed to sit outside the target kernel heap.

Ouch, ok.

quoted

Currently, the hibernation code switches to swapper_pg_dir and puts the
1:1 mappings in there; I'm starting to think that is safe if for no other
reason than swapper_pg_dir having _nothing_ in the user area ?

That sounds ok providing that, when you come out of hibernate, the 1:1 mapping
doesn't persist in swapper_pg_dir.

I can delete those mappings - as they're created by
identity_mapping_add() they can be ditched with
identity_mapping_del() - which might be sufficient, provided, and
there's the critical bit why I ask, that swapper_pg_dir _normally_
has nothing in the range anyway.

Thing is, just because something appears to work doesn't mean it has
no unexpected/undesired side effects ... and I'm no ARM VM guru.

quoted

However, stepping back a bit here, there is a bigger problem to tackle. If the
physical address of cpu_reset is bigger than TASK_SIZE, then we fail to map it
using the current identity mapping code [that piggy backs on the current task].
If PHYS_OFFSET > TASK_SIZE, this will *always* be the case and is probably quite
common for a 2:2 split.

I can think of two ways to get around this:

(1) Use /another/ set of page tables for mapping the cpu_reset code only [and
  nothing else]. This is tricky since it will need to be set extremely late-on
  where we don't care about mapping in the rest of the kernel, data, stack etc.

(2) Make the assumption that the physical address of cpu_reset will not conflict
  with a virtual address that points at the kernel text via the linear mapping.
  This is probably a reasonable thing to assume, given that the kernel lives
  near the start of memory on most platforms. We could then take out the ID map
  but leaving the kernel text alone. We'd probably have to change to a new stack,
  which we could place immediately after the kernel text.

What do you reckon?

cpu_reset itself is relocatable, isn't it ? Maybe one could do the
same thing with/for it as for the reset code itself. I.e. relocate
to a "suitable" page if the 1:1 mapping for all of lowmem isn't
possible. The catch with that is of course somehow, such a "1:1
candidate page" must be found.

If cpu_reset is guaranteed to be position-independent and self-contained, we could
just copy it (with fncpy() for example).  We would still need to know the length
of that function somehow though.  Maybe just assuming that it's not longer than
a page would be safe enough.

In this case we would just need to find an identity-mappable target location to
copy the code to; we can find such a location in the same way as that used to
find space for the alternate stack, i.e., somewhere after the end of the kernel
image.

Cheers
---Dave

On Tue, 7 Jun 2011, Dave Martin wrote:

On Tue, Jun 07, 2011 at 02:54:53PM +0100, Frank Hofmann wrote:

quoted

On Tue, 7 Jun 2011, Will Deacon wrote:

quoted

Hi Frank,

Thanks for looking at this.

Thanks for posting it ;-)

[ ... ]

quoted

quoted

quoted

quoted

+	/* Switch to the identity mapping. */
+	((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys);

void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset)

Sorry, pressed wrong key ... posted too early.

No problem.

quoted

I meant to say this line is magic, why not decouple the declaration bit
from the invocation so that at least the function call looks "normal" ?

You mean you don't like the LISP-ish look of it?! I take your point, I'll rework
that.

	typeof(cpu_reset)(*phys_reset) =
		(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

This is a declaration, but the extra parentheses confuse me.

How about:

	typeof(cpu_reset) *phys_reset =
		(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

Function pointers ;-)
Thanks.

or even:

	typedef typeof(cpu_reset) *phys_reset_t;
	phys_reset_t phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);

quoted

	reset_func(reset_code_phys);

Do you mean reset_func(phys_reset) ?

 	phys_reset(reset_code_phys);

me and my copy & paste ...

[ ... ]

quoted

cpu_reset itself is relocatable, isn't it ? Maybe one could do the
same thing with/for it as for the reset code itself. I.e. relocate
to a "suitable" page if the 1:1 mapping for all of lowmem isn't
possible. The catch with that is of course somehow, such a "1:1
candidate page" must be found.

If cpu_reset is guaranteed to be position-independent and self-contained, we could
just copy it (with fncpy() for example).  We would still need to know the length
of that function somehow though.  Maybe just assuming that it's not longer than
a page would be safe enough.

In all current CPU implementations it's position-dependent and 
self-contained; all they do are suitable control reg modifications, ending 
all with "mov pc, r0".

In this case we would just need to find an identity-mappable target location to
copy the code to; we can find such a location in the same way as that used to
find space for the alternate stack, i.e., somewhere after the end of the kernel
image.

If that's a normal thing to do, then this kind of 1:1-bounce-page seems a 
good way of avoiding the address range collision.

best regards,
FrankH.

Cheers
---Dave

On Tue, 7 Jun 2011, Frank Hofmann wrote:

On Tue, 7 Jun 2011, Dave Martin wrote:

[ ... ]

quoted

How about:

	typeof(cpu_reset) *phys_reset =
		(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

Function pointers ;-)
Thanks.

Hmmm ...

Just found a problem with this.

If you have a MULTI_CPU config, this doesn't compile. For two reasons:

1. you cannot use cpu_reset as argument to virt_to_phys because you can't
    take the address
    That bit can be fixed by changing the MULTI_CPU #define in
    <asm/proc-fns.h> not to include the macro argument.
    (There is no code in the arm tree using cpu_reset_whatever names which
    would break from that change ... still, not that nice)

2. even when you do that, you loose the "typeof()" information and the
    above still doesn't compile.

Only a manual type override,

 	void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset;

is accepted then.

FrankH.

quoted

or even:

	typedef typeof(cpu_reset) *phys_reset_t;
	phys_reset_t phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);

quoted

	reset_func(reset_code_phys);

Do you mean reset_func(phys_reset) ?

	phys_reset(reset_code_phys);

me and my copy & paste ...

[ ... ]

quoted

quoted

cpu_reset itself is relocatable, isn't it ? Maybe one could do the
same thing with/for it as for the reset code itself. I.e. relocate
to a "suitable" page if the 1:1 mapping for all of lowmem isn't
possible. The catch with that is of course somehow, such a "1:1
candidate page" must be found.

If cpu_reset is guaranteed to be position-independent and self-contained, 
we could
just copy it (with fncpy() for example).  We would still need to know the 
length
of that function somehow though.  Maybe just assuming that it's not longer 
than
a page would be safe enough.

In all current CPU implementations it's position-dependent and 
self-contained; all they do are suitable control reg modifications, ending 
all with "mov pc, r0".

quoted

In this case we would just need to find an identity-mappable target 
location to
copy the code to; we can find such a location in the same way as that used 
to
find space for the alternate stack, i.e., somewhere after the end of the 
kernel
image.

If that's a normal thing to do, then this kind of 1:1-bounce-page seems a 
good way of avoiding the address range collision.

best regards,
FrankH.

quoted

Cheers
---Dave

On Wed, Jun 08, 2011 at 04:55:11PM +0100, Frank Hofmann wrote:

On Tue, 7 Jun 2011, Frank Hofmann wrote:

quoted

On Tue, 7 Jun 2011, Dave Martin wrote:

[ ... ]

quoted

quoted

How about:

	typeof(cpu_reset) *phys_reset =
		(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

Function pointers ;-)
Thanks.

Hmmm ...

Just found a problem with this.

If you have a MULTI_CPU config, this doesn't compile. For two reasons:

1. you cannot use cpu_reset as argument to virt_to_phys because you can't
    take the address
    That bit can be fixed by changing the MULTI_CPU #define in
    <asm/proc-fns.h> not to include the macro argument.
    (There is no code in the arm tree using cpu_reset_whatever names which
    would break from that change ... still, not that nice)

2. even when you do that, you loose the "typeof()" information and the
    above still doesn't compile.

Only a manual type override,

 	void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset;

is accepted then.

Damn, yes, I assumed the MULTI_CPU case would just pointer at the structure field,
but it takes the argument as parameter for the invocation. Oh well, I'll hardcode
the type after all then!

I'll send a v2 once I've finished cleaning up the code as I've tried to make it
more useful following on from your earlier feedback.

Cheers,

Will

On Wed, 8 Jun 2011, Will Deacon wrote:

On Wed, Jun 08, 2011 at 04:55:11PM +0100, Frank Hofmann wrote:

quoted

On Tue, 7 Jun 2011, Frank Hofmann wrote:

quoted

On Tue, 7 Jun 2011, Dave Martin wrote:

[ ... ]

quoted

quoted

How about:

	typeof(cpu_reset) *phys_reset =
		(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

Function pointers ;-)
Thanks.

Hmmm ...

Just found a problem with this.

If you have a MULTI_CPU config, this doesn't compile. For two reasons:

1. you cannot use cpu_reset as argument to virt_to_phys because you can't
    take the address
    That bit can be fixed by changing the MULTI_CPU #define in
    <asm/proc-fns.h> not to include the macro argument.
    (There is no code in the arm tree using cpu_reset_whatever names which
    would break from that change ... still, not that nice)

2. even when you do that, you loose the "typeof()" information and the
    above still doesn't compile.

Only a manual type override,

 	void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset;

is accepted then.

Damn, yes, I assumed the MULTI_CPU case would just pointer at the structure field,
but it takes the argument as parameter for the invocation. Oh well, I'll hardcode
the type after all then!

It's not just that - the worse bit is that as long as the #define looks 
like:

#define	cpu_reset(addr)		processor.reset(addr)

compile is being refused; one has to ditch the argument part of the macro 
to be able to take the address.

I'm unsure how desirable that change is; it's got the unwanted consequence 
that people who decide to use function names like "graphics_cpu_reset" or 
"cpu_reset_specialregisters" would fall flat on their face.

As said, not that anyone does so right now; just not nice to introduce 
such behaviour.

FrankH.

I'll send a v2 once I've finished cleaning up the code as I've tried to make it
more useful following on from your earlier feedback.

Cheers,

Will

On Wed, Jun 08, 2011 at 05:10:05PM +0100, Frank Hofmann wrote:

quoted

quoted

Just found a problem with this.

If you have a MULTI_CPU config, this doesn't compile. For two reasons:

1. you cannot use cpu_reset as argument to virt_to_phys because you can't
    take the address
    That bit can be fixed by changing the MULTI_CPU #define in
    <asm/proc-fns.h> not to include the macro argument.
    (There is no code in the arm tree using cpu_reset_whatever names which
    would break from that change ... still, not that nice)

2. even when you do that, you loose the "typeof()" information and the
    above still doesn't compile.

Only a manual type override,

 	void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset;

is accepted then.

Damn, yes, I assumed the MULTI_CPU case would just pointer at the structure field,
but it takes the argument as parameter for the invocation. Oh well, I'll hardcode
the type after all then!

It's not just that - the worse bit is that as long as the #define looks 
like:

#define	cpu_reset(addr)		processor.reset(addr)

compile is being refused; one has to ditch the argument part of the macro 
to be able to take the address.

I'm unsure how desirable that change is; it's got the unwanted consequence 
that people who decide to use function names like "graphics_cpu_reset" or 
"cpu_reset_specialregisters" would fall flat on their face.

Isn't this already a problem in the !MULTI_CPU case? cpu_reset will be expanded
to something like armv7_cpu_reset by the macro.

As said, not that anyone does so right now; just not nice to introduce 
such behaviour.

But if that behaviour is already there, I don't think it's a big deal.

Will

On Wed, Jun 08, 2011 at 05:10:05PM +0100, Frank Hofmann wrote:

On Wed, 8 Jun 2011, Will Deacon wrote:

quoted

On Wed, Jun 08, 2011 at 04:55:11PM +0100, Frank Hofmann wrote:

quoted

On Tue, 7 Jun 2011, Frank Hofmann wrote:

quoted

On Tue, 7 Jun 2011, Dave Martin wrote:

[ ... ]

quoted

quoted

How about:

typeof(cpu_reset) *phys_reset =
	(typeof(cpu_reset) *)virt_to_phys(cpu_reset);

Function pointers ;-)
Thanks.

Hmmm ...

Just found a problem with this.

If you have a MULTI_CPU config, this doesn't compile. For two reasons:

1. you cannot use cpu_reset as argument to virt_to_phys because you can't
   take the address
   That bit can be fixed by changing the MULTI_CPU #define in
   <asm/proc-fns.h> not to include the macro argument.
   (There is no code in the arm tree using cpu_reset_whatever names which
   would break from that change ... still, not that nice)

2. even when you do that, you loose the "typeof()" information and the
   above still doesn't compile.

Only a manual type override,

	void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset;

is accepted then.

Damn, yes, I assumed the MULTI_CPU case would just pointer at the structure field,
but it takes the argument as parameter for the invocation. Oh well, I'll hardcode
the type after all then!

It's not just that - the worse bit is that as long as the #define
looks like:

#define	cpu_reset(addr)		processor.reset(addr)

compile is being refused; one has to ditch the argument part of the
macro to be able to take the address.

I'm unsure how desirable that change is; it's got the unwanted
consequence that people who decide to use function names like
"graphics_cpu_reset" or "cpu_reset_specialregisters" would fall flat
on their face.

The preprocessor won't expand a macro name unless it appears as a separate
token, surely?

So if I have

#define cpu_reset processor.reset

then 

	cpu_reset(graphics_cpu_reset)

expands to

	processor.reset(graphics_cpu_reset)

Which is the intention (even if it's a stupid example)

Cheers
---Dave

On Wed, 8 Jun 2011, Dave Martin wrote:

On Wed, Jun 08, 2011 at 05:10:05PM +0100, Frank Hofmann wrote:

[ ... ]

quoted

It's not just that - the worse bit is that as long as the #define
looks like:

#define	cpu_reset(addr)		processor.reset(addr)

compile is being refused; one has to ditch the argument part of the
macro to be able to take the address.

I'm unsure how desirable that change is; it's got the unwanted
consequence that people who decide to use function names like
"graphics_cpu_reset" or "cpu_reset_specialregisters" would fall flat
on their face.

The preprocessor won't expand a macro name unless it appears as a separate
token, surely?

So if I have

#define cpu_reset processor.reset

then

	cpu_reset(graphics_cpu_reset)

expands to

	processor.reset(graphics_cpu_reset)

Which is the intention (even if it's a stupid example)

Ah, I was concerned that it'd change

 	graphics_cpu_reset ===> graphics_processor.reset

but you're correct that kind of substitution is not happening.

Anyway as Will mentioned, the !MULTI_CPU case has the #define the 
following way in any case:

#define	____glue(name,fn)	name##fn
#define	__glue(name,fn)		____glue(name,fn)
#define	cpu_reset		__glue(CPU_NAME,_reset)

I.e. in the !MULTI_CPU case cpu_reset is defined without the macro 
argument, while the MULTI_CPU define _has_ the macro argument.

Would it be ok to remove the (args) part from the MULTI_CPU #define then ?

Thanks,
FrankH.

Cheers
---Dave

Hi Frank,

On Thu, Jun 09, 2011 at 11:00:56AM +0100, Frank Hofmann wrote:

Ah, I was concerned that it'd change

 	graphics_cpu_reset ===> graphics_processor.reset

but you're correct that kind of substitution is not happening.

Anyway as Will mentioned, the !MULTI_CPU case has the #define the 
following way in any case:

#define	____glue(name,fn)	name##fn
#define	__glue(name,fn)		____glue(name,fn)
#define	cpu_reset		__glue(CPU_NAME,_reset)

I.e. in the !MULTI_CPU case cpu_reset is defined without the macro 
argument, while the MULTI_CPU define _has_ the macro argument.

Would it be ok to remove the (args) part from the MULTI_CPU #define then ?

I was planning to do that in v2 of the patches.

Cheers,

Will