[PATCH v3 5/9] board_f: Fix corruption of relocaddr
Simon Glass
sjg at chromium.org
Fri Sep 22 20:27:27 CEST 2023
Hi Devarsh,
On Tue, 12 Sept 2023 at 08:35, Devarsh Thakkar <devarsht at ti.com> wrote:
>
> Hi Simon,
>
> On 11/09/23 04:44, Simon Glass wrote:
> > 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?
>
> TBH, I am not fully able to visualize how this fits current arch :
>
> So instead of blob address will we be passing ram_top inside the video blob ?
> .Or we will be using separate ram_top blob passing from SPL to u-boot ?
Yes, a separate ram_top in the bloblist, not in the video blob.
>
> Are we planning to enforce some restriction/hardcoding for framebuffer to be
> reserved at specific address (near top of RAM) or it would be just a general
> guideline to keep framebuffer near the RAM top ?
Well it makes sense to put it at the top, since U-Boot relocations
itself to the top.
>
> Currently don't see video_reserve API put such constraint and user is free to
> call it anywhere and it just reserve after previously reserved areas. Now,
> with this approach I guess we would deviate from the agnostic behavior of
> video_reserve API then if we plan to update the same API ?
>
> Also u-boot proper starts reserving regions for MMU and few other stuff much
> before reserving framebuffers so by the time we receive the blob containing
> updated ram_top, we would have already reserved those regions from old ram_top
> so moving the ram_top here seems little counter-intuitive to me. In such
> scenario, as per my opinion better option seems to be moving he gd->relocaddr
> instead.
>
> Lastly, I think as much the user keep framebuffer way from ram_top that much
> memory will be lost even with this approach (as ram_top will be moved after
> framebuffer for u-boot proper) and same behavior will be observed with
> https://lore.kernel.org/all/20230801140414.76216-1-devarsht@ti.com/ too
>
> but if we are planning to put just a general guideline to user to keep
> framebuffer near the RAM top then to me above patch looks much simpler than
> moving the ram_top.
I don't really have any more to say here. This is all just confusing
and I don't think it needs to be. If SPL allocs stuff, I believe it
should do so at the top of memory, then tell U-Boot about it, so
U-Boot's reservations can happen below that address.
Regards,
Simon
More information about the U-Boot
mailing list