diff mbox

[2/5] arm64: Fix the soft_restart routine

Message ID fbf47d394de6bd7332b00bc706a9f38cd7a68327.1386879684.git.geoff@infradead.org (mailing list archive)
State New, archived
Headers show

Commit Message

Geoff Levand Dec. 12, 2013, 8:39 p.m. UTC
Change the soft_restart() routine to call cpu_reset() at its identity mapped
physical address.

The cpu_reset() routine must be called at its identity mapped physical address
so that when the MMU is turned off the instruction pointer will be at the correct
location in physical memory.

Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
---
 arch/arm64/kernel/process.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Will Deacon Dec. 13, 2013, 4:46 p.m. UTC | #1
On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> Change the soft_restart() routine to call cpu_reset() at its identity mapped
> physical address.
> 
> The cpu_reset() routine must be called at its identity mapped physical address
> so that when the MMU is turned off the instruction pointer will be at the correct
> location in physical memory.
> 
> Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
> ---
>  arch/arm64/kernel/process.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index de17c89..985eb42 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -72,7 +72,13 @@ static void setup_restart(void)
>  void soft_restart(unsigned long addr)
>  {
>  	setup_restart();
> -	cpu_reset(addr);
> +
> +	/*
> +	 * cpu_reset turns the MMU off, so must be called at its identity
> +	 * mapped physical address.
> +	 */
> +
> +	(*(void(*)(unsigned long))virt_to_phys(cpu_reset))(addr);

This isn't right; although cpu_reset *does* need to run from the idmap,
actually the idmap only includes __turn_mmu_on. You just get lucky because
its section-mapped and happens to include the code you want.

The cast is also cleaner if you define a phys_reset_t type, like we do for
arch/arm/.

Will
Geoff Levand Dec. 17, 2013, 12:20 a.m. UTC | #2
Hi Will,

On Fri, 2013-12-13 at 16:46 +0000, Will Deacon wrote:
> On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> > +	/*
> > +	 * cpu_reset turns the MMU off, so must be called at its identity
> > +	 * mapped physical address.
> > +	 */
> > +
> > +	(*(void(*)(unsigned long))virt_to_phys(cpu_reset))(addr);
> 
> This isn't right; although cpu_reset *does* need to run from the idmap,
> actually the idmap only includes __turn_mmu_on. You just get lucky because
> its section-mapped and happens to include the code you want.

OK, I think this is different from fixing the call in soft_restart(), so
I'll make a separate patch for this.

I can see a few ways to do it but am not sure of the ramifications
of each.  Should we put cpu_reset into the idmap at startup when we add
__turn_mmu_on, or add cpu_reset on demand from say setup_mm_for_reboot()?

If the former, is all we need something like this?

@@ -438,6 +438,9 @@ __create_page_tables:
 	adr	x3, __turn_mmu_on		// virtual/physical address
 	create_pgd_entry x25, x0, x3, x5, x6
 	create_block_map x0, x7, x3, x5, x5, idmap=1
+	adr	x3, cpu_reset			// virtual/physical address
+	create_pgd_entry x25, x0, x3, x5, x6
+	create_block_map x0, x7, x3, x5, x5, idmap=1

> The cast is also cleaner if you define a phys_reset_t type, like we do for
> arch/arm/.

I'll send out an updated fix.

-Geoff
diff mbox

Patch

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index de17c89..985eb42 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -72,7 +72,13 @@  static void setup_restart(void)
 void soft_restart(unsigned long addr)
 {
 	setup_restart();
-	cpu_reset(addr);
+
+	/*
+	 * cpu_reset turns the MMU off, so must be called at its identity
+	 * mapped physical address.
+	 */
+
+	(*(void(*)(unsigned long))virt_to_phys(cpu_reset))(addr);
 }
 
 /*