[PATCH v3 5/9] board_f: Fix corruption of relocaddr
Simon Glass
sjg at chromium.org
Mon Sep 11 01:14:55 CEST 2023
Hi Devarsh,
On Thu, 17 Aug 2023 at 09:10, Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, Aug 16, 2023 at 09:16:05PM +0530, Devarsh Thakkar wrote:
> > Hi Simon,
> >
> > On 15/08/23 20:14, Simon Glass wrote:
> > > Hi Devarsh,
> > >
> > > On Tue, 15 Aug 2023 at 03:23, Devarsh Thakkar <devarsht at ti.com> wrote:
> > >>
> > >> Hi Simon, Tom,
> > >>
> > >> On 15/08/23 04:13, Simon Glass wrote:
> > >>> Hi Devarsh, Nikhil, Tom,
> > >>>
> > >>> On Wed, 9 Aug 2023 at 09:29, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >>>>
> > >>>> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> > >>>>>
> > >>>>> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> > >>>>>>
> > >>>>>> On Tue, Aug 1, 2023 at 12:00 AM Simon Glass <sjg at chromium.org> wrote:
> > >>>>>>>
> > >>>>>>> When the video framebuffer comes from the bloblist, we should not change
> > >>>>>>> relocaddr to this address, since it interfers with the normal memory
> > >>>>>>
> > >>>>>> typo: interferes
> > >>>>>>
> > >>>>>>> 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 v3:
> > >>>>>>> - Reword the Kconfig help as suggested
> > >>>>>>>
> > >>>>>>> Changes in v2:
> > >>>>>>> - Add a Kconfig as the suggested conditional did not work
> > >>>>>>>
> > >>>>>>> common/board_f.c | 3 ++-
> > >>>>>>> drivers/video/Kconfig | 9 +++++++++
> > >>>>>>> 2 files changed, 11 insertions(+), 1 deletion(-)
> > >>>>>>>
> > >>>>>>> diff --git a/common/board_f.c b/common/board_f.c
> > >>>>>>> index 7d2c380e91e..5173d0a0c2d 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;
> > >>>>>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > >>>>>>> index b41dc60cec5..f2e56204d52 100644
> > >>>>>>> --- a/drivers/video/Kconfig
> > >>>>>>> +++ b/drivers/video/Kconfig
> > >>>>>>> @@ -1106,6 +1106,15 @@ 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 framebuffer from being allocated by U-Boot proper,
> > >>>>>>> + thus preventing any further memory reservations done by U-Boot proper
> > >>>>>>> + from overwriting the framebuffer.
> > >>>>>>> +
> > >>>>>>> if SPL_SPLASH_SCREEN
> > >>>>>>>
> > >>>>>>> config SPL_SPLASH_SCREEN_ALIGN
> > >>>>>>
> > >>>>>> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> > >>>>>
> > >>>>> applied to u-boot-x86, thanks!
> > >>>>
> > >>>> Dropped this one from the x86 queue per the discussion.
> > >>>
> > >>> I just wanted to come back to this discussion.
> > >>>
> > >>> Do we have an agreed way forward? Who is waiting for who?
> > >>>
> > >>
> > >> I was waiting on feedback on
> > >> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aac34@ti.com/
> > >> but per my opinion, I would prefer to go with "Approach 2" with a
> > >> Kconfig as it looks simpler to me. It would look something like below :
> > >>
> > >> if (gd->relocaddr > (unsigned long)ho->fb) {
> > >> ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
> > >>
> > >> /* Relocate after framebuffer area if nearing too close to it */
> > >> if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> > >> gd->relocaddr = ho->fb;
> > >> }
> > >>
> > >> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
> > >> -> This describes minimum gap to keep between framebuffer address and
> > >> relocation address to avoid overlap when framebuffer address used by
> > >> blob is below the current relocation address
> > >>
> > >> -> It would be selected as default when CONFIG_BLOBLIST is selected with
> > >> default value set to 100Mb
> > >>
> > >> -> SoC specific Vendors can override this in their defconfigs to a
> > >> custom value if they feel 100Mb is not enough
> > >>
> > >> Also probably we can have some debug/error prints in the code to show
> > >> overlap happened (or is going happen) so that users can fine tune this
> > >> Kconfig if they got it wrong at first place.
> > >>
> > >> I can re-spin updated patch if we are aligned on this,
> > >> Kindly let me know your opinion.
> > >
> > > I'm just nervous about the whole idea, TBH. Perhaps I am missing some
> > > documentation on how people are supposed to lay out memory in SPL and
> > > U-Boot properr, but I'm not really aware of any guidance we give.
> > >
> > > Perhaps we should say that the SPL frame buffer should be at the top
> > > of memory, and U-Boot's reserve area should start below that?
> >
> > 1) As per my personal opinion, I don't like putting such constraints and would
> > instead like to give some flexibility to end user for choosing
> > framebuffer area as I earlier mentioned, as for that matter if we are using a
> > predefined address then there is no need of using framebuffer address on
> > videoblob,
>
> I think this is the wrong direction. We need to offer strong defaults
> that shouldn't be deviated without good reason, rather than "pick what
> you want". Very few cases will deviate from the defaults, and of those
> it's hard to know if they're being changed for the best, or because
> someone didn't fully understand the implications and breaks something
> else.
So what is next with this? I would like to clean it up...I feel that
having SPL pass the top of usable RAM (below the framebuffer) is a
reasonable solution. Does constraining things in that way cause any
problems for TI?
Regards,
Simon
More information about the U-Boot
mailing list