| Message ID | 20220520141048.20034-2-peng.fan@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | arm64: binman: use binman symbols for imx | expand |
On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > set the symbol as weak not work if LTO is enabled. Since u_boot_any is > only used on X86 for now, so guard it with X86, otherwise build break > if we use BINMAN_SYMBOLS on i.MX. > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > common/spl/spl.c | 8 ++++++-- > common/spl/spl_ram.c | 4 ++++ > 2 files changed, 10 insertions(+), 2 deletions(-) I think we long term need to figure this out and address it so LTO works. But for now can you please guard this with a test on LTO instead, so it's clear where the problem is?
> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > > On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > set the symbol as weak not work if LTO is enabled. Since u_boot_any is > > only used on X86 for now, so guard it with X86, otherwise build break > > if we use BINMAN_SYMBOLS on i.MX. > > > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > common/spl/spl.c | 8 ++++++-- > > common/spl/spl_ram.c | 4 ++++ > > 2 files changed, 10 insertions(+), 2 deletions(-) > > I think we long term need to figure this out and address it so LTO works. But > for now can you please guard this with a test on LTO instead, so it's clear > where the problem is? Sorry, I could not get your point about guard with a test on LTO. Actually binman weak symbol will report a warning log if there is no u_boot_any binman symbol. Since only X86 use it, I guard with X86. Thanks, Peng. > > -- > Tom
On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > > > > On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote: > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > set the symbol as weak not work if LTO is enabled. Since u_boot_any is > > > only used on X86 for now, so guard it with X86, otherwise build break > > > if we use BINMAN_SYMBOLS on i.MX. > > > > > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > common/spl/spl.c | 8 ++++++-- > > > common/spl/spl_ram.c | 4 ++++ > > > 2 files changed, 10 insertions(+), 2 deletions(-) > > > > I think we long term need to figure this out and address it so LTO works. But > > for now can you please guard this with a test on LTO instead, so it's clear > > where the problem is? > > Sorry, I could not get your point about guard with a test on LTO. > > Actually binman weak symbol will report a warning log if there is no u_boot_any > binman symbol. Since only X86 use it, I guard with X86. Why are you mentioning LTO in the commit message? When I read the commit message it sounds like you're saying the problem is that LTO doesn't like how this symbol is handled, but if LTO was disabled, everything would be fine. If it's not LTO-related, please re-word the message instead.
On 20/05/2022 17:10, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > set the symbol as weak not work if LTO is enabled. Since u_boot_any is > only used on X86 for now, so guard it with X86, otherwise build break > if we use BINMAN_SYMBOLS on i.MX. > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > common/spl/spl.c | 8 ++++++-- > common/spl/spl_ram.c | 4 ++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > [...] If I apply the series without this patch and try to build imx8mm-beacon, I get the following error, and I'm assuming that's what you're trying to solve here: binman: Section '/binman/u-boot-spl-ddr': \ Symbol '_binman_u_boot_any_prop_image_pos' in entry \ '/binman/u-boot-spl-ddr/u-boot-spl/u-boot-spl-nodtb': \ Entry 'u-boot-any' not found in list (...) The immediate cause for this is that your 'u-boot-spl-ddr' image has no 'u-boot'-like entry, meaning there's no reasonable value to write into the non-optional 'u_boot_any' symbol. (I think it might be OK to make this symbol optional, but LTO breaks that as you found out. The rest of this email is about my thoughts before I realized the LTO problem, but they're still relevant.) It looks like binman images were designed to be monolithic instead of modular, and it's assumed you'd have one fully-specified image for your 'flash.bin' where 'u-boot-spl' would know about (for example) a 'u-boot' in a FIT entry. (I would like things to be modular eventually, though). I tried to merge things into a single binman image, but mkimage is trying to read 'u-boot-spl-ddr.bin' which would no longer exist. I see this is because it's specified in 'spl/u-boot-spl.cfgout' as a LOADER file in a later patch. Maybe 'imx8mimage.c' can be changed to use mkimage's -d argument as the LOADER instead? That's how binman passes the input data to mkimage, but that seems to be ignored in this case. Then we can merge things into a single image which will be able to set the 'u_boot_any' symbols without error. Otherwise, the mkimage file reading error can be solved by creating a new binman 'imx8m-image' entry type that creates the 'cfgout' and calls mkimage with it. (Binman images would still need to be merged).
On 21/05/2022 15:05, Tom Rini wrote: > On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote: >>> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 >>> >>> On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote: >>> >>>> From: Peng Fan <peng.fan@nxp.com> >>>> >>>> set the symbol as weak not work if LTO is enabled. Since u_boot_any is >>>> only used on X86 for now, so guard it with X86, otherwise build break >>>> if we use BINMAN_SYMBOLS on i.MX. >>>> >>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice >>>> Signed-off-by: Peng Fan <peng.fan@nxp.com> >>>> --- >>>> common/spl/spl.c | 8 ++++++-- >>>> common/spl/spl_ram.c | 4 ++++ >>>> 2 files changed, 10 insertions(+), 2 deletions(-) >>> >>> I think we long term need to figure this out and address it so LTO works. But >>> for now can you please guard this with a test on LTO instead, so it's clear >>> where the problem is? >> >> Sorry, I could not get your point about guard with a test on LTO. >> >> Actually binman weak symbol will report a warning log if there is no u_boot_any >> binman symbol. Since only X86 use it, I guard with X86. > > Why are you mentioning LTO in the commit message? When I read the > commit message it sounds like you're saying the problem is that LTO > doesn't like how this symbol is handled, but if LTO was disabled, > everything would be fine. If it's not LTO-related, please re-word the > message instead. It looks like we should be able to change things in common/spl/spl.c to: #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) /* See spl.h for information about this */ binman_sym_declare_optional(ulong, u_boot_any, image_pos); binman_sym_declare_optional(ulong, u_boot_any, size); #endif which would mark the symbol as 'weak' and turn the error into a warning on the binman side. But that is somehow being undone by LTO. I'm trying to build for imx8mm-beacon with that change instead of this patch. With CONFIG_LTO=y, build fails and spl/u-boot-spl.sym has: > 00000000007fbe28 l O .binman_sym 0000000000000008 _binman_u_boot_any_prop_image_pos Looks like the size symbol is optimized out. With CONFIG_LTO unset, the build succeeds and the same file has: > 00000000007fe90c w O .binman_sym 0000000000000008 _binman_u_boot_any_prop_image_pos > 00000000007fe904 w O .binman_sym 0000000000000008 _binman_u_boot_any_prop_size I don't know much about linking stuff, so this is as deep as I could dig...
On Sun, May 22, 2022 at 04:56:08PM +0300, Alper Nebi Yasak wrote: > On 21/05/2022 15:05, Tom Rini wrote: > > On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote: > >>> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > >>> > >>> On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote: > >>> > >>>> From: Peng Fan <peng.fan@nxp.com> > >>>> > >>>> set the symbol as weak not work if LTO is enabled. Since u_boot_any is > >>>> only used on X86 for now, so guard it with X86, otherwise build break > >>>> if we use BINMAN_SYMBOLS on i.MX. > >>>> > >>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice > >>>> Signed-off-by: Peng Fan <peng.fan@nxp.com> > >>>> --- > >>>> common/spl/spl.c | 8 ++++++-- > >>>> common/spl/spl_ram.c | 4 ++++ > >>>> 2 files changed, 10 insertions(+), 2 deletions(-) > >>> > >>> I think we long term need to figure this out and address it so LTO works. But > >>> for now can you please guard this with a test on LTO instead, so it's clear > >>> where the problem is? > >> > >> Sorry, I could not get your point about guard with a test on LTO. > >> > >> Actually binman weak symbol will report a warning log if there is no u_boot_any > >> binman symbol. Since only X86 use it, I guard with X86. > > > > Why are you mentioning LTO in the commit message? When I read the > > commit message it sounds like you're saying the problem is that LTO > > doesn't like how this symbol is handled, but if LTO was disabled, > > everything would be fine. If it's not LTO-related, please re-word the > > message instead. > > It looks like we should be able to change things in common/spl/spl.c to: > > #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) > /* See spl.h for information about this */ > binman_sym_declare_optional(ulong, u_boot_any, image_pos); > binman_sym_declare_optional(ulong, u_boot_any, size); > #endif > > which would mark the symbol as 'weak' and turn the error into a warning > on the binman side. But that is somehow being undone by LTO. So, looking at binman_sym_declare_optional we tell the linker that it's weak and might even be unused. LTO gets very aggressive about finding things that aren't used in the resulting binary and discarding them. Typically we have the problem of a function that is used but it's hard for LTO to see it, so we give it the "used" attribute. But for something we're already saying is "unused" this would be wrong. So why do we mark things as unused here? I assume it's marked weak as it could be overridden at link time by a definition elsewhere.
> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > > On 21/05/2022 15:05, Tom Rini wrote: > > On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote: > >>> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > >>> > >>> On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote: > >>> > >>>> From: Peng Fan <peng.fan@nxp.com> > >>>> > >>>> set the symbol as weak not work if LTO is enabled. Since u_boot_any > >>>> is only used on X86 for now, so guard it with X86, otherwise build > >>>> break if we use BINMAN_SYMBOLS on i.MX. > >>>> > >>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice > >>>> Signed-off-by: Peng Fan <peng.fan@nxp.com> > >>>> --- > >>>> common/spl/spl.c | 8 ++++++-- > >>>> common/spl/spl_ram.c | 4 ++++ > >>>> 2 files changed, 10 insertions(+), 2 deletions(-) > >>> > >>> I think we long term need to figure this out and address it so LTO > >>> works. But for now can you please guard this with a test on LTO > >>> instead, so it's clear where the problem is? > >> > >> Sorry, I could not get your point about guard with a test on LTO. > >> > >> Actually binman weak symbol will report a warning log if there is no > >> u_boot_any binman symbol. Since only X86 use it, I guard with X86. > > > > Why are you mentioning LTO in the commit message? When I read the > > commit message it sounds like you're saying the problem is that LTO > > doesn't like how this symbol is handled, but if LTO was disabled, > > everything would be fine. If it's not LTO-related, please re-word the > > message instead. > > It looks like we should be able to change things in common/spl/spl.c to: > > #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) > /* See spl.h for information about this */ > binman_sym_declare_optional(ulong, u_boot_any, image_pos); > binman_sym_declare_optional(ulong, u_boot_any, size); > #endif > > which would mark the symbol as 'weak' and turn the error into a warning on > the binman side. But that is somehow being undone by LTO. > > I'm trying to build for imx8mm-beacon with that change instead of this patch. > With CONFIG_LTO=y, build fails and spl/u-boot-spl.sym has: > > > 00000000007fbe28 l O .binman_sym 0000000000000008 > _binman_u_boot_any_prop_image_pos > > Looks like the size symbol is optimized out. With CONFIG_LTO unset, the build > succeeds and the same file has: > > > 00000000007fe90c w O .binman_sym 0000000000000008 > _binman_u_boot_any_prop_image_pos > > 00000000007fe904 w O .binman_sym 0000000000000008 > _binman_u_boot_any_prop_size > > I don't know much about linking stuff, so this is as deep as I could dig... Thanks for the detailed sharing. Yes, this is the issue I try to work around. Some i.MX boards has LTO set, this leads me to use X86 as an extra guard. Thanks, Peng.
> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > > On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote: > > > Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > > > > > > On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote: > > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > set the symbol as weak not work if LTO is enabled. Since > > > > u_boot_any is only used on X86 for now, so guard it with X86, > > > > otherwise build break if we use BINMAN_SYMBOLS on i.MX. > > > > > > > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > --- > > > > common/spl/spl.c | 8 ++++++-- > > > > common/spl/spl_ram.c | 4 ++++ > > > > 2 files changed, 10 insertions(+), 2 deletions(-) > > > > > > I think we long term need to figure this out and address it so LTO > > > works. But for now can you please guard this with a test on LTO > > > instead, so it's clear where the problem is? > > > > Sorry, I could not get your point about guard with a test on LTO. > > > > Actually binman weak symbol will report a warning log if there is no > > u_boot_any binman symbol. Since only X86 use it, I guard with X86. > > Why are you mentioning LTO in the commit message? When I read the > commit message it sounds like you're saying the problem is that LTO doesn't > like how this symbol is handled, but if LTO was disabled, everything would be > fine. If it's not LTO-related, please re-word the message instead. Sorry, I could reword the commit message, but currently I have no better idea to address the issue unless use X86 as a guard in the code as this patch does. If you agree the code in this patch, I could reword commit msg and send v5. Thanks, Peng. > > -- > Tom
On Mon, May 23, 2022 at 06:28:44AM +0000, Peng Fan (OSS) wrote: > > Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > > > > On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote: > > > > Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 > > > > > > > > On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote: > > > > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > > > set the symbol as weak not work if LTO is enabled. Since > > > > > u_boot_any is only used on X86 for now, so guard it with X86, > > > > > otherwise build break if we use BINMAN_SYMBOLS on i.MX. > > > > > > > > > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > > --- > > > > > common/spl/spl.c | 8 ++++++-- > > > > > common/spl/spl_ram.c | 4 ++++ > > > > > 2 files changed, 10 insertions(+), 2 deletions(-) > > > > > > > > I think we long term need to figure this out and address it so LTO > > > > works. But for now can you please guard this with a test on LTO > > > > instead, so it's clear where the problem is? > > > > > > Sorry, I could not get your point about guard with a test on LTO. > > > > > > Actually binman weak symbol will report a warning log if there is no > > > u_boot_any binman symbol. Since only X86 use it, I guard with X86. > > > > Why are you mentioning LTO in the commit message? When I read the > > commit message it sounds like you're saying the problem is that LTO doesn't > > like how this symbol is handled, but if LTO was disabled, everything would be > > fine. If it's not LTO-related, please re-word the message instead. > > Sorry, I could reword the commit message, but currently I have no better > idea to address the issue unless use X86 as a guard in the code as this > patch does. If you agree the code in this patch, I could reword commit msg > and send v5. Well, lets see what Alper says in the other part of the thread. I'd really like to solve this not work around this. But I'll take documenting the problem for the person that has X86 && LTO as good enough for the moment.
On 22/05/2022 17:50, Tom Rini wrote: > On Sun, May 22, 2022 at 04:56:08PM +0300, Alper Nebi Yasak wrote: >> It looks like we should be able to change things in common/spl/spl.c to: >> >> #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) >> /* See spl.h for information about this */ >> binman_sym_declare_optional(ulong, u_boot_any, image_pos); >> binman_sym_declare_optional(ulong, u_boot_any, size); >> #endif >> >> which would mark the symbol as 'weak' and turn the error into a warning >> on the binman side. But that is somehow being undone by LTO. > > So, looking at binman_sym_declare_optional we tell the linker that it's > weak and might even be unused. LTO gets very aggressive about finding > things that aren't used in the resulting binary and discarding them. > Typically we have the problem of a function that is used but it's hard > for LTO to see it, so we give it the "used" attribute. But for > something we're already saying is "unused" this would be wrong. So why > do we mark things as unused here? I assume it's marked weak as it could > be overridden at link time by a definition elsewhere. I don't know, but the GCC manual says marking things 'unused' will suppress a warning if the variable is actually unused. I guess binman symbols may become unused due to some other config options being unset, and it's done for those cases? I think the optional symbols are marked weak just to signal the python side that they are optional. If I understand it right, binman: 1. Packs an image using the finalized 'spl/u-boot-spl.bin' 2. Figures out the image-pos and size values of every entry 3. Inspects the 'spl/u-boot-spl' ELF file for declared binman symbols 4. Raises an error if any non-'weak' symbol's value is undetermined 5. Updates the u-boot-spl.bin (in memory) with the calculated values 6. Recreates the final image based on that updated bin file.
On 23/05/2022 17:10, Tom Rini wrote: > On Mon, May 23, 2022 at 06:28:44AM +0000, Peng Fan (OSS) wrote: >>> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86 >>> >>> Why are you mentioning LTO in the commit message? When I read the >>> commit message it sounds like you're saying the problem is that LTO doesn't >>> like how this symbol is handled, but if LTO was disabled, everything would be >>> fine. If it's not LTO-related, please re-word the message instead. >> >> Sorry, I could reword the commit message, but currently I have no better >> idea to address the issue unless use X86 as a guard in the code as this >> patch does. If you agree the code in this patch, I could reword commit msg >> and send v5. > > Well, lets see what Alper says in the other part of the thread. I'd > really like to solve this not work around this. But I'll take > documenting the problem for the person that has X86 && LTO as good > enough for the moment. Alternatively, I think we can decide that all binman symbols are optional, and not raise an error when we can't find a value for them. They are de-facto optional anyway: people can use the non-binman-image build artifacts (which binman doesn't update wrt. symbols), thus the code that uses binman symbols should assume their values can be missing.
diff --git a/common/spl/spl.c b/common/spl/spl.c index c8c463f80bd..4b28180467a 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -50,7 +50,7 @@ DECLARE_GLOBAL_DATA_PTR; u32 *boot_params_ptr = NULL; -#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) +#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) && CONFIG_IS_ENABLED(X86) /* See spl.h for information about this */ binman_sym_declare(ulong, u_boot_any, image_pos); binman_sym_declare(ulong, u_boot_any, size); @@ -148,7 +148,7 @@ void spl_fixup_fdt(void *fdt_blob) #endif } -#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) +#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) && CONFIG_IS_ENABLED(X86) ulong spl_get_image_pos(void) { #ifdef CONFIG_VPL @@ -221,7 +221,11 @@ __weak struct image_header *spl_get_load_buffer(ssize_t offset, size_t size) void spl_set_header_raw_uboot(struct spl_image_info *spl_image) { +#if CONFIG_IS_ENABLED(X86) ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos); +#else + ulong u_boot_pos = BINMAN_SYM_MISSING; +#endif spl_image->size = CONFIG_SYS_MONITOR_LEN; diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c index 82964592571..083b14102ee 100644 --- a/common/spl/spl_ram.c +++ b/common/spl/spl_ram.c @@ -70,7 +70,11 @@ static int spl_ram_load_image(struct spl_image_info *spl_image, load.read = spl_ram_load_read; spl_load_simple_fit(spl_image, &load, 0, header); } else { +#if CONFIG_IS_ENABLED(X86) ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos); +#else + ulong u_boot_pos = BINMAN_SYM_MISSING; +#endif debug("Legacy image\n"); /*