[PATCH 5/9] board_f: Fix corruption of relocaddr

[Simon Glass]

Hi Devarsh,

On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht at ti.com> wrote:
>
> Hi Simon,
>
> On 26/07/23 02:58, Simon Glass wrote:
> > Hi Devarsh,
> >
> > On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht at ti.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 24/07/23 20:22, Simon Glass wrote:
> >>> When the video framebuffer comes from the bloblist, we should not change
> >>> relocaddr to this address, since it interfers with the normal memory
> >>> allocation.
> >>>
> >>> This fixes a boot loop in qemu-x86_64
> >>>
> >>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
> >>> ---
> >>>
> >>>  common/board_f.c | 1 -
> >>>  1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/common/board_f.c b/common/board_f.c
> >>> index 7d2c380e91e2..5c8646b22283 100644
> >>> --- a/common/board_f.c
> >>> +++ b/common/board_f.c
> >>> @@ -419,7 +419,6 @@ static int reserve_video(void)
> >>>               if (!ho)
> >>>                       return log_msg_ret("blf", -ENOENT);
> >>>               video_reserve_from_bloblist(ho);
> >>> -             gd->relocaddr = ho->fb;
> >>
> >> I think this change was done as relocaddr pointer was required to be updated
> >> to move after frame-buffer reserved area to ensure that any further memory
> >> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
> >> don't overlap with frame-buffer reserved area passed from blob, so I think
> >> removing this line may cause further memory reservations to overlap with
> >> reserved framebuffer.
> >>
> >> Could you please confirm?
> >
> > SPL and U-Boot have different memory layouts. The current code is
> > interrupting U-Boot's careful building up of chunks of memory used for
> > different purposes.
> >
>
> But it is possible they could be using similar memory layout for some
> components like framebuffer.
> For e.g. in our case we are using same video_reserve func in A53 SPL too
> and which allocates framebuffer from gd->relocaddr as seen here:
>
> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427

Even if it is similar, the point is that U-Boot proper needs to do its
own allocation stuff.

Of course, if SPL sets up the video, it will provide the framebuffer
address, but only that. The other addresses need to be done using the
normal mechanism.

>
> > The video memory has already been allocated by SPL, so we don't need
> > to insert a new one here, as your code demonstrates.
> >
>
> Agreed.
>
> > But also we have no way of knowing if it is legal to relocate U-Boot
> > (and various other things) just below the frame buffer chosen by SPL.
> >
>
> Yes, so i suppose your case is that framebuffer address which is being passed
> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is
> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in
> any manner since relocaddr points to ramtop (i.e. near to end address of DDR).
>
> In that case I agree it doesn't make sense to move relocaddr to ho->fb.
>
> But for the scenario where gd->relocaddr and ho->fb are nearby there is every
> possibility that gd->relocaddr may overlap with framebuffer, also the code in
> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is
> overlapping with framebuffer area or not.
>
> I think one thing that can probably be done here is to have a check that if
> passed framebuffer area falls within current relocaddr region, then update the
> relocaddr else don't touch relocaddr :
>
> if (ho->fb <= gd->relocaddr - ho->size)
>   //It means framebuffer are is overlapping with current relocaddr so update
> relocaddr
>     gd->relocaddr = ho->fb
> else
>   //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr
>
> Could you please share your opinion on this and if above logic suffice your
> case too ?

I don't think this line is needed at all, which is why this patch
removes it. What problem are you seeing?

Regards,
Simon


>
> Regards
> Devarsh
>
> > The SPL frame buffer needs to be considered pre-allocated. It makes no
> > sense to set relocaddr to this value. It will break all sorts of
> > things. E.g. qemu-x86_64 crashes with this.
> >
> >>
> >>
> >> Regards
> >> Devarsh
> >>
> >>>       } else if (CONFIG_IS_ENABLED(VIDEO)) {
> >>>               ulong addr;
> >>>               int ret;
> >
> > Regards,
> > Simon