Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Devarsh Thakkar <devarsht@ti.com>]

Hi Simon,

On 26/07/23 02:58, Simon Glass wrote:
> Hi Devarsh,
> 
> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@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@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

> 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
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 ?

Regards
Devarsh

> The SPL frame buffer needs to be considered pre-allocated. It makes no
> sense to set relocaddr to this value. It will break all sorts of
> things. E.g. qemu-x86_64 crashes with this.
> 
>>
>>
>> Regards
>> Devarsh
>>
>>>       } else if (CONFIG_IS_ENABLED(VIDEO)) {
>>>               ulong addr;
>>>               int ret;
> 
> Regards,
> Simon