[PATCH v3 5/9] board_f: Fix corruption of relocaddr
Simon Glass
sjg at chromium.org
Tue Oct 10 16:57:49 CEST 2023
Hi Devarsh,
On Fri, 22 Sept 2023 at 15:49, Devarsh Thakkar <devarsht at ti.com> wrote:
>
> Hi Simon,
>
> On 22/09/23 23:57, Simon Glass wrote:
> > 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.
> >
>
> Ok, but we still need to have video blob too for conveying frame-buffer
> information, right ?
Yes
>
> Also, just wanted to check if we really require a ram_top blob to be
> passed b/w SPL to u-boot, I thought ram_top addr is know by both stages
> before-hand if so then, why pass the ram_top blob separately?
I don't believe it can be known at build time, if that is what you mean. At
least not in general.
>
> >>
> >> 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.
> >
>
> Ok, thanks for explaining your design, I understand your suggestion here
> and the design you are proposing i.e. of putting framebuffer at the
> ram_top, I am just thinking on that if we are to go with this then what
> is the simplest way to fit it with current u-boot architecture where we
> are using same API i.e. video_reserve at SPL stage and u-boot proper for
> both reserving the memory, passing the blob and catching the blob for
> simplicity.
>
>
> I think we can probably achieve the same thing what you are proposing,
> if at u-boot proper also we follow the same paradigm i.e. reserve the
> video memory first (or for that matter any region which is reserve-able
> by SPL), this way if u-boot call reserve_video function first in this
> sequence
>
https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc4/common/board_f.c?ref_type=tags#L943
> then it will also trigger a call to video_reserve function which can
> read the blob and move the relocaddr as it is already doing (which is
> actually the ram_top since reserve_video is getting called at the start
> with this) after the reserved video area thus avoiding any gaps between
> reservations. This way we don't require to pass ram_top via blob.
>
> Is that possible to update sequence at
>
https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc4/common/board_f.c?ref_type=tags#L943
> to reserve video memory first since it is also shared/reserve-able with
> previous stage ?
> If so then I think we probably don't require much change there-after as
> blob will be read first and further reservation will only start below
> the frame-buffer area even with current video_reserve API.
>
> Kindly let me know your opinion.
Sure, it is worth a try. I don't see any problem with rearranging the
memory reservations.
Regards,
Simon
More information about the U-Boot
mailing list