[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Darwin Rambo]

If an earlier loader stage requires an image header and a specific
offset, then u-boot's base address (CONFIG_SYS_TEXT_BASE) may be
advanced beyond an aligned address. In this case the relocation
will not be done correctly for some sections such as vectors and rodata
string tables, which will show an incorrect offset after the symbols
are fixed up. Advance the relocation address by the image offset so that
the gd->reloc_off used will result in relocating these sections to their
correct addresses.

This change is done under CONFIG_ARM64 conditional compilation
because it has only been tested there and may not be appropriate
for other architectures.

Signed-off-by: Darwin Rambo <drambo@broadcom.com>
Reviewed-by: Steve Rae <srae@broadcom.com>
---

 arch/arm/lib/board.c |   13 +++++++++++++
 common/board_f.c     |   10 ++++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index 9b473b5..df520cc 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -449,6 +449,19 @@ void board_init_f(ulong bootflag)
 	dram_init_banksize();
 	display_dram_config();	/* and display it */
 
+#ifdef CONFIG_ARM64
+	/*
+	 * Fix relocation if u-boot is not on an aligned address.
+	 */
+	{
+		int offset = CONFIG_SYS_TEXT_BASE % 4096;
+		if (offset) {
+			addr += offset;
+			debug("Relocation Addr bumped to 0x%08lx\n", addr);
+		}
+	}
+#endif
+
 	gd->relocaddr = addr;
 	gd->start_addr_sp = addr_sp;
 	gd->reloc_off = addr - (ulong)&_start;
diff --git a/common/board_f.c b/common/board_f.c
index 4ea4cb2..1035d6f 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -728,6 +728,16 @@ static int reloc_fdt(void)
 
 static int setup_reloc(void)
 {
+#ifdef CONFIG_ARM64
+	/*
+	 * Fix relocation if u-boot is not on an aligned address.
+	 */
+	int offset = CONFIG_SYS_TEXT_BASE % 4096;
+	if (offset) {
+		gd->relocaddr += offset;
+		debug("Relocation Addr bumped to 0x%08lx\n", gd->relocaddr);
+	}
+#endif
 	gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE;
 	memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
 
-- 
1.7.9.5

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Jeroen Hofstee]

Hello Darwin,

On wo, 2014-05-14 at 15:05 -0700, Darwin Rambo wrote:

> +#ifdef CONFIG_ARM64
> +	/*
> +	 * Fix relocation if u-boot is not on an aligned address.
> +	 */
> +	{
> +		int offset = CONFIG_SYS_TEXT_BASE % 4096;
> +		if (offset) {
> +			addr += offset;
> +			debug("Relocation Addr bumped to 0x%08lx\n", addr);
> +		}
> +	}
> +#endif
> +

Do you really want to check a define at runtime? Placement is typically
at the end of RAM and allocation goes down, not up as in this patch.
Aren't you overlapping memory here?

>  
>  static int setup_reloc(void)
>  {
> +#ifdef CONFIG_ARM64
> +	/*
> +	 * Fix relocation if u-boot is not on an aligned address.
> +	 */
> +	int offset = CONFIG_SYS_TEXT_BASE % 4096;
> +	if (offset) {
> +		gd->relocaddr += offset;
> +		debug("Relocation Addr bumped to 0x%08lx\n", gd->relocaddr);
> +	}
> +#endif
>  	gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE;
>  	memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
>  

This is a generic file, hell breaks loose if you start using arch /
board / pre bootloader specific workarounds here afaiac.

lucky for you, I am not a u-boot maintainer, but this looks at least a
bit weird, glancing at it.

Regards,
Jeroen

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Darwin Rambo]

On 14-05-14 03:41 PM, Jeroen Hofstee wrote:
> Hello Darwin,
>
> On wo, 2014-05-14 at 15:05 -0700, Darwin Rambo wrote:
>
>> +#ifdef CONFIG_ARM64
>> +	/*
>> +	 * Fix relocation if u-boot is not on an aligned address.
>> +	 */
>> +	{
>> +		int offset = CONFIG_SYS_TEXT_BASE % 4096;
>> +		if (offset) {
>> +			addr += offset;
>> +			debug("Relocation Addr bumped to 0x%08lx\n", addr);
>> +		}
>> +	}
>> +#endif
>> +
> Do you really want to check a define at runtime? Placement is typically
> at the end of RAM and allocation goes down, not up as in this patch.
> Aren't you overlapping memory here?

Yes, I wanted the runtime check since the adjustment to the relocation
address is also done at runtime.

There is no overlap here. The reason is that the original masking operation
to mask to a 4K boundary removed the small offset and backed up too far. So
adding the lost offset is guaranteed to not overlap, and furthermore, correct
the relocation offset so that arm64 images can run at smaller alignments than
we normally use. This might even be a generic fix but can't be tested easily
by me.

>
>>   
>>   static int setup_reloc(void)
>>   {
>> +#ifdef CONFIG_ARM64
>> +	/*
>> +	 * Fix relocation if u-boot is not on an aligned address.
>> +	 */
>> +	int offset = CONFIG_SYS_TEXT_BASE % 4096;
>> +	if (offset) {
>> +		gd->relocaddr += offset;
>> +		debug("Relocation Addr bumped to 0x%08lx\n", gd->relocaddr);
>> +	}
>> +#endif
>>   	gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE;
>>   	memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
>>   
> This is a generic file, hell breaks loose if you start using arch /
> board / pre bootloader specific workarounds here afaiac.

I don't disagree with this statement. Please see my other comments to
Wolfgang on this topic.

>
> lucky for you, I am not a u-boot maintainer, but this looks at least a
> bit weird, glancing at it.
>
> Regards,
> Jeroen
>
Thanks for your comments Jeroen. They are appreciated.
Darwin

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

Dear Darwin,

In message <5374CD55.3010703@broadcom.com> you wrote:
> 
> > Do you really want to check a define at runtime? Placement is typically
> > at the end of RAM and allocation goes down, not up as in this patch.
> > Aren't you overlapping memory here?
> 
> Yes, I wanted the runtime check since the adjustment to the relocation
> address is also done at runtime.

This makes no sense to me.  CONFIG_SYS_TEXT_BASE is a compile time
constant.  So the result of all this is always known at compile time,
too.  I feel you misunderstand that CONFIG_SYS_TEXT_BASE is just the
start address of the text segment.  If you want to offset this by a
specific amount, you can just define this as needed.

> There is no overlap here. The reason is that the original masking operation
> to mask to a 4K boundary removed the small offset and backed up too far. So
> adding the lost offset is guaranteed to not overlap, and furthermore, correct
> the relocation offset so that arm64 images can run at smaller alignments than
> we normally use. This might even be a generic fix but can't be tested easily
> by me.

Argh... This is black magic depending on specific properties of your
process (which you don;t really explain).  Sorry, but this is a full
NAK for code that is build on sand like this.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I find this a nice feature but it is not according to  the  documen-
tation. Or is it a BUG?"   "Let's call it an accidental feature. :-)"
                       - Larry Wall in <6909@jpl-devvax.JPL.NASA.GOV>

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Darwin Rambo]

On 14-05-15 08:21 AM, Wolfgang Denk wrote:
> Dear Darwin,
>
> In message <5374CD55.3010703@broadcom.com> you wrote:
>>> Do you really want to check a define at runtime? Placement is typically
>>> at the end of RAM and allocation goes down, not up as in this patch.
>>> Aren't you overlapping memory here?
>> Yes, I wanted the runtime check since the adjustment to the relocation
>> address is also done at runtime.
> This makes no sense to me.  CONFIG_SYS_TEXT_BASE is a compile time
> constant.  So the result of all this is always known at compile time,
> too.  I feel you misunderstand that CONFIG_SYS_TEXT_BASE is just the
> start address of the text segment.  If you want to offset this by a
> specific amount, you can just define this as needed.

May I respectfully ask that you please bear with me a just a bit longer so
I can try to explain things better?

CONFIG_SYS_TEXT_BASE is also used by the relocation calculations at runtime
to determine the relocation offset, which is also used for the symbol fixup
logic. It is known at compile time, but is used at runtime by the stock
u-boot code to determine the relocation offset. I am doing nothing more than
existing code in this regard.

If I set it to 0x88000020, then the code crashes because the symbol fixup
depends on the relocation offset which is now wrong. The reason is that the
relocation offset calculated in the code doesn't account for this offset.
If we adjust the relocation address, then the relocation offset is now
correct and the symbol fixups work perfectly.

Here's an example:
CONFIG_SYS_TEXT_BASE = 0x88000020
relocated address = 0xfffa8000   (This 4K alignment assumption breaks the relocation)
relocation offset = 0x77fa7fe0   (This is now wrong and the relocation breaks)

My patch does the following:
CONFIG_SYS_TEXT_BASE = 0x88000020
relocated address = 0xfffa8020
relocation offset = 0x77fa8000   (symbol fixups now work)

So this patch just gives us a way to run u-boot at text sections with
smaller alignments. In the past I never really understood why
CONFIG_SYS_TEXT_BASE had to be on specific alignments like XXXXX000 for
our architecture and the symbol fixup issue helps to explain this.

I know this is not the normal use case but it does fix the crash in the
symbol relocation with arm64 and allows u-boot to be more flexible in
it's text alignment requirements.

Thanks.

Best regards,
Darwin

>
>> There is no overlap here. The reason is that the original masking operation
>> to mask to a 4K boundary removed the small offset and backed up too far. So
>> adding the lost offset is guaranteed to not overlap, and furthermore, correct
>> the relocation offset so that arm64 images can run at smaller alignments than
>> we normally use. This might even be a generic fix but can't be tested easily
>> by me.
> Argh... This is black magic depending on specific properties of your
> process (which you don;t really explain).  Sorry, but this is a full
> NAK for code that is build on sand like this.
>
> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

Dear Darwin,

In message <5374E64B.1060104@broadcom.com> you wrote:
> 
> > This makes no sense to me.  CONFIG_SYS_TEXT_BASE is a compile time
> > constant.  So the result of all this is always known at compile time,
> > too.  I feel you misunderstand that CONFIG_SYS_TEXT_BASE is just the
> > start address of the text segment.  If you want to offset this by a
> > specific amount, you can just define this as needed.
> 
> May I respectfully ask that you please bear with me a just a bit longer so
> I can try to explain things better?
> 
> CONFIG_SYS_TEXT_BASE is also used by the relocation calculations at runtime
> to determine the relocation offset, which is also used for the symbol fixup
> logic. It is known at compile time, but is used at runtime by the stock
> u-boot code to determine the relocation offset. I am doing nothing more than
> existing code in this regard.

Let's have a look at your proposed code:

>> +		int offset = CONFIG_SYS_TEXT_BASE % 4096;
>> +		if (offset) {
>> +			addr += offset;
>> +			debug("Relocation Addr bumped to 0x%08lx\n", addr);
>> +		}

> If I set it to 0x88000020, then the code crashes because the symbol fixup
> depends on the relocation offset which is now wrong. The reason is that the
> relocation offset calculated in the code doesn't account for this offset.
> If we adjust the relocation address, then the relocation offset is now
> correct and the symbol fixups work perfectly.

First, please keep in mind that you cannot set CONFIG_SYS_TEXT_BASE in
arbitrary ways, at least not without adjusting your linker script
accordingly; also you may have limitations due to the way how the code
gets loaded / booted.  

_If_ your code is correct, then the target address computed for the
relocation is completely irrelevant.

> Here's an example:
> CONFIG_SYS_TEXT_BASE = 0x88000020
> relocated address = 0xfffa8000   (This 4K alignment assumption breaks the relocation)
> relocation offset = 0x77fa7fe0   (This is now wrong and the relocation breaks)
> 
> My patch does the following:
> CONFIG_SYS_TEXT_BASE = 0x88000020
> relocated address = 0xfffa8020
> relocation offset = 0x77fa8000   (symbol fixups now work)

You are mixing up copying code to another address range and relocating
symbols.

> So this patch just gives us a way to run u-boot at text sections with
> smaller alignments. In the past I never really understood why
> CONFIG_SYS_TEXT_BASE had to be on specific alignments like XXXXX000 for
> our architecture and the symbol fixup issue helps to explain this.

In many cases there is no strict reason.  When storing the images in
NOR flash or other block oriented storage it is usually convenient to
align them to sector boundaaries.  In systems where you have a MMU on,
it may be useful / convenient / necessary to have it aligned to page
boundaries.  But al this is totally irrelevant here.  Consider the
address arbitrarily chosen, it does not matter.

Also, moving the relocation address around by arbitrary amounts (that
are big enough not to cause alignment issues, say anything that is a
multiple of the cache line size) does not change anything.

If there are problems, these are somewhere in your implementation, and
not in the generic code.

> I know this is not the normal use case but it does fix the crash in the
> symbol relocation with arm64 and allows u-boot to be more flexible in
> it's text alignment requirements.

You ask for a "flexibility" that actually is already there; it's just
conveniend when debugging etc. to have "nice" addresses.  If there is
something not working, it's in your code.

Setting CONFIG_SYS_TEXT_BASE to something like 0x88000020 is extremly
fishy.  If you want to add some header data to your image, you should
not shift the text segment, but rather include your header in the
start of the text segment.  Or keep it completely separate, without
messing with CONFIG_SYS_TEXT_BASE.  


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"We don't care.  We don't have to.  We're the Phone Company."

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Albert ARIBAUD]

Hi Wolfgang, Darwin,

On Thu, 15 May 2014 21:19:57 +0200, Wolfgang Denk <wd@denx.de> wrote:

> Setting CONFIG_SYS_TEXT_BASE to something like 0x88000020 is extremly
> fishy.  If you want to add some header data to your image, you should
> not shift the text segment, but rather include your header in the
> start of the text segment.  Or keep it completely separate, without
> messing with CONFIG_SYS_TEXT_BASE.  

Back to the origin of the discussion and patch:

Darwin, can you describe the actual technical difficulty which caused
you to you write and submitting this patch?

> Best regards,
> 
> Wolfgang Denk

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Darwin Rambo]

Hi Albert,

The previous stage bootloader (which I had no control over) wanted it's 
header to be aligned to a 512 byte MMC block boundary, presumably since 
this allowed DMA operations without copy/shifting. At the same time, I 
didn't want to hack a header into start.S because I didn't want to carry 
another downstream patch. So I investigated if I could shift u-boot's 
base address as a feature that would allow an aligned header to be used 
without the start.S patch.

I know that a custom header patch to start.S would work, and that a 
header plus padding will also work. But I found out that you can align 
the base on certain smaller offsets if you keep the relocation offset at 
nice boundaries like 0x1000 and if the relocation offset is a multiple 
of the maximum alignment requirements of the image.

The original patch I submitted didn't handle an end condition properly, 
was ARM64-specific (wasn't tested on other architectures), and because 
the patch was NAK'd, I didn't bother to submit a v2 patch and consider 
the idea to be dead. I'm happy to abandon the patch. I hope this helps.

Best regards,
Darwin

On 14-05-26 02:50 AM, Albert ARIBAUD wrote:
> Hi Wolfgang, Darwin,
>
> On Thu, 15 May 2014 21:19:57 +0200, Wolfgang Denk <wd@denx.de> wrote:
>
>> Setting CONFIG_SYS_TEXT_BASE to something like 0x88000020 is extremly
>> fishy.  If you want to add some header data to your image, you should
>> not shift the text segment, but rather include your header in the
>> start of the text segment.  Or keep it completely separate, without
>> messing with CONFIG_SYS_TEXT_BASE.
>
> Back to the origin of the discussion and patch:
>
> Darwin, can you describe the actual technical difficulty which caused
> you to you write and submitting this patch?
>
>> Best regards,
>>
>> Wolfgang Denk
>
> Amicalement,
>

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Albert ARIBAUD]

Hi Darwin,

On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com>
wrote:

> Hi Albert,
> 
> The previous stage bootloader (which I had no control over) wanted it's 
> header to be aligned to a 512 byte MMC block boundary, presumably since 
> this allowed DMA operations without copy/shifting. At the same time, I 
> didn't want to hack a header into start.S because I didn't want to carry 
> another downstream patch. So I investigated if I could shift u-boot's 
> base address as a feature that would allow an aligned header to be used 
> without the start.S patch.
> 
> I know that a custom header patch to start.S would work, and that a 
> header plus padding will also work. But I found out that you can align 
> the base on certain smaller offsets if you keep the relocation offset at 
> nice boundaries like 0x1000 and if the relocation offset is a multiple 
> of the maximum alignment requirements of the image.
> 
> The original patch I submitted didn't handle an end condition properly, 
> was ARM64-specific (wasn't tested on other architectures), and because 
> the patch was NAK'd, I didn't bother to submit a v2 patch and consider 
> the idea to be dead. I'm happy to abandon the patch. I hope this helps.

Thanks.

If I understand correctly, your target has a requirement for storing
the image on a 512-byte boundary. But how does this affect the loading
of the image into RAM, where the requirement is only that the vectors
table be 32-bytes aligned? I mean, if you store the image in MMC at
offset 0x200 (thus satisfying the 512-byte boundary requirement) and
load it to, say, offset 0x10020 in RAM, how is it a problem for
your target?

If my example above inadequately represents the issue, then can you
please provide a similar but adequate example, a failure case scenario,
so that I can hve a correct understanding of the problem?

> Best regards,
> Darwin

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Darwin Rambo]

On 14-06-02 12:26 AM, Albert ARIBAUD wrote:
> Hi Darwin,
>
> On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com>
> wrote:
>
>> Hi Albert,
>>
>> The previous stage bootloader (which I had no control over) wanted it's
>> header to be aligned to a 512 byte MMC block boundary, presumably since
>> this allowed DMA operations without copy/shifting. At the same time, I
>> didn't want to hack a header into start.S because I didn't want to carry
>> another downstream patch. So I investigated if I could shift u-boot's
>> base address as a feature that would allow an aligned header to be used
>> without the start.S patch.
>>
>> I know that a custom header patch to start.S would work, and that a
>> header plus padding will also work. But I found out that you can align
>> the base on certain smaller offsets if you keep the relocation offset at
>> nice boundaries like 0x1000 and if the relocation offset is a multiple
>> of the maximum alignment requirements of the image.
>>
>> The original patch I submitted didn't handle an end condition properly,
>> was ARM64-specific (wasn't tested on other architectures), and because
>> the patch was NAK'd, I didn't bother to submit a v2 patch and consider
>> the idea to be dead. I'm happy to abandon the patch. I hope this helps.
>
> Thanks.
>
> If I understand correctly, your target has a requirement for storing
> the image on a 512-byte boundary. But how does this affect the loading
> of the image into RAM, where the requirement is only that the vectors
> table be 32-bytes aligned? I mean, if you store the image in MMC at
> offset 0x200 (thus satisfying the 512-byte boundary requirement) and
> load it to, say, offset 0x10020 in RAM, how is it a problem for
> your target?
>
> If my example above inadequately represents the issue, then can you
> please provide a similar but adequate example, a failure case scenario,
> so that I can hve a correct understanding of the problem?

Hi Albert,

The constraints I have that I can't change, are that
- the 32 byte header is postprocessed and prepended to the image after 
the build is complete
- the header is at a 512 byte alignment in MMC
- the header and image are copied to SDRAM to an alignment like 
0x88000000. Thus the u-boot image is linked at and starts at 0x88000020.
- the vectors need to be 0x800 aligned for armv8 (.align 11 directive)

So the failure case is that when the relocation happens, it relocates to 
a 0x1000 alignment, say something like 0xffffa000. The relocation offset 
is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation 
fails. Adjusting the relocation offset to a multiple of 0x1000 (by 
making the relocation address end in 0xNNNNN020) fixes the issues and 
allows u-boot to relocate and run from this address without failing. I 
hope this helps explain it a bit better.

Best regards,
Darwin

>
>> Best regards,
>> Darwin
>
> Amicalement,
>

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Albert ARIBAUD]

Hi Darwin,

On Mon, 2 Jun 2014 17:37:25 -0700, Darwin Rambo <drambo@broadcom.com>
wrote:

> 
> 
> On 14-06-02 12:26 AM, Albert ARIBAUD wrote:
> > Hi Darwin,
> >
> > On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com>
> > wrote:
> >
> >> Hi Albert,
> >>
> >> The previous stage bootloader (which I had no control over) wanted it's
> >> header to be aligned to a 512 byte MMC block boundary, presumably since
> >> this allowed DMA operations without copy/shifting. At the same time, I
> >> didn't want to hack a header into start.S because I didn't want to carry
> >> another downstream patch. So I investigated if I could shift u-boot's
> >> base address as a feature that would allow an aligned header to be used
> >> without the start.S patch.
> >>
> >> I know that a custom header patch to start.S would work, and that a
> >> header plus padding will also work. But I found out that you can align
> >> the base on certain smaller offsets if you keep the relocation offset at
> >> nice boundaries like 0x1000 and if the relocation offset is a multiple
> >> of the maximum alignment requirements of the image.
> >>
> >> The original patch I submitted didn't handle an end condition properly,
> >> was ARM64-specific (wasn't tested on other architectures), and because
> >> the patch was NAK'd, I didn't bother to submit a v2 patch and consider
> >> the idea to be dead. I'm happy to abandon the patch. I hope this helps.
> >
> > Thanks.
> >
> > If I understand correctly, your target has a requirement for storing
> > the image on a 512-byte boundary. But how does this affect the loading
> > of the image into RAM, where the requirement is only that the vectors
> > table be 32-bytes aligned? I mean, if you store the image in MMC at
> > offset 0x200 (thus satisfying the 512-byte boundary requirement) and
> > load it to, say, offset 0x10020 in RAM, how is it a problem for
> > your target?
> >
> > If my example above inadequately represents the issue, then can you
> > please provide a similar but adequate example, a failure case scenario,
> > so that I can hve a correct understanding of the problem?
> 
> Hi Albert,
> 
> The constraints I have that I can't change, are that
> - the 32 byte header is postprocessed and prepended to the image after 
> the build is complete
> - the header is at a 512 byte alignment in MMC
> - the header and image are copied to SDRAM to an alignment like 
> 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020.
> - the vectors need to be 0x800 aligned for armv8 (.align 11 directive)

So far, so good -- I understand that the link-time location of the
vectors table is incorrect.

> So the failure case is that when the relocation happens, it relocates to 
> a 0x1000 alignment, say something like 0xffffa000. The relocation offset 
> is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation 
> fails.

What does "relocation fails" mean exactly, i.e., where and how exactly
does the relocation code behave differently from expected? I'm asking
because I don't understand why the relocation offset should be a
multiple of 0x1000.

> Adjusting the relocation offset to a multiple of 0x1000 (by 
> making the relocation address end in 0xNNNNN020) fixes the issues and 
> allows u-boot to relocate and run from this address without failing. I 
> hope this helps explain it a bit better.

I do understand, however, that if the relocation offset must indeed be a
multiple of 0x1000, then obviously the vectors table will end up as
misaligned as it was before relocation.

Also, personally I would like it if the vectors table was always
aligned as it should, and there are at least three other boards which
require a prefix/header before their vectors table, as Masahiro (cc:)
indicated recently, so that makes the problem a generic one: how to
properly integrate a header in-image (as opposed to an out-of-image
one, which is just a matter of doing a 'cat', so to speak.

Therefore I'd like a generic solution to this, where the header is
prepended *and* aligned properly without breaking the start symbol
alignment constraints. This /might/ be possible by cleverly adding
a '.header' or '.signature' section to the linker script, possibly
doing a two-stage link; but this should not require the source code to
contain ad hoc relocation tricks.

Let me tinker with it in the next few days; I'll try and come up with a
clean and generic solution to this "in-code header" question.

Thanks again for your explanation!

> Best regards,
> Darwin

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Steve Rae]

On 14-06-09 03:23 AM, Albert ARIBAUD wrote:
> Hi Darwin,
>
> On Mon, 2 Jun 2014 17:37:25 -0700, Darwin Rambo <drambo@broadcom.com>
> wrote:
>
>>
>>
>> On 14-06-02 12:26 AM, Albert ARIBAUD wrote:
>>> Hi Darwin,
>>>
>>> On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com>
>>> wrote:
>>>
>>>> Hi Albert,
>>>>
>>>> The previous stage bootloader (which I had no control over) wanted it's
>>>> header to be aligned to a 512 byte MMC block boundary, presumably since
>>>> this allowed DMA operations without copy/shifting. At the same time, I
>>>> didn't want to hack a header into start.S because I didn't want to carry
>>>> another downstream patch. So I investigated if I could shift u-boot's
>>>> base address as a feature that would allow an aligned header to be used
>>>> without the start.S patch.
>>>>
>>>> I know that a custom header patch to start.S would work, and that a
>>>> header plus padding will also work. But I found out that you can align
>>>> the base on certain smaller offsets if you keep the relocation offset at
>>>> nice boundaries like 0x1000 and if the relocation offset is a multiple
>>>> of the maximum alignment requirements of the image.
>>>>
>>>> The original patch I submitted didn't handle an end condition properly,
>>>> was ARM64-specific (wasn't tested on other architectures), and because
>>>> the patch was NAK'd, I didn't bother to submit a v2 patch and consider
>>>> the idea to be dead. I'm happy to abandon the patch. I hope this helps.
>>>
>>> Thanks.
>>>
>>> If I understand correctly, your target has a requirement for storing
>>> the image on a 512-byte boundary. But how does this affect the loading
>>> of the image into RAM, where the requirement is only that the vectors
>>> table be 32-bytes aligned? I mean, if you store the image in MMC at
>>> offset 0x200 (thus satisfying the 512-byte boundary requirement) and
>>> load it to, say, offset 0x10020 in RAM, how is it a problem for
>>> your target?
>>>
>>> If my example above inadequately represents the issue, then can you
>>> please provide a similar but adequate example, a failure case scenario,
>>> so that I can hve a correct understanding of the problem?
>>
>> Hi Albert,
>>
>> The constraints I have that I can't change, are that
>> - the 32 byte header is postprocessed and prepended to the image after
>> the build is complete
>> - the header is at a 512 byte alignment in MMC
>> - the header and image are copied to SDRAM to an alignment like
>> 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020.
>> - the vectors need to be 0x800 aligned for armv8 (.align 11 directive)
>
> So far, so good -- I understand that the link-time location of the
> vectors table is incorrect.
>
>> So the failure case is that when the relocation happens, it relocates to
>> a 0x1000 alignment, say something like 0xffffa000. The relocation offset
>> is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation
>> fails.
>
> What does "relocation fails" mean exactly, i.e., where and how exactly
> does the relocation code behave differently from expected? I'm asking
> because I don't understand why the relocation offset should be a
> multiple of 0x1000.
>
>> Adjusting the relocation offset to a multiple of 0x1000 (by
>> making the relocation address end in 0xNNNNN020) fixes the issues and
>> allows u-boot to relocate and run from this address without failing. I
>> hope this helps explain it a bit better.
>
> I do understand, however, that if the relocation offset must indeed be a
> multiple of 0x1000, then obviously the vectors table will end up as
> misaligned as it was before relocation.
>
> Also, personally I would like it if the vectors table was always
> aligned as it should, and there are at least three other boards which
> require a prefix/header before their vectors table, as Masahiro (cc:)
> indicated recently, so that makes the problem a generic one: how to
> properly integrate a header in-image (as opposed to an out-of-image
> one, which is just a matter of doing a 'cat', so to speak.
>
> Therefore I'd like a generic solution to this, where the header is
> prepended *and* aligned properly without breaking the start symbol
> alignment constraints. This /might/ be possible by cleverly adding
> a '.header' or '.signature' section to the linker script, possibly
> doing a two-stage link; but this should not require the source code to
> contain ad hoc relocation tricks.
>
> Let me tinker with it in the next few days; I'll try and come up with a
> clean and generic solution to this "in-code header" question.
>
> Thanks again for your explanation!
>
>> Best regards,
>> Darwin
>
> Amicalement,
>

Perhaps an oversimplified example of the current code would help to 
explain this better:

scenario #1:
CONFIG_SYS_TEXT_BASE		0x88000000
vectors:	.align 11	/* exception vectors need to be on a 0x800 byte 
boundary */
compile/linker produces (before relocation):
_start	symbol is at		0x88000000
vectors	symbol is at		0x88000800
the relocation offset is:	0x77f9b000
therefore, after relocation:
_start	symbol is at		0xfff9b000 (0x88000000+0xfff9b000)
vectors	symbol is at		0xfff9b800 (0x88000800+0x77f9b000)

scenario #2:
CONFIG_SYS_TEXT_BASE		0x88000020
vectors:	.align 11	/* exception vectors need to be on a 0x800 byte 
boundary */
compiler/linker produces (before relocation):
_start	symbol is at		0x88000020
vectors	symbol is at		0x88000800
the relocation offset is:	0x77f9afe0
therefore, after relocation:
_start	symbol is at		0xfff9b000 (0x88000020+0x77f9afe0)
vectors	symbol is at		0xfff9b7e0 (0x88000800+0x77f9afe0)

Note that in scenario #2, after relocation, the vectors are not on a 
0x800 byte boundary anymore.

Thanks, Steve

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Jeroen Hofstee]

On ma, 2014-06-09 at 13:45 -0700, Steve Rae wrote:
> 
[snip]
> Note that in scenario #2, after relocation, the vectors are not on a 
> 0x800 byte boundary anymore.
> 

As long as the compiler emits adrp instructions, which it already does
at -O0, it is not only limited to vectors. No code works as expected
after relocation.

Regards,
Jeroen

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Albert ARIBAUD]

Hi Steve,

(sorry for the duplicate)

On Mon, 9 Jun 2014 13:45:50 -0700, Steve Rae <srae@broadcom.com> wrote:

> 
> 
> On 14-06-09 03:23 AM, Albert ARIBAUD wrote:
> > Hi Darwin,
> >
> > On Mon, 2 Jun 2014 17:37:25 -0700, Darwin Rambo <drambo@broadcom.com>
> > wrote:
> >
> >>
> >>
> >> On 14-06-02 12:26 AM, Albert ARIBAUD wrote:
> >>> Hi Darwin,
> >>>
> >>> On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com>
> >>> wrote:
> >>>
> >>>> Hi Albert,
> >>>>
> >>>> The previous stage bootloader (which I had no control over) wanted it's
> >>>> header to be aligned to a 512 byte MMC block boundary, presumably since
> >>>> this allowed DMA operations without copy/shifting. At the same time, I
> >>>> didn't want to hack a header into start.S because I didn't want to carry
> >>>> another downstream patch. So I investigated if I could shift u-boot's
> >>>> base address as a feature that would allow an aligned header to be used
> >>>> without the start.S patch.
> >>>>
> >>>> I know that a custom header patch to start.S would work, and that a
> >>>> header plus padding will also work. But I found out that you can align
> >>>> the base on certain smaller offsets if you keep the relocation offset at
> >>>> nice boundaries like 0x1000 and if the relocation offset is a multiple
> >>>> of the maximum alignment requirements of the image.
> >>>>
> >>>> The original patch I submitted didn't handle an end condition properly,
> >>>> was ARM64-specific (wasn't tested on other architectures), and because
> >>>> the patch was NAK'd, I didn't bother to submit a v2 patch and consider
> >>>> the idea to be dead. I'm happy to abandon the patch. I hope this helps.
> >>>
> >>> Thanks.
> >>>
> >>> If I understand correctly, your target has a requirement for storing
> >>> the image on a 512-byte boundary. But how does this affect the loading
> >>> of the image into RAM, where the requirement is only that the vectors
> >>> table be 32-bytes aligned? I mean, if you store the image in MMC at
> >>> offset 0x200 (thus satisfying the 512-byte boundary requirement) and
> >>> load it to, say, offset 0x10020 in RAM, how is it a problem for
> >>> your target?
> >>>
> >>> If my example above inadequately represents the issue, then can you
> >>> please provide a similar but adequate example, a failure case scenario,
> >>> so that I can hve a correct understanding of the problem?
> >>
> >> Hi Albert,
> >>
> >> The constraints I have that I can't change, are that
> >> - the 32 byte header is postprocessed and prepended to the image after
> >> the build is complete
> >> - the header is at a 512 byte alignment in MMC
> >> - the header and image are copied to SDRAM to an alignment like
> >> 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020.
> >> - the vectors need to be 0x800 aligned for armv8 (.align 11 directive)
> >
> > So far, so good -- I understand that the link-time location of the
> > vectors table is incorrect.
> >
> >> So the failure case is that when the relocation happens, it relocates to
> >> a 0x1000 alignment, say something like 0xffffa000. The relocation offset
> >> is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation
> >> fails.
> >
> > What does "relocation fails" mean exactly, i.e., where and how exactly
> > does the relocation code behave differently from expected? I'm asking
> > because I don't understand why the relocation offset should be a
> > multiple of 0x1000.
> >
> >> Adjusting the relocation offset to a multiple of 0x1000 (by
> >> making the relocation address end in 0xNNNNN020) fixes the issues and
> >> allows u-boot to relocate and run from this address without failing. I
> >> hope this helps explain it a bit better.
> >
> > I do understand, however, that if the relocation offset must indeed be a
> > multiple of 0x1000, then obviously the vectors table will end up as
> > misaligned as it was before relocation.
> >
> > Also, personally I would like it if the vectors table was always
> > aligned as it should, and there are at least three other boards which
> > require a prefix/header before their vectors table, as Masahiro (cc:)
> > indicated recently, so that makes the problem a generic one: how to
> > properly integrate a header in-image (as opposed to an out-of-image
> > one, which is just a matter of doing a 'cat', so to speak.
> >
> > Therefore I'd like a generic solution to this, where the header is
> > prepended *and* aligned properly without breaking the start symbol
> > alignment constraints. This /might/ be possible by cleverly adding
> > a '.header' or '.signature' section to the linker script, possibly
> > doing a two-stage link; but this should not require the source code to
> > contain ad hoc relocation tricks.
> >
> > Let me tinker with it in the next few days; I'll try and come up with a
> > clean and generic solution to this "in-code header" question.
> >
> > Thanks again for your explanation!
> >
> >> Best regards,
> >> Darwin
> >
> > Amicalement,
> >
> 
> Perhaps an oversimplified example of the current code would help to 
> explain this better:
> 
> scenario #1:
> CONFIG_SYS_TEXT_BASE		0x88000000
> vectors:	.align 11	/* exception vectors need to be on a 0x800 byte 
> boundary */
> compile/linker produces (before relocation):
> _start	symbol is at		0x88000000
> vectors	symbol is at		0x88000800
> the relocation offset is:	0x77f9b000
> therefore, after relocation:
> _start	symbol is at		0xfff9b000 (0x88000000+0xfff9b000)
> vectors	symbol is at		0xfff9b800 (0x88000800+0x77f9b000)
> 
> scenario #2:
> CONFIG_SYS_TEXT_BASE		0x88000020
> vectors:	.align 11	/* exception vectors need to be on a 0x800 byte 
> boundary */
> compiler/linker produces (before relocation):
> _start	symbol is at		0x88000020
> vectors	symbol is at		0x88000800
> the relocation offset is:	0x77f9afe0
> therefore, after relocation:
> _start	symbol is at		0xfff9b000 (0x88000020+0x77f9afe0)
> vectors	symbol is at		0xfff9b7e0 (0x88000800+0x77f9afe0)
> 
> Note that in scenario #2, after relocation, the vectors are not on a 
> 0x800 byte boundary anymore.

Ok, so technically relocation works as it was told to, just was given
"garbage in", with an image with an unaligned base and _start symbol
(and an aligned vectors symbol); relocation worked as expected, and
aligned base and start, thus not aligning vectors.

Now, on to understanding why the CONFIG_SYS_TEXT_BASE is not aligned.

Can you provide details from the case where the actual issue has
arisen, or even better yet, provide a source tree and build command
line, that I could reproduce the build here?

> Thanks, Steve

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Steve Rae]

On 14-06-09 10:16 PM, Albert ARIBAUD wrote:
> Hi Steve,
>
> (sorry for the duplicate)
>
> On Mon, 9 Jun 2014 13:45:50 -0700, Steve Rae <srae@broadcom.com> wrote:
>
>>
>>
>> On 14-06-09 03:23 AM, Albert ARIBAUD wrote:
>>> Hi Darwin,
>>>
>>> On Mon, 2 Jun 2014 17:37:25 -0700, Darwin Rambo <drambo@broadcom.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On 14-06-02 12:26 AM, Albert ARIBAUD wrote:
>>>>> Hi Darwin,
>>>>>
>>>>> On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Albert,
>>>>>>
>>>>>> The previous stage bootloader (which I had no control over) wanted it's
>>>>>> header to be aligned to a 512 byte MMC block boundary, presumably since
>>>>>> this allowed DMA operations without copy/shifting. At the same time, I
>>>>>> didn't want to hack a header into start.S because I didn't want to carry
>>>>>> another downstream patch. So I investigated if I could shift u-boot's
>>>>>> base address as a feature that would allow an aligned header to be used
>>>>>> without the start.S patch.
>>>>>>
>>>>>> I know that a custom header patch to start.S would work, and that a
>>>>>> header plus padding will also work. But I found out that you can align
>>>>>> the base on certain smaller offsets if you keep the relocation offset at
>>>>>> nice boundaries like 0x1000 and if the relocation offset is a multiple
>>>>>> of the maximum alignment requirements of the image.
>>>>>>
>>>>>> The original patch I submitted didn't handle an end condition properly,
>>>>>> was ARM64-specific (wasn't tested on other architectures), and because
>>>>>> the patch was NAK'd, I didn't bother to submit a v2 patch and consider
>>>>>> the idea to be dead. I'm happy to abandon the patch. I hope this helps.
>>>>>
>>>>> Thanks.
>>>>>
>>>>> If I understand correctly, your target has a requirement for storing
>>>>> the image on a 512-byte boundary. But how does this affect the loading
>>>>> of the image into RAM, where the requirement is only that the vectors
>>>>> table be 32-bytes aligned? I mean, if you store the image in MMC at
>>>>> offset 0x200 (thus satisfying the 512-byte boundary requirement) and
>>>>> load it to, say, offset 0x10020 in RAM, how is it a problem for
>>>>> your target?
>>>>>
>>>>> If my example above inadequately represents the issue, then can you
>>>>> please provide a similar but adequate example, a failure case scenario,
>>>>> so that I can hve a correct understanding of the problem?
>>>>
>>>> Hi Albert,
>>>>
>>>> The constraints I have that I can't change, are that
>>>> - the 32 byte header is postprocessed and prepended to the image after
>>>> the build is complete
>>>> - the header is at a 512 byte alignment in MMC
>>>> - the header and image are copied to SDRAM to an alignment like
>>>> 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020.
>>>> - the vectors need to be 0x800 aligned for armv8 (.align 11 directive)
>>>
>>> So far, so good -- I understand that the link-time location of the
>>> vectors table is incorrect.
>>>
>>>> So the failure case is that when the relocation happens, it relocates to
>>>> a 0x1000 alignment, say something like 0xffffa000. The relocation offset
>>>> is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation
>>>> fails.
>>>
>>> What does "relocation fails" mean exactly, i.e., where and how exactly
>>> does the relocation code behave differently from expected? I'm asking
>>> because I don't understand why the relocation offset should be a
>>> multiple of 0x1000.
>>>
>>>> Adjusting the relocation offset to a multiple of 0x1000 (by
>>>> making the relocation address end in 0xNNNNN020) fixes the issues and
>>>> allows u-boot to relocate and run from this address without failing. I
>>>> hope this helps explain it a bit better.
>>>
>>> I do understand, however, that if the relocation offset must indeed be a
>>> multiple of 0x1000, then obviously the vectors table will end up as
>>> misaligned as it was before relocation.
>>>
>>> Also, personally I would like it if the vectors table was always
>>> aligned as it should, and there are at least three other boards which
>>> require a prefix/header before their vectors table, as Masahiro (cc:)
>>> indicated recently, so that makes the problem a generic one: how to
>>> properly integrate a header in-image (as opposed to an out-of-image
>>> one, which is just a matter of doing a 'cat', so to speak.
>>>
>>> Therefore I'd like a generic solution to this, where the header is
>>> prepended *and* aligned properly without breaking the start symbol
>>> alignment constraints. This /might/ be possible by cleverly adding
>>> a '.header' or '.signature' section to the linker script, possibly
>>> doing a two-stage link; but this should not require the source code to
>>> contain ad hoc relocation tricks.
>>>
>>> Let me tinker with it in the next few days; I'll try and come up with a
>>> clean and generic solution to this "in-code header" question.
>>>
>>> Thanks again for your explanation!
>>>
>>>> Best regards,
>>>> Darwin
>>>
>>> Amicalement,
>>>
>>
>> Perhaps an oversimplified example of the current code would help to
>> explain this better:
>>
>> scenario #1:
>> CONFIG_SYS_TEXT_BASE		0x88000000
>> vectors:	.align 11	/* exception vectors need to be on a 0x800 byte
>> boundary */
>> compile/linker produces (before relocation):
>> _start	symbol is at		0x88000000
>> vectors	symbol is at		0x88000800
>> the relocation offset is:	0x77f9b000
>> therefore, after relocation:
>> _start	symbol is at		0xfff9b000 (0x88000000+0xfff9b000)
>> vectors	symbol is at		0xfff9b800 (0x88000800+0x77f9b000)
>>
>> scenario #2:
>> CONFIG_SYS_TEXT_BASE		0x88000020
>> vectors:	.align 11	/* exception vectors need to be on a 0x800 byte
>> boundary */
>> compiler/linker produces (before relocation):
>> _start	symbol is at		0x88000020
>> vectors	symbol is at		0x88000800
>> the relocation offset is:	0x77f9afe0
>> therefore, after relocation:
>> _start	symbol is at		0xfff9b000 (0x88000020+0x77f9afe0)
>> vectors	symbol is at		0xfff9b7e0 (0x88000800+0x77f9afe0)
>>
>> Note that in scenario #2, after relocation, the vectors are not on a
>> 0x800 byte boundary anymore.
>
> Ok, so technically relocation works as it was told to, just was given
> "garbage in", with an image with an unaligned base and _start symbol
> (and an aligned vectors symbol); relocation worked as expected, and
> aligned base and start, thus not aligning vectors.
>
> Now, on to understanding why the CONFIG_SYS_TEXT_BASE is not aligned.
>
> Can you provide details from the case where the actual issue has
> arisen, or even better yet, provide a source tree and build command
> line, that I could reproduce the build here?
>
>> Thanks, Steve
>
> Amicalement,
>

Albert et al.

There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not 
aligned on a 0x1000 byte boundary (Darwin has attempted to document his 
particular use-case...)

But we think that the solution to support this is relatively 
straightforward:
(1) after determining the "relocation address" (which will be on a 
0x1000 byte boundary),
(2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address" 
(which effectively makes the "relocation offset" a multiple of 0x1000 
too...)
So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this 
algorithm changes nothing.
And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we 
would now get the following:
the relocation offset is:	0x77f9b000
therefore, after relocation:
_start	symbol is at		0xfff9b020 (0x88000020+0x77f9b000)
vectors	symbol is at		0xfff9b800 (0x88000800+0x77f9b000)
Which results in everything being aligned properly before AND after 
relocation.
(3) HOWEVER, shifting the address UP may cause the end of the relocated 
code to run past the end of the available memory... So we could:
(3.1) always relocate DOWN by ((CONFIG_SYS_TEXT_BASE % 4096) - 4096), or
(3.2) add (CONFIG_SYS_TEXT_BASE % 4096) to the calculated "gd->mon_len" 
(which is the value used to determine the "relocation address"; this 
would guarantee that there is always enough space for the relocated code...)

Note: 3.1 would usually waste up to 4096 bytes; whereas 3.2 would only 
adjust when necessary; therefore, we prefer 3.2 ....

I trust that everyone will find this explanation acceptable...
If so, we can publish a [v2] patch.
Thanks, Steve

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

Dear Steve,

In message <539746C4.9040004@broadcom.com> you wrote:
> 
> There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not 
> aligned on a 0x1000 byte boundary (Darwin has attempted to document his 
> particular use-case...)

We should be more precise here and ask for any _good_ reasons.

I can think of many good reasons to keep the text base aligned.  As
for the reasons to use an unaligned address that were brought up here,
I still think that it would have been better to use an aligned taxe
base and do the rest with a customized linker script.

> But we think that the solution to support this is relatively 
> straightforward:
> (1) after determining the "relocation address" (which will be on a 
> 0x1000 byte boundary),
> (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address" 
> (which effectively makes the "relocation offset" a multiple of 0x1000 
> too...)
> So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this 
> algorithm changes nothing.
> And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we 
> would now get the following:
> the relocation offset is:	0x77f9b000
> therefore, after relocation:
> _start	symbol is at		0xfff9b020 (0x88000020+0x77f9b000)
> vectors	symbol is at		0xfff9b800 (0x88000800+0x77f9b000)

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 ?

Then everything should be still the same for you, and no voodoo coding
would be needed.

> (3) HOWEVER, shifting the address UP may cause the end of the relocated 
> code to run past the end of the available memory... So we could:

This problem is void if you just use a poperly aligned text base in
combination with a prope start.S resp. linker script to make sure
_start is where you want it.

> I trust that everyone will find this explanation acceptable...

No, I do not see a good reason to add such code.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Let the programmers be many and the managers few -- then all will  be
productive.               -- Geoffrey James, "The Tao of Programming"

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Steve Rae]

On 14-06-10 11:13 AM, Wolfgang Denk wrote:
> Dear Steve,
>
> In message <539746C4.9040004@broadcom.com> you wrote:
>>
>> There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not
>> aligned on a 0x1000 byte boundary (Darwin has attempted to document his
>> particular use-case...)
>
> We should be more precise here and ask for any _good_ reasons.
>
> I can think of many good reasons to keep the text base aligned.  As
> for the reasons to use an unaligned address that were brought up here,
> I still think that it would have been better to use an aligned taxe
> base and do the rest with a customized linker script.
>
>> But we think that the solution to support this is relatively
>> straightforward:
>> (1) after determining the "relocation address" (which will be on a
>> 0x1000 byte boundary),
>> (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address"
>> (which effectively makes the "relocation offset" a multiple of 0x1000
>> too...)
>> So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this
>> algorithm changes nothing.
>> And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we
>> would now get the following:
>> the relocation offset is:	0x77f9b000
>> therefore, after relocation:
>> _start	symbol is at		0xfff9b020 (0x88000020+0x77f9b000)
>> vectors	symbol is at		0xfff9b800 (0x88000800+0x77f9b000)
>
> 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...
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 ....
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...
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.
Thanks, Steve

>
> Then everything should be still the same for you, and no voodoo coding
> would be needed.
>
>> (3) HOWEVER, shifting the address UP may cause the end of the relocated
>> code to run past the end of the available memory... So we could:
>
> This problem is void if you just use a poperly aligned text base in
> combination with a prope start.S resp. linker script to make sure
> _start is where you want it.
>
>> I trust that everyone will find this explanation acceptable...
>
> No, I do not see a good reason to add such code.
>
> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

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?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It seems intuitively obvious to me, which  means  that  it  might  be
wrong.                                                 -- Chris Torek

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Steve Rae]

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:

diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index 4b11aa4..8fd72f1 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -18,6 +18,10 @@
   *
 
*************************************************************************/

+#ifdef CONFIG_CUSTOM_HEADER_RESERVED_BYTES
+	.skip CONFIG_CUSTOM_HEADER_RESERVED_BYTES
+#endif
+
  .globl	_start
  _start:
  	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
>

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Half of the people in the world are below average.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Albert ARIBAUD]

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.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Steve Rae]

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

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Wisdom is one of the few things that looks bigger the further away it
is.                               - Terry Pratchett, _Witches Abroad_

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Albert ARIBAUD]

Hi Steve,

On Tue, 10 Jun 2014 12:38:42 -0700, Steve Rae <srae@broadcom.com> wrote:

> 
> 
> On 14-06-10 11:13 AM, Wolfgang Denk wrote:
> > Dear Steve,
> >
> > In message <539746C4.9040004@broadcom.com> you wrote:
> >>
> >> There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not
> >> aligned on a 0x1000 byte boundary (Darwin has attempted to document his
> >> particular use-case...)
> >
> > We should be more precise here and ask for any _good_ reasons.
> >
> > I can think of many good reasons to keep the text base aligned.  As
> > for the reasons to use an unaligned address that were brought up here,
> > I still think that it would have been better to use an aligned taxe
> > base and do the rest with a customized linker script.
> >
> >> But we think that the solution to support this is relatively
> >> straightforward:
> >> (1) after determining the "relocation address" (which will be on a
> >> 0x1000 byte boundary),
> >> (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address"
> >> (which effectively makes the "relocation offset" a multiple of 0x1000
> >> too...)
> >> So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this
> >> algorithm changes nothing.
> >> And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we
> >> would now get the following:
> >> the relocation offset is:	0x77f9b000
> >> therefore, after relocation:
> >> _start	symbol is at		0xfff9b020 (0x88000020+0x77f9b000)
> >> vectors	symbol is at		0xfff9b800 (0x88000800+0x77f9b000)
> >
> > 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...

That is not a good approach -- and I should know, as there are boards
out there which already insist on having a header (mind you, a simple
4-byte constant) in start.S, and I am already trying to tackle this
out of start.S.

Your 32-byte header should not be in start.S, it should be linked in
before start.o in the linker script. Then you would only have one
start.S file and as many headers as you require.

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

Not necessarily. You'll need to configure your target, that's sure,
because I guess different targets will have different (values in
the bytes of their) headers, but you don't need to have different
start.S files.

> 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...
> 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.
> Thanks, Steve

I (believe I) understand the problem, and I am discussing here because I
too want it solved. On the one hand I do agree with Wolfgang here:
 
> > No, I do not see a good reason to add such code.

... because I don't want to allow a feature that is designed to counter
a problem; but on the other hand I want the problem fixed. So Darwin,
in order to find a satisfactory solution, let me try to recap the
situation and then ask a few questions.

Recap:

1. A typical ARM binary image is linked to a base address, defined by
   preprocessor macro (aka U-Boot config option) CONFIG_SYS_TEXT_BASE.

2. A typical ARM target defines CONFIG_SYS_TEXT_BASE so that the image,
   and especially its exception vectors table, is properly aligned upon
   loading the image in RAM.

3. When the ARM image relocates itself, it does so to a destination
   address which is computed so that the image and its vectors table
   are still properly aligned after relocation.

4. Some targets require that the image stored in MMC be prepended with
   a 32-byte header (and aligned to a sector-related boundary).

5. For some targets (if not all) the header is not separate from the
   image, i.e., it will also be copied from MMC to RAM. Such headers
   must be prepended at the link stage or earlier, so that the
   link-time and run-time values of all image symbols are consistent.

6. However, because the header is put at the start of the image before
   the vectors table, the image is still properly aligned but th
   vectors tabled is not any more, both when the image is loaded from
   MMC to RAM, and when it is relocated.

7. This mis-alignment issue does not only affect the vectors table; it
   also affects any location addressed using adrp instructions (if I
   did correctly understand Jeroen, Cc:).

This is my understanding of the issue. If it is correct, then I
believe the following solves the issue without changing anything to
the current code:

Instead of prepending a 32-byte header to the image, prepend the header
then a block of 2048-32 bytes. This way, the MMC image base address
(the header address) is aligned, and the alignment of any address in
the image itself is preserved. 

If for some reason it is impossible to pad the header (such as, the
header is actually prepended by a closed-source tool which takes the
'raw' image in and spits the 'MMC image' out), then the raw image
should be prepended with 2048-32 bytes beforehand, so that the 32
header bytes make up a whole 2048-bytes block, and the image remains
aligned.   

However, I guess that you, Darwin and Steve, probably have thought of
this already and dismissed it for a reason. If so, please explain
what this reason is.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Steve Rae]

On 14-06-10 02:20 PM, Albert ARIBAUD wrote:
> Hi Steve,
>
> On Tue, 10 Jun 2014 12:38:42 -0700, Steve Rae <srae@broadcom.com> wrote:
>
>>
>>
>> On 14-06-10 11:13 AM, Wolfgang Denk wrote:
>>> Dear Steve,
>>>
>>> In message <539746C4.9040004@broadcom.com> you wrote:
>>>>
>>>> There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not
>>>> aligned on a 0x1000 byte boundary (Darwin has attempted to document his
>>>> particular use-case...)
>>>
>>> We should be more precise here and ask for any _good_ reasons.
>>>
>>> I can think of many good reasons to keep the text base aligned.  As
>>> for the reasons to use an unaligned address that were brought up here,
>>> I still think that it would have been better to use an aligned taxe
>>> base and do the rest with a customized linker script.
>>>
>>>> But we think that the solution to support this is relatively
>>>> straightforward:
>>>> (1) after determining the "relocation address" (which will be on a
>>>> 0x1000 byte boundary),
>>>> (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address"
>>>> (which effectively makes the "relocation offset" a multiple of 0x1000
>>>> too...)
>>>> So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this
>>>> algorithm changes nothing.
>>>> And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we
>>>> would now get the following:
>>>> the relocation offset is:	0x77f9b000
>>>> therefore, after relocation:
>>>> _start	symbol is at		0xfff9b020 (0x88000020+0x77f9b000)
>>>> vectors	symbol is at		0xfff9b800 (0x88000800+0x77f9b000)
>>>
>>> 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...
>
> That is not a good approach -- and I should know, as there are boards
> out there which already insist on having a header (mind you, a simple
> 4-byte constant) in start.S, and I am already trying to tackle this
> out of start.S.
>
> Your 32-byte header should not be in start.S, it should be linked in
> before start.o in the linker script. Then you would only have one
> start.S file and as many headers as you require.
>
>> 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 ....
>
> Not necessarily. You'll need to configure your target, that's sure,
> because I guess different targets will have different (values in
> the bytes of their) headers, but you don't need to have different
> start.S files.
>
>> 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...
>> 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.
>> Thanks, Steve
>
> I (believe I) understand the problem, and I am discussing here because I
> too want it solved. On the one hand I do agree with Wolfgang here:
>
>>> No, I do not see a good reason to add such code.
>
> ... because I don't want to allow a feature that is designed to counter
> a problem; but on the other hand I want the problem fixed. So Darwin,
> in order to find a satisfactory solution, let me try to recap the
> situation and then ask a few questions.
>
> Recap:
>
> 1. A typical ARM binary image is linked to a base address, defined by
>     preprocessor macro (aka U-Boot config option) CONFIG_SYS_TEXT_BASE.

OK

>
> 2. A typical ARM target defines CONFIG_SYS_TEXT_BASE so that the image,
>     and especially its exception vectors table, is properly aligned upon
>     loading the image in RAM.

Nope, the vectors respect the .align directive independent of the 
CONFIG_SYS_TEXT_BASE setting.

>
> 3. When the ARM image relocates itself, it does so to a destination
>     address which is computed so that the image and its vectors table
>     are still properly aligned after relocation.

Almost: the computed destination address is aligned on a 0x1000 byte 
boundary, so if the CONFIG_SYS_TEXT_BASE is also a multiple of 0x1000 
bytes, then this statement is true.

>
> 4. Some targets require that the image stored in MMC be prepended with
>     a 32-byte header (and aligned to a sector-related boundary).

OK

>
> 5. For some targets (if not all) the header is not separate from the
>     image, i.e., it will also be copied from MMC to RAM. Such headers
>     must be prepended at the link stage or earlier, so that the
>     link-time and run-time values of all image symbols are consistent.

Yes - copied from MMC to RAM, but not really necessary to do this at 
"link stage or earlier" (we do it with a post-processing tool to prepend 
a header onto 'u-boot.bin')

>
> 6. However, because the header is put at the start of the image before
>     the vectors table, the image is still properly aligned but th
>     vectors tabled is not any more, both when the image is loaded from
>     MMC to RAM, and when it is relocated.

Because we set the CONFIG_SYS_TEXT_BASE to 0xXXXX0020, the image is 
properly aligned, if CONFIG_SYS_TEXT_BASE is 0xXXXX0000, then the 
"before" image would not be properly aligned....

>
> 7. This mis-alignment issue does not only affect the vectors table; it
>     also affects any location addressed using adrp instructions (if I
>     did correctly understand Jeroen, Cc:).

Agree - it affects all of the "align" directives, in the code and the 
linker script file, and all instructions which assume alignment....
>
> This is my understanding of the issue. If it is correct, then I
> believe the following solves the issue without changing anything to
> the current code:
>
> Instead of prepending a 32-byte header to the image, prepend the header
> then a block of 2048-32 bytes. This way, the MMC image base address
> (the header address) is aligned, and the alignment of any address in
> the image itself is preserved.

Yes - if the header is an exact multiple of 0x1000 (4096), it will work 
correctly without any code modifications... (not certain about 2048...)

>
> If for some reason it is impossible to pad the header (such as, the
> header is actually prepended by a closed-source tool which takes the
> 'raw' image in and spits the 'MMC image' out), then the raw image
> should be prepended with 2048-32 bytes beforehand, so that the 32
> header bytes make up a whole 2048-bytes block, and the image remains
> aligned.
>
> However, I guess that you, Darwin and Steve, probably have thought of
> this already and dismissed it for a reason. If so, please explain
> what this reason is.

We wanted to implement a forward looking solution that worked for any 
size of prepended header, and which removed the constraint for the 
CONFIG_SYS_TEXT_BASE to be an exact multiple of 0x1000 (4096).

However, since this solution does not look like it will succeed, in a 
parallel thread, we are pursuing an alternate proposal to conditionally 
reserve space for the header in u-boot.bin (which is "link stage or 
earlier"!!!), which would then be modified by a post-processing tool.
So please review that proposal.
Thanks, Steve

>
> Amicalement,
>

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

Dear Steve,

In message <53979F6A.90607@broadcom.com> you wrote:
> 
> Yes - copied from MMC to RAM, but not really necessary to do this at 
> "link stage or earlier" (we do it with a post-processing tool to prepend 
> a header onto 'u-boot.bin')

What exactly is the fuction of this post-processing tool?  Does it
retrieve any data from the previously built image (like size
information), or could the header also be generated before the link
step, so we just include it there?

Also, can we not use the U-Boot build process to generate these 32
bytes of header data?

> > 6. However, because the header is put at the start of the image before
> >     the vectors table, the image is still properly aligned but th
> >     vectors tabled is not any more, both when the image is loaded from
> >     MMC to RAM, and when it is relocated.
> 
> Because we set the CONFIG_SYS_TEXT_BASE to 0xXXXX0020, the image is 
> properly aligned, if CONFIG_SYS_TEXT_BASE is 0xXXXX0000, then the 
> "before" image would not be properly aligned....

...unless your linker script properly aligns all segments.

> We wanted to implement a forward looking solution that worked for any 
> size of prepended header, and which removed the constraint for the 
> CONFIG_SYS_TEXT_BASE to be an exact multiple of 0x1000 (4096).

The generic approach is to use the linker to define the memory map of
the resulting image, i. e. to create proper alignment of segments
andproper placement of code such that specific address and alignment
requirements are met.

> However, since this solution does not look like it will succeed, in a 
> parallel thread, we are pursuing an alternate proposal to conditionally 
> reserve space for the header in u-boot.bin (which is "link stage or 
> earlier"!!!), which would then be modified by a post-processing tool.

Why would such post-processing be needed?  I think you should strive
to get rid of such an additional step, and try to get the header
generation either be done before linking, or even better as integral
part of the build process.

We are talking about 32 bytes of data here, so I speculate it should
not be too difficult to generate these, probably even without theuse
of a special external tool?  Can you explain the structure of this 32
byte header?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
And now remains  That we find out the cause of this effect, Or rather
say, the cause of this defect...           -- Hamlet, Act II, Scene 2

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

Dear Albert,

In message <E1WuTT8-0007AF-9l@janus> you wrote:
> 
> 1. A typical ARM binary image is linked to a base address, defined by
>    preprocessor macro (aka U-Boot config option) CONFIG_SYS_TEXT_BASE.

We shoul specifically keep in mind here that the "base address" is
defined as the start address of the text segment, which may or may not
be the same as the entry point address _start.  In general, these are
not the same.

> 2. A typical ARM target defines CONFIG_SYS_TEXT_BASE so that the image,
>    and especially its exception vectors table, is properly aligned upon
>    loading the image in RAM.
> 
> 3. When the ARM image relocates itself, it does so to a destination
>    address which is computed so that the image and its vectors table
>    are still properly aligned after relocation.

These items are not ARM specific.

> 4. Some targets require that the image stored in MMC be prepended with
>    a 32-byte header (and aligned to a sector-related boundary).

Why would such headers have to be part of the text segment?

> 5. For some targets (if not all) the header is not separate from the
>    image, i.e., it will also be copied from MMC to RAM. Such headers
>    must be prepended at the link stage or earlier, so that the
>    link-time and run-time values of all image symbols are consistent.

If the headers are in their own segment, this should be trivial.

> 6. However, because the header is put at the start of the image before
>    the vectors table, the image is still properly aligned but th
>    vectors tabled is not any more, both when the image is loaded from
>    MMC to RAM, and when it is relocated.

This would be a bug, then.  If we have an alignment requirement for
the text segment, and we load other data (or code) in front of the
text segment, then the size of such additional data must be made such
that the text segment is still properly aligned.  Usually I would
expect that the heaer is in a separate segment, and the linker script
would take care of the required alignment for the text segment and
insert the necessary gap to keep it properly aligned.

> Instead of prepending a 32-byte header to the image, prepend the header
> then a block of 2048-32 bytes. This way, the MMC image base address
> (the header address) is aligned, and the alignment of any address in
> the image itself is preserved. 

Agreed. Or should .text be aligned on a 4 k boundary?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I used to be indecisive, now I'm not sure.

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

Dear Darwin Rambo,

In message <1400105145-6628-1-git-send-email-drambo@broadcom.com> you wrote:
> If an earlier loader stage requires an image header and a specific
> offset, then u-boot's base address (CONFIG_SYS_TEXT_BASE) may be
> advanced beyond an aligned address. In this case the relocation

Sorry, I cannot parse that.  CONFIG_SYS_TEXT_BASE is a compile time
constant, it cannot be "advanced" by a loader.  Do you mean that some
loader loads U-Boot to an incorrect address?  Well, in this case the
loader should be fixed, or?

> This change is done under CONFIG_ARM64 conditional compilation
> because it has only been tested there and may not be appropriate
> for other architectures.

In any case, any such changes (if there should be agreement that they
are actually useful) should be done in an architecture-neutral way.
Implementing it for one specific architecture only is wrong.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
First, we must do our own personal work, then we tend  the  necessary
work of our family, then our community, then the world.     - Lao-tzu

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Darwin Rambo]

On 14-05-14 09:26 PM, Wolfgang Denk wrote:
> Dear Darwin Rambo,
>
> In message <1400105145-6628-1-git-send-email-drambo@broadcom.com> you wrote:
>> If an earlier loader stage requires an image header and a specific
>> offset, then u-boot's base address (CONFIG_SYS_TEXT_BASE) may be
>> advanced beyond an aligned address. In this case the relocation
> Sorry, I cannot parse that.  CONFIG_SYS_TEXT_BASE is a compile time
> constant, it cannot be "advanced" by a loader.  Do you mean that some
> loader loads U-Boot to an incorrect address?  Well, in this case the
> loader should be fixed, or?

Thank you for your comments.
  
I mean that the loader loads u-boot to it's correct address, which is
offset by a small amount because of a previous header requiring alignment.
Here's an example. u-boot is compiled to run at 0x88000020 because we want
to put a small header in front of the image, which starts at 0x88000000 and
needs to be aligned for its own reasons. Now for arm64, I believe that u-boot
cannot normally be positioned at any alignment less than 0x800 hex. So
u-boot would normally run at addresses like 0x88000000, 0x88000800, 0x88001000, etc.
And in these cases the relocation works fine. But if we want to position u-boot
at a smaller offset than 0x800, the symbol relocation breaks for arm64. It
turns out that there is a trivial fix so that u-boot can run at smaller offset
addresses, which I have provided here, is tested, and solves our problem nicely,
but only for arm64 right now.

>
>> This change is done under CONFIG_ARM64 conditional compilation
>> because it has only been tested there and may not be appropriate
>> for other architectures.
> In any case, any such changes (if there should be agreement that they
> are actually useful) should be done in an architecture-neutral way.
> Implementing it for one specific architecture only is wrong.

Yes, I agree, but I am not sure if this is a arm64-only problem or not.
Armv7 doesn't show this problem, and I can't test other architectures
for their alignment issues. So I thought that I would at least show the
fix for arm64 so we can decide if and how to proceed. Any suggestions you
can provide on how to proceed would be appreciated. And if the fix is
not suitable for upstreaming, then we should know it.

Is there a way to have architecture specific hooks like this called from
the generic common/board_f.c? The fix is also in arch/arm/lib/board.c but it
sounds like that file might be disappearing.

Also there might be a generic fix possible that works for all architectures
(by removing the ifdef CONFIG_ARM64), but I don't have the resources to test
them. Maybe it would be best to decide if we want to support this feature or
not first. Thanks!

Regards,
Darwin

>
> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [PATCH] arm: Allow u-boot to run from offset base address

[Wolfgang Denk]

Dear Darwin,

In message <5374CC3B.7000504@broadcom.com> you wrote:
> 
> I mean that the loader loads u-boot to it's correct address, which is
> offset by a small amount because of a previous header requiring alignment.
> Here's an example. u-boot is compiled to run at 0x88000020 because we want
> to put a small header in front of the image, which starts at 0x88000000 and
> needs to be aligned for its own reasons. Now for arm64, I believe that u-boot
> cannot normally be positioned at any alignment less than 0x800 hex. So

I thing you have some misunderstanding of the meaning of the start
address of the image versus the entry point address.  These are not
related.  You can add any headers or padding you like to the image,
and put the entry point at an arbitrary address.

The restrictions we usually face are due to the bootmodes of the SoC,
which may start from a fixed reset address (whwere we then must make
sure that this is also the entry point address in the image), or where
the ROM boot loader may have specific requirements.

If you add some custom image header, you can also start at some random
entry point addres.. Just adapt your linker script as needed.

> And in these cases the relocation works fine. But if we want to position u-boot
> at a smaller offset than 0x800, the symbol relocation breaks for arm64. It
> turns out that there is a trivial fix so that u-boot can run at smaller offset
> addresses, which I have provided here, is tested, and solves our problem nicely,
> but only for arm64 right now.

No, your patch is highly dubious actually.  Note that the entry point
address which we are talking about is where code execution starts
_before_ relocation.  Relocation is a totally different topic.  The
address where we relocate to gets computed at runtime, and does not
depend on CONFIG_SYS_TEXT_BASE at all.  Also, as had been pointed out
before, memory allocation happens top down, so any adjustments you
want to make must be to lower addresses, never upward.

> Yes, I agree, but I am not sure if this is a arm64-only problem or not.

Unfortunaltely you don't explain what you really want to do. We have
the same task (loading the U-Boot image and starting it) in many,
nmany configurations either when a ROM boot loader performs the
loading, or where the SPL does it.  This is a well understood
procedure, and it has no such problem as you claim to have.

I suspect you have bugs elsewhere in your port, may it be the linker
script or your implementation of a special image header or whatever.

> can provide on how to proceed would be appreciated. And if the fix is
> not suitable for upstreaming, then we should know it.

Please understand that this is not a question of suitable for
upstreaming or not, it is a matter of correct or not.  I am pretty
much convinced that your modifcation cannot be right.

> Also there might be a generic fix possible that works for all architectures
> (by removing the ifdef CONFIG_ARM64), but I don't have the resources to test
> them. Maybe it would be best to decide if we want to support this feature or
> not first. Thanks!

Please define "this feature" in an exact way, and why you think it
would cause any problems.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Disc space - the final frontier!