[PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com ---
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
worked as well for me - but it left the stack in danger.
arch/arm/lib/bootm.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 23b99a541c..f60ee3a7e6 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -43,7 +43,6 @@ DECLARE_GLOBAL_DATA_PTR;
static struct tag *params;
-#ifndef CONFIG_ARM64 static ulong get_sp(void) { ulong ret; @@ -87,7 +86,6 @@ void arch_lmb_reserve(struct lmb *lmb) break; } } -#endif
__weak void board_quiesce_devices(void) {
On 7/29/21 9:22 AM, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
So this is wrong and breaks a use case on rcar3, where there is more stuff which should not be reserved until the ram top.
I suspect the real fix here is to protect only the stack and tlb on arm64, not just everything from current stack bottom to ram top ?
On Thu, Jul 29, 2021 at 05:01:09PM +0200, Marek Vasut wrote:
On 7/29/21 9:22 AM, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
So this is wrong and breaks a use case on rcar3, where there is more stuff which should not be reserved until the ram top.
I suspect the real fix here is to protect only the stack and tlb on arm64, not just everything from current stack bottom to ram top ?
Wait, what? There's never been a U-Boot release that didn't reserve that area so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
On 7/29/21 5:23 PM, Tom Rini wrote:
On Thu, Jul 29, 2021 at 05:01:09PM +0200, Marek Vasut wrote:
On 7/29/21 9:22 AM, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
So this is wrong and breaks a use case on rcar3, where there is more stuff which should not be reserved until the ram top.
I suspect the real fix here is to protect only the stack and tlb on arm64, not just everything from current stack bottom to ram top ?
Wait, what? There's never been a U-Boot release that didn't reserve that area
That does not mean such a huge reservation is correct.
I apologize for not being overly clear in my answer, I already told Jan I am highly overloaded and simply lack the capacity to analyze this problem thoroughly on a short notice.
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
I didn't hit the tlb reservation issue Jan has in my tests on rcar3, so I suspect that is the real problem here, i.e. we need some more fine grained lmb reservation on arm64 ?
On Thu, Jul 29, 2021 at 06:47:02PM +0200, Marek Vasut wrote:
On 7/29/21 5:23 PM, Tom Rini wrote:
On Thu, Jul 29, 2021 at 05:01:09PM +0200, Marek Vasut wrote:
On 7/29/21 9:22 AM, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
So this is wrong and breaks a use case on rcar3, where there is more stuff which should not be reserved until the ram top.
I suspect the real fix here is to protect only the stack and tlb on arm64, not just everything from current stack bottom to ram top ?
Wait, what? There's never been a U-Boot release that didn't reserve that area
That does not mean such a huge reservation is correct.
I apologize for not being overly clear in my answer, I already told Jan I am highly overloaded and simply lack the capacity to analyze this problem thoroughly on a short notice.
I understand.
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so. This reservation is not at that spot, and a lot more than that. That commit is also only present in v2021.10-rc1 so it's not a great idea for another project to depend on it and can also revert their changes until someone is able to analyze the entire situation again.
I didn't hit the tlb reservation issue Jan has in my tests on rcar3, so I suspect that is the real problem here, i.e. we need some more fine grained lmb reservation on arm64 ?
Yes, some further analysis is needed here. But due to a lack of time I suspect the best answer for now is for rcar3 to add board_lmb_reserve() and lmb_free what's not needed there.
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
This reservation is not at that spot, and a lot more than that.
Can you please elaborate on this "lot more" part ? Because as much as I studied the reservation code, the "lot more" was ATAGS on arm32 and nothing on arm64.
That commit is also only present in v2021.10-rc1 so it's not a great idea for another project to depend on it and can also revert their changes until someone is able to analyze the entire situation again.
We are in rc1 so we still have enough time to analyze this problem, yes.
I didn't hit the tlb reservation issue Jan has in my tests on rcar3, so I suspect that is the real problem here, i.e. we need some more fine grained lmb reservation on arm64 ?
Yes, some further analysis is needed here. But due to a lack of time I suspect the best answer for now is for rcar3 to add board_lmb_reserve() and lmb_free what's not needed there.
So, no.
The question I would like answered is whether we even really have to reserve the TLB area with LMB at all. Linux will set up the TLB again, so I think we do not have to.
And I _think_ the problem is isolated to K3 due to the non-standard TLB and cache init it does in arch/arm/mach-k3/common.c spl_enable_dcache() in SPL. The code sets up TLB close to the end of DRAM and enables dcache early in SPL, and the end of DRAM is incidentally also where U-Boot is relocated shortly after and at that point U-Boot possibly overwrites or corrupts that TLB content. This hypothesis can be easily tested on K3, just comment out the content of spl_enable_dcache() and see if that fixes things.
It would also be interesting to know what the debug() print in spl_enable_dcache() prints on the K3 from Jan.
On 02.08.21 02:54, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
This reservation is not at that spot, and a lot more than that.
Can you please elaborate on this "lot more" part ? Because as much as I studied the reservation code, the "lot more" was ATAGS on arm32 and nothing on arm64.
See my commit log.
That commit is also only present in v2021.10-rc1 so it's not a great idea for another project to depend on it and can also revert their changes until someone is able to analyze the entire situation again.
We are in rc1 so we still have enough time to analyze this problem, yes.
I didn't hit the tlb reservation issue Jan has in my tests on rcar3, so I suspect that is the real problem here, i.e. we need some more fine grained lmb reservation on arm64 ?
Yes, some further analysis is needed here. But due to a lack of time I suspect the best answer for now is for rcar3 to add board_lmb_reserve() and lmb_free what's not needed there.
So, no.
The question I would like answered is whether we even really have to reserve the TLB area with LMB at all. Linux will set up the TLB again, so I think we do not have to.
See my commit log.
And I _think_ the problem is isolated to K3 due to the non-standard TLB and cache init it does in arch/arm/mach-k3/common.c spl_enable_dcache() in SPL. The code sets up TLB close to the end of DRAM and enables dcache early in SPL, and the end of DRAM is incidentally also where U-Boot is relocated shortly after and at that point U-Boot possibly overwrites or corrupts that TLB content. This hypothesis can be easily tested on K3, just comment out the content of spl_enable_dcache() and see if that fixes things.
This is not only TLB, see my commit log. Specifically the clash with the relocated U-Boot code causes the crash on the k3. But that location does not look like it is k3-specfic. Other boards are just lucky if the work.
It would also be interesting to know what the debug() print in spl_enable_dcache() prints on the K3 from Jan.
The information is already available, I shared bdinfo.
Jan
On 8/2/21 11:37 AM, Jan Kiszka wrote:
On 02.08.21 02:54, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
This reservation is not at that spot, and a lot more than that.
Can you please elaborate on this "lot more" part ? Because as much as I studied the reservation code, the "lot more" was ATAGS on arm32 and nothing on arm64.
See my commit log.
This is not particularly useful answer, considering the commit log says: "lot of crucial things", "Possibly more", "likely also on other boards" and other opaque statements. But really, the problem so far happens on one K3 board.
I am asking for specifics, what is it that should be reserved by the arch_lmb_reserve() ? I believe it is only ATAGS on arm32 and nothing on arm64.
That commit is also only present in v2021.10-rc1 so it's not a great idea for another project to depend on it and can also revert their changes until someone is able to analyze the entire situation again.
We are in rc1 so we still have enough time to analyze this problem, yes.
I didn't hit the tlb reservation issue Jan has in my tests on rcar3, so I suspect that is the real problem here, i.e. we need some more fine grained lmb reservation on arm64 ?
Yes, some further analysis is needed here. But due to a lack of time I suspect the best answer for now is for rcar3 to add board_lmb_reserve() and lmb_free what's not needed there.
So, no.
The question I would like answered is whether we even really have to reserve the TLB area with LMB at all. Linux will set up the TLB again, so I think we do not have to.
See my commit log.
The commit log does not explain why do you think the TLB should be reserved, sorry.
I explain above why I think it should not be reserved.
And I _think_ the problem is isolated to K3 due to the non-standard TLB and cache init it does in arch/arm/mach-k3/common.c spl_enable_dcache() in SPL. The code sets up TLB close to the end of DRAM and enables dcache early in SPL, and the end of DRAM is incidentally also where U-Boot is relocated shortly after and at that point U-Boot possibly overwrites or corrupts that TLB content. This hypothesis can be easily tested on K3, just comment out the content of spl_enable_dcache() and see if that fixes things.
This is not only TLB, see my commit log. Specifically the clash with the relocated U-Boot code causes the crash on the k3. But that location does not look like it is k3-specfic. Other boards are just lucky if the work.
Please have a look at arch/arm/mach-k3/common.c spl_enable_dcache() , that code sets up the TLB in a location which is overwritten by the relocated U-Boot. This is specific to K3 and no other platform does that.
It would also be interesting to know what the debug() print in spl_enable_dcache() prints on the K3 from Jan.
The information is already available, I shared bdinfo.
spl_enable_dcache() is called in SPL, it is not part of bdinfo.
On 02.08.21 12:48, Marek Vasut wrote:
On 8/2/21 11:37 AM, Jan Kiszka wrote:
On 02.08.21 02:54, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
This reservation is not at that spot, and a lot more than that.
Can you please elaborate on this "lot more" part ? Because as much as I studied the reservation code, the "lot more" was ATAGS on arm32 and nothing on arm64.
See my commit log.
This is not particularly useful answer, considering the commit log says: "lot of crucial things", "Possibly more", "likely also on other boards" and other opaque statements. But really, the problem so far happens on one K3 board.
"Such things are the page table (tlb_addr), relocated U-Boot and the active stack."
Jan
On 8/2/21 1:36 PM, Jan Kiszka wrote:
On 02.08.21 12:48, Marek Vasut wrote:
On 8/2/21 11:37 AM, Jan Kiszka wrote:
On 02.08.21 02:54, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
> so when did rcar3 introduce something there that shouldn't be > reserved? And you had phrased this to me on IRC as about reserving > spot > for ATAGS, and that not being needed of course on arm64. But that's > not > what's going on. Perhaps the answer is that rcar3 needs to > introduce a > board_lmb_reserve to free the normal arch one and provide whatever > more > narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
This reservation is not at that spot, and a lot more than that.
Can you please elaborate on this "lot more" part ? Because as much as I studied the reservation code, the "lot more" was ATAGS on arm32 and nothing on arm64.
See my commit log.
This is not particularly useful answer, considering the commit log says: "lot of crucial things", "Possibly more", "likely also on other boards" and other opaque statements. But really, the problem so far happens on one K3 board.
"Such things are the page table (tlb_addr), relocated U-Boot and the active stack."
Please read the rest of my answer, I don't believe the TLB should be reserved at all. DTTO for the stack. If you think otherwise, please explain why.
On 02.08.21 13:38, Marek Vasut wrote:
On 8/2/21 1:36 PM, Jan Kiszka wrote:
On 02.08.21 12:48, Marek Vasut wrote:
On 8/2/21 11:37 AM, Jan Kiszka wrote:
On 02.08.21 02:54, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
>> so when did rcar3 introduce something there that shouldn't be >> reserved? And you had phrased this to me on IRC as about reserving >> spot >> for ATAGS, and that not being needed of course on arm64. But >> that's >> not >> what's going on. Perhaps the answer is that rcar3 needs to >> introduce a >> board_lmb_reserve to free the normal arch one and provide whatever >> more >> narrow scope it needs. > > Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB > reservation for command line and board info on arm64") , this is > about ATAGS > and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
This reservation is not at that spot, and a lot more than that.
Can you please elaborate on this "lot more" part ? Because as much as I studied the reservation code, the "lot more" was ATAGS on arm32 and nothing on arm64.
See my commit log.
This is not particularly useful answer, considering the commit log says: "lot of crucial things", "Possibly more", "likely also on other boards" and other opaque statements. But really, the problem so far happens on one K3 board.
"Such things are the page table (tlb_addr), relocated U-Boot and the active stack."
Please read the rest of my answer, I don't believe the TLB should be reserved at all. DTTO for the stack. If you think otherwise, please explain why.
Marek, I've provided you with three generic examples of active memory blocks that are relevant while U-Boot is allocating from and also filling that LMB. Please follow those cases and explain to us why they aren't active - or at least prove why they are specific the k3 (for which I found no traces).
And stop following the TLB topic for now. That was only my first guess. The actual crash I'm seeing on my board come from plain code overwriting. It could have been TLB as well. It could also have been the stack. All those become unprotected via your reservation removal.
Jan
On Mon, Aug 02, 2021 at 01:54:57PM +0200, Jan Kiszka wrote:
On 02.08.21 13:38, Marek Vasut wrote:
On 8/2/21 1:36 PM, Jan Kiszka wrote:
On 02.08.21 12:48, Marek Vasut wrote:
On 8/2/21 11:37 AM, Jan Kiszka wrote:
On 02.08.21 02:54, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
>>> so when did rcar3 introduce something there that shouldn't be >>> reserved? And you had phrased this to me on IRC as about reserving >>> spot >>> for ATAGS, and that not being needed of course on arm64. But >>> that's >>> not >>> what's going on. Perhaps the answer is that rcar3 needs to >>> introduce a >>> board_lmb_reserve to free the normal arch one and provide whatever >>> more >>> narrow scope it needs. >> >> Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB >> reservation for command line and board info on arm64") , this is >> about ATAGS >> and we really don't need to reserve those on arm64. > > Commit 2359fa7a878 disables the entire arch_lmb_reserve function on > aarch64, yes. I assumed when we had talked that it was a small area > being set aside and perhaps mis-recalled that ATAGS tended to live at > DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
> This reservation is not at that spot, and a lot > more than that.
Can you please elaborate on this "lot more" part ? Because as much as I studied the reservation code, the "lot more" was ATAGS on arm32 and nothing on arm64.
See my commit log.
This is not particularly useful answer, considering the commit log says: "lot of crucial things", "Possibly more", "likely also on other boards" and other opaque statements. But really, the problem so far happens on one K3 board.
"Such things are the page table (tlb_addr), relocated U-Boot and the active stack."
Please read the rest of my answer, I don't believe the TLB should be reserved at all. DTTO for the stack. If you think otherwise, please explain why.
Marek, I've provided you with three generic examples of active memory blocks that are relevant while U-Boot is allocating from and also filling that LMB. Please follow those cases and explain to us why they aren't active - or at least prove why they are specific the k3 (for which I found no traces).
And stop following the TLB topic for now. That was only my first guess. The actual crash I'm seeing on my board come from plain code overwriting. It could have been TLB as well. It could also have been the stack. All those become unprotected via your reservation removal.
Jan, one thing I didn't see before is, are you also using include/configs/ti_armv7_common.h in the end, like the K3 reference platforms, and if not are you setting bootm_size in your environment? I have one more idea on why this fails on your board but not Marek's. Thanks.
On 02.08.21 15:04, Tom Rini wrote:
On Mon, Aug 02, 2021 at 01:54:57PM +0200, Jan Kiszka wrote:
On 02.08.21 13:38, Marek Vasut wrote:
On 8/2/21 1:36 PM, Jan Kiszka wrote:
On 02.08.21 12:48, Marek Vasut wrote:
On 8/2/21 11:37 AM, Jan Kiszka wrote:
On 02.08.21 02:54, Marek Vasut wrote: > On 7/29/21 6:58 PM, Tom Rini wrote: > > [...] > >>>> so when did rcar3 introduce something there that shouldn't be >>>> reserved? And you had phrased this to me on IRC as about reserving >>>> spot >>>> for ATAGS, and that not being needed of course on arm64. But >>>> that's >>>> not >>>> what's going on. Perhaps the answer is that rcar3 needs to >>>> introduce a >>>> board_lmb_reserve to free the normal arch one and provide whatever >>>> more >>>> narrow scope it needs. >>> >>> Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB >>> reservation for command line and board info on arm64") , this is >>> about ATAGS >>> and we really don't need to reserve those on arm64. >> >> Commit 2359fa7a878 disables the entire arch_lmb_reserve function on >> aarch64, yes. I assumed when we had talked that it was a small area >> being set aside and perhaps mis-recalled that ATAGS tended to live at >> DDR_BASE + 0x800 or so. > > That arch_lmb_reserve() is responsible for reserving architecture > specific memory. On arm32 it is ATAGS, on arm64 it is nothing as > far as > I can tell (and see below regarding the TLB). > >> This reservation is not at that spot, and a lot >> more than that. > > Can you please elaborate on this "lot more" part ? Because as much > as I > studied the reservation code, the "lot more" was ATAGS on arm32 and > nothing on arm64.
See my commit log.
This is not particularly useful answer, considering the commit log says: "lot of crucial things", "Possibly more", "likely also on other boards" and other opaque statements. But really, the problem so far happens on one K3 board.
"Such things are the page table (tlb_addr), relocated U-Boot and the active stack."
Please read the rest of my answer, I don't believe the TLB should be reserved at all. DTTO for the stack. If you think otherwise, please explain why.
Marek, I've provided you with three generic examples of active memory blocks that are relevant while U-Boot is allocating from and also filling that LMB. Please follow those cases and explain to us why they aren't active - or at least prove why they are specific the k3 (for which I found no traces).
And stop following the TLB topic for now. That was only my first guess. The actual crash I'm seeing on my board come from plain code overwriting. It could have been TLB as well. It could also have been the stack. All those become unprotected via your reservation removal.
Jan, one thing I didn't see before is, are you also using include/configs/ti_armv7_common.h in the end, like the K3 reference platforms, and if not are you setting bootm_size in your environment? I have one more idea on why this fails on your board but not Marek's. Thanks.
We are including that header but we didn't use DEFAULT_LINUX_BOOT_ENV, in fact. That left bootm_size undefined. Can you explain the impact?
Jan
On Mon, Aug 02, 2021 at 04:03:01PM +0200, Jan Kiszka wrote:
On 02.08.21 15:04, Tom Rini wrote:
On Mon, Aug 02, 2021 at 01:54:57PM +0200, Jan Kiszka wrote:
On 02.08.21 13:38, Marek Vasut wrote:
On 8/2/21 1:36 PM, Jan Kiszka wrote:
On 02.08.21 12:48, Marek Vasut wrote:
On 8/2/21 11:37 AM, Jan Kiszka wrote: > On 02.08.21 02:54, Marek Vasut wrote: >> On 7/29/21 6:58 PM, Tom Rini wrote: >> >> [...] >> >>>>> so when did rcar3 introduce something there that shouldn't be >>>>> reserved? And you had phrased this to me on IRC as about reserving >>>>> spot >>>>> for ATAGS, and that not being needed of course on arm64. But >>>>> that's >>>>> not >>>>> what's going on. Perhaps the answer is that rcar3 needs to >>>>> introduce a >>>>> board_lmb_reserve to free the normal arch one and provide whatever >>>>> more >>>>> narrow scope it needs. >>>> >>>> Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB >>>> reservation for command line and board info on arm64") , this is >>>> about ATAGS >>>> and we really don't need to reserve those on arm64. >>> >>> Commit 2359fa7a878 disables the entire arch_lmb_reserve function on >>> aarch64, yes. I assumed when we had talked that it was a small area >>> being set aside and perhaps mis-recalled that ATAGS tended to live at >>> DDR_BASE + 0x800 or so. >> >> That arch_lmb_reserve() is responsible for reserving architecture >> specific memory. On arm32 it is ATAGS, on arm64 it is nothing as >> far as >> I can tell (and see below regarding the TLB). >> >>> This reservation is not at that spot, and a lot >>> more than that. >> >> Can you please elaborate on this "lot more" part ? Because as much >> as I >> studied the reservation code, the "lot more" was ATAGS on arm32 and >> nothing on arm64. > > See my commit log.
This is not particularly useful answer, considering the commit log says: "lot of crucial things", "Possibly more", "likely also on other boards" and other opaque statements. But really, the problem so far happens on one K3 board.
"Such things are the page table (tlb_addr), relocated U-Boot and the active stack."
Please read the rest of my answer, I don't believe the TLB should be reserved at all. DTTO for the stack. If you think otherwise, please explain why.
Marek, I've provided you with three generic examples of active memory blocks that are relevant while U-Boot is allocating from and also filling that LMB. Please follow those cases and explain to us why they aren't active - or at least prove why they are specific the k3 (for which I found no traces).
And stop following the TLB topic for now. That was only my first guess. The actual crash I'm seeing on my board come from plain code overwriting. It could have been TLB as well. It could also have been the stack. All those become unprotected via your reservation removal.
Jan, one thing I didn't see before is, are you also using include/configs/ti_armv7_common.h in the end, like the K3 reference platforms, and if not are you setting bootm_size in your environment? I have one more idea on why this fails on your board but not Marek's. Thanks.
We are including that header but we didn't use DEFAULT_LINUX_BOOT_ENV, in fact. That left bootm_size undefined. Can you explain the impact?
I suspect the answer here is that Marek does not see this problem because on R-Car bootm_size is set to 0x10000000 and so no relocation of the device tree / kernel / initrd happens to overwrite the running U-Boot and blow everything up. If you don't revert this, and do set bootm_size does everything work? Marek, if you unset bootm_size, do you see failure? Thanks!
On 02.08.21 16:27, Tom Rini wrote:
On Mon, Aug 02, 2021 at 04:03:01PM +0200, Jan Kiszka wrote:
On 02.08.21 15:04, Tom Rini wrote:
On Mon, Aug 02, 2021 at 01:54:57PM +0200, Jan Kiszka wrote:
On 02.08.21 13:38, Marek Vasut wrote:
On 8/2/21 1:36 PM, Jan Kiszka wrote:
On 02.08.21 12:48, Marek Vasut wrote: > On 8/2/21 11:37 AM, Jan Kiszka wrote: >> On 02.08.21 02:54, Marek Vasut wrote: >>> On 7/29/21 6:58 PM, Tom Rini wrote: >>> >>> [...] >>> >>>>>> so when did rcar3 introduce something there that shouldn't be >>>>>> reserved? And you had phrased this to me on IRC as about reserving >>>>>> spot >>>>>> for ATAGS, and that not being needed of course on arm64. But >>>>>> that's >>>>>> not >>>>>> what's going on. Perhaps the answer is that rcar3 needs to >>>>>> introduce a >>>>>> board_lmb_reserve to free the normal arch one and provide whatever >>>>>> more >>>>>> narrow scope it needs. >>>>> >>>>> Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB >>>>> reservation for command line and board info on arm64") , this is >>>>> about ATAGS >>>>> and we really don't need to reserve those on arm64. >>>> >>>> Commit 2359fa7a878 disables the entire arch_lmb_reserve function on >>>> aarch64, yes. I assumed when we had talked that it was a small area >>>> being set aside and perhaps mis-recalled that ATAGS tended to live at >>>> DDR_BASE + 0x800 or so. >>> >>> That arch_lmb_reserve() is responsible for reserving architecture >>> specific memory. On arm32 it is ATAGS, on arm64 it is nothing as >>> far as >>> I can tell (and see below regarding the TLB). >>> >>>> This reservation is not at that spot, and a lot >>>> more than that. >>> >>> Can you please elaborate on this "lot more" part ? Because as much >>> as I >>> studied the reservation code, the "lot more" was ATAGS on arm32 and >>> nothing on arm64. >> >> See my commit log. > > This is not particularly useful answer, considering the commit log says: > "lot of crucial things", "Possibly more", "likely also on other boards" > and other opaque statements. But really, the problem so far happens on > one K3 board.
"Such things are the page table (tlb_addr), relocated U-Boot and the active stack."
Please read the rest of my answer, I don't believe the TLB should be reserved at all. DTTO for the stack. If you think otherwise, please explain why.
Marek, I've provided you with three generic examples of active memory blocks that are relevant while U-Boot is allocating from and also filling that LMB. Please follow those cases and explain to us why they aren't active - or at least prove why they are specific the k3 (for which I found no traces).
And stop following the TLB topic for now. That was only my first guess. The actual crash I'm seeing on my board come from plain code overwriting. It could have been TLB as well. It could also have been the stack. All those become unprotected via your reservation removal.
Jan, one thing I didn't see before is, are you also using include/configs/ti_armv7_common.h in the end, like the K3 reference platforms, and if not are you setting bootm_size in your environment? I have one more idea on why this fails on your board but not Marek's. Thanks.
We are including that header but we didn't use DEFAULT_LINUX_BOOT_ENV, in fact. That left bootm_size undefined. Can you explain the impact?
I suspect the answer here is that Marek does not see this problem because on R-Car bootm_size is set to 0x10000000 and so no relocation of the device tree / kernel / initrd happens to overwrite the running U-Boot and blow everything up. If you don't revert this, and do set bootm_size does everything work? Marek, if you unset bootm_size, do you see failure? Thanks!
I currently do not see the error, even with unset bootm_size and Marek's patch back in. But fdt indeed moves down when adopting those settings. That makes sense for us anyway, I think our custom env values are rather for historic reasons, and one had an issue anyway (incorrect kernel alignment).
But at least we understand why I was able to see this, sometimes.
Jan
On Mon, Aug 02, 2021 at 04:34:29PM +0200, Jan Kiszka wrote:
On 02.08.21 16:27, Tom Rini wrote:
On Mon, Aug 02, 2021 at 04:03:01PM +0200, Jan Kiszka wrote:
On 02.08.21 15:04, Tom Rini wrote:
On Mon, Aug 02, 2021 at 01:54:57PM +0200, Jan Kiszka wrote:
On 02.08.21 13:38, Marek Vasut wrote:
On 8/2/21 1:36 PM, Jan Kiszka wrote: > On 02.08.21 12:48, Marek Vasut wrote: >> On 8/2/21 11:37 AM, Jan Kiszka wrote: >>> On 02.08.21 02:54, Marek Vasut wrote: >>>> On 7/29/21 6:58 PM, Tom Rini wrote: >>>> >>>> [...] >>>> >>>>>>> so when did rcar3 introduce something there that shouldn't be >>>>>>> reserved? And you had phrased this to me on IRC as about reserving >>>>>>> spot >>>>>>> for ATAGS, and that not being needed of course on arm64. But >>>>>>> that's >>>>>>> not >>>>>>> what's going on. Perhaps the answer is that rcar3 needs to >>>>>>> introduce a >>>>>>> board_lmb_reserve to free the normal arch one and provide whatever >>>>>>> more >>>>>>> narrow scope it needs. >>>>>> >>>>>> Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB >>>>>> reservation for command line and board info on arm64") , this is >>>>>> about ATAGS >>>>>> and we really don't need to reserve those on arm64. >>>>> >>>>> Commit 2359fa7a878 disables the entire arch_lmb_reserve function on >>>>> aarch64, yes. I assumed when we had talked that it was a small area >>>>> being set aside and perhaps mis-recalled that ATAGS tended to live at >>>>> DDR_BASE + 0x800 or so. >>>> >>>> That arch_lmb_reserve() is responsible for reserving architecture >>>> specific memory. On arm32 it is ATAGS, on arm64 it is nothing as >>>> far as >>>> I can tell (and see below regarding the TLB). >>>> >>>>> This reservation is not at that spot, and a lot >>>>> more than that. >>>> >>>> Can you please elaborate on this "lot more" part ? Because as much >>>> as I >>>> studied the reservation code, the "lot more" was ATAGS on arm32 and >>>> nothing on arm64. >>> >>> See my commit log. >> >> This is not particularly useful answer, considering the commit log says: >> "lot of crucial things", "Possibly more", "likely also on other boards" >> and other opaque statements. But really, the problem so far happens on >> one K3 board. > > "Such things are the page table (tlb_addr), > relocated U-Boot and the active stack."
Please read the rest of my answer, I don't believe the TLB should be reserved at all. DTTO for the stack. If you think otherwise, please explain why.
Marek, I've provided you with three generic examples of active memory blocks that are relevant while U-Boot is allocating from and also filling that LMB. Please follow those cases and explain to us why they aren't active - or at least prove why they are specific the k3 (for which I found no traces).
And stop following the TLB topic for now. That was only my first guess. The actual crash I'm seeing on my board come from plain code overwriting. It could have been TLB as well. It could also have been the stack. All those become unprotected via your reservation removal.
Jan, one thing I didn't see before is, are you also using include/configs/ti_armv7_common.h in the end, like the K3 reference platforms, and if not are you setting bootm_size in your environment? I have one more idea on why this fails on your board but not Marek's. Thanks.
We are including that header but we didn't use DEFAULT_LINUX_BOOT_ENV, in fact. That left bootm_size undefined. Can you explain the impact?
I suspect the answer here is that Marek does not see this problem because on R-Car bootm_size is set to 0x10000000 and so no relocation of the device tree / kernel / initrd happens to overwrite the running U-Boot and blow everything up. If you don't revert this, and do set bootm_size does everything work? Marek, if you unset bootm_size, do you see failure? Thanks!
I currently do not see the error, even with unset bootm_size and Marek's patch back in. But fdt indeed moves down when adopting those settings. That makes sense for us anyway, I think our custom env values are rather for historic reasons, and one had an issue anyway (incorrect kernel alignment).
But at least we understand why I was able to see this, sometimes.
OK, thanks. Note that I'm not sure how I want to move forward here because a very frequent user/developer problem is "device tree relocated, everything crashed, why? oh, I'll just disable it (and lead to another problem down the line)".
On 8/2/21 4:44 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 04:34:29PM +0200, Jan Kiszka wrote:
On 02.08.21 16:27, Tom Rini wrote:
On Mon, Aug 02, 2021 at 04:03:01PM +0200, Jan Kiszka wrote:
On 02.08.21 15:04, Tom Rini wrote:
On Mon, Aug 02, 2021 at 01:54:57PM +0200, Jan Kiszka wrote:
On 02.08.21 13:38, Marek Vasut wrote: > On 8/2/21 1:36 PM, Jan Kiszka wrote: >> On 02.08.21 12:48, Marek Vasut wrote: >>> On 8/2/21 11:37 AM, Jan Kiszka wrote: >>>> On 02.08.21 02:54, Marek Vasut wrote: >>>>> On 7/29/21 6:58 PM, Tom Rini wrote: >>>>> >>>>> [...] >>>>> >>>>>>>> so when did rcar3 introduce something there that shouldn't be >>>>>>>> reserved? And you had phrased this to me on IRC as about reserving >>>>>>>> spot >>>>>>>> for ATAGS, and that not being needed of course on arm64. But >>>>>>>> that's >>>>>>>> not >>>>>>>> what's going on. Perhaps the answer is that rcar3 needs to >>>>>>>> introduce a >>>>>>>> board_lmb_reserve to free the normal arch one and provide whatever >>>>>>>> more >>>>>>>> narrow scope it needs. >>>>>>> >>>>>>> Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB >>>>>>> reservation for command line and board info on arm64") , this is >>>>>>> about ATAGS >>>>>>> and we really don't need to reserve those on arm64. >>>>>> >>>>>> Commit 2359fa7a878 disables the entire arch_lmb_reserve function on >>>>>> aarch64, yes. I assumed when we had talked that it was a small area >>>>>> being set aside and perhaps mis-recalled that ATAGS tended to live at >>>>>> DDR_BASE + 0x800 or so. >>>>> >>>>> That arch_lmb_reserve() is responsible for reserving architecture >>>>> specific memory. On arm32 it is ATAGS, on arm64 it is nothing as >>>>> far as >>>>> I can tell (and see below regarding the TLB). >>>>> >>>>>> This reservation is not at that spot, and a lot >>>>>> more than that. >>>>> >>>>> Can you please elaborate on this "lot more" part ? Because as much >>>>> as I >>>>> studied the reservation code, the "lot more" was ATAGS on arm32 and >>>>> nothing on arm64. >>>> >>>> See my commit log. >>> >>> This is not particularly useful answer, considering the commit log says: >>> "lot of crucial things", "Possibly more", "likely also on other boards" >>> and other opaque statements. But really, the problem so far happens on >>> one K3 board. >> >> "Such things are the page table (tlb_addr), >> relocated U-Boot and the active stack." > > Please read the rest of my answer, I don't believe the TLB should be > reserved at all. DTTO for the stack. If you think otherwise, please > explain why.
Marek, I've provided you with three generic examples of active memory blocks that are relevant while U-Boot is allocating from and also filling that LMB. Please follow those cases and explain to us why they aren't active - or at least prove why they are specific the k3 (for which I found no traces).
And stop following the TLB topic for now. That was only my first guess. The actual crash I'm seeing on my board come from plain code overwriting. It could have been TLB as well. It could also have been the stack. All those become unprotected via your reservation removal.
Jan, one thing I didn't see before is, are you also using include/configs/ti_armv7_common.h in the end, like the K3 reference platforms, and if not are you setting bootm_size in your environment? I have one more idea on why this fails on your board but not Marek's. Thanks.
We are including that header but we didn't use DEFAULT_LINUX_BOOT_ENV, in fact. That left bootm_size undefined. Can you explain the impact?
I suspect the answer here is that Marek does not see this problem because on R-Car bootm_size is set to 0x10000000 and so no relocation of the device tree / kernel / initrd happens to overwrite the running U-Boot and blow everything up. If you don't revert this, and do set bootm_size does everything work? Marek, if you unset bootm_size, do you see failure? Thanks!
I currently do not see the error, even with unset bootm_size and Marek's patch back in. But fdt indeed moves down when adopting those settings. That makes sense for us anyway, I think our custom env values are rather for historic reasons, and one had an issue anyway (incorrect kernel alignment).
But at least we understand why I was able to see this, sometimes.
OK, thanks. Note that I'm not sure how I want to move forward here because a very frequent user/developer problem is "device tree relocated, everything crashed, why? oh, I'll just disable it (and lead to another problem down the line)".
In rcar with bootm_size unset it looks like this:
=> bdinfo boot_params = 0x000000007beee240 DRAM bank = 0x0000000000000000 -> start = 0x0000000048000000 -> size = 0x0000000038000000 DRAM bank = 0x0000000000000001 -> start = 0x0000000500000000 -> size = 0x0000000040000000 DRAM bank = 0x0000000000000002 -> start = 0x0000000600000000 -> size = 0x0000000040000000 DRAM bank = 0x0000000000000003 -> start = 0x0000000700000000 -> size = 0x0000000040000000 flashstart = 0x0000000008000000 flashsize = 0x0000000004000000 flashoffset = 0x00000000000f5890 baudrate = 115200 bps relocaddr = 0x000000007fee8000 reloc off = 0x000000007fee8000 Build = 64-bit current eth = ethernet@e6800000 ... fdt_blob = 0x000000007beda0e0 new_fdt = 0x000000007beda0e0 fdt_size = 0x000000000000dcc0 multi_dtb_fit= 0x0000000049000000 lmb_dump_all: memory.cnt = 0x4 memory[0] [0x48000000-0x7fffffff], 0x38000000 bytes flags: 0 memory[1] [0x500000000-0x53fffffff], 0x40000000 bytes flags: 0 memory[2] [0x600000000-0x63fffffff], 0x40000000 bytes flags: 0 memory[3] [0x700000000-0x73fffffff], 0x40000000 bytes flags: 0 reserved.cnt = 0x1 reserved[0] [0x44100000-0x47efffff], 0x03e00000 bytes flags: 4 arch_number = 0x0000000000000000 TLB addr = 0x000000007fff0000 irq_sp = 0x000000007beda0d0 sp start = 0x000000007beda0d0 Early malloc usage: 1318 / 8000
...
## Loading kernel from FIT Image at 58000000 ... Using 'conf-1' configuration Trying 'kernel-1' kernel subimage Description: Linux kernel (Sat Jun 5 00:24:15 CEST 2021) Type: Kernel Image Compression: uncompressed Data Start: 0x58000154 Data Size: 16662536 Bytes = 15.9 MiB Architecture: AArch64 OS: Linux Load Address: 0x50200000 Entry Point: 0x50200000 Hash algo: crc32 Hash value: 0655cd1f Verifying Hash Integrity ... crc32+ OK ## Loading fdt from FIT Image at 58000000 ... Using 'conf-1' configuration Trying 'fdt-1' fdt subimage Description: Flattened Device Tree blob (Sat Jun 5 00:24:15 CEST 2021) Type: Flat Device Tree Compression: uncompressed Data Start: 0x58fe42a4 Data Size: 74686 Bytes = 72.9 KiB Architecture: AArch64 Hash algo: crc32 Hash value: 287b2438 Verifying Hash Integrity ... crc32+ OK Booting using the fdt blob at 0x58fe42a4 Loading Kernel Image Loading Device Tree to 000000007ffea000, end 000000007ffff3bd ... OK
Starting kernel ...
[ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x411fd073]
On Thu, Aug 05, 2021 at 11:52:05PM +0200, Marek Vasut wrote:
On 8/2/21 4:44 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 04:34:29PM +0200, Jan Kiszka wrote:
On 02.08.21 16:27, Tom Rini wrote:
On Mon, Aug 02, 2021 at 04:03:01PM +0200, Jan Kiszka wrote:
On 02.08.21 15:04, Tom Rini wrote:
On Mon, Aug 02, 2021 at 01:54:57PM +0200, Jan Kiszka wrote: > On 02.08.21 13:38, Marek Vasut wrote: > > On 8/2/21 1:36 PM, Jan Kiszka wrote: > > > On 02.08.21 12:48, Marek Vasut wrote: > > > > On 8/2/21 11:37 AM, Jan Kiszka wrote: > > > > > On 02.08.21 02:54, Marek Vasut wrote: > > > > > > On 7/29/21 6:58 PM, Tom Rini wrote: > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > so when did rcar3 introduce something there that shouldn't be > > > > > > > > > reserved? And you had phrased this to me on IRC as about reserving > > > > > > > > > spot > > > > > > > > > for ATAGS, and that not being needed of course on arm64. But > > > > > > > > > that's > > > > > > > > > not > > > > > > > > > what's going on. Perhaps the answer is that rcar3 needs to > > > > > > > > > introduce a > > > > > > > > > board_lmb_reserve to free the normal arch one and provide whatever > > > > > > > > > more > > > > > > > > > narrow scope it needs. > > > > > > > > > > > > > > > > Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB > > > > > > > > reservation for command line and board info on arm64") , this is > > > > > > > > about ATAGS > > > > > > > > and we really don't need to reserve those on arm64. > > > > > > > > > > > > > > Commit 2359fa7a878 disables the entire arch_lmb_reserve function on > > > > > > > aarch64, yes. I assumed when we had talked that it was a small area > > > > > > > being set aside and perhaps mis-recalled that ATAGS tended to live at > > > > > > > DDR_BASE + 0x800 or so. > > > > > > > > > > > > That arch_lmb_reserve() is responsible for reserving architecture > > > > > > specific memory. On arm32 it is ATAGS, on arm64 it is nothing as > > > > > > far as > > > > > > I can tell (and see below regarding the TLB). > > > > > > > > > > > > > This reservation is not at that spot, and a lot > > > > > > > more than that. > > > > > > > > > > > > Can you please elaborate on this "lot more" part ? Because as much > > > > > > as I > > > > > > studied the reservation code, the "lot more" was ATAGS on arm32 and > > > > > > nothing on arm64. > > > > > > > > > > See my commit log. > > > > > > > > This is not particularly useful answer, considering the commit log says: > > > > "lot of crucial things", "Possibly more", "likely also on other boards" > > > > and other opaque statements. But really, the problem so far happens on > > > > one K3 board. > > > > > > "Such things are the page table (tlb_addr), > > > relocated U-Boot and the active stack." > > > > Please read the rest of my answer, I don't believe the TLB should be > > reserved at all. DTTO for the stack. If you think otherwise, please > > explain why. > > Marek, I've provided you with three generic examples of active memory > blocks that are relevant while U-Boot is allocating from and also > filling that LMB. Please follow those cases and explain to us why they > aren't active - or at least prove why they are specific the k3 (for > which I found no traces). > > And stop following the TLB topic for now. That was only my first guess. > The actual crash I'm seeing on my board come from plain code > overwriting. It could have been TLB as well. It could also have been the > stack. All those become unprotected via your reservation removal.
Jan, one thing I didn't see before is, are you also using include/configs/ti_armv7_common.h in the end, like the K3 reference platforms, and if not are you setting bootm_size in your environment? I have one more idea on why this fails on your board but not Marek's. Thanks.
We are including that header but we didn't use DEFAULT_LINUX_BOOT_ENV, in fact. That left bootm_size undefined. Can you explain the impact?
I suspect the answer here is that Marek does not see this problem because on R-Car bootm_size is set to 0x10000000 and so no relocation of the device tree / kernel / initrd happens to overwrite the running U-Boot and blow everything up. If you don't revert this, and do set bootm_size does everything work? Marek, if you unset bootm_size, do you see failure? Thanks!
I currently do not see the error, even with unset bootm_size and Marek's patch back in. But fdt indeed moves down when adopting those settings. That makes sense for us anyway, I think our custom env values are rather for historic reasons, and one had an issue anyway (incorrect kernel alignment).
But at least we understand why I was able to see this, sometimes.
OK, thanks. Note that I'm not sure how I want to move forward here because a very frequent user/developer problem is "device tree relocated, everything crashed, why? oh, I'll just disable it (and lead to another problem down the line)".
In rcar with bootm_size unset it looks like this:
=> bdinfo boot_params = 0x000000007beee240 DRAM bank = 0x0000000000000000 -> start = 0x0000000048000000 -> size = 0x0000000038000000 DRAM bank = 0x0000000000000001 -> start = 0x0000000500000000 -> size = 0x0000000040000000 DRAM bank = 0x0000000000000002 -> start = 0x0000000600000000 -> size = 0x0000000040000000 DRAM bank = 0x0000000000000003 -> start = 0x0000000700000000 -> size = 0x0000000040000000 flashstart = 0x0000000008000000 flashsize = 0x0000000004000000 flashoffset = 0x00000000000f5890 baudrate = 115200 bps relocaddr = 0x000000007fee8000 reloc off = 0x000000007fee8000 Build = 64-bit current eth = ethernet@e6800000 ... fdt_blob = 0x000000007beda0e0 new_fdt = 0x000000007beda0e0 fdt_size = 0x000000000000dcc0 multi_dtb_fit= 0x0000000049000000 lmb_dump_all: memory.cnt = 0x4 memory[0] [0x48000000-0x7fffffff], 0x38000000 bytes flags: 0 memory[1] [0x500000000-0x53fffffff], 0x40000000 bytes flags: 0 memory[2] [0x600000000-0x63fffffff], 0x40000000 bytes flags: 0 memory[3] [0x700000000-0x73fffffff], 0x40000000 bytes flags: 0 reserved.cnt = 0x1 reserved[0] [0x44100000-0x47efffff], 0x03e00000 bytes flags: 4 arch_number = 0x0000000000000000 TLB addr = 0x000000007fff0000 irq_sp = 0x000000007beda0d0 sp start = 0x000000007beda0d0 Early malloc usage: 1318 / 8000
...
## Loading kernel from FIT Image at 58000000 ... Using 'conf-1' configuration Trying 'kernel-1' kernel subimage Description: Linux kernel (Sat Jun 5 00:24:15 CEST 2021) Type: Kernel Image Compression: uncompressed Data Start: 0x58000154 Data Size: 16662536 Bytes = 15.9 MiB Architecture: AArch64 OS: Linux Load Address: 0x50200000 Entry Point: 0x50200000 Hash algo: crc32 Hash value: 0655cd1f Verifying Hash Integrity ... crc32+ OK ## Loading fdt from FIT Image at 58000000 ... Using 'conf-1' configuration Trying 'fdt-1' fdt subimage Description: Flattened Device Tree blob (Sat Jun 5 00:24:15 CEST 2021) Type: Flat Device Tree Compression: uncompressed Data Start: 0x58fe42a4 Data Size: 74686 Bytes = 72.9 KiB Architecture: AArch64 Hash algo: crc32 Hash value: 287b2438 Verifying Hash Integrity ... crc32+ OK Booting using the fdt blob at 0x58fe42a4 Loading Kernel Image Loading Device Tree to 000000007ffea000, end 000000007ffff3bd ... OK
OK, I think we can say it's likely that in your case we're relocating the start of the device tree just a bit past where U-Boot is running. A bit of quick math says there's around 1MiB between relocaddr for U-Boot and startof the device tree relocation address.
On Mon, Aug 02, 2021 at 02:54:22AM +0200, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
I don't think that LMB ever covered ATAGS. ATAGS, I could have sworn, were at start of memory + 0x800 or so. This LMB is for the top of memory where U-Boot is.
On 8/2/21 3:00 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 02:54:22AM +0200, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
I don't think that LMB ever covered ATAGS. ATAGS, I could have sworn, were at start of memory + 0x800 or so. This LMB is for the top of memory where U-Boot is.
What is there to protect which Linux does not set up again ?
On Thu, Aug 05, 2021 at 11:53:24PM +0200, Marek Vasut wrote:
On 8/2/21 3:00 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 02:54:22AM +0200, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
so when did rcar3 introduce something there that shouldn't be reserved? And you had phrased this to me on IRC as about reserving spot for ATAGS, and that not being needed of course on arm64. But that's not what's going on. Perhaps the answer is that rcar3 needs to introduce a board_lmb_reserve to free the normal arch one and provide whatever more narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
I don't think that LMB ever covered ATAGS. ATAGS, I could have sworn, were at start of memory + 0x800 or so. This LMB is for the top of memory where U-Boot is.
What is there to protect which Linux does not set up again ?
What does Linux have to do this with? I don't see, but maybe have missed, where "U-Boot adds memory region to internal LMB list" is translated to "Update the device tree Linux will have with these regions". I do see code to read the reserved-memory regions from a DTB and add them to our LMB, but not the other direction.
On 8/6/21 1:31 AM, Tom Rini wrote:
On Thu, Aug 05, 2021 at 11:53:24PM +0200, Marek Vasut wrote:
On 8/2/21 3:00 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 02:54:22AM +0200, Marek Vasut wrote:
On 7/29/21 6:58 PM, Tom Rini wrote:
[...]
> so when did rcar3 introduce something there that shouldn't be > reserved? And you had phrased this to me on IRC as about reserving spot > for ATAGS, and that not being needed of course on arm64. But that's not > what's going on. Perhaps the answer is that rcar3 needs to introduce a > board_lmb_reserve to free the normal arch one and provide whatever more > narrow scope it needs.
Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB reservation for command line and board info on arm64") , this is about ATAGS and we really don't need to reserve those on arm64.
Commit 2359fa7a878 disables the entire arch_lmb_reserve function on aarch64, yes. I assumed when we had talked that it was a small area being set aside and perhaps mis-recalled that ATAGS tended to live at DDR_BASE + 0x800 or so.
That arch_lmb_reserve() is responsible for reserving architecture specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as I can tell (and see below regarding the TLB).
I don't think that LMB ever covered ATAGS. ATAGS, I could have sworn, were at start of memory + 0x800 or so. This LMB is for the top of memory where U-Boot is.
What is there to protect which Linux does not set up again ?
What does Linux have to do this with? I don't see, but maybe have missed, where "U-Boot adds memory region to internal LMB list" is translated to "Update the device tree Linux will have with these regions". I do see code to read the reserved-memory regions from a DTB and add them to our LMB, but not the other direction.
The whole LMB reservation is done only after calling bootm (and other boot commands), and the comment in arch/arm/lib/bootm.c arch_lmb_reserve() does indicate the same.
On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
worked as well for me - but it left the stack in danger.
I want to cycle back up to this practically part. Marek, would changing the arch_lmb_reserve (or possibly even making this a more global thing / option) still let the area you want exposed on rcar3 (I assume) be exposed ? Or would it be covered again? Part of the problem, practically speaking, is that we need to ensure that as part of (attempting and likely succeeding in) booting the OS we don't overwrite ourself and hang.
An alternative that I'm not 100% sure I like would be adjusting env_get_bootm_size() so that the case of bootm_size and bootm_low are unset, so we figure out a value based on gd->ram_top we also take in to account where U-Boot itself is atm and exclude that. It's possible that if we had something like that at the start of the DT world, we wouldn't have the code to disable fdt relocation, which really feels like it's largely been "things crash when we relocate stuff, disable relocation" and not so much "save a little boot time, we optimized things VERY CAREFULLY".
On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote:
On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
worked as well for me - but it left the stack in danger.
I want to cycle back up to this practically part. Marek, would changing the arch_lmb_reserve (or possibly even making this a more global thing / option) still let the area you want exposed on rcar3 (I assume) be exposed ? Or would it be covered again? Part of the problem, practically speaking, is that we need to ensure that as part of (attempting and likely succeeding in) booting the OS we don't overwrite ourself and hang.
An alternative that I'm not 100% sure I like would be adjusting env_get_bootm_size() so that the case of bootm_size and bootm_low are unset, so we figure out a value based on gd->ram_top we also take in to account where U-Boot itself is atm and exclude that. It's possible that if we had something like that at the start of the DT world, we wouldn't have the code to disable fdt relocation, which really feels like it's largely been "things crash when we relocate stuff, disable relocation" and not so much "save a little boot time, we optimized things VERY CAREFULLY".
Digging more. The comment about "Allocate space for command line and board info" was introduced in 2d1916e48bd8 ("ARM: add flat device tree support"). I happen to have the platform this was tested on, a Beagleboard, around still and was able to boot that commit up and poke a bit. It's effectively covering all of the running U-Boot with an LMB. You can move on to commit b485556be51d ("ARM: enable device tree for beagle"). That defines CONFIG_SYS_BOOTMAPSZ which is about start of DRAM + a bit, to reserve space there that would be questionable on ARM64. But all of that code area has been reworked since then.
So in sum, I think reverting 2359fa7a87848626bcbd3399e92c657595880cd7 is right. U-Boot needs to be protected from being overwritten as it's still alive at that point and relocating things around. This has always been true. This has always been related to device tree booting.
What rcar is trying to do needs to be better explained first so that we can figure out the right place to make some sort of update.
On 8/3/21 11:51 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote:
On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
worked as well for me - but it left the stack in danger.
I want to cycle back up to this practically part. Marek, would changing the arch_lmb_reserve (or possibly even making this a more global thing / option) still let the area you want exposed on rcar3 (I assume) be exposed ? Or would it be covered again? Part of the problem, practically speaking, is that we need to ensure that as part of (attempting and likely succeeding in) booting the OS we don't overwrite ourself and hang.
I think large part of the problem is the purpose of LMB is unclear at best.
The arch_lmb_reserve() says this:
54 void arch_lmb_reserve(struct lmb *lmb) [...] 59 /* 60 * Booting a (Linux) kernel image 61 * 62 * Allocate space for command line and board info - the 63 * address should be as high as possible within the reach of 64 * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused 65 * memory, which means far enough below the current stack 66 * pointer. 67 */
So basically reserve chunk of memory in the right place, which can be safely passed to the kernel.
If that's not the case, and the LMB is used also to protect U-Boot from overwriting itself, then the above comment is totally misleading and wrong (and I have a feeling that is what you are trying to get on).
And if that's the case, then what we should reserve isn't stack_bottom...ram_top, but really the work areas of U-Boot only (i.e. a more fine grained reservation might be needed). For starters, replacing ram_top with end of u-boot would do for my use case.
An alternative that I'm not 100% sure I like would be adjusting env_get_bootm_size() so that the case of bootm_size and bootm_low are unset, so we figure out a value based on gd->ram_top we also take in to account where U-Boot itself is atm and exclude that.
I think we should really clarify the purpose of the LMB and make sure we both understand it the same way.
It's possible that if we had something like that at the start of the DT world, we wouldn't have the code to disable fdt relocation, which really feels like it's largely been "things crash when we relocate stuff, disable relocation" and not so much "save a little boot time, we optimized things VERY CAREFULLY".
Digging more. The comment about "Allocate space for command line and board info" was introduced in 2d1916e48bd8 ("ARM: add flat device tree support"). I happen to have the platform this was tested on, a Beagleboard, around still and was able to boot that commit up and poke a bit. It's effectively covering all of the running U-Boot with an LMB. You can move on to commit b485556be51d ("ARM: enable device tree for beagle"). That defines CONFIG_SYS_BOOTMAPSZ which is about start of DRAM + a bit, to reserve space there that would be questionable on ARM64. But all of that code area has been reworked since then.
So in sum, I think reverting 2359fa7a87848626bcbd3399e92c657595880cd7 is right. U-Boot needs to be protected from being overwritten as it's still alive at that point and relocating things around. This has always been true. This has always been related to device tree booting.
What rcar is trying to do needs to be better explained first so that we can figure out the right place to make some sort of update.
rcar might not relocate u-boot all the way to the end of DRAM, because there might be some reserved memory at the end of DRAM used by one of the other CPUs in the SoC, that's all.
On Fri, Aug 06, 2021 at 12:22:43AM +0200, Marek Vasut wrote:
On 8/3/21 11:51 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote:
On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
worked as well for me - but it left the stack in danger.
I want to cycle back up to this practically part. Marek, would changing the arch_lmb_reserve (or possibly even making this a more global thing / option) still let the area you want exposed on rcar3 (I assume) be exposed ? Or would it be covered again? Part of the problem, practically speaking, is that we need to ensure that as part of (attempting and likely succeeding in) booting the OS we don't overwrite ourself and hang.
I think large part of the problem is the purpose of LMB is unclear at best.
The arch_lmb_reserve() says this:
54 void arch_lmb_reserve(struct lmb *lmb) [...] 59 /* 60 * Booting a (Linux) kernel image 61 * 62 * Allocate space for command line and board info - the 63 * address should be as high as possible within the reach of 64 * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused 65 * memory, which means far enough below the current stack 66 * pointer. 67 */
So basically reserve chunk of memory in the right place, which can be safely passed to the kernel.
No. This isn't the case. We reserve chunks of memory away from other usage by U-Boot.
If that's not the case, and the LMB is used also to protect U-Boot from overwriting itself, then the above comment is totally misleading and wrong (and I have a feeling that is what you are trying to get on).
And if that's the case, then what we should reserve isn't stack_bottom...ram_top, but really the work areas of U-Boot only (i.e. a more fine grained reservation might be needed). For starters, replacing ram_top with end of u-boot would do for my use case.
I addressed this I believe by going back and explaining what the code base did at the time this was added, and booting up the image on the platform it was tested on at the time. I think there's a case to be made that the comment is a bit misleading / unclear, when it was added. We can see when it was added CONFIG_SYS_BOOTMAPSZ being set to cover the start + 0x8000 or so of where ATAGS had traditionally been, and this function here covering where U-Boot was in memory, to avoid being overwritten by the device tree being relocated.
An alternative that I'm not 100% sure I like would be adjusting env_get_bootm_size() so that the case of bootm_size and bootm_low are unset, so we figure out a value based on gd->ram_top we also take in to account where U-Boot itself is atm and exclude that.
I think we should really clarify the purpose of the LMB and make sure we both understand it the same way.
It's possible that if we had something like that at the start of the DT world, we wouldn't have the code to disable fdt relocation, which really feels like it's largely been "things crash when we relocate stuff, disable relocation" and not so much "save a little boot time, we optimized things VERY CAREFULLY".
Digging more. The comment about "Allocate space for command line and board info" was introduced in 2d1916e48bd8 ("ARM: add flat device tree support"). I happen to have the platform this was tested on, a Beagleboard, around still and was able to boot that commit up and poke a bit. It's effectively covering all of the running U-Boot with an LMB. You can move on to commit b485556be51d ("ARM: enable device tree for beagle"). That defines CONFIG_SYS_BOOTMAPSZ which is about start of DRAM + a bit, to reserve space there that would be questionable on ARM64. But all of that code area has been reworked since then.
So in sum, I think reverting 2359fa7a87848626bcbd3399e92c657595880cd7 is right. U-Boot needs to be protected from being overwritten as it's still alive at that point and relocating things around. This has always been true. This has always been related to device tree booting.
What rcar is trying to do needs to be better explained first so that we can figure out the right place to make some sort of update.
rcar might not relocate u-boot all the way to the end of DRAM, because there might be some reserved memory at the end of DRAM used by one of the other CPUs in the SoC, that's all.
OK, so then there isn't a problem reverting this commit for rcar?
On 8/6/21 6:49 PM, Tom Rini wrote:
On Fri, Aug 06, 2021 at 12:22:43AM +0200, Marek Vasut wrote:
On 8/3/21 11:51 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote:
On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
worked as well for me - but it left the stack in danger.
I want to cycle back up to this practically part. Marek, would changing the arch_lmb_reserve (or possibly even making this a more global thing / option) still let the area you want exposed on rcar3 (I assume) be exposed ? Or would it be covered again? Part of the problem, practically speaking, is that we need to ensure that as part of (attempting and likely succeeding in) booting the OS we don't overwrite ourself and hang.
I think large part of the problem is the purpose of LMB is unclear at best.
The arch_lmb_reserve() says this:
54 void arch_lmb_reserve(struct lmb *lmb) [...] 59 /* 60 * Booting a (Linux) kernel image 61 * 62 * Allocate space for command line and board info - the 63 * address should be as high as possible within the reach of 64 * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused 65 * memory, which means far enough below the current stack 66 * pointer. 67 */
So basically reserve chunk of memory in the right place, which can be safely passed to the kernel.
No. This isn't the case. We reserve chunks of memory away from other usage by U-Boot.
Then I have to wonder, why is this not called in board_init_f or board_init_r , but only after bootm or similar boot command was called ?
If that's not the case, and the LMB is used also to protect U-Boot from overwriting itself, then the above comment is totally misleading and wrong (and I have a feeling that is what you are trying to get on).
And if that's the case, then what we should reserve isn't stack_bottom...ram_top, but really the work areas of U-Boot only (i.e. a more fine grained reservation might be needed). For starters, replacing ram_top with end of u-boot would do for my use case.
I addressed this I believe by going back and explaining what the code base did at the time this was added, and booting up the image on the platform it was tested on at the time. I think there's a case to be made that the comment is a bit misleading / unclear, when it was added.
I would say that is an understatement , but also see above.
We can see when it was added CONFIG_SYS_BOOTMAPSZ being set to cover the start + 0x8000 or so of where ATAGS had traditionally been, and this function here covering where U-Boot was in memory, to avoid being overwritten by the device tree being relocated.
An alternative that I'm not 100% sure I like would be adjusting env_get_bootm_size() so that the case of bootm_size and bootm_low are unset, so we figure out a value based on gd->ram_top we also take in to account where U-Boot itself is atm and exclude that.
I think we should really clarify the purpose of the LMB and make sure we both understand it the same way.
It's possible that if we had something like that at the start of the DT world, we wouldn't have the code to disable fdt relocation, which really feels like it's largely been "things crash when we relocate stuff, disable relocation" and not so much "save a little boot time, we optimized things VERY CAREFULLY".
Digging more. The comment about "Allocate space for command line and board info" was introduced in 2d1916e48bd8 ("ARM: add flat device tree support"). I happen to have the platform this was tested on, a Beagleboard, around still and was able to boot that commit up and poke a bit. It's effectively covering all of the running U-Boot with an LMB. You can move on to commit b485556be51d ("ARM: enable device tree for beagle"). That defines CONFIG_SYS_BOOTMAPSZ which is about start of DRAM + a bit, to reserve space there that would be questionable on ARM64. But all of that code area has been reworked since then.
So in sum, I think reverting 2359fa7a87848626bcbd3399e92c657595880cd7 is right. U-Boot needs to be protected from being overwritten as it's still alive at that point and relocating things around. This has always been true. This has always been related to device tree booting.
What rcar is trying to do needs to be better explained first so that we can figure out the right place to make some sort of update.
rcar might not relocate u-boot all the way to the end of DRAM, because there might be some reserved memory at the end of DRAM used by one of the other CPUs in the SoC, that's all.
OK, so then there isn't a problem reverting this commit for rcar?
The revert will break the use case where the other CPUs are using memory above U-Boot, but have a look at the following branch, it should permit me to parametrize the arch_lmb_reserve() better and reserve the right memory areas per architecture/mach/board, and even clean the arch_lmb_reserve up further: https://source.denx.de/u-boot/custodians/u-boot-sh/-/tree/lmb-v1 So yes, pick the revert and I'll submit the four patches for likely next release.
On Sun, Aug 08, 2021 at 03:45:30PM +0200, Marek Vasut wrote:
On 8/6/21 6:49 PM, Tom Rini wrote:
On Fri, Aug 06, 2021 at 12:22:43AM +0200, Marek Vasut wrote:
On 8/3/21 11:51 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote:
On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Practically,
void arch_lmb_reserve(struct lmb *lmb) { lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); }
worked as well for me - but it left the stack in danger.
I want to cycle back up to this practically part. Marek, would changing the arch_lmb_reserve (or possibly even making this a more global thing / option) still let the area you want exposed on rcar3 (I assume) be exposed ? Or would it be covered again? Part of the problem, practically speaking, is that we need to ensure that as part of (attempting and likely succeeding in) booting the OS we don't overwrite ourself and hang.
I think large part of the problem is the purpose of LMB is unclear at best.
The arch_lmb_reserve() says this:
54 void arch_lmb_reserve(struct lmb *lmb) [...] 59 /* 60 * Booting a (Linux) kernel image 61 * 62 * Allocate space for command line and board info - the 63 * address should be as high as possible within the reach of 64 * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused 65 * memory, which means far enough below the current stack 66 * pointer. 67 */
So basically reserve chunk of memory in the right place, which can be safely passed to the kernel.
No. This isn't the case. We reserve chunks of memory away from other usage by U-Boot.
Then I have to wonder, why is this not called in board_init_f or board_init_r , but only after bootm or similar boot command was called ?
I also wonder a little bit. That it does not is why we have the LMB checks under fs/ and net/ and doing this sooner would possibly make dealing with those CVEs either easier or would also address some other classes of them that may exist. I expect it was not simply because up until rather recently we didn't have any checks for "don't overwrite specific areas of memory" other than right before firing off the OS (and modify whatever memory you want to modify is a feature not a bug).
If that's not the case, and the LMB is used also to protect U-Boot from overwriting itself, then the above comment is totally misleading and wrong (and I have a feeling that is what you are trying to get on).
And if that's the case, then what we should reserve isn't stack_bottom...ram_top, but really the work areas of U-Boot only (i.e. a more fine grained reservation might be needed). For starters, replacing ram_top with end of u-boot would do for my use case.
I addressed this I believe by going back and explaining what the code base did at the time this was added, and booting up the image on the platform it was tested on at the time. I think there's a case to be made that the comment is a bit misleading / unclear, when it was added.
I would say that is an understatement , but also see above.
We can see when it was added CONFIG_SYS_BOOTMAPSZ being set to cover the start + 0x8000 or so of where ATAGS had traditionally been, and this function here covering where U-Boot was in memory, to avoid being overwritten by the device tree being relocated.
An alternative that I'm not 100% sure I like would be adjusting env_get_bootm_size() so that the case of bootm_size and bootm_low are unset, so we figure out a value based on gd->ram_top we also take in to account where U-Boot itself is atm and exclude that.
I think we should really clarify the purpose of the LMB and make sure we both understand it the same way.
It's possible that if we had something like that at the start of the DT world, we wouldn't have the code to disable fdt relocation, which really feels like it's largely been "things crash when we relocate stuff, disable relocation" and not so much "save a little boot time, we optimized things VERY CAREFULLY".
Digging more. The comment about "Allocate space for command line and board info" was introduced in 2d1916e48bd8 ("ARM: add flat device tree support"). I happen to have the platform this was tested on, a Beagleboard, around still and was able to boot that commit up and poke a bit. It's effectively covering all of the running U-Boot with an LMB. You can move on to commit b485556be51d ("ARM: enable device tree for beagle"). That defines CONFIG_SYS_BOOTMAPSZ which is about start of DRAM + a bit, to reserve space there that would be questionable on ARM64. But all of that code area has been reworked since then.
So in sum, I think reverting 2359fa7a87848626bcbd3399e92c657595880cd7 is right. U-Boot needs to be protected from being overwritten as it's still alive at that point and relocating things around. This has always been true. This has always been related to device tree booting.
What rcar is trying to do needs to be better explained first so that we can figure out the right place to make some sort of update.
rcar might not relocate u-boot all the way to the end of DRAM, because there might be some reserved memory at the end of DRAM used by one of the other CPUs in the SoC, that's all.
OK, so then there isn't a problem reverting this commit for rcar?
The revert will break the use case where the other CPUs are using memory above U-Boot, but have a look at the following branch, it should permit me to parametrize the arch_lmb_reserve() better and reserve the right memory areas per architecture/mach/board, and even clean the arch_lmb_reserve up further: https://source.denx.de/u-boot/custodians/u-boot-sh/-/tree/lmb-v1 So yes, pick the revert and I'll submit the four patches for likely next release.
Thanks for explaining, I'll pick up the revert patch then.
For your LMB tree, I like the initial approach but looking at 528915c71762 ("imx: Fix potential lmb memory overwritten by stack") I think that shows the general "4K is enough for stack we hope" is wrong, and we should do 16K instead for everyone as the default. But we can discuss that more too once you post the whole series which again, I think is the right direction.
On 8/8/21 4:00 PM, Tom Rini wrote:
On Sun, Aug 08, 2021 at 03:45:30PM +0200, Marek Vasut wrote:
On 8/6/21 6:49 PM, Tom Rini wrote:
On Fri, Aug 06, 2021 at 12:22:43AM +0200, Marek Vasut wrote:
On 8/3/21 11:51 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote:
On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
> From: Jan Kiszka jan.kiszka@siemens.com > > This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7. > > While the goal is valid and there is surely unused memory in that area, > we also have a lot of crucial things still located at the top-of-memory > while running lmb_alloc_base. Such things are the page table (tlb_addr), > relocated U-Boot and the active stack. Possibly more. So this patch was > premature, we will need relocations of those things first if we want to > use the range. > > Fixes booting on the IOT2050, but likely also on other boards. It got > stuck on relocating the FDT - over the relocated U-Boot code. > > Signed-off-by: Jan Kiszka jan.kiszka@siemens.com > --- > > Practically, > > void arch_lmb_reserve(struct lmb *lmb) > { > lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); > } > > worked as well for me - but it left the stack in danger.
I want to cycle back up to this practically part. Marek, would changing the arch_lmb_reserve (or possibly even making this a more global thing / option) still let the area you want exposed on rcar3 (I assume) be exposed ? Or would it be covered again? Part of the problem, practically speaking, is that we need to ensure that as part of (attempting and likely succeeding in) booting the OS we don't overwrite ourself and hang.
I think large part of the problem is the purpose of LMB is unclear at best.
The arch_lmb_reserve() says this:
54 void arch_lmb_reserve(struct lmb *lmb) [...] 59 /* 60 * Booting a (Linux) kernel image 61 * 62 * Allocate space for command line and board info - the 63 * address should be as high as possible within the reach of 64 * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused 65 * memory, which means far enough below the current stack 66 * pointer. 67 */
So basically reserve chunk of memory in the right place, which can be safely passed to the kernel.
No. This isn't the case. We reserve chunks of memory away from other usage by U-Boot.
Then I have to wonder, why is this not called in board_init_f or board_init_r , but only after bootm or similar boot command was called ?
I also wonder a little bit. That it does not is why we have the LMB checks under fs/ and net/ and doing this sooner would possibly make dealing with those CVEs either easier or would also address some other classes of them that may exist.
So, why not move it into the relocation code then ?
I expect it was not simply because up until rather recently we didn't have any checks for "don't overwrite specific areas of memory" other than right before firing off the OS (and modify whatever memory you want to modify is a feature not a bug).
The LMB has been around since forever though ?
[...]
OK, so then there isn't a problem reverting this commit for rcar?
The revert will break the use case where the other CPUs are using memory above U-Boot, but have a look at the following branch, it should permit me to parametrize the arch_lmb_reserve() better and reserve the right memory areas per architecture/mach/board, and even clean the arch_lmb_reserve up further: https://source.denx.de/u-boot/custodians/u-boot-sh/-/tree/lmb-v1 So yes, pick the revert and I'll submit the four patches for likely next release.
Thanks for explaining, I'll pick up the revert patch then.
For your LMB tree, I like the initial approach but looking at 528915c71762 ("imx: Fix potential lmb memory overwritten by stack") I think that shows the general "4K is enough for stack we hope" is wrong, and we should do 16K instead for everyone as the default. But we can discuss that more too once you post the whole series which again, I think is the right direction.
The IMX thing is odd indeed and raises a bigger question -- what is the "right" amount of stack to reserve ?
On Sun, Aug 08, 2021 at 04:28:14PM +0200, Marek Vasut wrote:
On 8/8/21 4:00 PM, Tom Rini wrote:
On Sun, Aug 08, 2021 at 03:45:30PM +0200, Marek Vasut wrote:
On 8/6/21 6:49 PM, Tom Rini wrote:
On Fri, Aug 06, 2021 at 12:22:43AM +0200, Marek Vasut wrote:
On 8/3/21 11:51 PM, Tom Rini wrote:
On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote: > On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote: > > > From: Jan Kiszka jan.kiszka@siemens.com > > > > This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7. > > > > While the goal is valid and there is surely unused memory in that area, > > we also have a lot of crucial things still located at the top-of-memory > > while running lmb_alloc_base. Such things are the page table (tlb_addr), > > relocated U-Boot and the active stack. Possibly more. So this patch was > > premature, we will need relocations of those things first if we want to > > use the range. > > > > Fixes booting on the IOT2050, but likely also on other boards. It got > > stuck on relocating the FDT - over the relocated U-Boot code. > > > > Signed-off-by: Jan Kiszka jan.kiszka@siemens.com > > --- > > > > Practically, > > > > void arch_lmb_reserve(struct lmb *lmb) > > { > > lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); > > } > > > > worked as well for me - but it left the stack in danger. > > I want to cycle back up to this practically part. Marek, would changing > the arch_lmb_reserve (or possibly even making this a more global thing / > option) still let the area you want exposed on rcar3 (I assume) be > exposed ? Or would it be covered again? Part of the problem, > practically speaking, is that we need to ensure that as part of > (attempting and likely succeeding in) booting the OS we don't overwrite > ourself and hang.
I think large part of the problem is the purpose of LMB is unclear at best.
The arch_lmb_reserve() says this:
54 void arch_lmb_reserve(struct lmb *lmb) [...] 59 /* 60 * Booting a (Linux) kernel image 61 * 62 * Allocate space for command line and board info - the 63 * address should be as high as possible within the reach of 64 * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused 65 * memory, which means far enough below the current stack 66 * pointer. 67 */
So basically reserve chunk of memory in the right place, which can be safely passed to the kernel.
No. This isn't the case. We reserve chunks of memory away from other usage by U-Boot.
Then I have to wonder, why is this not called in board_init_f or board_init_r , but only after bootm or similar boot command was called ?
I also wonder a little bit. That it does not is why we have the LMB checks under fs/ and net/ and doing this sooner would possibly make dealing with those CVEs either easier or would also address some other classes of them that may exist.
So, why not move it into the relocation code then ?
A further potential clean-up, yes. I don't think that was discussed around CVE-2018-18439 / CVE-2018-18440.
I expect it was not simply because up until rather recently we didn't have any checks for "don't overwrite specific areas of memory" other than right before firing off the OS (and modify whatever memory you want to modify is a feature not a bug).
The LMB has been around since forever though ?
Yes, LMB has been around since the PowerPC device tree days I suspect (I didn't dig that far back), but only used outside of the "don't overwrite the running U-Boot while we relocate device tree / initrd before booting OS" since 2018 or so.
[...]
OK, so then there isn't a problem reverting this commit for rcar?
The revert will break the use case where the other CPUs are using memory above U-Boot, but have a look at the following branch, it should permit me to parametrize the arch_lmb_reserve() better and reserve the right memory areas per architecture/mach/board, and even clean the arch_lmb_reserve up further: https://source.denx.de/u-boot/custodians/u-boot-sh/-/tree/lmb-v1 So yes, pick the revert and I'll submit the four patches for likely next release.
Thanks for explaining, I'll pick up the revert patch then.
For your LMB tree, I like the initial approach but looking at 528915c71762 ("imx: Fix potential lmb memory overwritten by stack") I think that shows the general "4K is enough for stack we hope" is wrong, and we should do 16K instead for everyone as the default. But we can discuss that more too once you post the whole series which again, I think is the right direction.
The IMX thing is odd indeed and raises a bigger question -- what is the "right" amount of stack to reserve ?
It's a good question, yes. And some more details about what exactly the NXP folks were doing to hit that would also be nice.
On 8/8/21 4:54 PM, Tom Rini wrote:
[...]
I expect it was not simply because up until rather recently we didn't have any checks for "don't overwrite specific areas of memory" other than right before firing off the OS (and modify whatever memory you want to modify is a feature not a bug).
The LMB has been around since forever though ?
Yes, LMB has been around since the PowerPC device tree days I suspect (I didn't dig that far back), but only used outside of the "don't overwrite the running U-Boot while we relocate device tree / initrd before booting OS" since 2018 or so.
So, are we using LMB for two different things now ?
[...]
OK, so then there isn't a problem reverting this commit for rcar?
The revert will break the use case where the other CPUs are using memory above U-Boot, but have a look at the following branch, it should permit me to parametrize the arch_lmb_reserve() better and reserve the right memory areas per architecture/mach/board, and even clean the arch_lmb_reserve up further: https://source.denx.de/u-boot/custodians/u-boot-sh/-/tree/lmb-v1 So yes, pick the revert and I'll submit the four patches for likely next release.
Thanks for explaining, I'll pick up the revert patch then.
For your LMB tree, I like the initial approach but looking at 528915c71762 ("imx: Fix potential lmb memory overwritten by stack") I think that shows the general "4K is enough for stack we hope" is wrong, and we should do 16K instead for everyone as the default. But we can discuss that more too once you post the whole series which again, I think is the right direction.
The IMX thing is odd indeed and raises a bigger question -- what is the "right" amount of stack to reserve ?
It's a good question, yes. And some more details about what exactly the NXP folks were doing to hit that would also be nice.
+CC Ye Li.
On Sun, Aug 08, 2021 at 05:25:33PM +0200, Marek Vasut wrote:
On 8/8/21 4:54 PM, Tom Rini wrote:
[...]
I expect it was not simply because up until rather recently we didn't have any checks for "don't overwrite specific areas of memory" other than right before firing off the OS (and modify whatever memory you want to modify is a feature not a bug).
The LMB has been around since forever though ?
Yes, LMB has been around since the PowerPC device tree days I suspect (I didn't dig that far back), but only used outside of the "don't overwrite the running U-Boot while we relocate device tree / initrd before booting OS" since 2018 or so.
So, are we using LMB for two different things now ?
For "now" == "2.5 years", yes, we've used the "LMB stops us from overwriting running U-Boot" as part of addressing the CVEs I quoted.
Hi Marek,
On Sun, 2021-08-08 at 17:25 +0200, Marek Vasut wrote:
Caution: EXT Email
On 8/8/21 4:54 PM, Tom Rini wrote:
[...]
I expect it was not simply because up until rather recently we didn't have any checks for "don't overwrite specific areas of memory" other than right before firing off the OS (and modify whatever memory you want to modify is a feature not a bug).
The LMB has been around since forever though ?
Yes, LMB has been around since the PowerPC device tree days I suspect (I didn't dig that far back), but only used outside of the "don't overwrite the running U-Boot while we relocate device tree / initrd before booting OS" since 2018 or so.
So, are we using LMB for two different things now ?
[...]
OK, so then there isn't a problem reverting this commit for rcar?
The revert will break the use case where the other CPUs are using memory above U-Boot, but have a look at the following branch, it should permit me to parametrize the arch_lmb_reserve() better and reserve the right memory areas per architecture/mach/board, and even clean the arch_lmb_reserve up further: https://eur01.safelinks.protection.outlook.com/?url=https%3A% 2F%2Fsource.denx.de%2Fu-boot%2Fcustodians%2Fu-boot-sh%2F- %2Ftree%2Flmb- v1&data=04%7C01%7Cye.li%40nxp.com%7Cb9bda480d0494a9249c70 8d95a80c552%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6376 40331407737098%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC JQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata =yhIbMHWZMjXy59BVDFVbY2owM7TNdWvk%2B3w2IHg78ok%3D&reserve d=0 So yes, pick the revert and I'll submit the four patches for likely next release.
Thanks for explaining, I'll pick up the revert patch then.
For your LMB tree, I like the initial approach but looking at 528915c71762 ("imx: Fix potential lmb memory overwritten by stack") I think that shows the general "4K is enough for stack we hope" is wrong, and we should do 16K instead for everyone as the default. But we can discuss that more too once you post the whole series which again, I think is the right direction.
The IMX thing is odd indeed and raises a bigger question -- what is the "right" amount of stack to reserve ?
It's a good question, yes. And some more details about what exactly the NXP folks were doing to hit that would also be nice.
+CC Ye Li.
On i.MX8QM/QXP, we implement the ft_system_setup to update kernel FDT. It needs larger stack size to parse the FDT to disable nodes if the corresponding resources are not owned by A core. When we enabled the initrd relocation in u-boot, it allocates a space from LMB for initrd just before the SP reservation. The stack overflow overwrites the initrd and cause kernel issue.
The size of stack reservation actually depends on the implementation. There are lots of board or soc level functions in the boot sequence. You can't predict how much stack is needed. So providing a way that can adjust the size is useful.
Best regards, Ye Li
On Mon, Aug 09, 2021 at 07:34:34AM +0000, Ye Li wrote:
Hi Marek,
On Sun, 2021-08-08 at 17:25 +0200, Marek Vasut wrote:
Caution: EXT Email
On 8/8/21 4:54 PM, Tom Rini wrote:
[...]
I expect it was not simply because up until rather recently we didn't have any checks for "don't overwrite specific areas of memory" other than right before firing off the OS (and modify whatever memory you want to modify is a feature not a bug).
The LMB has been around since forever though ?
Yes, LMB has been around since the PowerPC device tree days I suspect (I didn't dig that far back), but only used outside of the "don't overwrite the running U-Boot while we relocate device tree / initrd before booting OS" since 2018 or so.
So, are we using LMB for two different things now ?
[...]
> > OK, so then there isn't a problem reverting this commit for > rcar? The revert will break the use case where the other CPUs are using memory above U-Boot, but have a look at the following branch, it should permit me to parametrize the arch_lmb_reserve() better and reserve the right memory areas per architecture/mach/board, and even clean the arch_lmb_reserve up further: https://eur01.safelinks.protection.outlook.com/?url=https%3A% 2F%2Fsource.denx.de%2Fu-boot%2Fcustodians%2Fu-boot-sh%2F- %2Ftree%2Flmb- v1&data=04%7C01%7Cye.li%40nxp.com%7Cb9bda480d0494a9249c70 8d95a80c552%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6376 40331407737098%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC JQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata =yhIbMHWZMjXy59BVDFVbY2owM7TNdWvk%2B3w2IHg78ok%3D&reserve d=0 So yes, pick the revert and I'll submit the four patches for likely next release.
Thanks for explaining, I'll pick up the revert patch then.
For your LMB tree, I like the initial approach but looking at 528915c71762 ("imx: Fix potential lmb memory overwritten by stack") I think that shows the general "4K is enough for stack we hope" is wrong, and we should do 16K instead for everyone as the default. But we can discuss that more too once you post the whole series which again, I think is the right direction.
The IMX thing is odd indeed and raises a bigger question -- what is the "right" amount of stack to reserve ?
It's a good question, yes. And some more details about what exactly the NXP folks were doing to hit that would also be nice.
+CC Ye Li.
On i.MX8QM/QXP, we implement the ft_system_setup to update kernel FDT. It needs larger stack size to parse the FDT to disable nodes if the corresponding resources are not owned by A core. When we enabled the initrd relocation in u-boot, it allocates a space from LMB for initrd just before the SP reservation. The stack overflow overwrites the initrd and cause kernel issue.
The size of stack reservation actually depends on the implementation. There are lots of board or soc level functions in the boot sequence. You can't predict how much stack is needed. So providing a way that can adjust the size is useful.
Thanks for explaining. It sounds like arch/arm/mach-imx/imx8m/soc.c::the ft_system_setup() needs a comment that it uses a lot of stack, due to how complex it is, and that arch/arm/mach-imx/misc.c::board_lmb_reserve() should get moved, and reworked to just reserve another few kilobytes, for the imx8m case.
Dear Tom,
In message 20210809131640.GN858@bill-the-cat you wrote:
Thanks for explaining. It sounds like arch/arm/mach-imx/imx8m/soc.c::the=A0ft_system_setup() needs a comment that it uses a lot of stack, due to how complex it is, and that
I think this is the wrong approach. It sounds as if we were discouragin the use of the stack for dynamic buffers etc. We should not do that. Instead, we should make sure that the stack starts below any reserved memory locations, and is free to grow downward as long as there is still memory available.
If U-Boot was an OS, we would probably think about implementing a more complex memory management utilizing the MMU, but in a boot loader this is clearly overkill. But please, just let the stack grow as it wants.
There may be completely legal use cases with unpredictable stack requirements. Think about recursive shell scripts, for example...
Best regards,
Wolfgang Denk
On Mon, Aug 09, 2021 at 04:11:36PM +0200, Wolfgang Denk wrote:
Dear Tom,
In message 20210809131640.GN858@bill-the-cat you wrote:
Thanks for explaining. It sounds like arch/arm/mach-imx/imx8m/soc.c::the=A0ft_system_setup() needs a comment that it uses a lot of stack, due to how complex it is, and that
I think this is the wrong approach. It sounds as if we were discouragin the use of the stack for dynamic buffers etc. We should not do that. Instead, we should make sure that the stack starts below any reserved memory locations, and is free to grow downward as long as there is still memory available.
If U-Boot was an OS, we would probably think about implementing a more complex memory management utilizing the MMU, but in a boot loader this is clearly overkill. But please, just let the stack grow as it wants.
There may be completely legal use cases with unpredictable stack requirements. Think about recursive shell scripts, for example...
This is a problem of the specific flow of the code. Generally, you're right, and that's what we do. Here specifically, I'm not sure we could rework the flow to have a place where we call ft_system_setup prior to lmb. The alternative, which would cover most cases at least, is to make sure everyone sets bootm_size and we stop with "all of DRAM, with a few exceptions, is where to relocate kernel/dtb/initrd to" being the default case.
Dear Marek,
In message 03720507-5ea4-0fb9-0549-37df3128be2b@denx.de you wrote:
The IMX thing is odd indeed and raises a bigger question -- what is the "right" amount of stack to reserve ?
In the original U-Boot design (which is still documented in the README, see section "Memory Management", start reading at "typical memory configuration") The stack was located below the lowest reserved address range, so it could grow downward, only limited by available memory size. And yes, it was the responsibility of the user to make sure not to overwrite it for example by loading images at too high addresses or running "mw" with unsuitable address or size.
What I want to point out is: there are probably a number of areas in U-Boot which are based on this assumption, as reserving buffers or the like on the stack is a much cheaper operation with much less potential for memory leaks etc. than using malloc().
Any assumption that a stack size of 4 kB or 16 kB or such will be sufficient is ... optimistic at best.
Best regards,
Wolfgang Denk
On Mon, Aug 09, 2021 at 08:44:58AM +0200, Wolfgang Denk wrote:
Dear Marek,
In message 03720507-5ea4-0fb9-0549-37df3128be2b@denx.de you wrote:
The IMX thing is odd indeed and raises a bigger question -- what is the "right" amount of stack to reserve ?
In the original U-Boot design (which is still documented in the README, see section "Memory Management", start reading at "typical memory configuration") The stack was located below the lowest reserved address range, so it could grow downward, only limited by available memory size. And yes, it was the responsibility of the user to make sure not to overwrite it for example by loading images at too high addresses or running "mw" with unsuitable address or size.
What I want to point out is: there are probably a number of areas in U-Boot which are based on this assumption, as reserving buffers or the like on the stack is a much cheaper operation with much less potential for memory leaks etc. than using malloc().
Any assumption that a stack size of 4 kB or 16 kB or such will be sufficient is ... optimistic at best.
Ah right, and here's why we call, in the kernel path, the LMB code where we do. We take the current stack pointer, round it, relocate device tree/initrd and then go away.
On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
While the goal is valid and there is surely unused memory in that area, we also have a lot of crucial things still located at the top-of-memory while running lmb_alloc_base. Such things are the page table (tlb_addr), relocated U-Boot and the active stack. Possibly more. So this patch was premature, we will need relocations of those things first if we want to use the range.
Fixes booting on the IOT2050, but likely also on other boards. It got stuck on relocating the FDT - over the relocated U-Boot code.
Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
Applied to u-boot/master, thanks!
participants (5)
-
Jan Kiszka -
Marek Vasut -
Tom Rini -
Wolfgang Denk -
Ye Li