v6 software reset fails on 1176

v6 software reset fails on 1176

[Jamie Iles]

Hi Will,

I'm trying to use the cpu_v6_reset that you added in "ARM: proc: add 
definition of cpu_reset for ARMv6 and ARMv7 cores", but I've found that 
on my 1176 platform, it never gets to the branch to the reset vector.  

Removing the ISB allows the branch instruction to be in the pipeline by 
the time the MMU is disabled, but I'm not sure if this is the correct 
fix.  Having said that, I don't see how this can work with an ISB in 
there.

Jamie

diff --git a/arch/arm/mm/proc-v6.S b/arch/arm/mm/proc-v6.S
index 219138d..3b6737a 100644
--- a/arch/arm/mm/proc-v6.S
+++ b/arch/arm/mm/proc-v6.S
@@ -59,8 +59,6 @@ ENTRY(cpu_v6_reset)
 	mrc	p15, 0, r1, c1, c0, 0		@ ctrl register
 	bic	r1, r1, #0x1			@ ...............m
 	mcr	p15, 0, r1, c1, c0, 0		@ disable MMU
-	mov	r1, #0
-	mcr	p15, 0, r1, c7, c5, 4		@ ISB
 	mov	pc, r0
 
 /*

v6 software reset fails on 1176

[Will Deacon]

On Tue, Aug 23, 2011 at 05:32:47PM +0100, Jamie Iles wrote:
> Hi Will,

Hi Jamie,

> I'm trying to use the cpu_v6_reset that you added in "ARM: proc: add 
> definition of cpu_reset for ARMv6 and ARMv7 cores", but I've found that 
> on my 1176 platform, it never gets to the branch to the reset vector.  

Ok. How are you calling cpu_v6_reset? If you call it via arch_reset from
arm_machine_restart then there should be an identity mapping in place, so
you need to ensure that the reset code is called via this mapping in your
implementation of arch_reset.

Unfortunately, the current flat mapping only covers userspace, so it relies
on the physical address of the reset code not aliasing with the kernel virtual
addresses.

I have some (experimental) patches to fix this in my kexec branch:

http://www.linux-arm.org/git?p=linux-2.6-wd.git;a=shortlog;h=refs/heads/kexec-mmu-off

> Removing the ISB allows the branch instruction to be in the pipeline by 
> the time the MMU is disabled, but I'm not sure if this is the correct 
> fix.  Having said that, I don't see how this can work with an ISB in 
> there.

With modern CPUs, you can't rely on characteristics of the pipeline to play
tricks like this. Instead, you need to ensure that the reset code is
executed with a 1:1 mapping.

Will

v6 software reset fails on 1176

[Jamie Iles]

On Tue, Aug 23, 2011 at 06:09:55PM +0100, Will Deacon wrote:
> On Tue, Aug 23, 2011 at 05:32:47PM +0100, Jamie Iles wrote:
> > Hi Will,
> 
> Hi Jamie,
> 
> > I'm trying to use the cpu_v6_reset that you added in "ARM: proc: add 
> > definition of cpu_reset for ARMv6 and ARMv7 cores", but I've found that 
> > on my 1176 platform, it never gets to the branch to the reset vector.  
> 
> Ok. How are you calling cpu_v6_reset? If you call it via arch_reset from
> arm_machine_restart then there should be an identity mapping in place, so
> you need to ensure that the reset code is called via this mapping in your
> implementation of arch_reset.

Yes, this is being called from the arch_reset hook.

> Unfortunately, the current flat mapping only covers userspace, so it relies
> on the physical address of the reset code not aliasing with the kernel virtual
> addresses.

The reset code is in our bootrom (at physical address 0xffff0000).

> I have some (experimental) patches to fix this in my kexec branch:
> 
> http://www.linux-arm.org/git?p=linux-2.6-wd.git;a=shortlog;h=refs/heads/kexec-mmu-off
> 
> > Removing the ISB allows the branch instruction to be in the pipeline by 
> > the time the MMU is disabled, but I'm not sure if this is the correct 
> > fix.  Having said that, I don't see how this can work with an ISB in 
> > there.
> 
> With modern CPUs, you can't rely on characteristics of the pipeline to play
> tricks like this. Instead, you need to ensure that the reset code is
> executed with a 1:1 mapping.

Hmm, I don't really understand this - the cpu_v6_reset code turns off 
the MMU, then issues an ISB.  So for as long as cpu_v6_reset is 
executing in the kernel virtual address space I don't see how it can 
ever fetch the "mov pc, r0" instruction after the ISB without those 
instructions living in the 1:1 mapping?

Jamie

v6 software reset fails on 1176

[Will Deacon]

On Tue, Aug 23, 2011 at 06:34:10PM +0100, Jamie Iles wrote:
> On Tue, Aug 23, 2011 at 06:09:55PM +0100, Will Deacon wrote:
> > Ok. How are you calling cpu_v6_reset? If you call it via arch_reset from
> > arm_machine_restart then there should be an identity mapping in place, so
> > you need to ensure that the reset code is called via this mapping in your
> > implementation of arch_reset.
> 
> Yes, this is being called from the arch_reset hook.

Good, good.

> > Unfortunately, the current flat mapping only covers userspace, so it relies
> > on the physical address of the reset code not aliasing with the kernel virtual
> > addresses.
> 
> The reset code is in our bootrom (at physical address 0xffff0000).

Sorry, I was referring to cpu_reset rather than the code that it then jumps
to (which can be anywhere).

> > With modern CPUs, you can't rely on characteristics of the pipeline to play
> > tricks like this. Instead, you need to ensure that the reset code is
> > executed with a 1:1 mapping.
> 
> Hmm, I don't really understand this - the cpu_v6_reset code turns off 
> the MMU, then issues an ISB.  So for as long as cpu_v6_reset is 
> executing in the kernel virtual address space I don't see how it can 
> ever fetch the "mov pc, r0" instruction after the ISB without those 
> instructions living in the 1:1 mapping?

You need to make sure you call cpu_reset by jumping to its *physical*
address. If that happens to alias with the virtual address of the kernel, it
won't currently work but I have a solution to this in my kexec branch.

You can do something like:

	typedef void (*phys_reset_t)(unsigned long);

	phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);
	phys_reset(0xffff0000);

The problem with not having an ISB is that you can't guarantee at which point
the MMU is no longer active, so you're really walking on a tightrope in
that case.

Will

v6 software reset fails on 1176

[Jamie Iles]

On Tue, Aug 23, 2011 at 06:47:36PM +0100, Will Deacon wrote:
> You need to make sure you call cpu_reset by jumping to its *physical*
> address. If that happens to alias with the virtual address of the kernel, it
> won't currently work but I have a solution to this in my kexec branch.
> 
> You can do something like:
> 
> 	typedef void (*phys_reset_t)(unsigned long);
> 
> 	phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);
> 	phys_reset(0xffff0000);

OK, that makes perfect sense!  I'll give it a go on my hardware tomorrow 
when I'm back in the office.

All of the other mainline users of cpu_reset() just call it directly, so 
I guess they need to do the same thing?

Jamie

v6 software reset fails on 1176

[Will Deacon]

On Tue, Aug 23, 2011 at 06:56:38PM +0100, Jamie Iles wrote:
> On Tue, Aug 23, 2011 at 06:47:36PM +0100, Will Deacon wrote:
> > You need to make sure you call cpu_reset by jumping to its *physical*
> > address. If that happens to alias with the virtual address of the kernel, it
> > won't currently work but I have a solution to this in my kexec branch.
> > 
> > You can do something like:
> > 
> > 	typedef void (*phys_reset_t)(unsigned long);
> > 
> > 	phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);
> > 	phys_reset(0xffff0000);
> 
> OK, that makes perfect sense!  I'll give it a go on my hardware tomorrow 
> when I'm back in the office.

Great, let me know how you get on! I'll post my kexec series to the list
again once I've got on top of my current patchsets.

> All of the other mainline users of cpu_reset() just call it directly, so 
> I guess they need to do the same thing?

For v6 and v7, yes. For older ARM cores you could actually code for the
pipeline implementation and things were arguably easier that way.

Will

v6 software reset fails on 1176

[Jamie Iles]

On Tue, Aug 23, 2011 at 07:00:59PM +0100, Will Deacon wrote:
> On Tue, Aug 23, 2011 at 06:56:38PM +0100, Jamie Iles wrote:
> > On Tue, Aug 23, 2011 at 06:47:36PM +0100, Will Deacon wrote:
> > > You need to make sure you call cpu_reset by jumping to its *physical*
> > > address. If that happens to alias with the virtual address of the kernel, it
> > > won't currently work but I have a solution to this in my kexec branch.
> > > 
> > > You can do something like:
> > > 
> > > 	typedef void (*phys_reset_t)(unsigned long);
> > > 
> > > 	phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);
> > > 	phys_reset(0xffff0000);
> > 
> > OK, that makes perfect sense!  I'll give it a go on my hardware tomorrow 
> > when I'm back in the office.
> 
> Great, let me know how you get on! I'll post my kexec series to the list
> again once I've got on top of my current patchsets.

That works a treat, thanks Will!

Jamie