[PATCH v2] bootm: update os_entry from uimage

[PATCH v2] bootm: update os_entry from uimage

[Jean-Christophe PLAGNIOL-VILLARD]

if not uimage set 0 by default

today we do not see the issue as the kernel entry point is the same as the
load_addr but on other binary its not necessary the case

as today we ignore the entry point set in the uimage and just assume it's the
same as the load_addr

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 commands/bootm.c |  1 +
 common/bootm.c   | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/commands/bootm.c b/commands/bootm.c
index a4004df..7acf341 100644
--- a/commands/bootm.c
+++ b/commands/bootm.c
@@ -104,6 +104,7 @@ static int do_bootm(int argc, char *argv[])
 
 	data.initrd_address = UIMAGE_INVALID_ADDRESS;
 	data.os_address = UIMAGE_SOME_ADDRESS;
+	data.os_entry = UIMAGE_SOME_ADDRESS;
 	data.verify = 0;
 	data.verbose = 0;
 
diff --git a/common/bootm.c b/common/bootm.c
index 1987a09..17c8082 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -46,6 +46,7 @@ static struct image_handler *bootm_find_handler(enum filetype filetype,
 static int bootm_open_os_uimage(struct image_data *data)
 {
 	int ret;
+	int update_ep = 0;
 
 	data->os = uimage_open(data->os_file);
 	if (!data->os)
@@ -70,8 +71,10 @@ static int bootm_open_os_uimage(struct image_data *data)
 		return -EINVAL;
 	}
 
-	if (data->os_address == UIMAGE_SOME_ADDRESS)
+	if (data->os_address == UIMAGE_SOME_ADDRESS) {
 		data->os_address = data->os->header.ih_load;
+		update_ep = 1;
+	}
 
 	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
 		data->os_res = uimage_load_to_sdram(data->os, 0,
@@ -80,6 +83,13 @@ static int bootm_open_os_uimage(struct image_data *data)
 			uimage_close(data->os);
 			return -ENOMEM;
 		}
+
+	}
+
+	if (update_ep && data->os_entry == UIMAGE_SOME_ADDRESS) {
+		data->os_entry = data->os->header.ih_ep;
+		if (data->os_address != UIMAGE_INVALID_ADDRESS)
+			data->os_entry -= data->os_address;
 	}
 
 	return 0;
@@ -232,6 +242,9 @@ int bootm_boot(struct image_data *data)
 		}
 	}
 
+	if (data->os_entry == UIMAGE_SOME_ADDRESS)
+		data->os_entry = 0;
+
 	if (IS_ENABLED(CONFIG_CMD_BOOTM_INITRD) && data->initrd_file) {
 
 		initrd_type = file_name_detect_type(data->initrd_file);
-- 
1.8.4.rc3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Sascha Hauer]

On Sat, Sep 21, 2013 at 05:20:29PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> if not uimage set 0 by default
> 
> today we do not see the issue as the kernel entry point is the same as the
> load_addr but on other binary its not necessary the case
> 
> as today we ignore the entry point set in the uimage and just assume it's the
> same as the load_addr
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
> @@ -80,6 +83,13 @@ static int bootm_open_os_uimage(struct image_data *data)
>  			uimage_close(data->os);
>  			return -ENOMEM;
>  		}
> +
> +	}
> +
> +	if (update_ep && data->os_entry == UIMAGE_SOME_ADDRESS) {
> +		data->os_entry = data->os->header.ih_ep;
> +		if (data->os_address != UIMAGE_INVALID_ADDRESS)
> +			data->os_entry -= data->os_address;
>  	}

This is wrong. struct image_data stores the entry to the image relative
to the image start, not as absolute address.

The architecture support already handles this correctly:

./arch/nios2/lib/bootm.c:43:    kernel = (void *)(idata->os_address + idata->os_entry);
./arch/ppc/lib/ppclinux.c:76:   kernel = (void *)(data->os_address + data->os_entry);
./arch/arm/lib/bootm.c:58:      kernel = data->os_res->start + data->os_entry;
./arch/blackfin/lib/blackfin_linux.c:48:        appl = (void*)(idata->os_address + idata->os_entry);

What's missing is that we have to set data->os_entry to the correct
offset taken from the uImage.

Sascha

From 789bd923f6dd7b9de53f0df61ee749aaf53db83c Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Sun, 22 Sep 2013 19:51:53 +0200
Subject: [PATCH] bootm: handle image entry point correctly

The uImage format stores the entry point to the image in the ih_ep
field. Honor this field.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/bootm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/bootm.c b/common/bootm.c
index 1987a09..4e62006 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -73,6 +73,8 @@ static int bootm_open_os_uimage(struct image_data *data)
 	if (data->os_address == UIMAGE_SOME_ADDRESS)
 		data->os_address = data->os->header.ih_load;
 
+	data->os_entry = data->os->header.ih_ep - data->os->header.ih_load;
+
 	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
 		data->os_res = uimage_load_to_sdram(data->os, 0,
 				data->os_address);
-- 
1.8.4.rc3

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Jean-Christophe PLAGNIOL-VILLARD]

On 20:17 Sun 22 Sep     , Sascha Hauer wrote:
> On Sat, Sep 21, 2013 at 05:20:29PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > if not uimage set 0 by default
> > 
> > today we do not see the issue as the kernel entry point is the same as the
> > load_addr but on other binary its not necessary the case
> > 
> > as today we ignore the entry point set in the uimage and just assume it's the
> > same as the load_addr
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> > @@ -80,6 +83,13 @@ static int bootm_open_os_uimage(struct image_data *data)
> >  			uimage_close(data->os);
> >  			return -ENOMEM;
> >  		}
> > +
> > +	}
> > +
> > +	if (update_ep && data->os_entry == UIMAGE_SOME_ADDRESS) {
> > +		data->os_entry = data->os->header.ih_ep;
> > +		if (data->os_address != UIMAGE_INVALID_ADDRESS)
> > +			data->os_entry -= data->os_address;
> >  	}
> 
> This is wrong. struct image_data stores the entry to the image relative
> to the image start, not as absolute address.
> 
> The architecture support already handles this correctly:
> 
> ./arch/nios2/lib/bootm.c:43:    kernel = (void *)(idata->os_address + idata->os_entry);
> ./arch/ppc/lib/ppclinux.c:76:   kernel = (void *)(data->os_address + data->os_entry);
> ./arch/arm/lib/bootm.c:58:      kernel = data->os_res->start + data->os_entry;
> ./arch/blackfin/lib/blackfin_linux.c:48:        appl = (void*)(idata->os_address + idata->os_entry);
> 
> What's missing is that we have to set data->os_entry to the correct
> offset taken from the uImage.
> 
> Sascha
> 
> From 789bd923f6dd7b9de53f0df61ee749aaf53db83c Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Sun, 22 Sep 2013 19:51:53 +0200
> Subject: [PATCH] bootm: handle image entry point correctly
> 
> The uImage format stores the entry point to the image in the ih_ep
> field. Honor this field.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  common/bootm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/common/bootm.c b/common/bootm.c
> index 1987a09..4e62006 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -73,6 +73,8 @@ static int bootm_open_os_uimage(struct image_data *data)
>  	if (data->os_address == UIMAGE_SOME_ADDRESS)
>  		data->os_address = data->os->header.ih_load;
>  
> +	data->os_entry = data->os->header.ih_ep - data->os->header.ih_load;
> +
>  	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
>  		data->os_res = uimage_load_to_sdram(data->os, 0,
>  				data->os_address);

this does not work on INVALID_ADDRESS as this time teh ih_load is as
0xffffffff and the ih_ep is relative not absolute

Best Regards,
J.
> -- 
> 1.8.4.rc3
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Sascha Hauer]

On Mon, Sep 23, 2013 at 06:40:25AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 20:17 Sun 22 Sep     , Sascha Hauer wrote:
> >  common/bootm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/common/bootm.c b/common/bootm.c
> > index 1987a09..4e62006 100644
> > --- a/common/bootm.c
> > +++ b/common/bootm.c
> > @@ -73,6 +73,8 @@ static int bootm_open_os_uimage(struct image_data *data)
> >  	if (data->os_address == UIMAGE_SOME_ADDRESS)
> >  		data->os_address = data->os->header.ih_load;
> >  
> > +	data->os_entry = data->os->header.ih_ep - data->os->header.ih_load;
> > +
> >  	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
> >  		data->os_res = uimage_load_to_sdram(data->os, 0,
> >  				data->os_address);
> 
> this does not work on INVALID_ADDRESS as this time teh ih_load is as
> 0xffffffff and the ih_ep is relative not absolute

Right. How about this one then?


From da3192d5c763361ec08a51ebaa3cc2c3884c9827 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Sun, 22 Sep 2013 19:51:53 +0200
Subject: [PATCH] bootm: handle image entry point correctly

This fixes starting of uImages which have an entry point other than
the load address.

The uImage format stores the entry point to the image in the ih_ep
field. Normally it has the absolute address, so calculate the relative
offset to the image start address and put it into data->os_entry. In
case the uImage has a load address of 0xffffffff (UIMAGE_INVALID_ADDRESS)
the entry point is stored relative to the image start, so just transfer
it to data->os_address.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/bootm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/common/bootm.c b/common/bootm.c
index 1987a09..16d3416 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -73,7 +73,11 @@ static int bootm_open_os_uimage(struct image_data *data)
 	if (data->os_address == UIMAGE_SOME_ADDRESS)
 		data->os_address = data->os->header.ih_load;
 
-	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
+	if (data->os_address == UIMAGE_INVALID_ADDRESS) {
+		data->os_entry = data->os->header.ih_ep;
+	} else {
+		data->os_entry = data->os->header.ih_ep -
+			data->os->header.ih_load;
 		data->os_res = uimage_load_to_sdram(data->os, 0,
 				data->os_address);
 		if (!data->os_res) {
-- 
1.8.4.rc3

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:33 Mon 23 Sep     , Sascha Hauer wrote:
> On Mon, Sep 23, 2013 at 06:40:25AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 20:17 Sun 22 Sep     , Sascha Hauer wrote:
> > >  common/bootm.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/common/bootm.c b/common/bootm.c
> > > index 1987a09..4e62006 100644
> > > --- a/common/bootm.c
> > > +++ b/common/bootm.c
> > > @@ -73,6 +73,8 @@ static int bootm_open_os_uimage(struct image_data *data)
> > >  	if (data->os_address == UIMAGE_SOME_ADDRESS)
> > >  		data->os_address = data->os->header.ih_load;
> > >  
> > > +	data->os_entry = data->os->header.ih_ep - data->os->header.ih_load;
> > > +
> > >  	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
> > >  		data->os_res = uimage_load_to_sdram(data->os, 0,
> > >  				data->os_address);
> > 
> > this does not work on INVALID_ADDRESS as this time teh ih_load is as
> > 0xffffffff and the ih_ep is relative not absolute
> 
> Right. How about this one then?
> 
> 
> From da3192d5c763361ec08a51ebaa3cc2c3884c9827 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Sun, 22 Sep 2013 19:51:53 +0200
> Subject: [PATCH] bootm: handle image entry point correctly
> 
> This fixes starting of uImages which have an entry point other than
> the load address.
> 
> The uImage format stores the entry point to the image in the ih_ep
> field. Normally it has the absolute address, so calculate the relative
> offset to the image start address and put it into data->os_entry. In
> case the uImage has a load address of 0xffffffff (UIMAGE_INVALID_ADDRESS)
> the entry point is stored relative to the image start, so just transfer
> it to data->os_address.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  common/bootm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/common/bootm.c b/common/bootm.c
> index 1987a09..16d3416 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -73,7 +73,11 @@ static int bootm_open_os_uimage(struct image_data *data)
>  	if (data->os_address == UIMAGE_SOME_ADDRESS)
>  		data->os_address = data->os->header.ih_load;
>  
> -	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
> +	if (data->os_address == UIMAGE_INVALID_ADDRESS) {
> +		data->os_entry = data->os->header.ih_ep;
> +	} else {
> +		data->os_entry = data->os->header.ih_ep -
> +			data->os->header.ih_load;
>  		data->os_res = uimage_load_to_sdram(data->os, 0,
>  				data->os_address);
>  		if (!data->os_res) {

same as my patch do

Best Regards,
J.
> -- 
> 1.8.4.rc3
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:07 Mon 23 Sep     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 08:33 Mon 23 Sep     , Sascha Hauer wrote:
> > On Mon, Sep 23, 2013 at 06:40:25AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 20:17 Sun 22 Sep     , Sascha Hauer wrote:
> > > >  common/bootm.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/common/bootm.c b/common/bootm.c
> > > > index 1987a09..4e62006 100644
> > > > --- a/common/bootm.c
> > > > +++ b/common/bootm.c
> > > > @@ -73,6 +73,8 @@ static int bootm_open_os_uimage(struct image_data *data)
> > > >  	if (data->os_address == UIMAGE_SOME_ADDRESS)
> > > >  		data->os_address = data->os->header.ih_load;
> > > >  
> > > > +	data->os_entry = data->os->header.ih_ep - data->os->header.ih_load;
> > > > +
> > > >  	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
> > > >  		data->os_res = uimage_load_to_sdram(data->os, 0,
> > > >  				data->os_address);
> > > 
> > > this does not work on INVALID_ADDRESS as this time teh ih_load is as
> > > 0xffffffff and the ih_ep is relative not absolute
> > 
> > Right. How about this one then?
> > 
> > 
> > From da3192d5c763361ec08a51ebaa3cc2c3884c9827 Mon Sep 17 00:00:00 2001
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Date: Sun, 22 Sep 2013 19:51:53 +0200
> > Subject: [PATCH] bootm: handle image entry point correctly
> > 
> > This fixes starting of uImages which have an entry point other than
> > the load address.
> > 
> > The uImage format stores the entry point to the image in the ih_ep
> > field. Normally it has the absolute address, so calculate the relative
> > offset to the image start address and put it into data->os_entry. In
> > case the uImage has a load address of 0xffffffff (UIMAGE_INVALID_ADDRESS)
> > the entry point is stored relative to the image start, so just transfer
> > it to data->os_address.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  common/bootm.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/common/bootm.c b/common/bootm.c
> > index 1987a09..16d3416 100644
> > --- a/common/bootm.c
> > +++ b/common/bootm.c
> > @@ -73,7 +73,11 @@ static int bootm_open_os_uimage(struct image_data *data)
> >  	if (data->os_address == UIMAGE_SOME_ADDRESS)
> >  		data->os_address = data->os->header.ih_load;
> >  
> > -	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
> > +	if (data->os_address == UIMAGE_INVALID_ADDRESS) {
> > +		data->os_entry = data->os->header.ih_ep;
> > +	} else {
> > +		data->os_entry = data->os->header.ih_ep -
> > +			data->os->header.ih_load;
> >  		data->os_res = uimage_load_to_sdram(data->os, 0,
> >  				data->os_address);
> >  		if (!data->os_res) {
> 
> same as my patch do

except one point you forget to reset data->os_entry in the case of a non
uimage

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:20 Sat 21 Sep     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> if not uimage set 0 by default
> 
> today we do not see the issue as the kernel entry point is the same as the
> load_addr but on other binary its not necessary the case
> 
> as today we ignore the entry point set in the uimage and just assume it's the
> same as the load_addr
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---

this patch will also prepare to add a globalval and a getopt to overwrite the
os_entry

yours do not care about this preparation by not setting
data.os_entry = UIMAGE_SOME_ADDRESS;

Best Regards,
J.
>  commands/bootm.c |  1 +
>  common/bootm.c   | 15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/commands/bootm.c b/commands/bootm.c
> index a4004df..7acf341 100644
> --- a/commands/bootm.c
> +++ b/commands/bootm.c
> @@ -104,6 +104,7 @@ static int do_bootm(int argc, char *argv[])
>  
>  	data.initrd_address = UIMAGE_INVALID_ADDRESS;
>  	data.os_address = UIMAGE_SOME_ADDRESS;
> +	data.os_entry = UIMAGE_SOME_ADDRESS;
>  	data.verify = 0;
>  	data.verbose = 0;
>  
> diff --git a/common/bootm.c b/common/bootm.c
> index 1987a09..17c8082 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -46,6 +46,7 @@ static struct image_handler *bootm_find_handler(enum filetype filetype,
>  static int bootm_open_os_uimage(struct image_data *data)
>  {
>  	int ret;
> +	int update_ep = 0;
>  
>  	data->os = uimage_open(data->os_file);
>  	if (!data->os)
> @@ -70,8 +71,10 @@ static int bootm_open_os_uimage(struct image_data *data)
>  		return -EINVAL;
>  	}
>  
> -	if (data->os_address == UIMAGE_SOME_ADDRESS)
> +	if (data->os_address == UIMAGE_SOME_ADDRESS) {
>  		data->os_address = data->os->header.ih_load;
> +		update_ep = 1;
> +	}
>  
>  	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
>  		data->os_res = uimage_load_to_sdram(data->os, 0,
> @@ -80,6 +83,13 @@ static int bootm_open_os_uimage(struct image_data *data)
>  			uimage_close(data->os);
>  			return -ENOMEM;
>  		}
> +
> +	}
> +
> +	if (update_ep && data->os_entry == UIMAGE_SOME_ADDRESS) {
> +		data->os_entry = data->os->header.ih_ep;
> +		if (data->os_address != UIMAGE_INVALID_ADDRESS)
> +			data->os_entry -= data->os_address;
>  	}
>  
>  	return 0;
> @@ -232,6 +242,9 @@ int bootm_boot(struct image_data *data)
>  		}
>  	}
>  
> +	if (data->os_entry == UIMAGE_SOME_ADDRESS)
> +		data->os_entry = 0;
> +
>  	if (IS_ENABLED(CONFIG_CMD_BOOTM_INITRD) && data->initrd_file) {
>  
>  		initrd_type = file_name_detect_type(data->initrd_file);
> -- 
> 1.8.4.rc3
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Sascha Hauer]

On Mon, Sep 23, 2013 at 10:12:43AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 10:07 Mon 23 Sep     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 08:33 Mon 23 Sep     , Sascha Hauer wrote:
> > > On Mon, Sep 23, 2013 at 06:40:25AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 20:17 Sun 22 Sep     , Sascha Hauer wrote:
> > > > >  common/bootm.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/common/bootm.c b/common/bootm.c
> > > > > index 1987a09..4e62006 100644
> > > > > --- a/common/bootm.c
> > > > > +++ b/common/bootm.c
> > > > > @@ -73,6 +73,8 @@ static int bootm_open_os_uimage(struct image_data *data)
> > > > >  	if (data->os_address == UIMAGE_SOME_ADDRESS)
> > > > >  		data->os_address = data->os->header.ih_load;
> > > > >  
> > > > > +	data->os_entry = data->os->header.ih_ep - data->os->header.ih_load;
> > > > > +
> > > > >  	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
> > > > >  		data->os_res = uimage_load_to_sdram(data->os, 0,
> > > > >  				data->os_address);
> > > > 
> > > > this does not work on INVALID_ADDRESS as this time teh ih_load is as
> > > > 0xffffffff and the ih_ep is relative not absolute
> > > 
> > > Right. How about this one then?
> > > 
> > > 
> > > From da3192d5c763361ec08a51ebaa3cc2c3884c9827 Mon Sep 17 00:00:00 2001
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Date: Sun, 22 Sep 2013 19:51:53 +0200
> > > Subject: [PATCH] bootm: handle image entry point correctly
> > > 
> > > This fixes starting of uImages which have an entry point other than
> > > the load address.
> > > 
> > > The uImage format stores the entry point to the image in the ih_ep
> > > field. Normally it has the absolute address, so calculate the relative
> > > offset to the image start address and put it into data->os_entry. In
> > > case the uImage has a load address of 0xffffffff (UIMAGE_INVALID_ADDRESS)
> > > the entry point is stored relative to the image start, so just transfer
> > > it to data->os_address.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  common/bootm.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/common/bootm.c b/common/bootm.c
> > > index 1987a09..16d3416 100644
> > > --- a/common/bootm.c
> > > +++ b/common/bootm.c
> > > @@ -73,7 +73,11 @@ static int bootm_open_os_uimage(struct image_data *data)
> > >  	if (data->os_address == UIMAGE_SOME_ADDRESS)
> > >  		data->os_address = data->os->header.ih_load;
> > >  
> > > -	if (data->os_address != UIMAGE_INVALID_ADDRESS) {
> > > +	if (data->os_address == UIMAGE_INVALID_ADDRESS) {
> > > +		data->os_entry = data->os->header.ih_ep;
> > > +	} else {
> > > +		data->os_entry = data->os->header.ih_ep -
> > > +			data->os->header.ih_load;
> > >  		data->os_res = uimage_load_to_sdram(data->os, 0,
> > >  				data->os_address);
> > >  		if (!data->os_res) {
> > 
> > same as my patch do
> 
> except one point you forget to reset data->os_entry in the case of a non
> uimage

To what should data->os_entry be resetted in the case of non uImage? The
code above is never executed for non uImages, so data->os_entry is never
modified in this case.

data->os_entry is expected to be an offset to the image start. Your
patch sets this to an absolute address first and adjusts this to an
offset later which is quite hard to read and understand.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Sascha Hauer]

On Mon, Sep 23, 2013 at 10:16:00AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 17:20 Sat 21 Sep     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > if not uimage set 0 by default
> > 
> > today we do not see the issue as the kernel entry point is the same as the
> > load_addr but on other binary its not necessary the case
> > 
> > as today we ignore the entry point set in the uimage and just assume it's the
> > same as the load_addr
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> 
> this patch will also prepare to add a globalval and a getopt to overwrite the
> os_entry

We already have a getopt to overwrite the os_entry: -e

> 
> yours do not care about this preparation by not setting
> data.os_entry = UIMAGE_SOME_ADDRESS;

Because UIMAGE_SOME_ADDRESS is not suitable for a relative offset to the
image start.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:13 Mon 23 Sep     , Sascha Hauer wrote:
> On Mon, Sep 23, 2013 at 10:16:00AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 17:20 Sat 21 Sep     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > if not uimage set 0 by default
> > > 
> > > today we do not see the issue as the kernel entry point is the same as the
> > > load_addr but on other binary its not necessary the case
> > > 
> > > as today we ignore the entry point set in the uimage and just assume it's the
> > > same as the load_addr
> > > 
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > ---
> > 
> > this patch will also prepare to add a globalval and a getopt to overwrite the
> > os_entry
> 
> We already have a getopt to overwrite the os_entry: -e
> 
> > 
> > yours do not care about this preparation by not setting
> > data.os_entry = UIMAGE_SOME_ADDRESS;
> 
> Because UIMAGE_SOME_ADDRESS is not suitable for a relative offset to the
> image start.

I use this ti let known th e uimage code that the os_entry is not overwrite
and that it must not modify it

because if you specify  via getopt or global and do not set UIMAGE_SOME_ADDRESS
the uimage code will overwrite the provided value

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Sascha Hauer]

On Mon, Sep 23, 2013 at 04:32:26PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:13 Mon 23 Sep     , Sascha Hauer wrote:
> > On Mon, Sep 23, 2013 at 10:16:00AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 17:20 Sat 21 Sep     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > if not uimage set 0 by default
> > > > 
> > > > today we do not see the issue as the kernel entry point is the same as the
> > > > load_addr but on other binary its not necessary the case
> > > > 
> > > > as today we ignore the entry point set in the uimage and just assume it's the
> > > > same as the load_addr
> > > > 
> > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > > ---
> > > 
> > > this patch will also prepare to add a globalval and a getopt to overwrite the
> > > os_entry
> > 
> > We already have a getopt to overwrite the os_entry: -e
> > 
> > > 
> > > yours do not care about this preparation by not setting
> > > data.os_entry = UIMAGE_SOME_ADDRESS;
> > 
> > Because UIMAGE_SOME_ADDRESS is not suitable for a relative offset to the
> > image start.
> 
> I use this ti let known th e uimage code that the os_entry is not overwrite
> and that it must not modify it
> 
> because if you specify  via getopt or global and do not set UIMAGE_SOME_ADDRESS
> the uimage code will overwrite the provided value

So you have uImages which have the wrong entry point provided in the
images? Why don't you fix the images instead?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] bootm: update os_entry from uimage

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:54 Mon 23 Sep     , Sascha Hauer wrote:
> On Mon, Sep 23, 2013 at 04:32:26PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 11:13 Mon 23 Sep     , Sascha Hauer wrote:
> > > On Mon, Sep 23, 2013 at 10:16:00AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 17:20 Sat 21 Sep     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > if not uimage set 0 by default
> > > > > 
> > > > > today we do not see the issue as the kernel entry point is the same as the
> > > > > load_addr but on other binary its not necessary the case
> > > > > 
> > > > > as today we ignore the entry point set in the uimage and just assume it's the
> > > > > same as the load_addr
> > > > > 
> > > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > > > ---
> > > > 
> > > > this patch will also prepare to add a globalval and a getopt to overwrite the
> > > > os_entry
> > > 
> > > We already have a getopt to overwrite the os_entry: -e
> > > 
> > > > 
> > > > yours do not care about this preparation by not setting
> > > > data.os_entry = UIMAGE_SOME_ADDRESS;
> > > 
> > > Because UIMAGE_SOME_ADDRESS is not suitable for a relative offset to the
> > > image start.
> > 
> > I use this ti let known th e uimage code that the os_entry is not overwrite
> > and that it must not modify it
> > 
> > because if you specify  via getopt or global and do not set UIMAGE_SOME_ADDRESS
> > the uimage code will overwrite the provided value
> 
> So you have uImages which have the wrong entry point provided in the
> images? Why don't you fix the images instead?

flashed images we just update the booloader

Best Regards,
J.
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox