| Message ID | 20201110202603.20944-5-rasmus.villemoes@prevas.dk |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | allow default environment to be amended from dtb | expand |
Dear Rasmus Villemoes, In message <20201110202603.20944-5-rasmus.villemoes@prevas.dk> you wrote: > It can be useful to use the same U-Boot binary for multiple purposes, > say the normal one, one for developers that allow breaking into the > U-Boot shell, and one for use during bootstrapping which runs a > special-purpose bootcmd. To that end, allow the control dtb to contain > a /config/default-enviroment property, whose value will be used to > amend the default environment baked into the U-Boot binary itself. No, this is not what should be done. Please try to get used to the idea behind the so called "default environment". Only now I realize that this was a badly chosen name, but last_resort_in_case_of_emergencies_environment would have had other problems. The default environment is something which is NOT INTENDED for regular use. it is what you will fall back to in case (and ONLY in that case) when your regular persistent environment cannot be used, for example because it is not readable (I/O errors or such) or not properly initialized or corrupted (CRC checksum error). It is not the intended use but still somewhat acceptable to use it as initial data to populate the regular environment in other cases, too. But that's it. Apending data to it is not acceptable. If you need to append data, then only to the regular environment. And please, for the sake of avoiding further confusiion, please do not name this "default-environment". Thanks. Best regards, Wolfgang Denk
Hi Rasmus, On Thu, 12 Nov 2020 at 12:59, Wolfgang Denk <wd@denx.de> wrote: > > Dear Rasmus Villemoes, > > In message <20201110202603.20944-5-rasmus.villemoes@prevas.dk> you wrote: > > It can be useful to use the same U-Boot binary for multiple purposes, > > say the normal one, one for developers that allow breaking into the > > U-Boot shell, and one for use during bootstrapping which runs a > > special-purpose bootcmd. To that end, allow the control dtb to contain > > a /config/default-enviroment property, whose value will be used to > > amend the default environment baked into the U-Boot binary itself. > > No, this is not what should be done. > > Please try to get used to the idea behind the so called "default > environment". Only now I realize that this was a badly chosen name, > but last_resort_in_case_of_emergencies_environment would have had > other problems. > > The default environment is something which is NOT INTENDED for > regular use. it is what you will fall back to in case (and ONLY in > that case) when your regular persistent environment cannot be used, > for example because it is not readable (I/O errors or such) or not > properly initialized or corrupted (CRC checksum error). > > It is not the intended use but still somewhat acceptable to use it > as initial data to populate the regular environment in other cases, > too. But that's it. > > Apending data to it is not acceptable. If you need to append data, > then only to the regular environment. > > > And please, for the sake of avoiding further confusiion, please do > not name this "default-environment". Apart from what Wolfgang says here, it does seem useful. I wonder if we should have a way to load the (whole) environment from DT? Regards, Simon
On 17/11/2020 00.52, Simon Glass wrote: > Hi Rasmus, > > On Thu, 12 Nov 2020 at 12:59, Wolfgang Denk <wd@denx.de> wrote: >> >> Dear Rasmus Villemoes, >> >> In message <20201110202603.20944-5-rasmus.villemoes@prevas.dk> you wrote: >>> It can be useful to use the same U-Boot binary for multiple purposes, >>> say the normal one, one for developers that allow breaking into the >>> U-Boot shell, and one for use during bootstrapping which runs a >>> special-purpose bootcmd. To that end, allow the control dtb to contain >>> a /config/default-enviroment property, whose value will be used to >>> amend the default environment baked into the U-Boot binary itself. >> >> No, this is not what should be done. >> >> Please try to get used to the idea behind the so called "default >> environment". Only now I realize that this was a badly chosen name, >> but last_resort_in_case_of_emergencies_environment would have had >> other problems. >> >> The default environment is something which is NOT INTENDED for >> regular use. it is what you will fall back to in case (and ONLY in >> that case) when your regular persistent environment cannot be used, >> for example because it is not readable (I/O errors or such) or not >> properly initialized or corrupted (CRC checksum error). You seem to be ignoring the many cases where one chooses, for simplicity, robustness and/or security, not to have a writable environment at all. >> It is not the intended use but still somewhat acceptable to use it >> as initial data to populate the regular environment in other cases, >> too. But that's it. >> >> Apending data to it is not acceptable. If you need to append data, >> then only to the regular environment. I'm not really doing anything other than allowing a CONFIG_ENV_EXTRA_SETTINGS to live in .dtb rather than cooking all of it into the U-Boot binary itself. That's where board-specific additions are supposed to live nowadays I'd assume. >> And please, for the sake of avoiding further confusiion, please do >> not name this "default-environment". Sure. So would you be ok with some /config/extra-environment node which gets appended after the normal environment has been loaded? Then if I set CONFIG_ENV_IS_NOWHERE, I'd get what I have now. The only problem with that is that it might be a little weird to have static content of the chosen control dtb override something that might have been loaded from a writable storage location. But that's not really an intended combination anyway, and I can probably add a knob that allows one to choose whether the .dtb settings should be applied or not. > Apart from what Wolfgang says here, it does seem useful. > > I wonder if we should have a way to load the (whole) environment from DT? That will be my second choice, i.e. adding a "CONFIG_ENV_IS_IN_DTB" which obviously doesn't support saving, but has the advantages I'm after here of using one U-Boot binary with slightly different environments. Rasmus
Hi Rasmus, On 10.11.20 21:26, Rasmus Villemoes wrote: > It can be useful to use the same U-Boot binary for multiple purposes, > say the normal one, one for developers that allow breaking into the > U-Boot shell, and one for use during bootstrapping which runs a > special-purpose bootcmd. To that end, allow the control dtb to contain > a /config/default-enviroment property, whose value will be used to > amend the default environment baked into the U-Boot binary itself. > I have not checked the patch itself, but I disagree on the concept. What you suggest works outside a context where the environment is not just set by U-Boot. Simple use case is when you want to update the software and after the update you need to set the environment. The user space application needs to know the starting environment, else it breaks the board. It is already a pain that the default environemtn is really used as "the environment", that is the device simply boots. With your changes, we have then a set of "default" environment and on user space you cannot know which of them was taken by U-Boot (because this is dynamically done by U-Boot reading the fit Image). So from my point of view, this is NAK. Anyway, why do we try to addd everything to the "default" environment aka CONFIG_EXTRA_ENV_SETTINGS instead of storing together with u-boot a valid copy of the environment ? Best regards, Stefano > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > cmd/nvedit.c | 37 +++++++++++++++++++++++++++++++++++++ > env/Kconfig | 21 +++++++++++++++++++++ > env/common.c | 19 +++++++++++++++++++ > 3 files changed, 77 insertions(+) > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c > index 7fce723800..eda8b3b9d2 100644 > --- a/cmd/nvedit.c > +++ b/cmd/nvedit.c > @@ -703,6 +703,39 @@ char *from_env(const char *envvar) > return ret; > } > > +static int env_get_f_fdt(const char *name, char *buf, unsigned len) > +{ > + const char *env; > + int plen, nlen, n; > + > + if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT)) > + return 0; > + > + env = fdtdec_get_config_property(gd->fdt_blob, "default-environment", > + &plen); > + if (!env) > + return 0; > + > + nlen = strlen(name); > + while (plen > nlen) { > + if (memcmp(name, env, nlen) == 0 && env[nlen] == '=') { > + /* Found. Copy value. */ > + n = strlcpy(buf, &env[nlen + 1], len); > + if (n < len) > + return n; > + > + printf("env_buf [%u bytes] too small for value of \"%s\"\n", > + len, name); > + return len; > + } > + /* Skip this key=val pair. */ > + n = strlen(env) + 1; > + plen -= n; > + env += n; > + } > + return 0; > +} > + > /* > * Look up variable from environment for restricted C runtime env. > */ > @@ -710,6 +743,10 @@ int env_get_f(const char *name, char *buf, unsigned len) > { > int i, nxt, c; > > + i = env_get_f_fdt(name, buf, len); > + if (i) > + return i; > + > for (i = 0; env_get_char(i) != '\0'; i = nxt + 1) { > int val, n; > > diff --git a/env/Kconfig b/env/Kconfig > index 67ce93061b..66bbac42c7 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -646,6 +646,27 @@ config DELAY_ENVIRONMENT > later by U-Boot code. With CONFIG_OF_CONTROL this is instead > controlled by the value of /config/load-environment. > > +config ENV_AMEND_DEFAULT_FROM_FDT > + bool "Amend default environment by /config/default-environment property" > + depends on OF_CONTROL > + help > + The default environment built into the U-Boot binary is a > + static array defined from various CONFIG_ options, or via > + CONFIG_DEFAULT_ENV_FILE. Selecting this option means that > + whenever that default environment is used (either for > + populating the initial environment, or for resetting > + specific variables to their default value), the device tree > + property /config/default-environment is also consulted, and > + values found there have precedence over those in the static > + array. That property should be a series of "key=value" > + pairs, e.g. > + > + /config { > + default-environment = "ipaddr=1.2.3.4", > + "bootcmd=tftp $loadaddr foo.itb; bootm $loadaddr", > + "bootdelay=5"; > + } > + > config ENV_APPEND > bool "Always append the environment with new data" > default n > diff --git a/env/common.c b/env/common.c > index 7363da849b..8d0e45fde6 100644 > --- a/env/common.c > +++ b/env/common.c > @@ -63,6 +63,22 @@ char *env_get_default(const char *name) > return ret_val; > } > > +static void env_amend_default_from_fdt(int flags, int nvars, char *const vars[]) > +{ > + const void *val; > + int len; > + > + if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT)) > + return; > + > + val = fdtdec_get_config_property(gd->fdt_blob, "default-environment", > + &len); > + if (!val) > + return; > + > + himport_r(&env_htab, val, len, '\0', flags, 0, nvars, vars); > +} > + > void env_set_default(const char *s, int flags) > { > if (sizeof(default_environment) > ENV_SIZE) { > @@ -88,6 +104,8 @@ void env_set_default(const char *s, int flags) > pr_err("## Error: Environment import failed: errno = %d\n", > errno); > > + env_amend_default_from_fdt(flags | H_NOCLEAR, 0, NULL); > + > gd->flags |= GD_FLG_ENV_READY; > gd->flags |= GD_FLG_ENV_DEFAULT; > } > @@ -104,6 +122,7 @@ void env_set_default_vars(int nvars, char * const vars[], int flags) > himport_r(&env_htab, (const char *)default_environment, > sizeof(default_environment), '\0', > flags, 0, nvars, vars); > + env_amend_default_from_fdt(flags, nvars, vars); > } > > /* >
Dear Simon, In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=wg@mail.gmail.com> you wrote: > > > Apending data to it is not acceptable. If you need to append data, > > then only to the regular environment. > > > > And please, for the sake of avoiding further confusiion, please do > > not name this "default-environment". > > Apart from what Wolfgang says here, it does seem useful. On the other hand I wonder in which way "appending to the existing environment" is different from what "env import" does? > I wonder if we should have a way to load the (whole) environment from DT? You mean adding a DT type specifier to the "env import" command? Sounds good to me... Best regards, Wolfgang Denk
On Tue, Nov 17, 2020 at 12:31:07PM +0100, Wolfgang Denk wrote: > Dear Simon, > > In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=wg@mail.gmail.com> you wrote: > > > > > Apending data to it is not acceptable. If you need to append data, > > > then only to the regular environment. > > > > > > And please, for the sake of avoiding further confusiion, please do > > > not name this "default-environment". > > > > Apart from what Wolfgang says here, it does seem useful. > > On the other hand I wonder in which way "appending to the existing > environment" is different from what "env import" does? > > > I wonder if we should have a way to load the (whole) environment from DT? > > You mean adding a DT type specifier to the "env import" command? > > Sounds good to me... Adding in Marek as well since I believe he's been doing things with append-only environment, and it would be good to make sure we have something that fits everyones needs, and doesn't break what people are already doing as well.
Hi, On Tue, 17 Nov 2020 at 11:23, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Nov 17, 2020 at 12:31:07PM +0100, Wolfgang Denk wrote: > > Dear Simon, > > > > In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=wg@mail.gmail.com> you wrote: > > > > > > > Apending data to it is not acceptable. If you need to append data, > > > > then only to the regular environment. > > > > > > > > And please, for the sake of avoiding further confusiion, please do > > > > not name this "default-environment". > > > > > > Apart from what Wolfgang says here, it does seem useful. > > > > On the other hand I wonder in which way "appending to the existing > > environment" is different from what "env import" does? > > > > > I wonder if we should have a way to load the (whole) environment from DT? > > > > You mean adding a DT type specifier to the "env import" command? > > > > Sounds good to me... > > Adding in Marek as well since I believe he's been doing things with > append-only environment, and it would be good to make sure we have > something that fits everyones needs, and doesn't break what people are > already doing as well. Some years ago I did a series to allow the environment to come from a text file, thus avoiding the \0 stuff. Now binman has a 'u-boot-env' entry type, allowing creating an environment from a text file, with suitable checksumming. There is some advantage to having a default environment compiled into U-Boot that covers everything needed to boot. For one, the environment can be clobbered from userspace, which would otherwise render the device unbootable. For another, it is more secure to avoid loading unsigned data (the environment) from flash. Generally, for a secure boot, one would need to avoid loading the environment, at least without a lot of careful filtering. Regards, Simon
Dear Rasmus, In message <76615995-6700-1b3e-b598-4913e9882c26@prevas.dk> you wrote: > > >> The default environment is something which is NOT INTENDED for > >> regular use. it is what you will fall back to in case (and ONLY in > >> that case) when your regular persistent environment cannot be used, > >> for example because it is not readable (I/O errors or such) or not > >> properly initialized or corrupted (CRC checksum error). > > You seem to be ignoring the many cases where one chooses, for > simplicity, robustness and/or security, not to have a writable > environment at all. You seem to ignore what I write. Yes, there are many use cases where no writable environment is needed / wanted, but this is not what the default environment was intended for. For such cases some null driver similar to /dev/null should be used. > I'm not really doing anything other than allowing a > CONFIG_ENV_EXTRA_SETTINGS to live in .dtb rather than cooking all of it > into the U-Boot binary itself. That's where board-specific additions are > supposed to live nowadays I'd assume. Your statement is incorrect. CONFIG_ENV_EXTRA_SETTINGS is something that is processed at compile time, while your code attempts to modify the default environment in run time. these are totally different things. > >> And please, for the sake of avoiding further confusiion, please do > >> not name this "default-environment". > > Sure. So would you be ok with some /config/extra-environment node which > gets appended after the normal environment has been loaded? Then if I In principle yes, I see that this is a nice and useful feature. Just the notation of "append" seems wrong to me. We already have "env import" which can import additional / new environmane settings in different formats. What I think should be done is extending this by support for a new format (you may call it driver if you want) to import environment settings from a DTB. > set CONFIG_ENV_IS_NOWHERE, I'd get what I have now. The only problem Can't you see that this is not logical? If the environment is nowhere, then how can you add something to it? > with that is that it might be a little weird to have static content of > the chosen control dtb override something that might have been loaded > from a writable storage location. But that's not really an intended > combination anyway, and I can probably add a knob that allows one to > choose whether the .dtb settings should be applied or not. This is not weird in any way - this is what the "env import" command does by definition: it imports environment settigns from some other storage. > > I wonder if we should have a way to load the (whole) environment from DT? > > That will be my second choice, i.e. adding a "CONFIG_ENV_IS_IN_DTB" > which obviously doesn't support saving, but has the advantages I'm after > here of using one U-Boot binary with slightly different environments. I see no difference here. "env import" into an empty environment does just that. Best regards, Wolfgang Denk
Dear Tom, In message <20201117182353.GB5340@bill-the-cat> you wrote: > > Adding in Marek as well since I believe he's been doing things with > append-only environment, and it would be good to make sure we have > something that fits everyones needs, and doesn't break what people are > already doing as well. Actually we should stop people from doing things the wrong way. Misusing the "default environment" is not a good thing. That does not mean that the use cases should not be supported - but in a correct way. Best regards, Wolfgang Denk
Dear Simon, In message <CAPnjgZ1C3TQWkUuNQc0BHOGE957s0K-nxFmW3PVO_ObOcKwv+Q@mail.gmail.com> you wrote: > > Some years ago I did a series to allow the environment to come from a > text file, thus avoiding the \0 stuff. "env import -t" does that, you know? > Now binman has a 'u-boot-env' > entry type, allowing creating an environment from a text file, with > suitable checksumming. "env import -b" ... > There is some advantage to having a default environment compiled into > U-Boot that covers everything needed to boot. For one, the environment > can be clobbered from userspace, which would otherwise render the > device unbootable. For another, it is more secure to avoid loading > unsigned data (the environment) from flash. Generally, for a secure > boot, one would need to avoid loading the environment, at least > without a lot of careful filtering. One idea behind my rewrite of the environment handling (when I added hast table support) was that there should be more than one way to initialize the environment. Until then, we always had exactly one fixed location for the environment, probaly with a redundant copy. The code we have now actuially allows for a much greater flexibility. You can initialize the environment from a selection of copies, and (now, with proper driver support) also from several devices. If doen correctly, we could implement things like "profiles", where for example each user (or use case) can select his specific profile, initialize the environment from that, and save change to that. Ths could - for example - be used to switch between "development" and "production" modes. A "reset to factory defaults" would then just be an import from the (read-only) factory-defaults copy. etc. Importing from a DT is just a logical extension as it is considered just another storage device / driver. [In a Unix environment, all these would just be "files".] It's all there. We just have to use it. Best regards, Wolfgang Denk
On 20/11/2020 11.13, Wolfgang Denk wrote: > Dear Rasmus, > > In message <76615995-6700-1b3e-b598-4913e9882c26@prevas.dk> you wrote: >> >> Sure. So would you be ok with some /config/extra-environment node which >> gets appended after the normal environment has been loaded? Then if I > > In principle yes, I see that this is a nice and useful feature. > > Just the notation of "append" seems wrong to me. We already have > "env import" which can import additional / new environmane settings > in different formats. What I think should be done is extending this > by support for a new format (you may call it driver if you want) > to import environment settings from a DTB. > >> set CONFIG_ENV_IS_NOWHERE, I'd get what I have now. The only problem > > Can't you see that this is not logical? If the environment is > nowhere, then how can you add something to it? That's silly. Even with CONFIG_ENV_IS_NOWHERE, "env set" still works just fine. So of course one can add something to the (runtime) environment, even if the source of the original runtime environment happened to be default_environment[] and those changes cannot be persisted. >>> I wonder if we should have a way to load the (whole) environment from DT? >> >> That will be my second choice, i.e. adding a "CONFIG_ENV_IS_IN_DTB" >> which obviously doesn't support saving, but has the advantages I'm after >> here of using one U-Boot binary with slightly different environments. > > I see no difference here. "env import" into an empty environment > does just that. The problem is, by the time it's possible to do an "env import" (no sooner than $preboot is executed I assume?) or any other shell command, U-Boot may already have read a bunch variables affecting its execution from the environment. So I think that for my _current_ use cases, doing it via a preboot command may be enough - from reading the code, it does seem that e.g. bootretry is only read after the preboot command has run. I'll see if I can come up with something else. Rasmus
Dear Rasmus, In message <2edc1fb5-e723-fbd6-56da-bc0dea282343@prevas.dk> you wrote: > > >> set CONFIG_ENV_IS_NOWHERE, I'd get what I have now. The only problem > > > > Can't you see that this is not logical? If the environment is > > nowhere, then how can you add something to it? > > That's silly. Even with CONFIG_ENV_IS_NOWHERE, "env set" still works > just fine. So of course one can add something to the (runtime) I'm sorry, but it seems you lack basic understanding of the design of the environment system in U-Boot. I have explained this repeatedly in the last few weeks, so here once more: CONFIG_ENV_IS_* defines, where the persistent storage for the environment is located, that is where it is initialized from at reset, and where it is saved to when you use the "env save" command. Of course ""env set" still works perfectly fine on the in-memory copy of the environment, as this has nothing to do with whether you can save this data to some persistent storage or not. > environment, even if the source of the original runtime environment > happened to be default_environment[] and those changes cannot be persisted. And this is where the mis-use starts. default_environment[] has never been intended to be used as regular data to initialize the in-memory copy of the environment from. It was there to be used in emergency cases only, like then there were I/O errors on the storage device or the saved copy of the environment had been corrupted, so the checksum would not macth. > > I see no difference here. "env import" into an empty environment > > does just that. > > The problem is, by the time it's possible to do an "env import" (no > sooner than $preboot is executed I assume?) or any other shell command, > U-Boot may already have read a bunch variables affecting its execution > from the environment. You can call the C function equivalent of the "env import" shell command right ater the initialization of the environment - i. e. at lease as early as any of your code can be used. > So I think that for my _current_ use cases, doing it via a preboot > command may be enough - from reading the code, it does seem that e.g. > bootretry is only read after the preboot command has run. I was not talking about implementing it by scripts only. I just wanted to point to the existing functionality - whether you call this from th shell or from C code is your choice. Best regards, Wolfgang Denk
Hi Wolfgang, On Fri, 20 Nov 2020 at 04:28, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon, > > In message <CAPnjgZ1C3TQWkUuNQc0BHOGE957s0K-nxFmW3PVO_ObOcKwv+Q@mail.gmail.com> you wrote: > > > > Some years ago I did a series to allow the environment to come from a > > text file, thus avoiding the \0 stuff. > > "env import -t" does that, you know? Not to derail the discussion, but I mean as a way of specifying the default environment. > > > Now binman has a 'u-boot-env' > > entry type, allowing creating an environment from a text file, with > > suitable checksumming. > > "env import -b" ... For the binman case, this is an environment area which is loaded by U-Boot on boot, so does not need any such command. > > > There is some advantage to having a default environment compiled into > > U-Boot that covers everything needed to boot. For one, the environment > > can be clobbered from userspace, which would otherwise render the > > device unbootable. For another, it is more secure to avoid loading > > unsigned data (the environment) from flash. Generally, for a secure > > boot, one would need to avoid loading the environment, at least > > without a lot of careful filtering. > > One idea behind my rewrite of the environment handling (when I added > hast table support) was that there should be more than one way to > initialize the environment. Until then, we always had exactly one > fixed location for the environment, probaly with a redundant copy. > > The code we have now actuially allows for a much greater > flexibility. You can initialize the environment from a selection of > copies, and (now, with proper driver support) also from several > devices. If doen correctly, we could implement things like > "profiles", where for example each user (or use case) can select his > specific profile, initialize the environment from that, and save > change to that. Ths could - for example - be used to switch between > "development" and "production" modes. A "reset to factory defaults" > would then just be an import from the (read-only) factory-defaults > copy. etc. > > Importing from a DT is just a logical extension as it is considered > just another storage device / driver. [In a Unix environment, all > these would just be "files".] > > It's all there. We just have to use it. OK I'll let you figure this out with Rasmus. Regards, Simon
Dear Simon, In message <CAPnjgZ1mnMwoTZJSMGEnRpcbc7Bs4GhzBmd=D5y5-mrnb+gpnw@mail.gmail.com> you wrote: > > > "env import -t" does that, you know? > > Not to derail the discussion, but I mean as a way of specifying the > default environment. That was a build-time option, then. Sorry, I now remeber. > OK I'll let you figure this out with Rasmus. Hm... Best regards, Wolfgang Denk
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 7fce723800..eda8b3b9d2 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -703,6 +703,39 @@ char *from_env(const char *envvar) return ret; } +static int env_get_f_fdt(const char *name, char *buf, unsigned len) +{ + const char *env; + int plen, nlen, n; + + if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT)) + return 0; + + env = fdtdec_get_config_property(gd->fdt_blob, "default-environment", + &plen); + if (!env) + return 0; + + nlen = strlen(name); + while (plen > nlen) { + if (memcmp(name, env, nlen) == 0 && env[nlen] == '=') { + /* Found. Copy value. */ + n = strlcpy(buf, &env[nlen + 1], len); + if (n < len) + return n; + + printf("env_buf [%u bytes] too small for value of \"%s\"\n", + len, name); + return len; + } + /* Skip this key=val pair. */ + n = strlen(env) + 1; + plen -= n; + env += n; + } + return 0; +} + /* * Look up variable from environment for restricted C runtime env. */ @@ -710,6 +743,10 @@ int env_get_f(const char *name, char *buf, unsigned len) { int i, nxt, c; + i = env_get_f_fdt(name, buf, len); + if (i) + return i; + for (i = 0; env_get_char(i) != '\0'; i = nxt + 1) { int val, n; diff --git a/env/Kconfig b/env/Kconfig index 67ce93061b..66bbac42c7 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -646,6 +646,27 @@ config DELAY_ENVIRONMENT later by U-Boot code. With CONFIG_OF_CONTROL this is instead controlled by the value of /config/load-environment. +config ENV_AMEND_DEFAULT_FROM_FDT + bool "Amend default environment by /config/default-environment property" + depends on OF_CONTROL + help + The default environment built into the U-Boot binary is a + static array defined from various CONFIG_ options, or via + CONFIG_DEFAULT_ENV_FILE. Selecting this option means that + whenever that default environment is used (either for + populating the initial environment, or for resetting + specific variables to their default value), the device tree + property /config/default-environment is also consulted, and + values found there have precedence over those in the static + array. That property should be a series of "key=value" + pairs, e.g. + + /config { + default-environment = "ipaddr=1.2.3.4", + "bootcmd=tftp $loadaddr foo.itb; bootm $loadaddr", + "bootdelay=5"; + } + config ENV_APPEND bool "Always append the environment with new data" default n diff --git a/env/common.c b/env/common.c index 7363da849b..8d0e45fde6 100644 --- a/env/common.c +++ b/env/common.c @@ -63,6 +63,22 @@ char *env_get_default(const char *name) return ret_val; } +static void env_amend_default_from_fdt(int flags, int nvars, char *const vars[]) +{ + const void *val; + int len; + + if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT)) + return; + + val = fdtdec_get_config_property(gd->fdt_blob, "default-environment", + &len); + if (!val) + return; + + himport_r(&env_htab, val, len, '\0', flags, 0, nvars, vars); +} + void env_set_default(const char *s, int flags) { if (sizeof(default_environment) > ENV_SIZE) { @@ -88,6 +104,8 @@ void env_set_default(const char *s, int flags) pr_err("## Error: Environment import failed: errno = %d\n", errno); + env_amend_default_from_fdt(flags | H_NOCLEAR, 0, NULL); + gd->flags |= GD_FLG_ENV_READY; gd->flags |= GD_FLG_ENV_DEFAULT; } @@ -104,6 +122,7 @@ void env_set_default_vars(int nvars, char * const vars[], int flags) himport_r(&env_htab, (const char *)default_environment, sizeof(default_environment), '\0', flags, 0, nvars, vars); + env_amend_default_from_fdt(flags, nvars, vars); } /*
It can be useful to use the same U-Boot binary for multiple purposes, say the normal one, one for developers that allow breaking into the U-Boot shell, and one for use during bootstrapping which runs a special-purpose bootcmd. To that end, allow the control dtb to contain a /config/default-enviroment property, whose value will be used to amend the default environment baked into the U-Boot binary itself. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- cmd/nvedit.c | 37 +++++++++++++++++++++++++++++++++++++ env/Kconfig | 21 +++++++++++++++++++++ env/common.c | 19 +++++++++++++++++++ 3 files changed, 77 insertions(+)