[PATCH v3 5/9] board_f: Fix corruption of relocaddr
Devarsh Thakkar
devarsht at ti.com
Mon Oct 16 18:15:01 CEST 2023
Hi Simon, Tom,
On 10/10/23 20:27, Simon Glass wrote:
> 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
>
Thanks for the feedback, as suggested and discussed, I moved the fb allocation
at the end of RAM for SPL and catching bloblist at the start in u-boot proper
so that it doesn't impact u-boot's
reservation sequence and posted
https://lore.kernel.org/all/20231016160611.1353458-1-devarsht@ti.com/
Could you please have a look at these series and share your opinion ?
P.S I decided against changing reservation sequence at u-boot proper though as
thought that this might potentially break backward compatibility with various
device-trees in linux kernel which are reserving hard-coded framebuffer
addresses based on current sequence of reservation (i.e. video memory being
reserved after page table)
If the approach looks good, then I will do some more testing before sending
out again.
Regards
Devarsh
More information about the U-Boot
mailing list