| Message ID | 20210916173021.3347-1-trini@konsulko.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | xtensa: Put U-Boot version string at correct place by linker script | expand |
On Thu, Sep 16, 2021 at 01:30:21PM -0400, Tom Rini wrote: > *(.literal .text) \ > + *(.literal .text_version_string) \ Isn't ".litteral" a duplication? Even if I'm pretty sure it will not cause any difference in the generated binary I would remove it. Francesco
On Thu, Sep 16, 2021 at 09:38:19PM +0200, Francesco Dolcini wrote: > On Thu, Sep 16, 2021 at 01:30:21PM -0400, Tom Rini wrote: > > *(.literal .text) \ > > + *(.literal .text_version_string) \ > > Isn't ".litteral" a duplication? Even if I'm pretty sure it will not cause any difference > in the generated binary I would remove it. Honestly, I don't know xtensa. We also don't have qemu support for it currently, and I'm a bit worried in general about the state of the platform (it's on my TODO list to poke some people now). So, I hesitate to make any change that's not basically copy/paste of the existing lines.
On Thu, Sep 16, 2021 at 03:42:20PM -0400, Tom Rini wrote: > On Thu, Sep 16, 2021 at 09:38:19PM +0200, Francesco Dolcini wrote: > > On Thu, Sep 16, 2021 at 01:30:21PM -0400, Tom Rini wrote: > > > *(.literal .text) \ > > > + *(.literal .text_version_string) \ > > > > Isn't ".litteral" a duplication? Even if I'm pretty sure it will not cause any difference > > in the generated binary I would remove it. > > Honestly, I don't know xtensa. We also don't have qemu support for it > currently, and I'm a bit worried in general about the state of the > platform (it's on my TODO list to poke some people now). So, I hesitate > to make any change that's not basically copy/paste of the existing > lines. I have no experience on xtensa either, but this is just the section name, and you are just telling to put ".literal" there 2 times. Francesco
On Thu, Sep 16, 2021 at 09:50:35PM +0200, Francesco Dolcini wrote: > On Thu, Sep 16, 2021 at 03:42:20PM -0400, Tom Rini wrote: > > On Thu, Sep 16, 2021 at 09:38:19PM +0200, Francesco Dolcini wrote: > > > On Thu, Sep 16, 2021 at 01:30:21PM -0400, Tom Rini wrote: > > > > *(.literal .text) \ > > > > + *(.literal .text_version_string) \ > > > > > > Isn't ".litteral" a duplication? Even if I'm pretty sure it will not cause any difference > > > in the generated binary I would remove it. > > > > Honestly, I don't know xtensa. We also don't have qemu support for it > > currently, and I'm a bit worried in general about the state of the > > platform (it's on my TODO list to poke some people now). So, I hesitate > > to make any change that's not basically copy/paste of the existing > > lines. > > I have no experience on xtensa either, but this is just the section > name, and you are just telling to put ".literal" there 2 times. I don't know. Looking at the resulting linker script (and with your suggestion): .text (((0x00002000 - 0x00002000) + ((128 << 20))) - 0x00040000) : AT(((LOADADDR(.DoubleExceptionVector.text) + SIZEOF(.DoubleExceptionVector.text) + 4 -1)) & ~(4 -1)) { _text_start = ABSOLUTE(.); *(.literal .text) *(.text_version_string) *(.literal.* .text.* .stub) *(.gnu.warning .gnu.linkonce.literal.*) *(.gnu.linkonce.t.*.literal .gnu.linkonce.t.*) *(.fini.literal) *(.fini) *(.gnu.version) _text_end = ABSOLUTE(.); } So there's "literal" scattered everywhere. If it doesn't matter, it reads more consistently to me to toss literal in one more time. But I emailed around and we'll see if xtensa stays around, it's also our oldest toolchain and virtually untouched since submission :(
On Thu, Sep 16, 2021 at 04:13:31PM -0400, Tom Rini wrote: > On Thu, Sep 16, 2021 at 09:50:35PM +0200, Francesco Dolcini wrote: > > On Thu, Sep 16, 2021 at 03:42:20PM -0400, Tom Rini wrote: > > > On Thu, Sep 16, 2021 at 09:38:19PM +0200, Francesco Dolcini wrote: > > > > On Thu, Sep 16, 2021 at 01:30:21PM -0400, Tom Rini wrote: > > > > > *(.literal .text) \ > > > > > + *(.literal .text_version_string) \ > > > > > > > > Isn't ".litteral" a duplication? Even if I'm pretty sure it will not cause any difference > > > > in the generated binary I would remove it. > > > > > > Honestly, I don't know xtensa. We also don't have qemu support for it > > > currently, and I'm a bit worried in general about the state of the > > > platform (it's on my TODO list to poke some people now). So, I hesitate > > > to make any change that's not basically copy/paste of the existing > > > lines. > > > > I have no experience on xtensa either, but this is just the section > > name, and you are just telling to put ".literal" there 2 times. > > I don't know. Looking at the resulting linker script (and with your > suggestion): > .text (((0x00002000 - 0x00002000) + ((128 << 20))) - 0x00040000) : AT(((LOADADDR(.DoubleExceptionVector.text) + SIZEOF(.DoubleExceptionVector.text) + 4 -1)) & ~(4 -1)) { _text_start = ABSOLUTE(.); *(.literal .text) *(.text_version_string) *(.literal.* .text.* .stub) *(.gnu.warning .gnu.linkonce.literal.*) *(.gnu.linkonce.t.*.literal .gnu.linkonce.t.*) > *(.fini.literal) *(.fini) *(.gnu.version) _text_end = ABSOLUTE(.); } > > So there's "literal" scattered everywhere. If it doesn't matter, it > reads more consistently to me to toss literal in one more time. Heheh, ".literal" is present only once here, ".literal.*" is matching something else, likewise all the others that are scathered all around. This looks fine to me. > But I emailed around and we'll see if xtensa stays around, it's also our > oldest toolchain and virtually untouched since submission :( My small comment is just about the linker script format, I guess some other folk should be able to confirm what I wrote. Unfortunately it will not solve the general problem with xtensa ... Francesco
On Thu, Sep 16, 2021 at 10:30 AM Tom Rini <trini@konsulko.com> wrote: > > Update the linker script macros to know that we need to include the > .text_version_string section now as well. > > Signed-off-by: Tom Rini <trini@konsulko.com> > --- > arch/xtensa/include/asm/ldscript.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/xtensa/include/asm/ldscript.h b/arch/xtensa/include/asm/ldscript.h > index 08f5d0135ed0..84c496e09f1b 100644 > --- a/arch/xtensa/include/asm/ldscript.h > +++ b/arch/xtensa/include/asm/ldscript.h > @@ -72,6 +72,7 @@ > { \ > _text_start = ABSOLUTE(.); \ > *(.literal .text) \ > + *(.literal .text_version_string) \ This does not belong to .text, as far as I understand it's rodata and so it should go with rodata, probably like this: ---8<--- diff --git a/arch/xtensa/include/asm/ldscript.h b/arch/xtensa/include/asm/ldscript.h index 08f5d0135ed0..e03fcffdd6f1 100644 --- a/arch/xtensa/include/asm/ldscript.h +++ b/arch/xtensa/include/asm/ldscript.h @@ -87,6 +87,7 @@ _rodata_start = ABSOLUTE(.); \ *(.rodata) \ *(.rodata.*) \ + *(.text_version_string) \ *(.dtb.init.rodata) \ *(.gnu.linkonce.r.*) \ *(.rodata1) \ ---8<---
On Thu, Sep 16, 2021 at 12:42 PM Tom Rini <trini@konsulko.com> wrote:
> We also don't have qemu support for it
I'm curious what happened to it and what I should do to update it?
xtensa is still supported in the QEMU and AFAIK nothing has changed
about it: neither building nor invocation.
On Thu, Sep 16, 2021 at 10:21:29PM -0700, Max Filippov wrote: > On Thu, Sep 16, 2021 at 12:42 PM Tom Rini <trini@konsulko.com> wrote: > > We also don't have qemu support for it > > I'm curious what happened to it and what I should do to update it? > > xtensa is still supported in the QEMU and AFAIK nothing has changed > about it: neither building nor invocation. OK, yes, I was just wrong there. It's still there, it's part of Azure and GitLab CI, tested every time. Sorry for the confusion.
On Thu, Sep 16, 2021 at 10:14:03PM -0700, Max Filippov wrote: > On Thu, Sep 16, 2021 at 10:30 AM Tom Rini <trini@konsulko.com> wrote: > > > > Update the linker script macros to know that we need to include the > > .text_version_string section now as well. > > > > Signed-off-by: Tom Rini <trini@konsulko.com> > > --- > > arch/xtensa/include/asm/ldscript.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/xtensa/include/asm/ldscript.h b/arch/xtensa/include/asm/ldscript.h > > index 08f5d0135ed0..84c496e09f1b 100644 > > --- a/arch/xtensa/include/asm/ldscript.h > > +++ b/arch/xtensa/include/asm/ldscript.h > > @@ -72,6 +72,7 @@ > > { \ > > _text_start = ABSOLUTE(.); \ > > *(.literal .text) \ > > + *(.literal .text_version_string) \ > > This does not belong to .text, as far as I understand it's rodata and so > it should go with rodata, probably like this: > ---8<--- > diff --git a/arch/xtensa/include/asm/ldscript.h > b/arch/xtensa/include/asm/ldscript.h > index 08f5d0135ed0..e03fcffdd6f1 100644 > --- a/arch/xtensa/include/asm/ldscript.h > +++ b/arch/xtensa/include/asm/ldscript.h > @@ -87,6 +87,7 @@ > _rodata_start = ABSOLUTE(.); \ > *(.rodata) \ > *(.rodata.*) \ > + *(.text_version_string) \ > *(.dtb.init.rodata) \ > *(.gnu.linkonce.r.*) \ > *(.rodata1) \ So this is in context with: https://patchwork.ozlabs.org/project/uboot/list/?series=256258 and specifically: https://patchwork.ozlabs.org/project/uboot/patch/20210802131838.21097-7-pali@kernel.org/ Now that said, I think the whole point of that has been removed with: https://patchwork.ozlabs.org/project/uboot/patch/20210916195648.9424-1-trini@konsulko.com/ and no longer needing to reference it from the linker script for PowerPC. So maybe I can just drop this whole part.
On Fri, Sep 17, 2021 at 5:04 AM Tom Rini <trini@konsulko.com> wrote: > So this is in context with: > https://patchwork.ozlabs.org/project/uboot/list/?series=256258 > and specifically: > https://patchwork.ozlabs.org/project/uboot/patch/20210802131838.21097-7-pali@kernel.org/ Yes, I've found that series and the purpose of this patch puzzled me, given the absence of actual references to the newly added section in the series. > Now that said, I think the whole point of that has been removed with: > https://patchwork.ozlabs.org/project/uboot/patch/20210916195648.9424-1-trini@konsulko.com/ > and no longer needing to reference it from the linker script for > PowerPC. Aha, that explains the purpose of that section, thanks. > So maybe I can just drop this whole part.
diff --git a/arch/xtensa/include/asm/ldscript.h b/arch/xtensa/include/asm/ldscript.h index 08f5d0135ed0..84c496e09f1b 100644 --- a/arch/xtensa/include/asm/ldscript.h +++ b/arch/xtensa/include/asm/ldscript.h @@ -72,6 +72,7 @@ { \ _text_start = ABSOLUTE(.); \ *(.literal .text) \ + *(.literal .text_version_string) \ *(.literal.* .text.* .stub) \ *(.gnu.warning .gnu.linkonce.literal.*) \ *(.gnu.linkonce.t.*.literal .gnu.linkonce.t.*) \
Update the linker script macros to know that we need to include the .text_version_string section now as well. Signed-off-by: Tom Rini <trini@konsulko.com> --- arch/xtensa/include/asm/ldscript.h | 1 + 1 file changed, 1 insertion(+)