[PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Jan Kiszka]

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)
 {
-- 
2.26.2

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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 ?

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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 ?

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3392 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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.

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Jan Kiszka]

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

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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.

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Jan Kiszka]

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

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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.

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Jan Kiszka]

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

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3395 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Jan Kiszka]

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

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4195 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Jan Kiszka]

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

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5125 bytes --]

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)".

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2022 bytes --]

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".

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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 ?

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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.

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2015 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 9210 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5740 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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.

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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.

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7953 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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 ?

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5765 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

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.

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On the subject of C program indentation: "In My Egotistical  Opinion,
most  people's  C  programs  should be indented six feet downward and
covered with dirt."                               - Blair P. Houghton

Re: [EXT] Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Ye Li]

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

Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [EXT] Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4117 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [EXT] Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"Remember, Information is not knowledge;  Knowledge  is  not  Wisdom;
Wisdom is not truth; Truth is not beauty; Beauty is not love; Love is
not music; Music is the best."                          - Frank Zappa

Re: [EXT] Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]