[PATCH 5/9] board_f: Fix corruption of relocaddr
Nikhil M Jain
n-jain1 at ti.com
Fri Jul 28 10:38:07 CEST 2023
On 28/07/23 14:05, Nikhil M Jain wrote:
> Hi Simon,
>
> On 27/07/23 23:31, Simon Glass wrote:
>> Hi Nikhil,
>>
>> On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1 at ti.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 27/07/23 06:23, Simon Glass wrote:
>>>> Hi Devarsh,
>>>>
>>>> On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht at ti.com> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On 26/07/23 02:58, Simon Glass wrote:
>>>>>> Hi Devarsh,
>>>>>>
>>>>>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht at ti.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> On 24/07/23 20:22, Simon Glass wrote:
>>>>>>>> When the video framebuffer comes from the bloblist, we should
>>>>>>>> not change
>>>>>>>> relocaddr to this address, since it interfers with the normal
>>>>>>>> memory
>>>>>>>> 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")
>>>>>>>> ---
>>>>>>>>
>>>>>>>> common/board_f.c | 1 -
>>>>>>>> 1 file changed, 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>>>>>> index 7d2c380e91e2..5c8646b22283 100644
>>>>>>>> --- a/common/board_f.c
>>>>>>>> +++ b/common/board_f.c
>>>>>>>> @@ -419,7 +419,6 @@ static int reserve_video(void)
>>>>>>>> if (!ho)
>>>>>>>> return log_msg_ret("blf", -ENOENT);
>>>>>>>> video_reserve_from_bloblist(ho);
>>>>>>>> - gd->relocaddr = ho->fb;
>>>>>>>
>>>>>>> I think this change was done as relocaddr pointer was required to
>>>>>>> be updated
>>>>>>> to move after frame-buffer reserved area to ensure that any
>>>>>>> further memory
>>>>>>> reservations done using gd->relocaddr (for e.g. in
>>>>>>> reserve_trace/uboot/malloc)
>>>>>>> don't overlap with frame-buffer reserved area passed from blob,
>>>>>>> so I think
>>>>>>> removing this line may cause further memory reservations to
>>>>>>> overlap with
>>>>>>> reserved framebuffer.
>>>>>>>
>>>>>>> Could you please confirm?
>>>>>>
>>>>>> SPL and U-Boot have different memory layouts. The current code is
>>>>>> interrupting U-Boot's careful building up of chunks of memory used
>>>>>> for
>>>>>> different purposes.
>>>>>>
>>>>>
>>>>> But it is possible they could be using similar memory layout for some
>>>>> components like framebuffer.
>>>>> For e.g. in our case we are using same video_reserve func in A53
>>>>> SPL too
>>>>> and which allocates framebuffer from gd->relocaddr as seen here:
>>>>>
>>>>> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427
>>>>
>>>> Even if it is similar, the point is that U-Boot proper needs to do its
>>>> own allocation stuff.
>>>>
>>>> Of course, if SPL sets up the video, it will provide the framebuffer
>>>> address, but only that. The other addresses need to be done using the
>>>> normal mechanism.
>>>>
>>>>>
>>>>>> The video memory has already been allocated by SPL, so we don't need
>>>>>> to insert a new one here, as your code demonstrates.
>>>>>>
>>>>>
>>>>> Agreed.
>>>>>
>>>>>> But also we have no way of knowing if it is legal to relocate U-Boot
>>>>>> (and various other things) just below the frame buffer chosen by SPL.
>>>>>>
>>>>>
>>>>> Yes, so i suppose your case is that framebuffer address which is
>>>>> being passed
>>>>> by SPL is totally disjoint and too far away from gd->relocaddr, for
>>>>> e.g. it is
>>>>> at the start (or bottom of DDR) and doesn't interfere with
>>>>> gd->relocaddr in
>>>>> any manner since relocaddr points to ramtop (i.e. near to end
>>>>> address of DDR).
>>>>>
>>>>> In that case I agree it doesn't make sense to move relocaddr to
>>>>> ho->fb.
>>>>>
>>>>> But for the scenario where gd->relocaddr and ho->fb are nearby
>>>>> there is every
>>>>> possibility that gd->relocaddr may overlap with framebuffer, also
>>>>> the code in
>>>>> reserve_trace, reserve_uboot doesn't have any intelligence or check
>>>>> that it is
>>>>> overlapping with framebuffer area or not.
>>>>>
>>>>> I think one thing that can probably be done here is to have a check
>>>>> that if
>>>>> passed framebuffer area falls within current relocaddr region, then
>>>>> update the
>>>>> relocaddr else don't touch relocaddr :
>>>>>
>>>>> if (ho->fb <= gd->relocaddr - ho->size)
Just a small correction here
if (ho->fb >= gd->relocaddr - ho->size)
>>>>> //It means framebuffer are is overlapping with current
>>>>> relocaddr so update
>>>>> relocaddr
>>>>> gd->relocaddr = ho->fb
>
> We should go ahead with this check because it won't disrupt u-boot's
> allocation of memory and will allow both the cases if a platform is
> using same memory layout or different memory layout across SPL and
> u-boot proper. Below are the logs for both scenarios.
>
> https://gist.github.com/NikMJain/aca198ae77b6f1855459bc8fbdd683df
>
>>>>> else
>>>>> //don't update gd->relocaddr since ho->fb is disjoint to
>>>>> gd->relocaddr
>>>>>
>>>>> Could you please share your opinion on this and if above logic
>>>>> suffice your
>>>>> case too ?
>>>>
>>>> I don't think this line is needed at all, which is why this patch
>>>> removes it. What problem are you seeing?
>>>>
>>> Across SPL stage and U-boot we are keeping same memory layout and
>>> ensuring that same memory regions are used, this way it doesn't
>>> interfere in the way of u-boot while allocating memory regions for
>>> various purposes. This allowed us to display splash screen without any
>>> flicker across the stages.
>>>
>>> Now if you remove the line gd->relocaddr = ho->fb, the frame buffer
>>> region will be used for reserving memory for other purposes which
>>> corrupts the frame buffer.
>>>
>>> One solution which we are planning to implement is move the ram_top to a
>>> lower address leaving out a region for video buffer and u-boot can do
>>> the allocation from the new ram_top address without spl video handoff
>>> interfering in the u-boot's allocation of memory.The region above the
>>> ram_top can be used for video.
>>>
>>> Present Scenario
>>> +---------------------+ram_top
>>> | |
>>> | page_table |
>>> | |
>>> | |
>>> +---------------------+
>>> | |
>>> | |
>>> | |
>>> | |
>>> | |
>>> | video frame buffer |
>>> | |
>>> | |
>>> | |
>>> | |
>>> | |
>>> | |
>>> +---------------------+
>>> | |
>>> | |
>>> | reserve_trace |
>>> | |
>>> | |
>>> | |
>>> +---------------------+
>>>
>>>
>>> Proposed Solution
>>> +---------------------+
>>> | |
>>> | |
>>> | |
>>> | |
>>> | |
>>> | video frame buffer |
>>> | |
>>> | |
>>> | |
>>> | |
>>> | |
>>> +---------------------+ram_top
>>> | |
>>> | |
>>> | page_table |
>>> | |
>>> | |
>>> | |
>>> +---------------------+
>>> | |
>>> | |
>>> | reserve_trace |
>>> | |
>>> | |
>>> +---------------------+
>>
>> Sure, whatever you need to do is fine. You could pass the ram top from
>> SPL to U-Boot perhaps.
>>
>
> In this solution problem arises when user doesn't want's to hand-off
> buffer, the frame buffer region will be reallocated by u-boot or will
> require us to hard code buffer address, which we don't want to do.
>
>> Regards,
>> Simon
>
> Thanks,
> Nikhil
More information about the U-Boot
mailing list