[U-Boot] [PATCH] cmd: booti: fix the image runtime location

[U-Boot] [PATCH] cmd: booti: fix the image runtime location

[Peng Fan]

We should not use "bi_dram[0].start + text_offset" as the image dst.
The text_offset maybe 0 for some images, such as XEN. Then the dst
is actually bi_dram[0].start, which maybe the location of spin table.

Let's use "images->ep & ~(ih->text_offset)" as the dst address.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Tom Rini <trini@konsulko.com>
---
 cmd/booti.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cmd/booti.c b/cmd/booti.c
index 6c1c998..afc87e3 100644
--- a/cmd/booti.c
+++ b/cmd/booti.c
@@ -54,7 +54,9 @@ static int booti_setup(bootm_headers_t *images)
 	 * If we are not at the correct run-time location, set the new
 	 * correct location and then move the image there.
 	 */
-	dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset);
+	dst = images->ep & ~(ih->text_offset);
+	if (dst < gd->bd->bi_dram[0].start)
+		dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset);
 
 	unmap_sysmem(ih);
 
-- 
2.6.2

[U-Boot] [PATCH] cmd: booti: fix the image runtime location

[Peng Fan]

On Mon, Sep 12, 2016 at 05:07:58PM +0800, Peng Fan wrote:
>We should not use "bi_dram[0].start + text_offset" as the image dst.
>The text_offset maybe 0 for some images, such as XEN. Then the dst
>is actually bi_dram[0].start, which maybe the location of spin table.
>
>Let's use "images->ep & ~(ih->text_offset)" as the dst address.

This patch maybe not that correct according to the doc from Linux kernel.
"
The Image must be placed text_offset bytes from a 2MB aligned base
address anywhere in usable system RAM and called there. The region
between the 2 MB aligned base address and the start of the image has no
special significance to the kernel, and may be used for other purposes.
"

Now I do not have a good idea that we may have a spin table in the start
of DRAM or even a small firmware.

Is it better to change gd->bd->bi_dram[0].start?
For example physical dram starts from 0x80000000, but 4K is reserved at the beginning.
So is it ok to change gd->bd->bi_dram[0].start to 0x80001000?

Thanks,
Peng.

>
>Signed-off-by: Peng Fan <peng.fan@nxp.com>
>Cc: Tom Rini <trini@konsulko.com>
>---
> cmd/booti.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/cmd/booti.c b/cmd/booti.c
>index 6c1c998..afc87e3 100644
>--- a/cmd/booti.c
>+++ b/cmd/booti.c
>@@ -54,7 +54,9 @@ static int booti_setup(bootm_headers_t *images)
> 	 * If we are not at the correct run-time location, set the new
> 	 * correct location and then move the image there.
> 	 */
>-	dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset);
>+	dst = images->ep & ~(ih->text_offset);
>+	if (dst < gd->bd->bi_dram[0].start)
>+		dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset);
> 
> 	unmap_sysmem(ih);
> 
>-- 
>2.6.2
>

[U-Boot] [PATCH] cmd: booti: fix the image runtime location

[Tom Rini]

On Mon, Sep 12, 2016 at 05:18:53PM +0800, Peng Fan wrote:
> On Mon, Sep 12, 2016 at 05:07:58PM +0800, Peng Fan wrote:
> >We should not use "bi_dram[0].start + text_offset" as the image dst.
> >The text_offset maybe 0 for some images, such as XEN. Then the dst
> >is actually bi_dram[0].start, which maybe the location of spin table.
> >
> >Let's use "images->ep & ~(ih->text_offset)" as the dst address.
> 
> This patch maybe not that correct according to the doc from Linux kernel.
> "
> The Image must be placed text_offset bytes from a 2MB aligned base
> address anywhere in usable system RAM and called there. The region
> between the 2 MB aligned base address and the start of the image has no
> special significance to the kernel, and may be used for other purposes.
> "
> 
> Now I do not have a good idea that we may have a spin table in the start
> of DRAM or even a small firmware.
> 
> Is it better to change gd->bd->bi_dram[0].start?
> For example physical dram starts from 0x80000000, but 4K is reserved at the beginning.
> So is it ok to change gd->bd->bi_dram[0].start to 0x80001000?

So, to be more precise, with v4.5 the memory location restrictions were
relaxed.  So we first need to update cmd/booti.c to know that res1 is
now 'flags' and that we must check bit3 to determine where / how to
align the image.  In the case of bit3 == 0 we must continue to do what
we do today.  In the case of bit3 == 1, we have to decide what exactly
to do.  My first thought is to see if images->ep is 2MB aligned and if
so move to images->ep + ih->text_offset.  If not 2MB aligned, align that
and then add ih->text_offset.

Then any areas that need to be reserved in memory, wherever they are,
can still be marked off as reserved in the DT.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160912/1a5ff6e4/attachment.sig>

[U-Boot] [PATCH] cmd: booti: fix the image runtime location

[Peng Fan]

Hi Tom,

On Mon, Sep 12, 2016 at 08:04:01AM -0400, Tom Rini wrote:
>On Mon, Sep 12, 2016 at 05:18:53PM +0800, Peng Fan wrote:
>> On Mon, Sep 12, 2016 at 05:07:58PM +0800, Peng Fan wrote:
>> >We should not use "bi_dram[0].start + text_offset" as the image dst.
>> >The text_offset maybe 0 for some images, such as XEN. Then the dst
>> >is actually bi_dram[0].start, which maybe the location of spin table.
>> >
>> >Let's use "images->ep & ~(ih->text_offset)" as the dst address.
>> 
>> This patch maybe not that correct according to the doc from Linux kernel.
>> "
>> The Image must be placed text_offset bytes from a 2MB aligned base
>> address anywhere in usable system RAM and called there. The region
>> between the 2 MB aligned base address and the start of the image has no
>> special significance to the kernel, and may be used for other purposes.
>> "
>> 
>> Now I do not have a good idea that we may have a spin table in the start
>> of DRAM or even a small firmware.
>> 
>> Is it better to change gd->bd->bi_dram[0].start?
>> For example physical dram starts from 0x80000000, but 4K is reserved at the beginning.
>> So is it ok to change gd->bd->bi_dram[0].start to 0x80001000?
>
>So, to be more precise, with v4.5 the memory location restrictions were
>relaxed.  So we first need to update cmd/booti.c to know that res1 is
>now 'flags' and that we must check bit3 to determine where / how to
>align the image.  In the case of bit3 == 0 we must continue to do what
>we do today.  In the case of bit3 == 1, we have to decide what exactly
>to do.  My first thought is to see if images->ep is 2MB aligned and if
>so move to images->ep + ih->text_offset.  If not 2MB aligned, align that
>and then add ih->text_offset.
>
>Then any areas that need to be reserved in memory, wherever they are,
>can still be marked off as reserved in the DT.


How about the following patch?
If it is ok, I'll send out a formal one.

diff --git a/cmd/booti.c b/cmd/booti.c
index 6c1c998..efa8bc8 100644
--- a/cmd/booti.c
+++ b/cmd/booti.c
@@ -20,7 +20,7 @@ struct Image_header {
 	uint32_t	code1;		/* Executable code */
 	uint64_t	text_offset;	/* Image load offset, LE */
 	uint64_t	image_size;	/* Effective Image size, LE */
-	uint64_t	res1;		/* reserved */
+	uint64_t	flags;		/* Information flags, LE */
 	uint64_t	res2;		/* reserved */
 	uint64_t	res3;		/* reserved */
 	uint64_t	res4;		/* reserved */
@@ -29,6 +29,7 @@ struct Image_header {
 };
 
 #define LINUX_ARM64_IMAGE_MAGIC	0x644d5241
+#define LINUX_PHYSICAL_PLACEMENT (0x1 << 3)
 
 static int booti_setup(bootm_headers_t *images)
 {
@@ -54,7 +55,11 @@ static int booti_setup(bootm_headers_t *images)
 	 * If we are not@the correct run-time location, set the new
 	 * correct location and then move the image there.
 	 */
-	dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset);
+	dst = gd->bd->bi_dram[0].start;
+	if (le64_to_cpu(ih->flags) & LINUX_PHYSICAL_PLACEMENT)
+		dst = round_up(images->ep, SZ_2M);
+
+	dst += le64_to_cpu(ih->text_offset);
 
 	unmap_sysmem(ih);
 
Thanks,
Peng.
>
>-- 
>Tom

[U-Boot] [PATCH] cmd: booti: fix the image runtime location

[Tom Rini]

On Tue, Sep 13, 2016 at 02:02:53PM +0800, Peng Fan wrote:
> Hi Tom,
> 
> On Mon, Sep 12, 2016 at 08:04:01AM -0400, Tom Rini wrote:
> >On Mon, Sep 12, 2016 at 05:18:53PM +0800, Peng Fan wrote:
> >> On Mon, Sep 12, 2016 at 05:07:58PM +0800, Peng Fan wrote:
> >> >We should not use "bi_dram[0].start + text_offset" as the image dst.
> >> >The text_offset maybe 0 for some images, such as XEN. Then the dst
> >> >is actually bi_dram[0].start, which maybe the location of spin table.
> >> >
> >> >Let's use "images->ep & ~(ih->text_offset)" as the dst address.
> >> 
> >> This patch maybe not that correct according to the doc from Linux kernel.
> >> "
> >> The Image must be placed text_offset bytes from a 2MB aligned base
> >> address anywhere in usable system RAM and called there. The region
> >> between the 2 MB aligned base address and the start of the image has no
> >> special significance to the kernel, and may be used for other purposes.
> >> "
> >> 
> >> Now I do not have a good idea that we may have a spin table in the start
> >> of DRAM or even a small firmware.
> >> 
> >> Is it better to change gd->bd->bi_dram[0].start?
> >> For example physical dram starts from 0x80000000, but 4K is reserved at the beginning.
> >> So is it ok to change gd->bd->bi_dram[0].start to 0x80001000?
> >
> >So, to be more precise, with v4.5 the memory location restrictions were
> >relaxed.  So we first need to update cmd/booti.c to know that res1 is
> >now 'flags' and that we must check bit3 to determine where / how to
> >align the image.  In the case of bit3 == 0 we must continue to do what
> >we do today.  In the case of bit3 == 1, we have to decide what exactly
> >to do.  My first thought is to see if images->ep is 2MB aligned and if
> >so move to images->ep + ih->text_offset.  If not 2MB aligned, align that
> >and then add ih->text_offset.
> >
> >Then any areas that need to be reserved in memory, wherever they are,
> >can still be marked off as reserved in the DT.
> 
> 
> How about the following patch?
> If it is ok, I'll send out a formal one.

Pretty close:

> 
> diff --git a/cmd/booti.c b/cmd/booti.c
> index 6c1c998..efa8bc8 100644
> --- a/cmd/booti.c
> +++ b/cmd/booti.c
> @@ -20,7 +20,7 @@ struct Image_header {
>  	uint32_t	code1;		/* Executable code */
>  	uint64_t	text_offset;	/* Image load offset, LE */
>  	uint64_t	image_size;	/* Effective Image size, LE */
> -	uint64_t	res1;		/* reserved */
> +	uint64_t	flags;		/* Information flags, LE */
>  	uint64_t	res2;		/* reserved */
>  	uint64_t	res3;		/* reserved */
>  	uint64_t	res4;		/* reserved */
> @@ -29,6 +29,7 @@ struct Image_header {
>  };
>  
>  #define LINUX_ARM64_IMAGE_MAGIC	0x644d5241
> +#define LINUX_PHYSICAL_PLACEMENT (0x1 << 3)

Just '1' not '0x1'

>  
>  static int booti_setup(bootm_headers_t *images)
>  {
> @@ -54,7 +55,11 @@ static int booti_setup(bootm_headers_t *images)
>  	 * If we are not at the correct run-time location, set the new
>  	 * correct location and then move the image there.
>  	 */
> -	dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset);
> +	dst = gd->bd->bi_dram[0].start;
> +	if (le64_to_cpu(ih->flags) & LINUX_PHYSICAL_PLACEMENT)
> +		dst = round_up(images->ep, SZ_2M);

Can you do this as if/else and expand the comment so that it explains
that earlier it was recommended to be as close to the start of memory
as possible but later a flag was introduced to allow a more flexible
placement?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160913/d95dab95/attachment.sig>