| Message ID | 20131220074339.GA23977@pek-khao-d1.corp.ad.wrs.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, 2013-12-20 at 15:43 +0800, Kevin Hao wrote: > On Wed, Dec 18, 2013 at 05:48:25PM -0600, Scott Wood wrote: > > On Wed, Aug 07, 2013 at 09:18:31AM +0800, Kevin Hao wrote: > > > This is based on the codes in the head_44x.S. The difference is that > > > the init tlb size we used is 64M. With this patch we can only load the > > > kernel at address between memstart_addr ~ memstart_addr + 64M. We will > > > fix this restriction in the following patches. > > > > Which following patch fixes the restriction? With all seven patches > > applied, I was still only successful booting within 64M of memstart_addr. > > There is bug in this patch series when booting above the 64M. It seems > that I missed to test this previously. Sorry for that. With the following > change I can boot the kernel at 0x5000000. I tried v4 and it still doesn't work for me over 64M (without increasing the start of memory). I pulled the following out of the log buffer when booting at 0x5000000 (after cleaning up the binary goo -- is that something new?): Unable to handle kernel paging request for data at address 0xbffe4008 Faulting instruction address: 0xc16ee934 Oops: Kernel access of bad area, sig: 11 [#1] SMP NR_CPUS=8 Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 3.13.0-rc1-00065-g422752a #12 task: c18f2340 ti: c192c000 task.ti: c192c000 NIP: c16ee934 LR: c16d1860 CTR: c100e96c REGS: c192dee0 TRAP: 0300 Not tainted (3.13.0-rc1-00065-g422752a) MSR: 00021002 <CE,ME> CR: 42044022 XER: 20000000 DEAR: bffe4008 ESR: 00000000 GPR00: c16d1860 c192df90 c18f2340 c16eec58 00000000 00000000 05000000 00000000 GPR08: 00000000 bffe4000 00000000 bc000000 00000000 0570ad40 ffffffff 7ffb0aec GPR16: 00000000 00000000 7fe4dd70 00000000 7fe4ddb0 00000000 c192c000 00000007 GPR24: 00000000 c1940000 c16eec58 00000000 ffffffff 03fe4000 00000000 c18f0000 NIP [c16ee934] of_scan_flat_dt+0x28/0x148 LR [c16d1860] early_get_first_memblock_info+0x38/0x84 Call Trace: [c192dfc0] [c16d1860] early_get_first_memblock_info+0x38/0x84 [c192dfd0] [c16d4888] relocate_init+0x98/0x160 [c192dff0] [c100045c] set_ivor+0x144/0x190 Instruction dump: 7c0803a6 4e800020 9421ffd0 7c0802a6 bee1000c 3f20c194 8139a45c 7c7a1b78 90010034 7c9b2378 3b80ffff 3ae00007 <83e90008> 3b00fff8 7fe9fa14 48000008 ---[ end trace 41ed10ed80b8d831 ]--- Kernel panic - not syncing: Attempted to kill the idle task! > diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c > index 048d716ae706..ce0c7d7db6c3 100644 > --- a/arch/powerpc/mm/fsl_booke_mmu.c > +++ b/arch/powerpc/mm/fsl_booke_mmu.c > @@ -171,11 +171,10 @@ unsigned long calc_cam_sz(unsigned long ram, unsigned long virt, > return 1UL << camsize; > } > > -unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) > +static unsigned long map_mem_in_cams_addr(phys_addr_t phys, unsigned long virt, > + unsigned long ram, int max_cam_idx) > { > int i; > - unsigned long virt = PAGE_OFFSET; > - phys_addr_t phys = memstart_addr; > unsigned long amount_mapped = 0; > > /* Calculate CAM values */ > @@ -195,6 +194,14 @@ unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) > return amount_mapped; > } > > +unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) > +{ > + unsigned long virt = PAGE_OFFSET; > + phys_addr_t phys = memstart_addr; > + > + return map_mem_in_cams_addr(phys, virt, ram, max_cam_idx); > +} > + > #ifdef CONFIG_PPC32 > > #if defined(CONFIG_LOWMEM_CAM_NUM_BOOL) && (CONFIG_LOWMEM_CAM_NUM >= NUM_TLBCAMS) > @@ -289,7 +296,11 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start) > is_second_reloc = 1; > n = switch_to_as1(); > /* map a 64M area for the second relocation */ > - map_mem_in_cams(0x4000000UL, CONFIG_LOWMEM_CAM_NUM); > + if (memstart_addr > start) > + map_mem_in_cams(0x4000000, CONFIG_LOWMEM_CAM_NUM); > + else > + map_mem_in_cams_addr(start, PAGE_OFFSET - offset, > + 0x4000000, CONFIG_LOWMEM_CAM_NUM); > restore_to_as0(n, offset, __va(dt_ptr)); > /* We should never reach here */ > panic("Relocation error"); > I'm having a hard time following the logic here. What is PAGE_OFFSET - offset supposed to be? Why would we map anything belowe PAGE_OFFSET? -Scott
On Fri, Jan 03, 2014 at 06:49:09PM -0600, Scott Wood wrote: > On Fri, 2013-12-20 at 15:43 +0800, Kevin Hao wrote: > > On Wed, Dec 18, 2013 at 05:48:25PM -0600, Scott Wood wrote: > > > On Wed, Aug 07, 2013 at 09:18:31AM +0800, Kevin Hao wrote: > > > > This is based on the codes in the head_44x.S. The difference is that > > > > the init tlb size we used is 64M. With this patch we can only load the > > > > kernel at address between memstart_addr ~ memstart_addr + 64M. We will > > > > fix this restriction in the following patches. > > > > > > Which following patch fixes the restriction? With all seven patches > > > applied, I was still only successful booting within 64M of memstart_addr. > > > > There is bug in this patch series when booting above the 64M. It seems > > that I missed to test this previously. Sorry for that. With the following > > change I can boot the kernel at 0x5000000. > > I tried v4 and it still doesn't work for me over 64M (without increasing > the start of memory). I pulled the following out of the log buffer when > booting at 0x5000000 (after cleaning up the binary goo -- is that > something new?): > > Unable to handle kernel paging request for data at address 0xbffe4008 Actually there still have one limitation that we have to make sure that the kernel and dtb are in the 64M memory mapped by the init tlb entry. I booted the kernel successfully by using the following u-boot commands: setenv fdt_high 0xffffffff dhcp 6000000 128.224.162.196:/vlm-boards/p5020/uImage tftp 6f00000 128.224.162.196:/vlm-boards/p5020/p5020ds.dtb bootm 6000000 - 6f00000 > Faulting instruction address: 0xc16ee934 > Oops: Kernel access of bad area, sig: 11 [#1] > SMP NR_CPUS=8 > Modules linked in: > CPU: 0 PID: 0 Comm: swapper Not tainted 3.13.0-rc1-00065-g422752a #12 > task: c18f2340 ti: c192c000 task.ti: c192c000 > NIP: c16ee934 LR: c16d1860 CTR: c100e96c > REGS: c192dee0 TRAP: 0300 Not tainted (3.13.0-rc1-00065-g422752a) > MSR: 00021002 <CE,ME> CR: 42044022 XER: 20000000 > DEAR: bffe4008 ESR: 00000000 > GPR00: c16d1860 c192df90 c18f2340 c16eec58 00000000 00000000 05000000 00000000 > GPR08: 00000000 bffe4000 00000000 bc000000 00000000 0570ad40 ffffffff 7ffb0aec > GPR16: 00000000 00000000 7fe4dd70 00000000 7fe4ddb0 00000000 c192c000 00000007 > GPR24: 00000000 c1940000 c16eec58 00000000 ffffffff 03fe4000 00000000 c18f0000 > NIP [c16ee934] of_scan_flat_dt+0x28/0x148 > LR [c16d1860] early_get_first_memblock_info+0x38/0x84 > Call Trace: > [c192dfc0] [c16d1860] early_get_first_memblock_info+0x38/0x84 > [c192dfd0] [c16d4888] relocate_init+0x98/0x160 > [c192dff0] [c100045c] set_ivor+0x144/0x190 > Instruction dump: > 7c0803a6 4e800020 9421ffd0 7c0802a6 bee1000c 3f20c194 8139a45c 7c7a1b78 > 90010034 7c9b2378 3b80ffff 3ae00007 <83e90008> 3b00fff8 7fe9fa14 48000008 > ---[ end trace 41ed10ed80b8d831 ]--- > Kernel panic - not syncing: Attempted to kill the idle task! > > > > diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c > > index 048d716ae706..ce0c7d7db6c3 100644 > > --- a/arch/powerpc/mm/fsl_booke_mmu.c > > +++ b/arch/powerpc/mm/fsl_booke_mmu.c > > @@ -171,11 +171,10 @@ unsigned long calc_cam_sz(unsigned long ram, unsigned long virt, > > return 1UL << camsize; > > } > > > > -unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) > > +static unsigned long map_mem_in_cams_addr(phys_addr_t phys, unsigned long virt, > > + unsigned long ram, int max_cam_idx) > > { > > int i; > > - unsigned long virt = PAGE_OFFSET; > > - phys_addr_t phys = memstart_addr; > > unsigned long amount_mapped = 0; > > > > /* Calculate CAM values */ > > @@ -195,6 +194,14 @@ unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) > > return amount_mapped; > > } > > > > +unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) > > +{ > > + unsigned long virt = PAGE_OFFSET; > > + phys_addr_t phys = memstart_addr; > > + > > + return map_mem_in_cams_addr(phys, virt, ram, max_cam_idx); > > +} > > + > > #ifdef CONFIG_PPC32 > > > > #if defined(CONFIG_LOWMEM_CAM_NUM_BOOL) && (CONFIG_LOWMEM_CAM_NUM >= NUM_TLBCAMS) > > @@ -289,7 +296,11 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start) > > is_second_reloc = 1; > > n = switch_to_as1(); > > /* map a 64M area for the second relocation */ > > - map_mem_in_cams(0x4000000UL, CONFIG_LOWMEM_CAM_NUM); > > + if (memstart_addr > start) > > + map_mem_in_cams(0x4000000, CONFIG_LOWMEM_CAM_NUM); > > + else > > + map_mem_in_cams_addr(start, PAGE_OFFSET - offset, > > + 0x4000000, CONFIG_LOWMEM_CAM_NUM); > > restore_to_as0(n, offset, __va(dt_ptr)); > > /* We should never reach here */ > > panic("Relocation error"); > > > > I'm having a hard time following the logic here. What is PAGE_OFFSET - > offset supposed to be? Why would we map anything belowe PAGE_OFFSET? No, we don't map the address below PAGE_OFFSET. memstart_addr is the physical start address of RAM. start is the kernel running physical address aligned with 64M. offset = memstart_addr - start So if memstart_addr < start, the offset is negative. The PAGE_OFFSET - offset is the virtual start address we should use for the init 64M map. It's above the PAGE_OFFSET instead of below. For example, if we boot the kernel at 0x5000000: memstart_addr = 0x0 start = 0x4000000 offset = -0x4000000 PAGE_OFFSET - offset = 0xc4000000. Then we should create a 64M map for the virtual address from 0xc4000000 ~ 0xc8000000. This is the final virtual address that the kernel symbols would use. Thanks, Kevin > > -Scott > >
On Sat, 2014-01-04 at 14:34 +0800, Kevin Hao wrote: > On Fri, Jan 03, 2014 at 06:49:09PM -0600, Scott Wood wrote: > > On Fri, 2013-12-20 at 15:43 +0800, Kevin Hao wrote: > > > On Wed, Dec 18, 2013 at 05:48:25PM -0600, Scott Wood wrote: > > > > On Wed, Aug 07, 2013 at 09:18:31AM +0800, Kevin Hao wrote: > > > > > This is based on the codes in the head_44x.S. The difference is that > > > > > the init tlb size we used is 64M. With this patch we can only load the > > > > > kernel at address between memstart_addr ~ memstart_addr + 64M. We will > > > > > fix this restriction in the following patches. > > > > > > > > Which following patch fixes the restriction? With all seven patches > > > > applied, I was still only successful booting within 64M of memstart_addr. > > > > > > There is bug in this patch series when booting above the 64M. It seems > > > that I missed to test this previously. Sorry for that. With the following > > > change I can boot the kernel at 0x5000000. > > > > I tried v4 and it still doesn't work for me over 64M (without increasing > > the start of memory). I pulled the following out of the log buffer when > > booting at 0x5000000 (after cleaning up the binary goo -- is that > > something new?): > > > > Unable to handle kernel paging request for data at address 0xbffe4008 > > Actually there still have one limitation that we have to make sure > that the kernel and dtb are in the 64M memory mapped by the init tlb entry. > I booted the kernel successfully by using the following u-boot commands: > setenv fdt_high 0xffffffff > dhcp 6000000 128.224.162.196:/vlm-boards/p5020/uImage > tftp 6f00000 128.224.162.196:/vlm-boards/p5020/p5020ds.dtb > bootm 6000000 - 6f00000 OK, that was it -- I hadn't set fdt_high and thus U-Boot was relocating the fdt under 64M. We should probably be using ioremap_prot() (or some other mechanism) to create a special mapping, rather than assuming the fdt is covered by the initial TLB entry. That doesn't need to happen as part of this patchset, of course, as it's not a new limitation. > > I'm having a hard time following the logic here. What is PAGE_OFFSET - > > offset supposed to be? Why would we map anything belowe PAGE_OFFSET? > > No, we don't map the address below PAGE_OFFSET. > memstart_addr is the physical start address of RAM. > start is the kernel running physical address aligned with 64M. > > offset = memstart_addr - start > > So if memstart_addr < start, the offset is negative. The PAGE_OFFSET - offset > is the virtual start address we should use for the init 64M map. It's above > the PAGE_OFFSET instead of below. Oh. I think it'd be more readable to do "offset = start - memstart_addr" and add offset instead of subtracting it. Also, offset should be phys_addr_t -- even if you don't expect to support offsets greater than 4G on 32-bit, it's semantically the right type to use. Plus, "int" would break if this code were ever used with 64-bit. If you're OK with these changes, I can fix it while applying. -Scott
On Tue, Jan 07, 2014 at 05:46:04PM -0600, Scott Wood wrote: > On Sat, 2014-01-04 at 14:34 +0800, Kevin Hao wrote: > > Actually there still have one limitation that we have to make sure > > that the kernel and dtb are in the 64M memory mapped by the init tlb entry. > > I booted the kernel successfully by using the following u-boot commands: > > setenv fdt_high 0xffffffff > > dhcp 6000000 128.224.162.196:/vlm-boards/p5020/uImage > > tftp 6f00000 128.224.162.196:/vlm-boards/p5020/p5020ds.dtb > > bootm 6000000 - 6f00000 > > OK, that was it -- I hadn't set fdt_high and thus U-Boot was relocating > the fdt under 64M. > > We should probably be using ioremap_prot() (or some other mechanism) to It is too early to use ioremap_prot() for this case. > create a special mapping, rather than assuming the fdt is covered by the > initial TLB entry. That doesn't need to happen as part of this > patchset, of course, as it's not a new limitation. In order to fix this limitation we would have to create a separate map for the dtb if it is not covered by the init 64M tlb. I would like to give it a try if I can get some time. > > > > I'm having a hard time following the logic here. What is PAGE_OFFSET - > > > offset supposed to be? Why would we map anything belowe PAGE_OFFSET? > > > > No, we don't map the address below PAGE_OFFSET. > > memstart_addr is the physical start address of RAM. > > start is the kernel running physical address aligned with 64M. > > > > offset = memstart_addr - start > > > > So if memstart_addr < start, the offset is negative. The PAGE_OFFSET - offset > > is the virtual start address we should use for the init 64M map. It's above > > the PAGE_OFFSET instead of below. > > Oh. I think it'd be more readable to do "offset = start - > memstart_addr" and add offset instead of subtracting it. Yes, I agree. The reason that I use "offset = memstart_addr - start" is that it seems "memstart_addr" is always greater than "start" when we are booting a kdump kernel with a kernel option like "crashkernel=64M@80M". :-) > > Also, offset should be phys_addr_t -- even if you don't expect to > support offsets greater than 4G on 32-bit, it's semantically the right > type to use. Plus, "int" would break if this code were ever used with > 64-bit. I thought about using phy_addr_t for the "offset" originally but gave it up for the following reasons: * It will not be greater than 4G. * We have to use the ugly #ifdef CONFIG_PHYS_64BIT in restore_to_as0(). * Need more registers for arguments for restore_to_as0(). Of course you can change it to phys_addr_t if you prefer. Thanks, Kevin > > If you're OK with these changes, I can fix it while applying. > > -Scott > >
On Wed, 2014-01-08 at 10:42 +0800, Kevin Hao wrote: > On Tue, Jan 07, 2014 at 05:46:04PM -0600, Scott Wood wrote: > > On Sat, 2014-01-04 at 14:34 +0800, Kevin Hao wrote: > > > > I'm having a hard time following the logic here. What is PAGE_OFFSET - > > > > offset supposed to be? Why would we map anything belowe PAGE_OFFSET? > > > > > > No, we don't map the address below PAGE_OFFSET. > > > memstart_addr is the physical start address of RAM. > > > start is the kernel running physical address aligned with 64M. > > > > > > offset = memstart_addr - start > > > > > > So if memstart_addr < start, the offset is negative. The PAGE_OFFSET - offset > > > is the virtual start address we should use for the init 64M map. It's above > > > the PAGE_OFFSET instead of below. > > > > Oh. I think it'd be more readable to do "offset = start - > > memstart_addr" and add offset instead of subtracting it. > > Yes, I agree. The reason that I use "offset = memstart_addr - start" is that > it seems "memstart_addr" is always greater than "start" when we are booting > a kdump kernel with a kernel option like "crashkernel=64M@80M". :-) ...so there is a situation where you map below PAGE_OFFSET. :-) > > Also, offset should be phys_addr_t -- even if you don't expect to > > support offsets greater than 4G on 32-bit, it's semantically the right > > type to use. Plus, "int" would break if this code were ever used with > > 64-bit. > > I thought about using phy_addr_t for the "offset" originally but gave it up > for the following reasons: > * It will not be greater than 4G. > * We have to use the ugly #ifdef CONFIG_PHYS_64BIT in restore_to_as0(). > * Need more registers for arguments for restore_to_as0(). > > Of course you can change it to phys_addr_t if you prefer. I'd at least like to make it "long". -Scott
diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c index 048d716ae706..ce0c7d7db6c3 100644 --- a/arch/powerpc/mm/fsl_booke_mmu.c +++ b/arch/powerpc/mm/fsl_booke_mmu.c @@ -171,11 +171,10 @@ unsigned long calc_cam_sz(unsigned long ram, unsigned long virt, return 1UL << camsize; } -unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) +static unsigned long map_mem_in_cams_addr(phys_addr_t phys, unsigned long virt, + unsigned long ram, int max_cam_idx) { int i; - unsigned long virt = PAGE_OFFSET; - phys_addr_t phys = memstart_addr; unsigned long amount_mapped = 0; /* Calculate CAM values */ @@ -195,6 +194,14 @@ unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) return amount_mapped; } +unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx) +{ + unsigned long virt = PAGE_OFFSET; + phys_addr_t phys = memstart_addr; + + return map_mem_in_cams_addr(phys, virt, ram, max_cam_idx); +} + #ifdef CONFIG_PPC32 #if defined(CONFIG_LOWMEM_CAM_NUM_BOOL) && (CONFIG_LOWMEM_CAM_NUM >= NUM_TLBCAMS) @@ -289,7 +296,11 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start) is_second_reloc = 1; n = switch_to_as1(); /* map a 64M area for the second relocation */ - map_mem_in_cams(0x4000000UL, CONFIG_LOWMEM_CAM_NUM); + if (memstart_addr > start) + map_mem_in_cams(0x4000000, CONFIG_LOWMEM_CAM_NUM); + else + map_mem_in_cams_addr(start, PAGE_OFFSET - offset, + 0x4000000, CONFIG_LOWMEM_CAM_NUM); restore_to_as0(n, offset, __va(dt_ptr)); /* We should never reach here */ panic("Relocation error");