[PATCH] spl: spl_legacy: Fix spl_end address for non ARM target

[PATCH] spl: spl_legacy: Fix spl_end address for non ARM target

[Shiji Yang]

Only ARM target defines _image_binary_end symbol as char*, All other
targets define it as an ulong type in include/asm-generic/sections.h.

This patch fixes the boot failure on MIPS target. Error log:
SPL: Image overlaps SPL

Fixes: 1b8a1be1a1f1 ("spl: spl_legacy: Fix spl_end address")
Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
---
 common/spl/spl_legacy.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
index 095443c63d..0fef890384 100644
--- a/common/spl/spl_legacy.c
+++ b/common/spl/spl_legacy.c
@@ -18,9 +18,13 @@
 
 static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size)
 {
+	uintptr_t end = start + size;
 	uintptr_t spl_start = (uintptr_t)_start;
+#ifdef CONFIG_ARM
 	uintptr_t spl_end = (uintptr_t)_image_binary_end;
-	uintptr_t end = start + size;
+#else
+	uintptr_t spl_end = (uintptr_t)&_image_binary_end;
+#endif /* CONFIG_ARM */
 
 	if ((start >= spl_start && start < spl_end) ||
 	    (end > spl_start && end <= spl_end) ||
-- 
2.39.2

[PATCH] spl: spl_legacy: Fix spl_end address for non ARM target

[Shiji Yang]

Only ARM target defines _image_binary_end symbol as char*, All other
targets define it as an ulong type in include/asm-generic/sections.h.

This patch fixes the boot failure on MIPS target. Error log:
SPL: Image overlaps SPL

Fixes: 1b8a1be1a1f1 ("spl: spl_legacy: Fix spl_end address")
Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
---
 common/spl/spl_legacy.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
index 095443c63d..0fef890384 100644
--- a/common/spl/spl_legacy.c
+++ b/common/spl/spl_legacy.c
@@ -18,9 +18,13 @@
 
 static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size)
 {
+	uintptr_t end = start + size;
 	uintptr_t spl_start = (uintptr_t)_start;
+#ifdef CONFIG_ARM
 	uintptr_t spl_end = (uintptr_t)_image_binary_end;
-	uintptr_t end = start + size;
+#else
+	uintptr_t spl_end = (uintptr_t)&_image_binary_end;
+#endif /* CONFIG_ARM */
 
 	if ((start >= spl_start && start < spl_end) ||
 	    (end > spl_start && end <= spl_end) ||
-- 
2.39.2

Re: [PATCH] spl: spl_legacy: Fix spl_end address for non ARM target

[Marek Vasut]

On 7/31/23 13:57, Shiji Yang wrote:
> Only ARM target defines _image_binary_end symbol as char*, All other
> targets define it as an ulong type in include/asm-generic/sections.h.
> 
> This patch fixes the boot failure on MIPS target. Error log:
> SPL: Image overlaps SPL
> 
> Fixes: 1b8a1be1a1f1 ("spl: spl_legacy: Fix spl_end address")
> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
> ---
>   common/spl/spl_legacy.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
> index 095443c63d..0fef890384 100644
> --- a/common/spl/spl_legacy.c
> +++ b/common/spl/spl_legacy.c
> @@ -18,9 +18,13 @@
>   
>   static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size)
>   {
> +	uintptr_t end = start + size;
>   	uintptr_t spl_start = (uintptr_t)_start;
> +#ifdef CONFIG_ARM
>   	uintptr_t spl_end = (uintptr_t)_image_binary_end;
> -	uintptr_t end = start + size;
> +#else
> +	uintptr_t spl_end = (uintptr_t)&_image_binary_end;

I _think_ only this extra & should be enough and that should also work 
on ARM, right ?

[...]

Re: [PATCH] spl: spl_legacy: Fix spl_end address for non ARM target

[Shiji Yang]

On Mon, 31 Jul 2023 16:17:39 +0200, Marek Vasut wrote:
>On 7/31/23 13:57, Shiji Yang wrote:
>> Only ARM target defines _image_binary_end symbol as char*, All other
>> targets define it as an ulong type in include/asm-generic/sections.h.
>> 
>> This patch fixes the boot failure on MIPS target. Error log:
>> SPL: Image overlaps SPL
>> 
>> Fixes: 1b8a1be1a1f1 ("spl: spl_legacy: Fix spl_end address")
>> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
>> ---
>>   common/spl/spl_legacy.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
>> index 095443c63d..0fef890384 100644
>> --- a/common/spl/spl_legacy.c
>> +++ b/common/spl/spl_legacy.c
>> @@ -18,9 +18,13 @@
>>   
>>   static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size)
>>   {
>> +     uintptr_t end = start + size;
>>        uintptr_t spl_start = (uintptr_t)_start;
>> +#ifdef CONFIG_ARM
>>        uintptr_t spl_end = (uintptr_t)_image_binary_end;
>> -     uintptr_t end = start + size;
>> +#else
>> +     uintptr_t spl_end = (uintptr_t)&_image_binary_end;
>
>I _think_ only this extra & should be enough and that should also work 
>on ARM, right ?
>
>[...]

Hi! Thanks for your review. For ARM target, My understanding is that
'&_image_binary_end' is a random value. '&_image_binary_end' is a
pointer which points to the spl image end address. I think it's not
okay for ARM target.

I do not have any relevant ARM devices, so I can't test it.

Re: [PATCH] spl: spl_legacy: Fix spl_end address for non ARM target

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]

On Tue, Aug 01, 2023 at 12:10:52AM +0800, Shiji Yang wrote:
> On Mon, 31 Jul 2023 16:17:39 +0200, Marek Vasut wrote:
> >On 7/31/23 13:57, Shiji Yang wrote:
> >> Only ARM target defines _image_binary_end symbol as char*, All other
> >> targets define it as an ulong type in include/asm-generic/sections.h.
> >> 
> >> This patch fixes the boot failure on MIPS target. Error log:
> >> SPL: Image overlaps SPL
> >> 
> >> Fixes: 1b8a1be1a1f1 ("spl: spl_legacy: Fix spl_end address")
> >> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
> >> ---
> >>   common/spl/spl_legacy.c | 6 +++++-
> >>   1 file changed, 5 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
> >> index 095443c63d..0fef890384 100644
> >> --- a/common/spl/spl_legacy.c
> >> +++ b/common/spl/spl_legacy.c
> >> @@ -18,9 +18,13 @@
> >>   
> >>   static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size)
> >>   {
> >> +     uintptr_t end = start + size;
> >>        uintptr_t spl_start = (uintptr_t)_start;
> >> +#ifdef CONFIG_ARM
> >>        uintptr_t spl_end = (uintptr_t)_image_binary_end;
> >> -     uintptr_t end = start + size;
> >> +#else
> >> +     uintptr_t spl_end = (uintptr_t)&_image_binary_end;
> >
> >I _think_ only this extra & should be enough and that should also work 
> >on ARM, right ?
> >
> >[...]
> 
> Hi! Thanks for your review. For ARM target, My understanding is that
> '&_image_binary_end' is a random value. '&_image_binary_end' is a
> pointer which points to the spl image end address. I think it's not
> okay for ARM target.
> 
> I do not have any relevant ARM devices, so I can't test it.

OK, but why isn't the answer that _image_binary_end should be char for
everyone, and that it was simply mis-placed in the header to start with
(which is why it was then later added to the header for non-ARM)? Given
how everyone not-ARM treats it exactly like ARM does in linker scripts,
that seems the most likely case.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] spl: spl_legacy: Fix spl_end address for non ARM target

[Shiji Yang]

On Mon, 31 Jul 2023 15:12:27 -0400, Tom Rini wrote:
> On Tue, Aug 01, 2023 at 12:10:52AM +0800, Shiji Yang wrote:
> > On Mon, 31 Jul 2023 16:17:39 +0200, Marek Vasut wrote:
> > >On 7/31/23 13:57, Shiji Yang wrote:
> > >> Only ARM target defines _image_binary_end symbol as char*, All other
> > >> targets define it as an ulong type in include/asm-generic/sections.h.
> > >>
> > >> This patch fixes the boot failure on MIPS target. Error log:
> > >> SPL: Image overlaps SPL
> > >>
> > >> Fixes: 1b8a1be1a1f1 ("spl: spl_legacy: Fix spl_end address")
> > >> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
> > >> ---
> > >>   common/spl/spl_legacy.c | 6 +++++-
> > >>   1 file changed, 5 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
> > >> index 095443c63d..0fef890384 100644
> > >> --- a/common/spl/spl_legacy.c
> > >> +++ b/common/spl/spl_legacy.c
> > >> @@ -18,9 +18,13 @@
> > >>  
> > >>   static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size)
> > >>   {
> > >> +     uintptr_t end = start + size;
> > >>        uintptr_t spl_start = (uintptr_t)_start;
> > >> +#ifdef CONFIG_ARM
> > >>        uintptr_t spl_end = (uintptr_t)_image_binary_end;
> > >> -     uintptr_t end = start + size;
> > >> +#else
> > >> +     uintptr_t spl_end = (uintptr_t)&_image_binary_end;
> > >
> > >I _think_ only this extra & should be enough and that should also work
> > >on ARM, right ?
> > >
> > >[...]
> >
> > Hi! Thanks for your review. For ARM target, My understanding is that
> > '&_image_binary_end' is a random value. '&_image_binary_end' is a
> > pointer which points to the spl image end address. I think it's not
> > okay for ARM target.
> >
> > I do not have any relevant ARM devices, so I can't test it.
> 
> OK, but why isn't the answer that _image_binary_end should be char for
> everyone, and that it was simply mis-placed in the header to start with
> (which is why it was then later added to the header for non-ARM)? Given
> how everyone not-ARM treats it exactly like ARM does in linker scripts,
> that seems the most likely case.
>
> -- 
> Tom

Importing variables from linker scripts is a littile magic to me.
Based on my test, yes you are right. I can make MIPS target work
again by converting '_image_binary_end' from ulong to char[] type.
I'll try to cook some patches to convert all symbols in 'sections.h'
to char[]. I believe this is the right way to fix some potential
issues.

If you want to do this work, please let me know. I am a newbie for
u-boot. It seems that you are more familiar with the u-boot framework.

Regards
Shiji Yang

Re: [PATCH] spl: spl_legacy: Fix spl_end address for non ARM target

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3126 bytes --]

On Tue, Aug 01, 2023 at 10:35:19AM +0800, Shiji Yang wrote:
> On Mon, 31 Jul 2023 15:12:27 -0400, Tom Rini wrote:
> > On Tue, Aug 01, 2023 at 12:10:52AM +0800, Shiji Yang wrote:
> > > On Mon, 31 Jul 2023 16:17:39 +0200, Marek Vasut wrote:
> > > >On 7/31/23 13:57, Shiji Yang wrote:
> > > >> Only ARM target defines _image_binary_end symbol as char*, All other
> > > >> targets define it as an ulong type in include/asm-generic/sections.h.
> > > >>
> > > >> This patch fixes the boot failure on MIPS target. Error log:
> > > >> SPL: Image overlaps SPL
> > > >>
> > > >> Fixes: 1b8a1be1a1f1 ("spl: spl_legacy: Fix spl_end address")
> > > >> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
> > > >> ---
> > > >>   common/spl/spl_legacy.c | 6 +++++-
> > > >>   1 file changed, 5 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
> > > >> index 095443c63d..0fef890384 100644
> > > >> --- a/common/spl/spl_legacy.c
> > > >> +++ b/common/spl/spl_legacy.c
> > > >> @@ -18,9 +18,13 @@
> > > >>  
> > > >>   static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size)
> > > >>   {
> > > >> +     uintptr_t end = start + size;
> > > >>        uintptr_t spl_start = (uintptr_t)_start;
> > > >> +#ifdef CONFIG_ARM
> > > >>        uintptr_t spl_end = (uintptr_t)_image_binary_end;
> > > >> -     uintptr_t end = start + size;
> > > >> +#else
> > > >> +     uintptr_t spl_end = (uintptr_t)&_image_binary_end;
> > > >
> > > >I _think_ only this extra & should be enough and that should also work
> > > >on ARM, right ?
> > > >
> > > >[...]
> > >
> > > Hi! Thanks for your review. For ARM target, My understanding is that
> > > '&_image_binary_end' is a random value. '&_image_binary_end' is a
> > > pointer which points to the spl image end address. I think it's not
> > > okay for ARM target.
> > >
> > > I do not have any relevant ARM devices, so I can't test it.
> > 
> > OK, but why isn't the answer that _image_binary_end should be char for
> > everyone, and that it was simply mis-placed in the header to start with
> > (which is why it was then later added to the header for non-ARM)? Given
> > how everyone not-ARM treats it exactly like ARM does in linker scripts,
> > that seems the most likely case.
> >
> > -- 
> > Tom
> 
> Importing variables from linker scripts is a littile magic to me.
> Based on my test, yes you are right. I can make MIPS target work
> again by converting '_image_binary_end' from ulong to char[] type.
> I'll try to cook some patches to convert all symbols in 'sections.h'
> to char[]. I believe this is the right way to fix some potential
> issues.
> 
> If you want to do this work, please let me know. I am a newbie for
> u-boot. It seems that you are more familiar with the u-boot framework.

The only symbol I see for sure that needs to be changed is
'_image_binary_end' so if you'd like to have a go at converting that and
making a patch and running CI:
https://u-boot.readthedocs.io/en/latest/develop/ci_testing.html
I'd appreciate it, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]