[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Per Forlin]

PC runs virtual addresses when calling cpu_v7_reset(). The MMU
is switched off and "mov pc, r0" sets pc back to
virtual addresses even though the MMU is switched off.
This will result in a crash if the pipeline delay after
MMU disable is one instruction. To prevent this set PC
to physical addresses before disabling the MMU.

Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
Signed-off-by: Per Forlin <per.forlin@stericsson.com>
---
 arch/arm/mm/proc-v7.S |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index 9049c07..f26e831 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -64,6 +64,7 @@ ENDPROC(cpu_v7_proc_fin)
  */
 	.align	5
 ENTRY(cpu_v7_reset)
+	sub	pc, pc, #PAGE_OFFSET+4		@ go to physical addresses
 	mrc	p15, 0, r1, c1, c0, 0		@ ctrl register
 	bic	r1, r1, #0x1			@ ...............m
  THUMB(	bic	r1, r1, #1 << 30 )		@ SCTLR.TE (Thumb exceptions)
-- 
1.6.3.3

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Russell King - ARM Linux]

On Sat, Sep 24, 2011 at 10:02:50AM +0200, Per Forlin wrote:
> PC runs virtual addresses when calling cpu_v7_reset(). The MMU
> is switched off and "mov pc, r0" sets pc back to
> virtual addresses even though the MMU is switched off.
> This will result in a crash if the pipeline delay after
> MMU disable is one instruction. To prevent this set PC
> to physical addresses before disabling the MMU.

This is broken - it assumes physical memory starts at zero.  That's
not the case for many platforms.

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Mika Westerberg]

On Sat, Sep 24, 2011 at 10:02:50AM +0200, Per Forlin wrote:
> PC runs virtual addresses when calling cpu_v7_reset(). The MMU
> is switched off and "mov pc, r0" sets pc back to
> virtual addresses even though the MMU is switched off.
> This will result in a crash if the pipeline delay after
> MMU disable is one instruction. To prevent this set PC
> to physical addresses before disabling the MMU.
> 
> Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
> Signed-off-by: Per Forlin <per.forlin@stericsson.com>
> ---
>  arch/arm/mm/proc-v7.S |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index 9049c07..f26e831 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -64,6 +64,7 @@ ENDPROC(cpu_v7_proc_fin)
>   */
>  	.align	5
>  ENTRY(cpu_v7_reset)
> +	sub	pc, pc, #PAGE_OFFSET+4		@ go to physical addresses

This only works on machines where PHYS_OFFSET is zero.

IIRC, there is a P<->V offset calculated at boot time. Maybe you could use
that instead here?

>  	mrc	p15, 0, r1, c1, c0, 0		@ ctrl register
>  	bic	r1, r1, #0x1			@ ...............m
>   THUMB(	bic	r1, r1, #1 << 30 )		@ SCTLR.TE (Thumb exceptions)
> -- 
> 1.6.3.3
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Per Förlin]

On 09/24/2011 10:13 AM, Mika Westerberg wrote:
> On Sat, Sep 24, 2011 at 10:02:50AM +0200, Per Forlin wrote:
>> PC runs virtual addresses when calling cpu_v7_reset(). The MMU
>> is switched off and "mov pc, r0" sets pc back to
>> virtual addresses even though the MMU is switched off.
>> This will result in a crash if the pipeline delay after
>> MMU disable is one instruction. To prevent this set PC
>> to physical addresses before disabling the MMU.
>>
>> Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
>> Signed-off-by: Per Forlin <per.forlin@stericsson.com>
>> ---
>>  arch/arm/mm/proc-v7.S |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
>> index 9049c07..f26e831 100644
>> --- a/arch/arm/mm/proc-v7.S
>> +++ b/arch/arm/mm/proc-v7.S
>> @@ -64,6 +64,7 @@ ENDPROC(cpu_v7_proc_fin)
>>   */
>>  	.align	5
>>  ENTRY(cpu_v7_reset)
>> +	sub	pc, pc, #PAGE_OFFSET+4		@ go to physical addresses
> 
> This only works on machines where PHYS_OFFSET is zero.
> 
You are right! Russell was very quick to point out the same thing. 

> IIRC, there is a P<->V offset calculated at boot time. Maybe you could use
> that instead here?
> 
I am trying to figure out how I can get the value of PHYS_OFFSET in assembler code (proc-v7.S).
I guess I could use the value calculated in arch/arm/kernel.head.S but wouldn't that make
this patch dependent on CONFIG_ARM_PATCH_PHYS_VIRT?

Regards,
Per

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Will Deacon]

On Sat, Sep 24, 2011 at 10:32:48AM +0100, Per F?rlin wrote:
> I am trying to figure out how I can get the value of PHYS_OFFSET in assembler code (proc-v7.S).
> I guess I could use the value calculated in arch/arm/kernel.head.S but wouldn't that make
> this patch dependent on CONFIG_ARM_PATCH_PHYS_VIRT?

Wouldn't we just be better off passing a physical address to cpu_v7_reset
instead?

Will

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Per Förlin]

On 09/24/2011 01:00 PM, Will Deacon wrote:
> On Sat, Sep 24, 2011 at 10:32:48AM +0100, Per F?rlin wrote:
>> I am trying to figure out how I can get the value of PHYS_OFFSET in assembler code (proc-v7.S).
>> I guess I could use the value calculated in arch/arm/kernel.head.S but wouldn't that make
>> this patch dependent on CONFIG_ARM_PATCH_PHYS_VIRT?
> 
> Wouldn't we just be better off passing a physical address to cpu_v7_reset
> instead?
> 
Good point! I'm fine with that.

/Per

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Russell King - ARM Linux]

On Sat, Sep 24, 2011 at 02:55:42PM +0200, Per F?rlin wrote:
> On 09/24/2011 01:00 PM, Will Deacon wrote:
> > On Sat, Sep 24, 2011 at 10:32:48AM +0100, Per F?rlin wrote:
> >> I am trying to figure out how I can get the value of PHYS_OFFSET in assembler code (proc-v7.S).
> >> I guess I could use the value calculated in arch/arm/kernel.head.S but wouldn't that make
> >> this patch dependent on CONFIG_ARM_PATCH_PHYS_VIRT?
> > 
> > Wouldn't we just be better off passing a physical address to cpu_v7_reset
> > instead?
> > 
> Good point! I'm fine with that.

Alternatively, call it using the phys address.

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Per Förlin]

On 09/24/2011 03:02 PM, Russell King - ARM Linux wrote:
> On Sat, Sep 24, 2011 at 02:55:42PM +0200, Per F?rlin wrote:
>> On 09/24/2011 01:00 PM, Will Deacon wrote:
>>> On Sat, Sep 24, 2011 at 10:32:48AM +0100, Per F?rlin wrote:
>>>> I am trying to figure out how I can get the value of PHYS_OFFSET in assembler code (proc-v7.S).
>>>> I guess I could use the value calculated in arch/arm/kernel.head.S but wouldn't that make
>>>> this patch dependent on CONFIG_ARM_PATCH_PHYS_VIRT?
>>>
>>> Wouldn't we just be better off passing a physical address to cpu_v7_reset
>>> instead?
>>>
>> Good point! I'm fine with that.
> 
> Alternatively, call it using the phys address.
I changed the code to use a label instead of the ARM specific instruction offset +4. A few more lines but the intention is that it should work with both ARM and THUMB. I'm in favour of Russell's idea of setting pc to physical before calling cpu_reset().
      /* Go to physical addresses to be ready for MMU disable */
      asm("ADR r1, pc_phy_here \n\t"
          "sub r1, %0 \n\t"
          "mov pc, r1 \n\t"
          "pc_phy_here: \n\t"
          : :"r" (PAGE_OFFSET - PHYS_OFFSET) : "r1", "cc");

Where is the best place for this code?
Should this be run for all ARM cpu_reset()?

I haven't found a nice way to add to cpu_reset() yet. This is what I have so far.
1. Redirect cpu_reset to c-function __cpu_reset_phy() that sets the pc to phy and then calls __cpu_reset() (the original one)
1.1 May add ifdefs in __cpu_reset_phy() to only do the virt to phy transition for some explicit ARM versions before calling __cpu_reset()
2. Rename cpu_v7_reset() to __cpu_v7_reset(). Only redirect cpu_v7_reset() to the c-function that goes to phy and then calls __cpu_v7_reset().
3. Add a new in parameter "phys_offset" to cpu_reset(). Modify only those CPU_reset() that needs this parameter. Redirect cpu_reset(x,y) to CPU_reset(x) or CPU_reset(x,y) based on ARM version.

Regards,
Per

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Will Deacon]

Hi Per,

On Fri, Sep 30, 2011 at 06:37:41AM +0100, Per F?rlin wrote:
> On 09/24/2011 03:02 PM, Russell King - ARM Linux wrote:
> > On Sat, Sep 24, 2011 at 02:55:42PM +0200, Per F?rlin wrote:
> >> On 09/24/2011 01:00 PM, Will Deacon wrote:
> >>> On Sat, Sep 24, 2011 at 10:32:48AM +0100, Per F?rlin wrote:
> >>>> I am trying to figure out how I can get the value of PHYS_OFFSET in assembler code (proc-v7.S).
> >>>> I guess I could use the value calculated in arch/arm/kernel.head.S but wouldn't that make
> >>>> this patch dependent on CONFIG_ARM_PATCH_PHYS_VIRT?
> >>>
> >>> Wouldn't we just be better off passing a physical address to cpu_v7_reset
> >>> instead?
> >>>
> >> Good point! I'm fine with that.
> > 
> > Alternatively, call it using the phys address.
> I changed the code to use a label instead of the ARM specific instruction offset +4. A few more lines but the intention is that it should work with both ARM and THUMB. I'm in favour of Russell's idea of setting pc to physical before calling cpu_reset().
>       /* Go to physical addresses to be ready for MMU disable */
>       asm("ADR r1, pc_phy_here \n\t"
>           "sub r1, %0 \n\t"
>           "mov pc, r1 \n\t"
>           "pc_phy_here: \n\t"
>           : :"r" (PAGE_OFFSET - PHYS_OFFSET) : "r1", "cc");

I'm not sure why all of this is necessary. Take a look at my kexec patches
here:

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

The interesting bit is __arm_machine_reset in kernel/process.c

Will

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Per Förlin]

On 09/30/2011 10:34 AM, Will Deacon wrote:
> Hi Per,
> 
> On Fri, Sep 30, 2011 at 06:37:41AM +0100, Per F?rlin wrote:
>> On 09/24/2011 03:02 PM, Russell King - ARM Linux wrote:
>>> On Sat, Sep 24, 2011 at 02:55:42PM +0200, Per F?rlin wrote:
>>>> On 09/24/2011 01:00 PM, Will Deacon wrote:
>>>>> On Sat, Sep 24, 2011 at 10:32:48AM +0100, Per F?rlin wrote:
>>>>>> I am trying to figure out how I can get the value of PHYS_OFFSET in assembler code (proc-v7.S).
>>>>>> I guess I could use the value calculated in arch/arm/kernel.head.S but wouldn't that make
>>>>>> this patch dependent on CONFIG_ARM_PATCH_PHYS_VIRT?
>>>>>
>>>>> Wouldn't we just be better off passing a physical address to cpu_v7_reset
>>>>> instead?
>>>>>
>>>> Good point! I'm fine with that.
>>>
>>> Alternatively, call it using the phys address.
>> I changed the code to use a label instead of the ARM specific instruction offset +4. A few more lines but the intention is that it should work with both ARM and THUMB. I'm in favour of Russell's idea of setting pc to physical before calling cpu_reset().
>>       /* Go to physical addresses to be ready for MMU disable */
>>       asm("ADR r1, pc_phy_here \n\t"
>>           "sub r1, %0 \n\t"
>>           "mov pc, r1 \n\t"
>>           "pc_phy_here: \n\t"
>>           : :"r" (PAGE_OFFSET - PHYS_OFFSET) : "r1", "cc");
> 
> I'm not sure why all of this is necessary. Take a look at my kexec patches
> here:
> 
> http://www.linux-arm.org/git?p=linux-2.6-wd.git;a=shortlog;h=refs/heads/kexec-mmu-off
> 
> The interesting bit is __arm_machine_reset in kernel/process.c
> 
/* Switch to the identity mapping. */
phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);
phys_reset(reset_args->reset_code_phys);
Yes this is definitely how it should be done. My mind was stuck in the assembler code.

If I get this right the bug I run into is already fixed by your kexec patches. I could simply use your patches?

Thanks,
Per

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Will Deacon]

On Fri, Sep 30, 2011 at 10:34:06AM +0100, Per F?rlin wrote:
> On 09/30/2011 10:34 AM, Will Deacon wrote:
> > I'm not sure why all of this is necessary. Take a look at my kexec patches
> > here:
> > 
> > http://www.linux-arm.org/git?p=linux-2.6-wd.git;a=shortlog;h=refs/heads/kexec-mmu-off
> > 
> > The interesting bit is __arm_machine_reset in kernel/process.c
> > 
> /* Switch to the identity mapping. */
> phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);
> phys_reset(reset_args->reset_code_phys);
> Yes this is definitely how it should be done. My mind was stuck in the assembler code.

Easy mistake to make!

> If I get this right the bug I run into is already fixed by your kexec patches. I could simply use your patches?

Sure, I reworked them the other day so I'll post them to the list again
soon. There are still some open problems to solve but the reset code is
more-or-less there.

Will

[PATCH] arm: proc-v7: pc phy addresses before disable MMU

[Dave Martin]

On Sat, Sep 24, 2011 at 11:32:48AM +0200, Per F?rlin wrote:
> On 09/24/2011 10:13 AM, Mika Westerberg wrote:
> > On Sat, Sep 24, 2011 at 10:02:50AM +0200, Per Forlin wrote:
> >> PC runs virtual addresses when calling cpu_v7_reset(). The MMU
> >> is switched off and "mov pc, r0" sets pc back to
> >> virtual addresses even though the MMU is switched off.
> >> This will result in a crash if the pipeline delay after
> >> MMU disable is one instruction. To prevent this set PC
> >> to physical addresses before disabling the MMU.
> >>
> >> Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
> >> Signed-off-by: Per Forlin <per.forlin@stericsson.com>
> >> ---
> >>  arch/arm/mm/proc-v7.S |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> >> index 9049c07..f26e831 100644
> >> --- a/arch/arm/mm/proc-v7.S
> >> +++ b/arch/arm/mm/proc-v7.S
> >> @@ -64,6 +64,7 @@ ENDPROC(cpu_v7_proc_fin)
> >>   */
> >>  	.align	5
> >>  ENTRY(cpu_v7_reset)
> >> +	sub	pc, pc, #PAGE_OFFSET+4		@ go to physical addresses
> > 
> > This only works on machines where PHYS_OFFSET is zero.
> > 
> You are right! Russell was very quick to point out the same thing. 

For reference, I'll also point out that except for "mov pc,<Rm>", 
almost no instruction involving an explicit reference to the pc will
safely do what you want in Thumb-2 kernels, due to the variable instruction
size.

Cheers
---Dave