[PATCH v3 5/9] board_f: Fix corruption of relocaddr
Devarsh Thakkar
devarsht at ti.com
Wed Aug 16 17:46:05 CEST 2023
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,
2) Also per current reserve flow in u-boot using the reserve_video helper it
reserve page table first and then framebuffer, so we would possibly have to
change that order to do framebuffer reservation first at the top and that's
why we required this condition patch at first place.
3) Also If we are thinking on lines of putting constraints then, I think
current constraint i.e. "Relocation will always happen below the framebuffer
area so that there is no overlap" as we put in the patch
https://lore.kernel.org/all/20230801140414.76216-1-devarsht@ti.com/ seems
little better to me as it gives little bit of flexiblity and avoids problem
mentinoed in 2)
4) Also as per your feedback on this patch at
https://lore.kernel.org/all/CAPnjgZ03iQ6iu3gz=Xkp1vHS43=1XXEk2TKdYgj7v4FhxVbj9g@mail.gmail.com/
you mentioned a scenario where although framebuffer region is below current
relocation pointer but the gap between the two is 1Gb so we don't require to
move relocation pointer and instead continue reservations without worrying
about overlap due to huge gap, so to address such scenarios I thought to go
with this Kconfig approach.
5) From my POV, Ideal solution could be to have some function that estimates
memory requirements for reservation and relocation well in advance and then we
can take decision on whether to continue reservation from current relocaddr or
move it after receiving the framebuffer address from videoblob but I am not
sure how much feasible is it at this point and hence I suggested this Kconfig
based approach.
Regards
Devarsh
>
> Regards,
> Simon
More information about the U-Boot
mailing list