[PATCH] spl: spl_legacy: Fix spl_end address for non ARM target
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) ||
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 ?
[...]
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.
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.
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
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!
participants (3)
-
Marek Vasut -
Shiji Yang -
Tom Rini