Re: [PATCH v3 2/3] arm: Add an __image_copy_start symbol for ARMv8

[Scott Wood <oss@buserror.net>]

On Sat, 2021-09-25 at 09:46 -0600, Simon Glass wrote:
> Hi Scott,
> 
> On Thu, 5 Aug 2021 at 13:20, Simon Glass <sjg@chromium.org> wrote:
> > 
> > Hi Scott,
> > 
> > On Wed, 4 Aug 2021 at 13:53, Scott Wood <oss@buserror.net> wrote:
> > > 
> > > On Sun, 2021-08-01 at 14:59 -0600, Simon Glass wrote:
> > > > This symbol is needed for binman to locate the start of the image. Add
> > > > it.
> > > > 
> > > > Note: the existing line to bring in the .__image_copy_start symbol
> > > > does
> > > > not appear to do anything.
> > > > 
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > > I have copied Scott Wood who originally added the line about the
> > > > __image_copy_start in the hope that he can decide if we should remove
> > > > it.
> > > 
> > > It's been a long time since I looked at this stuff, but
> > > __image_copy_start is
> > > used for relocation and that code does not seem to be in the SPL, so the
> > > *(.__image_copy_start) was probably just a copy-and-paste leftover from
> > > the
> > > main SPL that can go away.
> > > 
> > > Of course, that doesn't resolve the binman issue. :-)
> > > 
> > > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-
> > > > boot-
> > > > spl.lds
> > > > index 9edb662b094..2827a07590d 100644
> > > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds
> > > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds
> > > > @@ -22,6 +22,7 @@ ENTRY(_start)
> > > >  SECTIONS
> > > >  {
> > > >         .text : {
> > > > +               __image_copy_start = .;
> > > >                 . = ALIGN(8);
> > > >                 *(.__image_copy_start)
> > > >                 CPUDIR/start.o (.text*)
> > > 
> > > If for whatever reason you did need to define the symbol this way,
> > > shouldn't
> > > it be after the alignment?
> > 
> > Well I don't want to miss out any of the image, otherwise the offsets
> > would be off.
> > 
> > But perhaps that is another question. Since this is the first section,
> > it should already be aligned (to 16 I suspect). So why do we need it?
> 
> I'd like to get this applied, assuming it is correct, because at
> present binman is silently ignoring problems where it cannot find
> symbols. The fix for that is:
> 
> http://patchwork.ozlabs.org/project/uboot/patch/20210722210216.1.Id1246d1ff1cb5750f8c7ddde9665cf6f09615a7c@changeid/
> 
> which has been sitting there for a while.

I'm not sure what you need from me... as I said before, as far as I can tell
the __image_copy_start stuff should not be needed by the SPL itself.  If you
want to add it just for binman, there should probably be a comment to that
effect, but I'm not the ARM maintainer (or involved in U-Boot development at
all these days) so I can't help you get anything applied.

As for why the alignment is there, it's probably just paranoia, but in any
case, the SPL linker script probably shouldn't handle alignment in a
gratuitously different way from the main linker script.

-Scott