| Message ID | 1567530547-14331-1-git-send-email-patrick.delaunay@st.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] env: Add CONFIG_ENV_SUPPORT | expand |
Hi Patrick, > Add a new flag CONFIG_ENV_SUPPORT to compile all > the environment features in U-Boot (attributes, callbacks > and flags). It is the equivalent of the 2 existing flags > - CONFIG_SPL_ENV_SUPPORT for SPL > - CONFIG_TPL_ENV_SUPPORT for TPL Shouldn't it be the "supplement" ? I guess that it is possible to define CONFIG_SPL_ENV_SUPPORT to have ENV support in SPL CONFIG_TPL_ENV_SUPPORT to have ENV support in TPL and there is a third flag - CONFIG_ENV_SUPPORT to enable envs support in U-Boot proper? In that way one can enable ENV support only in SPL (via SPL_ENV_SUPPORT) and disable envs in U-Boot proper (by NOT defining ENV_SUPPORT) and use build in envs (in the binary). > > This new configuration allows to use the macro > CONFIG_IS_ENABLED(ENV_SUPPORT) in the code without > issue and solves the regression introduced by > commit 7d4776545b0f ("env: solve compilation error > in SPL"); change_ok was always NULL in U-Boot. > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > --- > > cmd/Kconfig | 2 ++ > env/Kconfig | 7 +++++++ > env/Makefile | 11 ++++------- > include/env_callback.h | 4 ++++ > include/env_flags.h | 4 ++++ > 5 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/cmd/Kconfig b/cmd/Kconfig > index 05872fa..f7a1b1f 100644 > --- a/cmd/Kconfig > +++ b/cmd/Kconfig > @@ -465,6 +465,7 @@ config CMD_ENV_EXISTS > > config CMD_ENV_CALLBACK > bool "env callbacks - print callbacks and their associated > variables" > + depends on ENV_SUPPORT > help > Some environment variable have callbacks defined by > U_BOOT_ENV_CALLBACK. These are called when the variable > changes. @@ -473,6 +474,7 @@ config CMD_ENV_CALLBACK > > config CMD_ENV_FLAGS > bool "env flags -print variables that have non-default flags" > + depends on ENV_SUPPORT > help > Some environment variables have special flags that control > their behaviour. For example, serial# can only be written once and > cannot diff --git a/env/Kconfig b/env/Kconfig > index 74db2f3..f0c5a7a 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -1,5 +1,12 @@ > menu "Environment" > > +config ENV_SUPPORT > + bool "Support all environment features" > + default y > + help > + Enable full environment support in U-Boot, > + including attributes, callbacks and flags. > + > config ENV_IS_NOWHERE > bool "Environment is not stored" > default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \ > diff --git a/env/Makefile b/env/Makefile > index 90144d6..2a468ac 100644 > --- a/env/Makefile > +++ b/env/Makefile > @@ -5,10 +5,11 @@ > > obj-y += common.o env.o > > +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o > +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o > +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o > + > ifndef CONFIG_SPL_BUILD > -obj-y += attr.o > -obj-y += callback.o > -obj-y += flags.o > obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o > extra-$(CONFIG_ENV_IS_EMBEDDED) += embedded.o > obj-$(CONFIG_ENV_IS_IN_EEPROM) += embedded.o > @@ -19,10 +20,6 @@ obj-$(CONFIG_ENV_IS_IN_ONENAND) += onenand.o > obj-$(CONFIG_ENV_IS_IN_SATA) += sata.o > obj-$(CONFIG_ENV_IS_IN_REMOTE) += remote.o > obj-$(CONFIG_ENV_IS_IN_UBI) += ubi.o > -else > -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o > -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o > -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o > endif > > obj-$(CONFIG_$(SPL_TPL_)ENV_IS_NOWHERE) += nowhere.o > diff --git a/include/env_callback.h b/include/env_callback.h > index 982c078..a757fe6 100644 > --- a/include/env_callback.h > +++ b/include/env_callback.h > @@ -72,6 +72,10 @@ > "serial#:serialno," \ > CONFIG_ENV_CALLBACK_LIST_STATIC > > +#if CONFIG_IS_ENABLED(ENV_SUPPORT) > void env_callback_init(struct env_entry *var_entry); > +#else > +static inline void env_callback_init(struct env_entry *var_entry) { } > +#endif > > #endif /* __ENV_CALLBACK_H__ */ > diff --git a/include/env_flags.h b/include/env_flags.h > index e5380f2..ec480e3 100644 > --- a/include/env_flags.h > +++ b/include/env_flags.h > @@ -153,7 +153,11 @@ int env_flags_validate_env_set_params(char > *name, char *const val[], int count); > * When adding a variable to the environment, initialize the flags > for that > * variable. > */ > +#if CONFIG_IS_ENABLED(ENV_SUPPORT) > void env_flags_init(struct env_entry *var_entry); > +#else > +static inline void env_flags_init(struct env_entry *var_entry) { } > +#endif > > /* > * Validate the newval for to conform with the requirements defined > by its flags Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, > From: Lukasz Majewski <lukma@denx.de> > Sent: mercredi 4 septembre 2019 10:52 > > Hi Patrick, > > > Add a new flag CONFIG_ENV_SUPPORT to compile all the environment > > features in U-Boot (attributes, callbacks and flags). It is the > > equivalent of the 2 existing flags > > - CONFIG_SPL_ENV_SUPPORT for SPL > > - CONFIG_TPL_ENV_SUPPORT for TPL > > Shouldn't it be the "supplement" ? > > I guess that it is possible to define > > CONFIG_SPL_ENV_SUPPORT to have ENV support in SPL > > CONFIG_TPL_ENV_SUPPORT to have ENV support in TPL > > and there is a third flag - CONFIG_ENV_SUPPORT to enable envs support in U- > Boot proper? > > In that way one can enable ENV support only in SPL (via SPL_ENV_SUPPORT) > and disable envs in U-Boot proper (by NOT defining ENV_SUPPORT) and use > build in envs (in the binary). Yes exactly the support for U-Boot, SPL or TPL are now independent, I will update the commit message in V2, "supplement" is more clear. > > > > > This new configuration allows to use the macro > > CONFIG_IS_ENABLED(ENV_SUPPORT) in the code without issue and solves > > the regression introduced by commit 7d4776545b0f ("env: solve > > compilation error in SPL"); change_ok was always NULL in U-Boot. > > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > > --- > > > > cmd/Kconfig | 2 ++ > > env/Kconfig | 7 +++++++ > > env/Makefile | 11 ++++------- > > include/env_callback.h | 4 ++++ > > include/env_flags.h | 4 ++++ > > 5 files changed, 21 insertions(+), 7 deletions(-) > > > > Best regards, > > Lukasz Majewski > Best regards Patrick Delaunay
Dear Patrick, In message <1567530547-14331-1-git-send-email-patrick.delaunay@st.com> you wrote: > Add a new flag CONFIG_ENV_SUPPORT to compile all > the environment features in U-Boot (attributes, callbacks > and flags). It is the equivalent of the 2 existing flags I think this is a bda name, as it is misleading. It sounds as if it is used to enable the support of environment vaiables at all, which it does not - instead it only enables / disables a few specific extended features. This must be reflected in the name. > - CONFIG_SPL_ENV_SUPPORT for SPL > - CONFIG_TPL_ENV_SUPPORT for TPL This scares me. Why are there different settings for SPL, TPL and U-Boot proper? This looks conceptually broken to me - if a system is configured to use a specific set of environment features and extensions, then the exact same settings must be use in all of SPL, TPL and U-Boot proper. I understand that it may be desirable for example to reduce the size of the SPL by omitting some environment extensions, but provide these in U-Boot proper, but this is broken and dangerous. For example, U-Boot flags are often used to enforce a certain level of security (say, by making environment variables read-only or such). The same level of handling and protection must also be maintained in SPL and TPL. So please reconsider this whole implementation, and make sure that only a single macro ise used everywhere to enable these features. Best regards, Wolfgang Denk
Hi, > From: Wolfgang Denk <wd@denx.de> > Sent: samedi 7 septembre 2019 13:52 > > Dear Patrick, > > In message <1567530547-14331-1-git-send-email-patrick.delaunay@st.com> you > wrote: > > Add a new flag CONFIG_ENV_SUPPORT to compile all the environment > > features in U-Boot (attributes, callbacks and flags). It is the > > equivalent of the 2 existing flags > > I think this is a bda name, as it is misleading. It sounds as if it is used to enable > the support of environment vaiables at all, which it does not - instead it only > enables / disables a few specific extended features. This must be reflected in the > name. Yes, I use the name than SPL/TPL to use the macro CONFIG_IS_ENABLED(...) And I agree the name seens not perfect. > > - CONFIG_SPL_ENV_SUPPORT for SPL > > - CONFIG_TPL_ENV_SUPPORT for TPL These pre-existing name are defined in common/spl/Kconfig With the same issue (env/common.o env/env.o are always compiled for SPL/TPL so it is alo bad names) > This scares me. Why are there different settings for SPL, TPL and U-Boot > proper? This looks conceptually broken to me - if a system is configured to use a > specific set of environment features and extensions, then the exact same settings > must be use in all of SPL, TPL and U-Boot proper. > > I understand that it may be desirable for example to reduce the size of the SPL by > omitting some environment extensions, but provide these in U-Boot proper, but > this is broken and dangerous. For example, U-Boot flags are often used to > enforce a certain level of security (say, by making environment variables read- > only or such). I agree, that scare me also. For me also ENV_SUPPORT should be always enable for U-Boot. My preferred proposal was only to solve the regression introduced by my initial patch and always set change_ok for U-Boot proper (when CONFIG_SPL_BUILD is not defined): struct hsearch_data env_htab = { - #if CONFIG_IS_ENABLED(ENV_SUPPORT) - /* defined in flags.c, only compile with ENV_SUPPORT */ +#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT) + /* defined in flags.c, only compile with ENV_SUPPORT for SPL / TPL */ .change_ok = env_flags_validate, #endif }; http://u-boot.10912.n7.nabble.com/U-Boot-Environment-flags-broken-for-U-Boot-tt382673.html#a382688 The same test is already done in: drivers/reset/reset-socfpga.c:47:#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT) drivers/input/input.c:656:#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT) > The same level of handling and protection must also be maintained in SPL and > TPL. if I understood correctly the proposed dependency is : TPL ENV_SUPPORT select SPL_ENV_SUPPORT SPL ENV_SUPPORT select ENV_SUPPORT > So please reconsider this whole implementation, and make sure that only a single > macro ise used everywhere to enable these features. But, if I use the same CONFIG for the 3 binary SPL,TPL and U-Boot, l increase the size of TPL/SPL for all the platforms when these additional features are not needed. And I am not the sure of the correct dependency for ENV between binary. Heiko what is you feedback on Wolfgang remarks.... Do you think that I need to come back to the first/simple proposal ? > > Best regards, > > Wolfgang Denk > > -- Best regards Patrick Delaunay
On Tue, Sep 10, 2019 at 11:01:18AM +0000, Patrick DELAUNAY wrote: > Hi, > > > From: Wolfgang Denk <wd@denx.de> > > Sent: samedi 7 septembre 2019 13:52 > > > > Dear Patrick, > > > > In message <1567530547-14331-1-git-send-email-patrick.delaunay@st.com> you > > wrote: > > > Add a new flag CONFIG_ENV_SUPPORT to compile all the environment > > > features in U-Boot (attributes, callbacks and flags). It is the > > > equivalent of the 2 existing flags > > > > I think this is a bda name, as it is misleading. It sounds as if it is used to enable > > the support of environment vaiables at all, which it does not - instead it only > > enables / disables a few specific extended features. This must be reflected in the > > name. > > Yes, I use the name than SPL/TPL to use the macro CONFIG_IS_ENABLED(...) > > And I agree the name seens not perfect. > > > > - CONFIG_SPL_ENV_SUPPORT for SPL > > > - CONFIG_TPL_ENV_SUPPORT for TPL > > These pre-existing name are defined in common/spl/Kconfig > > With the same issue (env/common.o env/env.o are always compiled for SPL/TPL > so it is alo bad names) > > > This scares me. Why are there different settings for SPL, TPL and U-Boot > > proper? This looks conceptually broken to me - if a system is configured to use a > > specific set of environment features and extensions, then the exact same settings > > must be use in all of SPL, TPL and U-Boot proper. > > > > I understand that it may be desirable for example to reduce the size of the SPL by > > omitting some environment extensions, but provide these in U-Boot proper, but > > this is broken and dangerous. For example, U-Boot flags are often used to > > enforce a certain level of security (say, by making environment variables read- > > only or such). > > I agree, that scare me also. > For me also ENV_SUPPORT should be always enable for U-Boot. > > My preferred proposal was only to solve the regression introduced by my initial patch and > always set change_ok for U-Boot proper (when CONFIG_SPL_BUILD is not defined): > > struct hsearch_data env_htab = { > - #if CONFIG_IS_ENABLED(ENV_SUPPORT) > - /* defined in flags.c, only compile with ENV_SUPPORT */ > +#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT) > + /* defined in flags.c, only compile with ENV_SUPPORT for SPL / TPL */ > .change_ok = env_flags_validate, > #endif > }; > > http://u-boot.10912.n7.nabble.com/U-Boot-Environment-flags-broken-for-U-Boot-tt382673.html#a382688 > > The same test is already done in: > > drivers/reset/reset-socfpga.c:47:#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT) > drivers/input/input.c:656:#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT) > > > The same level of handling and protection must also be maintained in SPL and > > TPL. > > if I understood correctly the proposed dependency is : > TPL ENV_SUPPORT select SPL_ENV_SUPPORT > SPL ENV_SUPPORT select ENV_SUPPORT > > > So please reconsider this whole implementation, and make sure that only a single > > macro ise used everywhere to enable these features. > > But, if I use the same CONFIG for the 3 binary SPL,TPL and U-Boot, > l increase the size of TPL/SPL for all the platforms when these additional features are not needed. > > And I am not the sure of the correct dependency for ENV between binary. > > Heiko what is you feedback on Wolfgang remarks.... > > Do you think that I need to come back to the first/simple proposal ? My two cents is that I would like to see a regression fix "soon" and for this release, and some corrections / updates to names, what is/isn't possible to enable (keeping in mind what is desirable to allow) for the next release. Thanks all!
Dear Patrick, In message <9c7801afb8c94c638933cf33746ae300@SFHDAG6NODE3.st.com> you wrote: > > And I agree the name seens not perfect. > > > > - CONFIG_SPL_ENV_SUPPORT for SPL > > > - CONFIG_TPL_ENV_SUPPORT for TPL > > These pre-existing name are defined in common/spl/Kconfig > > With the same issue (env/common.o env/env.o are always compiled for SPL/TPL > so it is alo bad names) Correct. > > So please reconsider this whole implementation, and make sure that only a single > > macro ise used everywhere to enable these features. > > But, if I use the same CONFIG for the 3 binary SPL,TPL and U-Boot, > l increase the size of TPL/SPL for all the platforms when these > additional features are not needed. Either the U-Boot environment makes use of these features, then they have to be enabled, and exactly the same way in SPL, TPL and U-Boot proper. Or you don't need them, then they can be disabled, but again in a consistent way in SPL, TPL, and U-Boot proper. It is not acceptable to have for example .flags support in U-Boot, but not is SPL. If you cannot affort the size in SPL (and need environment there at all), then you cannot have it in U-Boot either. Yes, this is sad, but anything else would break the implementation of these features, and given that they are often used to implement some level of protection or security, introduce massive security issues. So if SPL size is critical, you can try do not access the environment at all and omit _all_ of the environment code there; or you can try to arrange for a read-only implementation (omitting at least the code needed for "env save" including write routines to storage). But you CANNOT omit the extensions if these are present in U-Boot proper. Best regards, Wolfgang Denk
Hi Wolfgang an Tom, > > Dear Patrick, > > In message <9c7801afb8c94c638933cf33746ae300@SFHDAG6NODE3.st.com> > you wrote: > > > > And I agree the name seens not perfect. > > > > > > - CONFIG_SPL_ENV_SUPPORT for SPL > > > > - CONFIG_TPL_ENV_SUPPORT for TPL > > > > These pre-existing name are defined in common/spl/Kconfig > > > > With the same issue (env/common.o env/env.o are always compiled for > > SPL/TPL so it is alo bad names) > > Correct. > > > > So please reconsider this whole implementation, and make sure that > > > only a single macro ise used everywhere to enable these features. > > > > But, if I use the same CONFIG for the 3 binary SPL,TPL and U-Boot, l > > increase the size of TPL/SPL for all the platforms when these > > additional features are not needed. > > Either the U-Boot environment makes use of these features, then they have to be > enabled, and exactly the same way in SPL, TPL and U-Boot proper. Or you don't > need them, then they can be disabled, but again in a consistent way in SPL, TPL, > and U-Boot proper. > > It is not acceptable to have for example .flags support in U-Boot, but not is SPL. > If you cannot affort the size in SPL (and need environment there at all), then you > cannot have it in U-Boot either. > Yes, this is sad, but anything else would break the implementation of these > features, and given that they are often used to implement some level of protection > or security, introduce massive security issues. > > > So if SPL size is critical, you can try do not access the environment at all and omit > _all_ of the environment code there; or you can try to arrange for a read-only > implementation (omitting at least the code needed for "env save" including write > routines to storage). But you CANNOT omit the extensions if these are present in > U-Boot proper. I am working on a update of the first proposal and I will delivered it in 2 step: 1- a simple patch to solve the regression (to have short term solution / integration in master as requested by Tom) 2- introduction of CONFGI_ENV_FULL_SUPPORT => compilation of attr / flags / callback for TPL/SPL/U-boot => feature can't be remove in SPL/TPL independently As proposed by Wolfgang > Best regards, > > Wolfgang Denk > Regards Patrick
diff --git a/cmd/Kconfig b/cmd/Kconfig index 05872fa..f7a1b1f 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -465,6 +465,7 @@ config CMD_ENV_EXISTS config CMD_ENV_CALLBACK bool "env callbacks - print callbacks and their associated variables" + depends on ENV_SUPPORT help Some environment variable have callbacks defined by U_BOOT_ENV_CALLBACK. These are called when the variable changes. @@ -473,6 +474,7 @@ config CMD_ENV_CALLBACK config CMD_ENV_FLAGS bool "env flags -print variables that have non-default flags" + depends on ENV_SUPPORT help Some environment variables have special flags that control their behaviour. For example, serial# can only be written once and cannot diff --git a/env/Kconfig b/env/Kconfig index 74db2f3..f0c5a7a 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -1,5 +1,12 @@ menu "Environment" +config ENV_SUPPORT + bool "Support all environment features" + default y + help + Enable full environment support in U-Boot, + including attributes, callbacks and flags. + config ENV_IS_NOWHERE bool "Environment is not stored" default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \ diff --git a/env/Makefile b/env/Makefile index 90144d6..2a468ac 100644 --- a/env/Makefile +++ b/env/Makefile @@ -5,10 +5,11 @@ obj-y += common.o env.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o + ifndef CONFIG_SPL_BUILD -obj-y += attr.o -obj-y += callback.o -obj-y += flags.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o extra-$(CONFIG_ENV_IS_EMBEDDED) += embedded.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += embedded.o @@ -19,10 +20,6 @@ obj-$(CONFIG_ENV_IS_IN_ONENAND) += onenand.o obj-$(CONFIG_ENV_IS_IN_SATA) += sata.o obj-$(CONFIG_ENV_IS_IN_REMOTE) += remote.o obj-$(CONFIG_ENV_IS_IN_UBI) += ubi.o -else -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o endif obj-$(CONFIG_$(SPL_TPL_)ENV_IS_NOWHERE) += nowhere.o diff --git a/include/env_callback.h b/include/env_callback.h index 982c078..a757fe6 100644 --- a/include/env_callback.h +++ b/include/env_callback.h @@ -72,6 +72,10 @@ "serial#:serialno," \ CONFIG_ENV_CALLBACK_LIST_STATIC +#if CONFIG_IS_ENABLED(ENV_SUPPORT) void env_callback_init(struct env_entry *var_entry); +#else +static inline void env_callback_init(struct env_entry *var_entry) { } +#endif #endif /* __ENV_CALLBACK_H__ */ diff --git a/include/env_flags.h b/include/env_flags.h index e5380f2..ec480e3 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -153,7 +153,11 @@ int env_flags_validate_env_set_params(char *name, char *const val[], int count); * When adding a variable to the environment, initialize the flags for that * variable. */ +#if CONFIG_IS_ENABLED(ENV_SUPPORT) void env_flags_init(struct env_entry *var_entry); +#else +static inline void env_flags_init(struct env_entry *var_entry) { } +#endif /* * Validate the newval for to conform with the requirements defined by its flags
Add a new flag CONFIG_ENV_SUPPORT to compile all the environment features in U-Boot (attributes, callbacks and flags). It is the equivalent of the 2 existing flags - CONFIG_SPL_ENV_SUPPORT for SPL - CONFIG_TPL_ENV_SUPPORT for TPL This new configuration allows to use the macro CONFIG_IS_ENABLED(ENV_SUPPORT) in the code without issue and solves the regression introduced by commit 7d4776545b0f ("env: solve compilation error in SPL"); change_ok was always NULL in U-Boot. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- cmd/Kconfig | 2 ++ env/Kconfig | 7 +++++++ env/Makefile | 11 ++++------- include/env_callback.h | 4 ++++ include/env_flags.h | 4 ++++ 5 files changed, 21 insertions(+), 7 deletions(-)