| Message ID | alpine.LFD.2.20.1511181156250.22569@knanqh.ubzr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Nicolas, > Here's the patch with proper changelog, etc. I don't have XIP > capable hardware to test it with though. I'm testing it now...but it's crashing. I fired up GDB, so here the reason: __v7_ca17mp_setup: mov r10, #0 1: adr r0, __v7_setup_stack_ptr r0=0x18213df4 ldr r12, [r0] r12=0x10174cc add r12, r12, r0 @ the local stack r12=0x1922b2c0 stmia r12, {r1-r6, lr} @ v7_invalidate_l1 touches r0-r6 bl v7_invalidate_l1 0x1922b2c0 is NOT RAM....it's nothing. As point of reference, here's the memory map of my XIP system: Physical ROM address: 0x18000000 (I have my XIP kernel starting at 0x18200000) Physical RAM address: 0x20000000 Virtual ROM address: 0xBF0000000 Virtual RAM address: 0xC00000000 Basically, you made the same mistake that Magnus first did: You can't rely on the current PC address to obtain an address in physical RAM because the ROM virt-to-phys relationship is different than the RAM virt-to-phys relationship. No matter how you look at it, this early in boot, the only 'thing' that really knows where physical RAM is located is PLAT_PHYS_OFFSET. That's why both Magnus's an my patches (that I both tested) contain PLAT_PHYS_OFFSET. The only other 'slight' piece of info would be the DTB pointer is still sitting in R2 from boot, and at least for my system, that is in RAM. However, the DTB could be in ROM if the user wanted it to be, and I also think that this code might run twice in a SMP system...so even R2 would not be reliable. So...forget I mentioned it... I don't see any way around not having a "#ifdef XIP_KERNEL" statement in order to expose PLAT_PHYS_OFFSET so you can properly do your translation math. Chris ps, I will add that I appreciate your efforts with this.
On Wed, 18 Nov 2015, Chris Brandt wrote: > Hi Nicolas, > > > Here's the patch with proper changelog, etc. I don't have XIP > > capable hardware to test it with though. > > > I'm testing it now...but it's crashing. > > I fired up GDB, so here the reason: > > > __v7_ca17mp_setup: > mov r10, #0 > 1: adr r0, __v7_setup_stack_ptr > r0=0x18213df4 > > ldr r12, [r0] > r12=0x10174cc > > add r12, r12, r0 @ the local stack > r12=0x1922b2c0 > > stmia r12, {r1-r6, lr} @ v7_invalidate_l1 touches r0-r6 > bl v7_invalidate_l1 > > > 0x1922b2c0 is NOT RAM....it's nothing. > > > As point of reference, here's the memory map of my XIP system: > Physical ROM address: 0x18000000 (I have my XIP kernel starting at 0x18200000) > Physical RAM address: 0x20000000 > Virtual ROM address: 0xBF0000000 > Virtual RAM address: 0xC00000000 > > > Basically, you made the same mistake that Magnus first did: You can't > rely on the current PC address to obtain an address in physical RAM > because the ROM virt-to-phys relationship is different than the RAM > virt-to-phys relationship. Crap... you're right of course. I suspect a couple other places might have problems as they use similar constructs. See kernel/sleep.S for example. Probably the best way to fix it would be something like: in asm/memory.h or similar: #ifdef CONFIG_XIP_KERNEL #define PHYS_OFFSET_FIXUP \ ( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \ PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define PHYS_OFFSET_FIXUP 0 #endif And then, after my patch is applied, changing: __v7_setup_stack_ptr: .word __v7_setup_stack - . into: __v7_setup_stack_ptr: .word __v7_setup_stack - . + PHYS_OFFSET_FIXUP should do the trick. This way it'll work for all those places where the code is getting at the data area when the MMU is off with no XIP conditionals in the code. I think my patch should be applied as is (minus the mention of XIP) to remove the write access to the .text area for the general case which is a worthy goal in itself. We did a bunch of similar cleanups a while ago. Then another patch could bring all those places XIP compatible with the simple addition of that PHYS_OFFSET_FIXUP constant. Nicolas
> Probably the best way to fix it would be something like: > > in asm/memory.h or similar: > > #ifdef CONFIG_XIP_KERNEL > #define PHYS_OFFSET_FIXUP \ > ( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \ > PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define > PHYS_OFFSET_FIXUP 0 #endif > > And then, after my patch is applied, changing: > > __v7_setup_stack_ptr: > .word __v7_setup_stack - . > > into: > > __v7_setup_stack_ptr: > .word __v7_setup_stack - . + PHYS_OFFSET_FIXUP > > should do the trick. This way it'll work for all those places where > the code is getting at the data area when the MMU is off with no XIP > conditionals in the code. Tested....it works! Here's your new results: __v7_ca17mp_setup: mov r10, #0 1: adr r0, __v7_setup_stack_ptr r0=0x18213df4 ldr r12, [r0] r12=0x7e174cc add r12, r12, r0 @ the local stack r12=0x2002b2c0 stmia r12, {r1-r6, lr} @ v7_invalidate_l1 touches r0-r6 bl v7_invalidate_l1 0x2002b2c0 is indeed the correct physical address of __v7_setup_stack_ptr. So, you still need to use #ifdef XIP_KERNEL and PLAT_PHYS_OFFSET....but...you bury it in another file. I'm good with that!! > Then another patch could bring all those places XIP compatible with > the simple addition of that PHYS_OFFSET_FIXUP constant. Yes, the less CONFIG_XIP_KERNEL is sprinkled around the arm tree code, the better. Chris
On Wed, 18 Nov 2015, Chris Brandt wrote: > > Probably the best way to fix it would be something like: > > > > in asm/memory.h or similar: > > > > #ifdef CONFIG_XIP_KERNEL > > #define PHYS_OFFSET_FIXUP \ > > ( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \ > > PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define > > PHYS_OFFSET_FIXUP 0 #endif > > > > And then, after my patch is applied, changing: > > > > __v7_setup_stack_ptr: > > .word __v7_setup_stack - . > > > > into: > > > > __v7_setup_stack_ptr: > > .word __v7_setup_stack - . + PHYS_OFFSET_FIXUP > > > > should do the trick. This way it'll work for all those places where > > the code is getting at the data area when the MMU is off with no XIP > > conditionals in the code. > > > Tested....it works! Excellent. Nicolas
On Wed, 18 Nov 2015, Nicolas Pitre wrote: > On Wed, 18 Nov 2015, Chris Brandt wrote: > > > > Probably the best way to fix it would be something like: > > > > > > in asm/memory.h or similar: > > > > > > #ifdef CONFIG_XIP_KERNEL > > > #define PHYS_OFFSET_FIXUP \ > > > ( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \ > > > PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define > > >PHYS_OFFSET_FIXUP 0 #endif > > > > > > And then, after my patch is applied, changing: > > > > > > __v7_setup_stack_ptr: > > > .word __v7_setup_stack - . > > > > > > into: > > > > > > __v7_setup_stack_ptr: > > > .word __v7_setup_stack - . + PHYS_OFFSET_FIXUP > > > > > > should do the trick. This way it'll work for all those places where > > > the code is getting at the data area when the MMU is off with no XIP > > > conditionals in the code. > > > > > > Tested....it works! > > Excellent. > > > Nicolas Nicolas, I applied your PHYS_OFFSET_FIXUP macro code (shown above) to the current tree which already has your commit "ARM: 8453/2: proc-v7.S: don't locate temporary stack space in .text section". After testing, it shows the same results for an XIP_KERNEL build as it did in November: No PHYS_OFFSET_FIXUP = crash with PHYS_OFFSET_FIXUP = good Can I submit a patch with your code for review and inclusion to the kernel? Chris
On Fri, 29 Jan 2016, Chris Brandt wrote: > On Wed, 18 Nov 2015, Nicolas Pitre wrote: > > > On Wed, 18 Nov 2015, Chris Brandt wrote: > > > > > > Probably the best way to fix it would be something like: > > > > > > > > in asm/memory.h or similar: > > > > > > > > #ifdef CONFIG_XIP_KERNEL > > > > #define PHYS_OFFSET_FIXUP \ > > > > ( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \ > > > > PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define > > > >PHYS_OFFSET_FIXUP 0 #endif > > > > > > > > And then, after my patch is applied, changing: > > > > > > > > __v7_setup_stack_ptr: > > > > .word __v7_setup_stack - . > > > > > > > > into: > > > > > > > > __v7_setup_stack_ptr: > > > > .word __v7_setup_stack - . + PHYS_OFFSET_FIXUP > > > > > > > > should do the trick. This way it'll work for all those places where > > > > the code is getting at the data area when the MMU is off with no XIP > > > > conditionals in the code. > > > > > > > > > Tested....it works! > > > > Excellent. > > > > > > Nicolas > > > Nicolas, > > I applied your PHYS_OFFSET_FIXUP macro code (shown above) to the current tree which already has your commit "ARM: 8453/2: proc-v7.S: don't locate temporary stack space in .text section". > > After testing, it shows the same results for an XIP_KERNEL build as it did in November: > > No PHYS_OFFSET_FIXUP = crash > > with PHYS_OFFSET_FIXUP = good > > > Can I submit a patch with your code for review and inclusion to the kernel? Sure. Signed-off-by: Nicolas Pitre <nico@linaro.org> Nicolas
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S index de2b246fed..2d0ac32320 100644 --- a/arch/arm/mm/proc-v7.S +++ b/arch/arm/mm/proc-v7.S @@ -274,10 +274,12 @@ __v7_ca15mp_setup: __v7_b15mp_setup: __v7_ca17mp_setup: mov r10, #0 -1: adr r12, __v7_setup_stack @ the local stack - stmia r12, {r0-r5, lr} @ v7_invalidate_l1 touches r0-r6 +1: adr r0, __v7_setup_stack_ptr + ldr r12, [r0] + add r12, r12, r0 @ the local stack + stmia r12, {r1-r6, lr} @ v7_invalidate_l1 touches r0-r6 bl v7_invalidate_l1 - ldmia r12, {r0-r5, lr} + ldmia r12, {r1-r6, lr} #ifdef CONFIG_SMP ALT_SMP(mrc p15, 0, r0, c1, c0, 1) ALT_UP(mov r0, #(1 << 6)) @ fake it for UP @@ -415,10 +417,12 @@ __v7_pj4b_setup: #endif /* CONFIG_CPU_PJ4B */ __v7_setup: - adr r12, __v7_setup_stack @ the local stack - stmia r12, {r0-r5, lr} @ v7_invalidate_l1 touches r0-r6 + adr r0, __v7_setup_stack_ptr + ldr r12, [r0] + add r12, r12, r0 @ the local stack + stmia r12, {r1-r6, lr} @ v7_invalidate_l1 touches r0-r6 bl v7_invalidate_l1 - ldmia r12, {r0-r5, lr} + ldmia r12, {r1-r6, lr} __v7_setup_cont: and r0, r9, #0xff000000 @ ARM? @@ -480,11 +484,16 @@ __errata_finish: orr r0, r0, r6 @ set them THUMB( orr r0, r0, #1 << 30 ) @ Thumb exceptions ret lr @ return to head.S:__ret + + .align 2 +__v7_setup_stack_ptr: + .word __v7_setup_stack - . ENDPROC(__v7_setup) + .bss .align 2 __v7_setup_stack: - .space 4 * 7 @ 12 registers + .space 4 * 7 @ 7 registers __INITDATA diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S index 67d9209077..8cd465927a 100644 --- a/arch/arm/mm/proc-v7m.S +++ b/arch/arm/mm/proc-v7m.S @@ -123,10 +123,12 @@ __v7m_setup: ret lr ENDPROC(__v7m_setup) + .pushsection .bss .align 2 __v7m_setup_stack: .space 4 * 8 @ 8 registers __v7m_setup_stack_top: + .previous define_processor_functions v7m, dabort=nommu_early_abort, pabort=legacy_pabort, nommu=1