[PATCH 5/9] board_f: Fix corruption of relocaddr
Simon Glass
sjg at chromium.org
Thu Jul 27 20:01:18 CEST 2023
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
> >> 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.
Regards,
Simon
More information about the U-Boot
mailing list