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

[Simon Glass]

Hi Devarsh,

On Mon, 31 Jul 2023 at 05:10, Devarsh Thakkar <devarsht at ti.com> wrote:
>
> Hi Simon,
>
> Thanks for the patch.
>
> On 30/07/23 22:46, 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")
> > Suggested-by: Nikhil M Jain <n-jain1 at ti.com>
> > ---
> >
> > Changes in v2:
> > - Add a Kconfig as the suggested conditional did not work
>
> Overall this approach looks fine too but just curious to know, what is the
> ho->fb and gd->relocaddr in your case, so that we could understand your
> scenario better and maybe devise a more generic condition as a later-on patch
> since you mentioned that shared condition did not work for you.

Well, bear in mind that you are not filling in the video handoff
struct fully, so I need to disable it for qemu-x86_64.

Here is the trace. You can try it yourself with qemu-x86_64.

qemu-system-x86_64 -m 8G -smp 4 -bios /tmp/b/qemu-x86_64/u-boot.rom
-drive id=fdisk,file=root.img,if=virtio,driver=raw -serial mon:stdio

U-Boot SPL 2023.10-rc1-00119-g53cf4476d95-dirty (Jul 31 2023 - 09:51:11 -0600)
Video: 1024x768x32
Trying to boot from SPI
Jumping to 64-bit U-Boot: Note many features are missing
initcall: 0000000001133cdd
initcall: 000000000118c88a
initcall: 000000000113c550
initcall: 000000000113e006
   initcall_run_list() initcall: 000000000113441a
   initcall_run_list() initcall: 000000000113c75a
   initcall_run_list() initcall: 000000000113ac52
   initcall_run_list() initcall: 0000000001133cfe
   initcall_run_list() initcall: 00000000011112d8
   initcall_run_list() initcall: 0000000001133d01
   initcall_run_list() initcall: 0000000001134444
   initcall_run_list() initcall: 000000000118f81e
   initcall_run_list() initcall: 000000000115c528
   initcall_run_list() initcall: 00000000011337ff
   initcall_run_list() initcall: 000000000114f33b
   initcall_run_list() initcall: 000000000118d8b7


U-Boot 2023.10-rc1-00119-g53cf4476d95-dirty (Jul 31 2023 - 09:51:11 -0600)

   initcall_run_list() initcall: 000000000113382b
   display_text_info() U-Boot code: 01110000 -> 011C87B0  BSS: -> 011D2524
   initcall_run_list() initcall: 0000000001111d3c
   initcall_run_list() initcall: 0000000001133872
   initcall_run_list() initcall: 0000000001133943
CPU:   QEMU Virtual CPU version 2.5+
   initcall_run_list() initcall: 0000000001134480
   initcall_run_list() initcall: 0000000001133a01
DRAM:     initcall_run_list() initcall: 0000000001111332
   initcall_run_list() initcall: 0000000001133d07
     setup_dest_addr() Monitor len: 000C2524
     setup_dest_addr() Ram size: 200000000
     setup_dest_addr() Ram top: C0000000
   initcall_run_list() initcall: 0000000001133deb
   initcall_run_list() initcall: 0000000001133e00
   initcall_run_list() initcall: 0000000001133e03
gd->relocaddr=c0000000, ho->fb=d0000000
### ERROR ### Please RESET the board ###
QEMU: Terminated

The tree for this is basically u-boot-dm/bryd-working, just before this patch:

53cf4476d95 (HEAD) Revert "x86: Switch QEMU over to use the bochs driver"
5f78655f29d x86: Run QEMU machine setup in SPL
2952bc2f091 video: Tidy up Makefile rule for video
cd48d152138 x86: spl: Drop unwanted debug()
a4ce5665d55 Revert "efi debug"
530d87910dc efi debug
a36d59ba99a (x86/master, x86-public/master) Merge tag
'efi-2023-10-rc2' of
https://source.denx.de/u-boot/custodians/u-boot-efi

>
> >
> >  common/board_f.c      | 3 ++-
> >  drivers/video/Kconfig | 8 ++++++++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/board_f.c b/common/board_f.c
> > index 7d2c380e91e2..5173d0a0c2d5 100644
> > --- a/common/board_f.c
> > +++ b/common/board_f.c
> > @@ -419,7 +419,8 @@ static int reserve_video(void)
> >               if (!ho)
> >                       return log_msg_ret("blf", -ENOENT);
> >               video_reserve_from_bloblist(ho);
> > -             gd->relocaddr = ho->fb;
> > +             if (IS_ENABLED(CONFIG_VIDEO_RESERVE_SPL))
> > +                     gd->relocaddr = ho->fb;
> >       } else if (CONFIG_IS_ENABLED(VIDEO)) {
> >               ulong addr;
> >               int ret;
>
> Ok, so as I understand in your case you have enabled CONFIG_SPL_VIDEO but not
> calling reserve_video in SPL stage ?

Yes, see below.

>
> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index b41dc60cec59..e0e07ed0cda5 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -1106,6 +1106,14 @@ config SPL_VIDEO_REMOVE
> >         if this  option is enabled video driver will be removed at the end of
> >         SPL stage, beforeloading the next stage.
> >
> > +config VIDEO_RESERVE_SPL
> > +     bool
> > +     help
> > +       This adjusts reserve_video() to redirect memory reservation when it
> > +       sees a video handoff blob (BLOBLISTT_U_BOOT_VIDEO). This avoids the
> > +       memory used for video being allocated to U-Boot, thus having some
> {some minor suggestion for correction on above text}
> avoids the memory used for framebuffer from being allocated to u-boot, thus
> preventing any further memory reservations done by u-boot from overwriting the
> framebuffer ?
>
> I think as mentioned earlier, overall this approach looks ok to me too, maybe
> later on we can add some conditions to check validity of passed framebuffer
> address and move the relocaddr pointer more intelligently.

The problem is really that by pre-allocating the display in SPL we
need to excluded it from the reservations done by U-Boot proper. It
should not be done again. For that to work, we may need to tell U-Boot
proper that it should start its reservations at a certain address,
e.g. the bottom of the framebuffer.

Also. could you send a patch to fully fill in the video handoff? At
present it is missing some fields.

Regards,
Simon