arm: Allow u-boot to run from offset base address

On 14-06-10 01:35 PM, Wolfgang Denk wrote:
> Dear Steve,
>
> In message <53975EC2.1080209@broadcom.com> you wrote:
>>
>>> I still cannot understand why _start and CONFIG_SYS_TEXT_BASE would
>>> have to be the same.  There is no such requirement.  What exactly
>>> prevents you from assigning _start a location at offset 0x20 to the
>>> start of the text segment, i. e. CONFIG_SYS_TEXT_BASE ?
>>
>> Wolfgang et al.
>>
>> I agree that they do not need to be the same...
>> So our issue is that basically "for every ARMv7 board in the company",
>> we are currently maintaining our own modified/customized version of
>> "arch/arm/cpu/armv7/start.S" in order to add the appropriate 32 byte
>> header...
>
> There should be more clever ways to implement this.  If nothing else
> comes to mind, an #ifdef in "arch/arm/cpu/armv7/start.S" should be
> sufficient to condistionally insert / adjust any offset that might be
> needed for a specific board.
>
>> And we could choose to do it using other methods, but they all require
>> us to maintain a customized version of linker scripts, or some other
>> code, or ....
>
> ... or a CONFIG setting in your board config file resp. your board's
> defconfig.
>
>> The request here is that with the addition of some relatively
>> straightforward (upstreamed) code, then this can be handled
>> automatically by a post-processing step and we would be able to use a
>> pristine version of the upstreamed code...
>
> I'm sorry, but I disagree especially on the "straightforward" part.
> Also, I see no reason to make the existing code unnecessarily complex,
> and add more disadvantages (like increased meory footprint etc.) when
> the same purpose can be implemented without adding any special code at
> all.
>
>> Our desire is to get this into the beginnings of the "ARMv8" boards, so
>> that we can avoid the maintenance issues we have with the current ARMv7
>> boards.
>>
>> We appreciate your consideration of this request.
>
> These are two different things: implementing a clean and easy way to
> support a necessary feature is one thing; to do it in the suggested
> way by adding code where none is needed is another thing.
>
> I'm all for adding support for any features that are useful, even if
> only for a single user, as long as they don't hurt other users.  All
> I'm asking is to chose another way to implement this feature.  So far,
> I did not see any arguments that my alternative proposals would cause
> any problems to you?
>

OK - I think that one of the alternate proposals would be to 
conditionally reserve a "32 byte block" prior to the _start symbol (in 
"arch/arm/cpu/armv8/start.S") which would then be filled in by a 
post-processing step... This could be implemented by:

  	b	reset

And then in our board config file:
#define CONFIG_CUSTOM_HEADER_RESERVED_BYTES 32

And we could implement a similar algorithm in the armv7/start.S as you 
stated above.

Are we understanding your proposals properly? Would this be an 
acceptable solution?
Thanks, Steve

> Best regards,
>
> Wolfgang Denk
>

Dear Steve,

In message <53979199.5010100@broadcom.com> you wrote:
> 
> OK - I think that one of the alternate proposals would be to 
> conditionally reserve a "32 byte block" prior to the _start symbol (in 
> "arch/arm/cpu/armv8/start.S") which would then be filled in by a 
> post-processing step... This could be implemented by:

Yes, that illustrates the idea.  However, this implementation suffers
from the use of an #ifdef where none is actually needed.  Instead, you
can create your own source file which defines the header; this could
be then even in it's own segment, say:

your_header.c:

struct your_header {
	u_int32[8];
} your_header __attribute__ ((__section__ (".your_hdr")));

All that is needed then is to make the linker place this segment in
front of the text segment.

This avoids an ugly #ifdef, and also modifications in the common code.


Best regards,

Wolfgang Denk

Hi Wolfgang,

On Wed, 11 Jun 2014 06:49:28 +0200, Wolfgang Denk <wd@denx.de> wrote:

> Dear Steve,
> 
> In message <53979199.5010100@broadcom.com> you wrote:
> > 
> > OK - I think that one of the alternate proposals would be to 
> > conditionally reserve a "32 byte block" prior to the _start symbol (in 
> > "arch/arm/cpu/armv8/start.S") which would then be filled in by a 
> > post-processing step... This could be implemented by:
> 
> Yes, that illustrates the idea.  However, this implementation suffers
> from the use of an #ifdef where none is actually needed.  Instead, you
> can create your own source file which defines the header; this could
> be then even in it's own segment, say:
> 
> your_header.c:
> 
> struct your_header {
> 	u_int32[8];
> } your_header __attribute__ ((__section__ (".your_hdr")));
> 
> All that is needed then is to make the linker place this segment in
> front of the text segment.
> 
> This avoids an ugly #ifdef, and also modifications in the common code.

Agreed and seconded.

Plus, using a dedicated 'header' section and a separate C source file
for the header makes it automatic that if no input 'header' section
were provided then no output 'header' section would be emitted; IOW,
we would not need two different linker scripts, a single one would be
useable for both 'headerless' and 'headerful' image types.

Also, the alignment constraint should be configurable.

> Best regards,
> 
> Wolfgang Denk

Amicalement,

On 14-06-10 11:45 PM, Albert ARIBAUD wrote:
> Hi Wolfgang,
>
> On Wed, 11 Jun 2014 06:49:28 +0200, Wolfgang Denk <wd@denx.de> wrote:
>
>> Dear Steve,
>>
>> In message <53979199.5010100@broadcom.com> you wrote:
>>>
>>> OK - I think that one of the alternate proposals would be to
>>> conditionally reserve a "32 byte block" prior to the _start symbol (in
>>> "arch/arm/cpu/armv8/start.S") which would then be filled in by a
>>> post-processing step... This could be implemented by:
>>
>> Yes, that illustrates the idea.  However, this implementation suffers
>> from the use of an #ifdef where none is actually needed.  Instead, you
>> can create your own source file which defines the header; this could
>> be then even in it's own segment, say:
>>
>> your_header.c:
>>
>> struct your_header {
>> 	u_int32[8];
>> } your_header __attribute__ ((__section__ (".your_hdr")));
>>
>> All that is needed then is to make the linker place this segment in
>> front of the text segment.
>>
>> This avoids an ugly #ifdef, and also modifications in the common code.
>
> Agreed and seconded.
>
> Plus, using a dedicated 'header' section and a separate C source file
> for the header makes it automatic that if no input 'header' section
> were provided then no output 'header' section would be emitted; IOW,
> we would not need two different linker scripts, a single one would be
> useable for both 'headerless' and 'headerful' image types.
>
> Also, the alignment constraint should be configurable.
>
>> Best regards,
>>
>> Wolfgang Denk
>
> Amicalement,
>

Albert, Wolfgang, et al.

I didn't know about the automatic handling of "conditional" sections in 
the linker script file - Thanks!!!

So if I add a "your_header.c" as above, then

(1) I need to modify "arch/arm/cpu/armv8/u-boot.lds":
  	. = 0x00000000;

+ 	. = ALIGN(8);
+	.your_hdr : {
+		KEEP(*(.your_hdr*));
+	}
+
	. = ALIGN(8);
  	.text :
  	{

(2) then (I believe) I need to modify the "Makefile" to define the start 
address of this new section:
+LDFLAGS_u-boot += --section-start=".your_hdr"=CONFIG_YOUR_HEADER_ADDR

(3) and (I believe) I need to modify the OBJCOPYFLAGS (somewhere) in 
order to include this new section in the u-boot.bin:
+OBJCOPYFLAGS += -j .your_hdr

(... I don't actually have this working yet; so I suspect more changes 
are required ...)

And in the end, (I believe) I am just going to have a block (likely 4096 
bytes) prepended to the "original" u-boot.bin; which I can do today with 
no code changes at all....

Remember that original design request was effectively a two line change:
+	gd->mon_len += CONFIG_SYS_TEXT_BASE % 4096;
and
+	gd->relocaddr += CONFIG_SYS_TEXT_BASE % 4096;

Regrettably, since this is not going to be accepted, I am abandoning 
this change.
Thanks, Steve

Dear Steve,

In message <5398A640.3050105@broadcom.com> you wrote:
> 
> So if I add a "your_header.c" as above, then
> 
> (1) I need to modify "arch/arm/cpu/armv8/u-boot.lds":
>   	. = 0x00000000;
> 
> + 	. = ALIGN(8);
> +	.your_hdr : {
> +		KEEP(*(.your_hdr*));
> +	}
> +
> 	. = ALIGN(8);
>   	.text :
>   	{

ALIGN(8) looks pretty much bogus to me?

Quoting the "ld" docs:

'ALIGN(ALIGN)'
'ALIGN(EXP,ALIGN)'
     Return the location counter ('.') or arbitrary expression aligned
     to the next ALIGN boundary.  The single operand 'ALIGN' doesn't
     change the value of the location counter--it just does arithmetic
     on it.  The two operand 'ALIGN' allows an arbitrary expression to
     be aligned upwards ('ALIGN(ALIGN)' is equivalent to 'ALIGN(.,
     ALIGN)').

     Here is an example which aligns the output '.data' section to the
     next '0x2000' byte boundary after the preceding section and sets a
     variable within the section to the next '0x8000' boundary after the
     input sections:
          SECTIONS { ...
            .data ALIGN(0x2000): {
              *(.data)
              variable = ALIGN(0x8000);
            }
          ... }
     The first use of 'ALIGN' in this example specifies the location of
     a section because it is used as the optional ADDRESS attribute of a
     section definition (*note Output Section Address::).  The second
     use of 'ALIGN' is used to defines the value of a symbol.


Are you sure you do not ant to hae an ALIGN(4096) there?

> (2) then (I believe) I need to modify the "Makefile" to define the start 
> address of this new section:
> +LDFLAGS_u-boot += --section-start=".your_hdr"=CONFIG_YOUR_HEADER_ADDR

Why?

> (3) and (I believe) I need to modify the OBJCOPYFLAGS (somewhere) in 
> order to include this new section in the u-boot.bin:
> +OBJCOPYFLAGS += -j .your_hdr

Why?

> And in the end, (I believe) I am just going to have a block (likely 4096 
> bytes) prepended to the "original" u-boot.bin; which I can do today with 
> no code changes at all....

Well, we don't chaneg any code here, right? Just the linker script.
It's this, or some other script.  But the linker is the standard way
to create a linked image with the correct memory map, so this is the
right place...

> Remember that original design request was effectively a two line change:

Yes, indeed  But a pretty much bogus one.

Best regards,

Wolfgang Denk

Hi Wolfgang,

On Wed, 11 Jun 2014 23:16:51 +0200, Wolfgang Denk <wd@denx.de> wrote:

> Dear Steve,
> 
> In message <5398A640.3050105@broadcom.com> you wrote:
> > 
> > So if I add a "your_header.c" as above, then
> > 
> > (1) I need to modify "arch/arm/cpu/armv8/u-boot.lds":
> >   	. = 0x00000000;
> > 
> > + 	. = ALIGN(8);
> > +	.your_hdr : {
> > +		KEEP(*(.your_hdr*));
> > +	}
> > +
> > 	. = ALIGN(8);
> >   	.text :
> >   	{
> 
> ALIGN(8) looks pretty much bogus to me?
> 
> Quoting the "ld" docs:
> 
> 'ALIGN(ALIGN)'
> 'ALIGN(EXP,ALIGN)'
>      Return the location counter ('.') or arbitrary expression aligned
>      to the next ALIGN boundary.  The single operand 'ALIGN' doesn't
>      change the value of the location counter--it just does arithmetic
>      on it.  The two operand 'ALIGN' allows an arbitrary expression to
>      be aligned upwards ('ALIGN(ALIGN)' is equivalent to 'ALIGN(.,
>      ALIGN)').
> 
>      Here is an example which aligns the output '.data' section to the
>      next '0x2000' byte boundary after the preceding section and sets a
>      variable within the section to the next '0x8000' boundary after the
>      input sections:
>           SECTIONS { ...
>             .data ALIGN(0x2000): {
>               *(.data)
>               variable = ALIGN(0x8000);
>             }
>           ... }
>      The first use of 'ALIGN' in this example specifies the location of
>      a section because it is used as the optional ADDRESS attribute of a
>      section definition (*note Output Section Address::).  The second
>      use of 'ALIGN' is used to defines the value of a symbol.
> 
> 
> Are you sure you do not ant to hae an ALIGN(4096) there?

Better yet, align to some CONFIG_SYS_ALIGN_xxx option. Linker scripts
are preprocessed, so that should be a breeze.
 
> > (2) then (I believe) I need to modify the "Makefile" to define the start 
> > address of this new section:
> > +LDFLAGS_u-boot += --section-start=".your_hdr"=CONFIG_YOUR_HEADER_ADDR
> 
> Why?

I see no reason for this indeed.

> > (3) and (I believe) I need to modify the OBJCOPYFLAGS (somewhere) in 
> > order to include this new section in the u-boot.bin:
> > +OBJCOPYFLAGS += -j .your_hdr
> 
> Why?

I can see reasons for this one, but it depends on whether the C
file for the added section produces the section's content or just
reserves space for it.

If the C file just reserves space, we can dispense with copying it from
ELF to bin and the -j option above needs not be added (and then, an
external utility should prepend the actual header content to the .bin).

If the C file produces the content, then we *must* copy it from ELF to
bin and the -j option above (and any utility that wants to set the
section's content should overwrite it, not prepend to it). 

I am not personally sure which option is best.

TBH, my personal preference is that if a header must be added to the
image, it should not affect at all the image build, load and run
addresses... For instance, if the image's text section start is
0x00100000 and its entry point is 0x00100100, and we need to add a 4KB
header to it, the resulting biary should *still* load so that the
original image's text segment starts at 0x00100000; if what is loaded
is the pair header+image, then it should load at 0x000FF000 (and the
entry point would still be 0x00100100).

But I can understand that some platfoms /will/ load the header along
with the image, and that in this case, we need to take the header into
account in the build process.

Steve: can the header content be expressed entirely in the source C
module? If it can, then there might be no need for an external utility
to set it -- but that's up for debate, as it might be useful to be able
to modify the header without rebuilding anyway.

> > And in the end, (I believe) I am just going to have a block (likely 4096 
> > bytes) prepended to the "original" u-boot.bin; which I can do today with 
> > no code changes at all....
> 
> Well, we don't chaneg any code here, right? Just the linker script.
> It's this, or some other script.  But the linker is the standard way
> to create a linked image with the correct memory map, so this is the
> right place...

Just nitpicking here, but...

The problem we were presented with initially was as follows:

	link some code for a given run address;
	prepend resulting image with a 4KB header;
	load prepended header followed by image at original image run
	address. (iow, load original image 4KB above intended run
	address)

With the code as it is, we know this fails. Ergo, whatever solution
fixes the issue will *necessarily* change the code. The initial
proposal was to try and put the change in the relocation process; the
final one puts the change the linker script -- which BTW *is* code, at
least in my book, since it is input to the build process and controls
the content of the generated image.

> > Remember that original design request was effectively a two line change:
> 
> Yes, indeed  But a pretty much bogus one.
> 
> Best regards,
> 
> Wolfgang Denk

Amicalement,