| Message ID | 20190824132315.53130-27-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | binman: Various improvements and tidy-ups | expand |
At present binman adds the image base address to the symbol value before
it writes it to the binary. This is not correct since the symbol value
itself (e.g. image position) has no relationship to the image base.
Fix this and update the tests to cover this case.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
tools/binman/elf.py | 4 +---
tools/binman/test/u_boot_binman_syms.lds | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
Applied to u-boot-dm, thanks!
On 9/26/19 6:38 PM, sjg@google.com wrote: > At present binman adds the image base address to the symbol value before > it writes it to the binary. This is not correct since the symbol value > itself (e.g. image position) has no relationship to the image base. > > Fix this and update the tests to cover this case. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > tools/binman/elf.py | 4 +--- > tools/binman/test/u_boot_binman_syms.lds | 2 +- > 2 files changed, 2 insertions(+), 4 deletions(-) > > Applied to u-boot-dm, thanks! This seems to have only just been pushed. This patch breaks boot on Jetson TK1; u-boot-dm.git master hangs in SPL or before the main U-Boot prints anything, whereas after reverting this patch solves the issue. With this patch applied, all I get is: U-Boot SPL 2019.10-00490-g4f035abcde98 (Oct 14 2019 - 09:48:30 -0600) Trying to boot from RAM
Hi Stephen, On Mon, 14 Oct 2019 at 09:49, Stephen Warren <swarren@wwwdotorg.org> wrote: > > On 9/26/19 6:38 PM, sjg@google.com wrote: > > At present binman adds the image base address to the symbol value before > > it writes it to the binary. This is not correct since the symbol value > > itself (e.g. image position) has no relationship to the image base. > > > > Fix this and update the tests to cover this case. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > tools/binman/elf.py | 4 +--- > > tools/binman/test/u_boot_binman_syms.lds | 2 +- > > 2 files changed, 2 insertions(+), 4 deletions(-) > > > > Applied to u-boot-dm, thanks! > > This seems to have only just been pushed. This patch breaks boot on > Jetson TK1; u-boot-dm.git master hangs in SPL or before the main U-Boot > prints anything, whereas after reverting this patch solves the issue. > > With this patch applied, all I get is: > > U-Boot SPL 2019.10-00490-g4f035abcde98 (Oct 14 2019 - 09:48:30 -0600) > Trying to boot from RAM Yes, just pushed as I had to wait for u-boot-dm/testing to pass, and it has some flaky runs and then I went on holiday. This is unfortunate. It looks like we were missing test coverage. I'll see if I can look at it later in the week, but for now I think I might drop this patch. Regards, Simon
On 10/15/19 8:07 AM, Simon Glass wrote: > Hi Stephen, > > On Mon, 14 Oct 2019 at 09:49, Stephen Warren <swarren@wwwdotorg.org> wrote: >> >> On 9/26/19 6:38 PM, sjg@google.com wrote: >>> At present binman adds the image base address to the symbol value before >>> it writes it to the binary. This is not correct since the symbol value >>> itself (e.g. image position) has no relationship to the image base. >>> >>> Fix this and update the tests to cover this case. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> tools/binman/elf.py | 4 +--- >>> tools/binman/test/u_boot_binman_syms.lds | 2 +- >>> 2 files changed, 2 insertions(+), 4 deletions(-) >>> >>> Applied to u-boot-dm, thanks! >> >> This seems to have only just been pushed. This patch breaks boot on >> Jetson TK1; u-boot-dm.git master hangs in SPL or before the main U-Boot >> prints anything, whereas after reverting this patch solves the issue. >> >> With this patch applied, all I get is: >> >> U-Boot SPL 2019.10-00490-g4f035abcde98 (Oct 14 2019 - 09:48:30 -0600) >> Trying to boot from RAM > > Yes, just pushed as I had to wait for u-boot-dm/testing to pass, and > it has some flaky runs and then I went on holiday. > > This is unfortunate. It looks like we were missing test coverage. I'll > see if I can look at it later in the week, but for now I think I might > drop this patch. Thanks. The latest push to u-boot-dm/master solves/removes this issue.
On 10/15/19 10:09 AM, Stephen Warren wrote: > On 10/15/19 8:07 AM, Simon Glass wrote: >> Hi Stephen, >> >> On Mon, 14 Oct 2019 at 09:49, Stephen Warren <swarren@wwwdotorg.org> >> wrote: >>> >>> On 9/26/19 6:38 PM, sjg@google.com wrote: >>>> At present binman adds the image base address to the symbol value >>>> before >>>> it writes it to the binary. This is not correct since the symbol value >>>> itself (e.g. image position) has no relationship to the image base. >>>> >>>> Fix this and update the tests to cover this case. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> --- >>>> >>>> tools/binman/elf.py | 4 +--- >>>> tools/binman/test/u_boot_binman_syms.lds | 2 +- >>>> 2 files changed, 2 insertions(+), 4 deletions(-) >>>> >>>> Applied to u-boot-dm, thanks! >>> >>> This seems to have only just been pushed. This patch breaks boot on >>> Jetson TK1; u-boot-dm.git master hangs in SPL or before the main U-Boot >>> prints anything, whereas after reverting this patch solves the issue. >>> >>> With this patch applied, all I get is: >>> >>> U-Boot SPL 2019.10-00490-g4f035abcde98 (Oct 14 2019 - 09:48:30 -0600) >>> Trying to boot from RAM >> >> Yes, just pushed as I had to wait for u-boot-dm/testing to pass, and >> it has some flaky runs and then I went on holiday. >> >> This is unfortunate. It looks like we were missing test coverage. I'll >> see if I can look at it later in the week, but for now I think I might >> drop this patch. > > Thanks. The latest push to u-boot-dm/master solves/removes this issue. This patch has now shown in in u-boot/master and u-boot-video/master, so Jetson TK1 testing is broken there now. Reverting this patch fixes the issue (I only tested that in u-boot/master).
Hi Stephen, On Mon, 4 Nov 2019 at 10:34, Stephen Warren <swarren@wwwdotorg.org> wrote: > > On 10/15/19 10:09 AM, Stephen Warren wrote: > > On 10/15/19 8:07 AM, Simon Glass wrote: > >> Hi Stephen, > >> > >> On Mon, 14 Oct 2019 at 09:49, Stephen Warren <swarren@wwwdotorg.org> > >> wrote: > >>> > >>> On 9/26/19 6:38 PM, sjg@google.com wrote: > >>>> At present binman adds the image base address to the symbol value > >>>> before > >>>> it writes it to the binary. This is not correct since the symbol value > >>>> itself (e.g. image position) has no relationship to the image base. > >>>> > >>>> Fix this and update the tests to cover this case. > >>>> > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > >>>> --- > >>>> > >>>> tools/binman/elf.py | 4 +--- > >>>> tools/binman/test/u_boot_binman_syms.lds | 2 +- > >>>> 2 files changed, 2 insertions(+), 4 deletions(-) > >>>> > >>>> Applied to u-boot-dm, thanks! > >>> > >>> This seems to have only just been pushed. This patch breaks boot on > >>> Jetson TK1; u-boot-dm.git master hangs in SPL or before the main U-Boot > >>> prints anything, whereas after reverting this patch solves the issue. > >>> > >>> With this patch applied, all I get is: > >>> > >>> U-Boot SPL 2019.10-00490-g4f035abcde98 (Oct 14 2019 - 09:48:30 -0600) > >>> Trying to boot from RAM > >> > >> Yes, just pushed as I had to wait for u-boot-dm/testing to pass, and > >> it has some flaky runs and then I went on holiday. > >> > >> This is unfortunate. It looks like we were missing test coverage. I'll > >> see if I can look at it later in the week, but for now I think I might > >> drop this patch. > > > > Thanks. The latest push to u-boot-dm/master solves/removes this issue. > > This patch has now shown in in u-boot/master and u-boot-video/master, so > Jetson TK1 testing is broken there now. Reverting this patch fixes the > issue (I only tested that in u-boot/master). Arggh OK I will sort this out this week. Regards, Simon
diff --git a/tools/binman/elf.py b/tools/binman/elf.py index 7bc7cf61b5..0c1a5b44b6 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -135,9 +135,7 @@ def LookupAndWriteSymbols(elf_fname, entry, section): # Look up the symbol in our entry tables. value = section.LookupSymbol(name, sym.weak, msg) - if value is not None: - value += base.address - else: + if value is None: value = -1 pack_string = pack_string.lower() value_bytes = struct.pack(pack_string, value) diff --git a/tools/binman/test/u_boot_binman_syms.lds b/tools/binman/test/u_boot_binman_syms.lds index 926df873cb..825fc3f649 100644 --- a/tools/binman/test/u_boot_binman_syms.lds +++ b/tools/binman/test/u_boot_binman_syms.lds @@ -9,7 +9,7 @@ ENTRY(_start) SECTIONS { - . = 0x00000000; + . = 0x00000010; _start = .; . = ALIGN(4);
At present binman adds the image base address to the symbol value before it writes it to the binary. This is not correct since the symbol value itself (e.g. image position) has no relationship to the image base. Fix this and update the tests to cover this case. Signed-off-by: Simon Glass <sjg@chromium.org> --- tools/binman/elf.py | 4 +--- tools/binman/test/u_boot_binman_syms.lds | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-)