[PATCH 5/9] board_f: Fix corruption of relocaddr
Nikhil M Jain
n-jain1 at ti.com
Fri Jul 28 10:35:46 CEST 2023
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)
>>>> //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