[PATCH v3 5/9] board_f: Fix corruption of relocaddr
Devarsh Thakkar
devarsht at ti.com
Fri Sep 22 23:49:44 CEST 2023
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 ?
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?
>>
>> 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.
Regards
Devarsh
> Regards,
> Simon
More information about the U-Boot
mailing list