diff mbox series

[v2,01/21] spl: fix entry_point equal to load_addr

Message ID 20200110144711.81938-2-giulio.benetti@benettiengineering.com
State Accepted
Commit 1054344fa42b1f6568d35eae7ceecc859ad42258
Headers show
Series Add i.MXRT family support | expand

Commit Message

Giulio Benetti Jan. 10, 2020, 2:46 p.m. UTC
At the moment entry_point is set to image_get_load(header) that sets it
to "load address" instead of "entry point", assuming entry_point is
equal to load_addr, but it's not true. Then load_addr is set to
"entry_point - header_size", but this is wrong too since load_addr is
not an entry point.

So use image_get_ep() for entry_point assignment and image_get_load()
for load_addr assignment.

Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
---
 common/spl/spl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stefano Babic Jan. 15, 2020, 12:46 p.m. UTC | #1
> At the moment entry_point is set to image_get_load(header) that sets it
> to "load address" instead of "entry point", assuming entry_point is
> equal to load_addr, but it's not true. Then load_addr is set to
> "entry_point - header_size", but this is wrong too since load_addr is
> not an entry point.
> So use image_get_ep() for entry_point assignment and image_get_load()
> for load_addr assignment.
> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic
Lukasz Majewski Jan. 28, 2020, 8:09 a.m. UTC | #2
Hi Giulio,

> At the moment entry_point is set to image_get_load(header) that sets
> it to "load address" instead of "entry point", assuming entry_point is
> equal to load_addr, but it's not true. Then load_addr is set to
> "entry_point - header_size", but this is wrong too since load_addr is
> not an entry point.
> 
> So use image_get_ep() for entry_point assignment and image_get_load()
> for load_addr assignment.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
> ---
>  common/spl/spl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index c1fce62b91..19085ad270 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -284,9 +284,9 @@ int spl_parse_image_header(struct spl_image_info
> *spl_image, spl_image->entry_point = image_get_ep(header);
>  			spl_image->size =
> image_get_data_size(header); } else {
> -			spl_image->entry_point =
> image_get_load(header);
> +			spl_image->entry_point =
> image_get_ep(header); /* Load including the header */
> -			spl_image->load_addr =
> spl_image->entry_point -
> +			spl_image->load_addr =
> image_get_load(header) - header_size;
>  			spl_image->size =
> image_get_data_size(header) + header_size;

I'm concerned, that this change will silently break several boards -
the problem is with assumption that entry point is equal to load_addr.

It would be best to pull this change ASAP, so we would have a chance to
fix this by next release.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200128/4a2d9831/attachment-0001.sig>
Giulio Benetti Jan. 28, 2020, 4:37 p.m. UTC | #3
Hi Lukasz,

all patch series has already been applied, anyway I answer to your 
suggestions since something was missing and I'm going to create a patch 
for that.

So...

On 1/28/20 9:09 AM, Lukasz Majewski wrote:
> Hi Giulio,
> 
>> At the moment entry_point is set to image_get_load(header) that sets
>> it to "load address" instead of "entry point", assuming entry_point is
>> equal to load_addr, but it's not true. Then load_addr is set to
>> "entry_point - header_size", but this is wrong too since load_addr is
>> not an entry point.
>>
>> So use image_get_ep() for entry_point assignment and image_get_load()
>> for load_addr assignment.
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
>> ---
>>   common/spl/spl.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index c1fce62b91..19085ad270 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -284,9 +284,9 @@ int spl_parse_image_header(struct spl_image_info
>> *spl_image, spl_image->entry_point = image_get_ep(header);
>>   			spl_image->size =
>> image_get_data_size(header); } else {
>> -			spl_image->entry_point =
>> image_get_load(header);
>> +			spl_image->entry_point =
>> image_get_ep(header); /* Load including the header */
>> -			spl_image->load_addr =
>> spl_image->entry_point -
>> +			spl_image->load_addr =
>> image_get_load(header) - header_size;
>>   			spl_image->size =
>> image_get_data_size(header) + header_size;
> 
> I'm concerned, that this change will silently break several boards -
> the problem is with assumption that entry point is equal to load_addr.
> 
> It would be best to pull this change ASAP, so we would have a chance to
> fix this by next release.

It's been committed after Patrice fixed a lot of boards:
https://gitlab.denx.de/u-boot/u-boot/commit/38a6cce65737096b836d43a22f09b7a54c9d020c
and
https://gitlab.denx.de/u-boot/u-boot/commit/74bb4570a952b06fecfafc5b961a5cb5147ec544

Indeed at the first moment it's been committed by Tom Rini and started 
to cause boot failure as you were worried for, then it's been reverted, 
then it's been re-applied after applying Patrice series.

Best regards
Lukasz Majewski Jan. 29, 2020, 8:33 a.m. UTC | #4
On Tue, 28 Jan 2020 17:37:17 +0100
Giulio Benetti <giulio.benetti at benettiengineering.com> wrote:

> Hi Lukasz,
> 
> all patch series has already been applied, anyway I answer to your 
> suggestions since something was missing and I'm going to create a
> patch for that.
> 
> So...
> 
> On 1/28/20 9:09 AM, Lukasz Majewski wrote:
> > Hi Giulio,
> >   
> >> At the moment entry_point is set to image_get_load(header) that
> >> sets it to "load address" instead of "entry point", assuming
> >> entry_point is equal to load_addr, but it's not true. Then
> >> load_addr is set to "entry_point - header_size", but this is wrong
> >> too since load_addr is not an entry point.
> >>
> >> So use image_get_ep() for entry_point assignment and
> >> image_get_load() for load_addr assignment.
> >>
> >> Signed-off-by: Giulio Benetti
> >> <giulio.benetti at benettiengineering.com> ---
> >>   common/spl/spl.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/common/spl/spl.c b/common/spl/spl.c
> >> index c1fce62b91..19085ad270 100644
> >> --- a/common/spl/spl.c
> >> +++ b/common/spl/spl.c
> >> @@ -284,9 +284,9 @@ int spl_parse_image_header(struct
> >> spl_image_info *spl_image, spl_image->entry_point =
> >> image_get_ep(header); spl_image->size =
> >> image_get_data_size(header); } else {
> >> -			spl_image->entry_point =
> >> image_get_load(header);
> >> +			spl_image->entry_point =
> >> image_get_ep(header); /* Load including the header */
> >> -			spl_image->load_addr =
> >> spl_image->entry_point -
> >> +			spl_image->load_addr =
> >> image_get_load(header) - header_size;
> >>   			spl_image->size =
> >> image_get_data_size(header) + header_size;  
> > 
> > I'm concerned, that this change will silently break several boards -
> > the problem is with assumption that entry point is equal to
> > load_addr.
> > 
> > It would be best to pull this change ASAP, so we would have a
> > chance to fix this by next release.  
> 
> It's been committed after Patrice fixed a lot of boards:
> https://gitlab.denx.de/u-boot/u-boot/commit/38a6cce65737096b836d43a22f09b7a54c9d020c
> and
> https://gitlab.denx.de/u-boot/u-boot/commit/74bb4570a952b06fecfafc5b961a5cb5147ec544
> 
> Indeed at the first moment it's been committed by Tom Rini and
> started to cause boot failure as you were worried for, then it's been
> reverted, then it's been re-applied after applying Patrice series.
> 

Ach... Ok. No problem then.

> Best regards




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200129/dad41610/attachment.sig>
diff mbox series

Patch

diff --git a/common/spl/spl.c b/common/spl/spl.c
index c1fce62b91..19085ad270 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -284,9 +284,9 @@  int spl_parse_image_header(struct spl_image_info *spl_image,
 			spl_image->entry_point = image_get_ep(header);
 			spl_image->size = image_get_data_size(header);
 		} else {
-			spl_image->entry_point = image_get_load(header);
+			spl_image->entry_point = image_get_ep(header);
 			/* Load including the header */
-			spl_image->load_addr = spl_image->entry_point -
+			spl_image->load_addr = image_get_load(header) -
 				header_size;
 			spl_image->size = image_get_data_size(header) +
 				header_size;