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

[Tom Rini <trini@konsulko.com>]

[-- 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 --]