| Message ID | 20201019135602.3943835-12-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | bootm: Support substitions in bootargs and add tests | expand |
On 19/10/2020 15.56, Simon Glass wrote: > In some cases it is necessary to pass parameters to Linux so that it will > boot correctly. For example, the rootdev parameter is often used to > specify the root device. However the root device may change depending on > whence U-Boot loads the kernel. At present it is necessary to build up > the command line by adding device strings to it one by one. > > It is often more convenient to provide a template for bootargs, with > U-Boot doing the substitution from other environment variables. > > Add a way to substitute strings in the bootargs variable. This allows > things like "rootdev=%U" to be used in bootargs, with the %U substitution > providing the UUID of the root device. > > For example, to substitute the GUID of the kernel partition: > > setenv bootargs "console=/dev/ttyS0 rootdev=%U/PARTNROFF=1 kern_guid=%U" > part uuid mmc 2:2 uuid > setenv bootargs_U $uuid > bootm > > This is particularly useful when the command line from another place. For > example, Chrome OS stores the command line next to the kernel itself. It > depends on the kernel version being used as well as the hardware features, > so it is extremely difficult to devise a U-Boot script that works on all > boards and kernel versions. With this feature, the command line can be > read from disk and used directly, with a few substitutions set up. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > README | 16 +++++++ > arch/Kconfig | 1 + > common/Kconfig.boot | 20 ++++++++ > common/bootm.c | 72 +++++++++++++++++++++++++++-- > include/bootm.h | 14 ++++-- > test/bootm.c | 110 ++++++++++++++++++++++++++++++++++++++++++-- > 6 files changed, 221 insertions(+), 12 deletions(-) > > diff --git a/README b/README > index 91c5a1a8fa3..263e31ab7f6 100644 > --- a/README > +++ b/README > @@ -3229,6 +3229,22 @@ List of environment variables (most likely not complete): > > bootargs - Boot arguments when booting an RTOS image > > + bootargs_subst - Substitution parameters for bootargs. These are applied after > + the commandline has been built. The format is: > + > + <key>=<value>[!<key>=<value>...] > + > + where > + <key> is a string to replace > + <value> is the value to replace it with (either a simple > + string or an environment variable starting with $ > + > + One use for this is to insert the root-disk UUID into the > + command line where bootargs contains "root=%U" > + > + part uuid mmc 2:2 uuid > + setenv cmdline_repl %U=$uuid cmdline_repl seems to be stale, it should be bootargs_subst. But the whole paragraph seems stale, as you actually implement and test the bootargs_X approach. Anyway, this does seem useful, but I really question the choice of leaving %A in there if bootargs_A does not exist. It's hard if not impossible to create an environment variable whose value is empty, and I can easily imagine it would be useful to allow a %A to expand to nothing. So why not just use the usual semantics of requiring a double %% to put a single % in the output? It's probably quite rare that one would need that anyway. Rasmus
Dear Simon, In message <20201019135602.3943835-12-sjg@chromium.org> you wrote: > In some cases it is necessary to pass parameters to Linux so that it will > boot correctly. For example, the rootdev parameter is often used to > specify the root device. However the root device may change depending on > whence U-Boot loads the kernel. At present it is necessary to build up > the command line by adding device strings to it one by one. > > It is often more convenient to provide a template for bootargs, with > U-Boot doing the substitution from other environment variables. > > Add a way to substitute strings in the bootargs variable. This allows > things like "rootdev=%U" to be used in bootargs, with the %U substitution > providing the UUID of the root device. Argh, no, please don't. You add something unconditionally to common code which very few people need. U-Boot size is growing all the time because of such ... features. This may be acceptable on the systems you have in mind, but I consider this selfish. Why do we have to add yet another non-standard way of substituting variables in a string? Can we not use alreay existing methonds instead? Why do you have to use "%U" in your template instead of for example "${uuid}" ? Best regards, Wolfgang Denk
Am 2020-10-19 16:54, schrieb Wolfgang Denk: > Dear Simon, > > In message <20201019135602.3943835-12-sjg@chromium.org> you wrote: >> In some cases it is necessary to pass parameters to Linux so that it >> will >> boot correctly. For example, the rootdev parameter is often used to >> specify the root device. However the root device may change depending >> on >> whence U-Boot loads the kernel. At present it is necessary to build up >> the command line by adding device strings to it one by one. >> >> It is often more convenient to provide a template for bootargs, with >> U-Boot doing the substitution from other environment variables. >> >> Add a way to substitute strings in the bootargs variable. This allows >> things like "rootdev=%U" to be used in bootargs, with the %U >> substitution >> providing the UUID of the root device. > > Argh, no, please don't. > > You add something unconditionally to common code which very few > people need. U-Boot size is growing all the time because of such > ... features. This may be acceptable on the systems you have in > mind, but I consider this selfish. > > Why do we have to add yet another non-standard way of substituting > variables in a string? Can we not use alreay existing methonds > instead? > > Why do you have to use "%U" in your template instead of for example > "${uuid}" ? I'd second that. Having variables evaluated inside the bootargs would be very valuable IMHO. At the moment we have some cumbersome constructs like set_bootargs="setenv bootargs bla ${var}" -michael
Hi Wolfgang, On Mon, 19 Oct 2020 at 08:55, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon, > > In message <20201019135602.3943835-12-sjg@chromium.org> you wrote: > > In some cases it is necessary to pass parameters to Linux so that it will > > boot correctly. For example, the rootdev parameter is often used to > > specify the root device. However the root device may change depending on > > whence U-Boot loads the kernel. At present it is necessary to build up > > the command line by adding device strings to it one by one. > > > > It is often more convenient to provide a template for bootargs, with > > U-Boot doing the substitution from other environment variables. > > > > Add a way to substitute strings in the bootargs variable. This allows > > things like "rootdev=%U" to be used in bootargs, with the %U substitution > > providing the UUID of the root device. > > Argh, no, please don't. > > You add something unconditionally to common code which very few > people need. U-Boot size is growing all the time because of such > ... features. This may be acceptable on the systems you have in > mind, but I consider this selfish. Did you see the Kconfig option? > > Why do we have to add yet another non-standard way of substituting > variables in a string? Can we not use alreay existing methonds > instead? What sort of methods? > > Why do you have to use "%U" in your template instead of for example > "${uuid}" ? This is what Chrome OS uses, so it is easier this way, Otherwise I have to replace %U with ${uuid} first. Which code can I use to parse the ${var} thing? Regards, Simon
Hi Rasmus, On Mon, 19 Oct 2020 at 08:43, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 19/10/2020 15.56, Simon Glass wrote: > > In some cases it is necessary to pass parameters to Linux so that it will > > boot correctly. For example, the rootdev parameter is often used to > > specify the root device. However the root device may change depending on > > whence U-Boot loads the kernel. At present it is necessary to build up > > the command line by adding device strings to it one by one. > > > > It is often more convenient to provide a template for bootargs, with > > U-Boot doing the substitution from other environment variables. > > > > Add a way to substitute strings in the bootargs variable. This allows > > things like "rootdev=%U" to be used in bootargs, with the %U substitution > > providing the UUID of the root device. > > > > For example, to substitute the GUID of the kernel partition: > > > > setenv bootargs "console=/dev/ttyS0 rootdev=%U/PARTNROFF=1 kern_guid=%U" > > part uuid mmc 2:2 uuid > > setenv bootargs_U $uuid > > bootm > > > > This is particularly useful when the command line from another place. For > > example, Chrome OS stores the command line next to the kernel itself. It > > depends on the kernel version being used as well as the hardware features, > > so it is extremely difficult to devise a U-Boot script that works on all > > boards and kernel versions. With this feature, the command line can be > > read from disk and used directly, with a few substitutions set up. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > README | 16 +++++++ > > arch/Kconfig | 1 + > > common/Kconfig.boot | 20 ++++++++ > > common/bootm.c | 72 +++++++++++++++++++++++++++-- > > include/bootm.h | 14 ++++-- > > test/bootm.c | 110 ++++++++++++++++++++++++++++++++++++++++++-- > > 6 files changed, 221 insertions(+), 12 deletions(-) > > > > diff --git a/README b/README > > index 91c5a1a8fa3..263e31ab7f6 100644 > > --- a/README > > +++ b/README > > @@ -3229,6 +3229,22 @@ List of environment variables (most likely not complete): > > > > bootargs - Boot arguments when booting an RTOS image > > > > + bootargs_subst - Substitution parameters for bootargs. These are applied after > > + the commandline has been built. The format is: > > + > > + <key>=<value>[!<key>=<value>...] > > + > > + where > > + <key> is a string to replace > > + <value> is the value to replace it with (either a simple > > + string or an environment variable starting with $ > > + > > + One use for this is to insert the root-disk UUID into the > > + command line where bootargs contains "root=%U" > > + > > + part uuid mmc 2:2 uuid > > + setenv cmdline_repl %U=$uuid > > cmdline_repl seems to be stale, it should be bootargs_subst. But the > whole paragraph seems stale, as you actually implement and test the > bootargs_X approach. Yes, thanks, will fix. > > Anyway, this does seem useful, but I really question the choice of > leaving %A in there if bootargs_A does not exist. It's hard if not > impossible to create an environment variable whose value is empty, and I > can easily imagine it would be useful to allow a %A to expand to > nothing. So why not just use the usual semantics of requiring a double > %% to put a single % in the output? It's probably quite rare that one > would need that anyway. I did wonder about the empty env var thing. IMO it would be nice to support empty variables, so we can distinguish between an empty one and a missing one. My concern with removing the var is if people have % in the string. This way I don't have to worry about quoting, etc. See Wolgang's email in this thread too. Regards, Simon
Hi Michael, On Mon, 19 Oct 2020 at 09:47, Michael Walle <michael@walle.cc> wrote: > > Am 2020-10-19 16:54, schrieb Wolfgang Denk: > > Dear Simon, > > > > In message <20201019135602.3943835-12-sjg@chromium.org> you wrote: > >> In some cases it is necessary to pass parameters to Linux so that it > >> will > >> boot correctly. For example, the rootdev parameter is often used to > >> specify the root device. However the root device may change depending > >> on > >> whence U-Boot loads the kernel. At present it is necessary to build up > >> the command line by adding device strings to it one by one. > >> > >> It is often more convenient to provide a template for bootargs, with > >> U-Boot doing the substitution from other environment variables. > >> > >> Add a way to substitute strings in the bootargs variable. This allows > >> things like "rootdev=%U" to be used in bootargs, with the %U > >> substitution > >> providing the UUID of the root device. > > > > Argh, no, please don't. > > > > You add something unconditionally to common code which very few > > people need. U-Boot size is growing all the time because of such > > ... features. This may be acceptable on the systems you have in > > mind, but I consider this selfish. > > > > Why do we have to add yet another non-standard way of substituting > > variables in a string? Can we not use alreay existing methonds > > instead? > > > > Why do you have to use "%U" in your template instead of for example > > "${uuid}" ? > > I'd second that. Having variables evaluated inside the bootargs would > be very valuable IMHO. > > At the moment we have some cumbersome constructs like > set_bootargs="setenv bootargs bla ${var}" Yes it is a real pain. The substitution happens on first parse two, so you have to put these commands in separate variables if you are building things up. Another thing to mention is that years ago I sent a series to help with that last bit, i.e. make variables evaluate recursively. I'm trying to remember what happened to it. Regards, Simon
Dear Simon, In message <CAPnjgZ3sg693VXsEpvjnvkuUzfF612cxWvpWuvx_XiVNgQH4Jg@mail.gmail.com> you wrote: > > > You add something unconditionally to common code which very few > > people need. U-Boot size is growing all the time because of such > > ... features. This may be acceptable on the systems you have in > > mind, but I consider this selfish. > > Did you see the Kconfig option? So you claim the size does not grow with the feature not selected? > > Why do we have to add yet another non-standard way of substituting > > variables in a string? Can we not use alreay existing methonds > > instead? > > What sort of methods? Variable substitution? > > Why do you have to use "%U" in your template instead of for example > > "${uuid}" ? > > This is what Chrome OS uses, so it is easier this way, Otherwise I > have to replace %U with ${uuid} first. That's what I meant when I wrote "selfish" ;-) > Which code can I use to parse the ${var} thing? setenv and run ? Best regards, Wolfgang Denk
Dear Simon, In message <CAPnjgZ3gkCO6_DdQh3XJrsf9SdVa0v3pGuwejGBoDHPEijyRkg@mail.gmail.com> you wrote: > > I did wonder about the empty env var thing. IMO it would be nice to > support empty variables, so we can distinguish between an empty one > and a missing one. What exactly is the use case for this that justifies the additional code needed to implement this feature? Please keep in mind that this is a boot loader, and we are not attending a contest for feature-completeness. [Non-random signature this time.] Best regards, Wolfgang Denk
Dear Simon, In message <CAPnjgZ2rAQZN_bdQ5C-9cB4u8F8PrhZkBWYx0zWxj_yEBrV2qw@mail.gmail.com> you wrote: > > > At the moment we have some cumbersome constructs like > > set_bootargs="setenv bootargs bla ${var}" > > Yes it is a real pain. The substitution happens on first parse two, so > you have to put these commands in separate variables if you are > building things up. Come on, is it really that big a problem? You define all your needed settings (foo, bar, baz, and maybe uuid), and then you run a single command setenv bootargs "${foo} ${bar} %{baz} ${uuid}" ? Yes, it takes one additional step, but it's simple and does not need extra code. [And if someone bothered to update hush to a recent version, and while doing so revisited the adaptions needed for U-Boot, we could also do much better. IIRC, things like command substitution were omitted then because of the code size it would have required; given today's resources this might be an optional feature for many.] Best regards, Wolfgang Denk
Hi Wolfgang, On Tue, 20 Oct 2020 at 07:17, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon, > > In message <CAPnjgZ3sg693VXsEpvjnvkuUzfF612cxWvpWuvx_XiVNgQH4Jg@mail.gmail.com> you wrote: > > > > > You add something unconditionally to common code which very few > > > people need. U-Boot size is growing all the time because of such > > > ... features. This may be acceptable on the systems you have in > > > mind, but I consider this selfish. > > > > Did you see the Kconfig option? > > So you claim the size does not grow with the feature not selected? That's what I see, except that there is some size growth due to the refactoring / adding tests. > > > > Why do we have to add yet another non-standard way of substituting > > > variables in a string? Can we not use alreay existing methonds > > > instead? > > > > What sort of methods? > > Variable substitution? > > > > Why do you have to use "%U" in your template instead of for example > > > "${uuid}" ? > > > > This is what Chrome OS uses, so it is easier this way, Otherwise I > > have to replace %U with ${uuid} first. > > That's what I meant when I wrote "selfish" ;-) > > > Which code can I use to parse the ${var} thing? > > setenv and run ? Will respond on other emails. Regards, SImon
Hi Wolfgang, On Tue, 20 Oct 2020 at 07:26, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon, > > In message <CAPnjgZ2rAQZN_bdQ5C-9cB4u8F8PrhZkBWYx0zWxj_yEBrV2qw@mail.gmail.com> you wrote: > > > > > At the moment we have some cumbersome constructs like > > > set_bootargs="setenv bootargs bla ${var}" > > > > Yes it is a real pain. The substitution happens on first parse two, so > > you have to put these commands in separate variables if you are > > building things up. > > Come on, is it really that big a problem? > > You define all your needed settings (foo, bar, baz, and maybe uuid), > and then you run a single command > > setenv bootargs "${foo} ${bar} %{baz} ${uuid}" > > ? > > Yes, it takes one additional step, but it's simple and does not need > extra code. It is actually not simple, for three reasons: 1. With zboot the args come from the kernel setup.bin and must be modified 2. With Chrome OS the args come from the kernel partition and must be augmented / substituted 3. The above command cannot be in the same env var as anything else, since substitution breaks in that case So you end up with a really complicated mess of environment variables and scripts that is barely manageable. I want it to be simple. See here for example (this only deals with 3 above, not 1 and 2, which would still need custom code without my series) https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/refs/heads/chromeos-v2013.06/include/configs/chromeos.h#204 > > [And if someone bothered to update hush to a recent version, and > while doing so revisited the adaptions needed for U-Boot, we could > also do much better. IIRC, things like command substitution were > omitted then because of the code size it would have required; given > today's resources this might be an optional feature for many.] Yes I agree, so long as the code size increase is not horrific. +Tom Rini what do you think? Regards, Simon
Dear Simon, In message <CAPnjgZ1SfwRa_LXh0zu3Oug=GLEUQx5MdCCvqW90-FV5hvX9uw@mail.gmail.com> you wrote: > > > Yes, it takes one additional step, but it's simple and does not need > > extra code. > > It is actually not simple, for three reasons: > > 1. With zboot the args come from the kernel setup.bin and must be modified Don't use zboot, then. In my opinion, zboot is crap and should never have been added, but there were so many requests for it. Nevertheless, the use of crippleware is a sin that carries with it its own punishment. Don't do it, then. Or, if you feel there is some value in zboot that should be conserved, fix it to work like the rest of U-Boot. > 2. With Chrome OS the args come from the kernel partition and must be > augmented / substituted OK. But then why can we not still use the standard variable substitution mechanism we already have? > 3. The above command cannot be in the same env var as anything else, > since substitution breaks in that case Sorry, I don't understand what you mean heare. What is "the same env var" versus "anything else"? Maybe you can give a specific example? > So you end up with a really complicated mess of environment variables > and scripts that is barely manageable. I want it to be simple. Again, I can't follow you. Why must there be a mess? > See here for example (this only deals with 3 above, not 1 and 2, which > would still need custom code without my series) > > https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/refs/heads/chromeos-v2013.06/include/configs/chromeos.h#204 Sorry - all I see there is some complex settings if these make sense I can't tell) - but how would things be better if you could - for example - use "%U" instead of "${uuid}" as you suggested? Also, is your approach not necessarily limited? You can now think of a handful of variables you may want to pass, say root device, root partition, uuid, maybe MAC address etc. But the next user will need kernel args that you did not think of - so how do we proceed? Add all features anybody needs to that new code? That certainly does not scale. Or mix "%FOO" and "${foo}" style arguments? That's even worse. I really fail to see the benefits, I see only ugliness and problems. Best regards, Wolfgang Denk
Hi Wolfgang, On Wed, 21 Oct 2020 at 01:16, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon, > > In message <CAPnjgZ1SfwRa_LXh0zu3Oug=GLEUQx5MdCCvqW90-FV5hvX9uw@mail.gmail.com> you wrote: > > > > > Yes, it takes one additional step, but it's simple and does not need > > > extra code. > > > > It is actually not simple, for three reasons: > > > > 1. With zboot the args come from the kernel setup.bin and must be modified > > Don't use zboot, then. In my opinion, zboot is crap and should > never have been added, but there were so many requests for it. > Nevertheless, the use of crippleware is a sin that carries with it > its own punishment. > > Don't do it, then. > > Or, if you feel there is some value in zboot that should be > conserved, fix it to work like the rest of U-Boot. Well my series does that to a large extent. It is much more like bootm now, in that it is properly split into stages. I think it would be possible to combine parts of it into bootm as a future step, although it is non-trivial, and I think we should build out more tests first. But zboot does make use of an existing command line and does all the weird x86 processing, so I don't know how we could get rid of it. > > > 2. With Chrome OS the args come from the kernel partition and must be > > augmented / substituted > > OK. But then why can we not still use the standard variable > substitution mechanism we already have? I don't think the actual mechanism is a big deal. We could do a string replace (does U-Boot support that?) of %U with ${uuid} for example, to make it work for my case. Or we could go with an idea I previously rejected, to just provide a simple string replace feature, with any special characters in the search string. For example: setenv bootargs_repl "%U=${uuid} %P=${partid}" > > > 3. The above command cannot be in the same env var as anything else, > > since substitution breaks in that case > > Sorry, I don't understand what you mean heare. What is "the same > env var" versus "anything else"? Maybe you can give a specific example? Did you see the 'run regen_scripts' script? At present I can do setenv uuid blah; setenv partid blah; bootm but with your scheme I need the 'run regen_script' to set the variables correctly, which is a real pain as my example shows. > > > > So you end up with a really complicated mess of environment variables > > and scripts that is barely manageable. I want it to be simple. > > Again, I can't follow you. Why must there be a mess? > > > See here for example (this only deals with 3 above, not 1 and 2, which > > would still need custom code without my series) > > > > https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/refs/heads/chromeos-v2013.06/include/configs/chromeos.h#204 > > Sorry - all I see there is some complex settings if these make sense > I can't tell) - but how would things be better if you could - for > example - use "%U" instead of "${uuid}" as you suggested? My point here is not about %U, it is about the pain of multiple stages to get the variables right. WIth bootargs substitution we can just set the variables and boot. > > Also, is your approach not necessarily limited? You can now think of > a handful of variables you may want to pass, say root device, root > partition, uuid, maybe MAC address etc. But the next user will need > kernel args that you did not think of - so how do we proceed? Add > all features anybody needs to that new code? That certainly does > not scale. Or mix "%FOO" and "${foo}" style arguments? That's even > worse. I really fail to see the benefits, I see only ugliness and > problems. These scripts are board-specific, so each board can do what it likes here. But the nice thing is not having to manually build up the bootargs. Can we step past the %U business? I think we can use the ${var} stuff with some substitution. Regards, Simon
Dear Simon, In message <CAPnjgZ2Vn3oWAHy71smw-+fRo28HUA0K32DP6FPoGtjtK3XoFQ@mail.gmail.com> you wrote: > > Well my series does that to a large extent. It is much more like bootm > now, in that it is properly split into stages. I think it would be > possible to combine parts of it into bootm as a future step, although > it is non-trivial, and I think we should build out more tests first. Hm... I fear that this is rather cementing the zboot support so we never get rid of it... > > But zboot does make use of an existing command line and does all the > weird x86 processing, so I don't know how we could get rid of it. How about a command that takes this existing command line and "imports" it into a standard bootargs variable? > I don't think the actual mechanism is a big deal. We could do a string > replace (does U-Boot support that?) of %U with ${uuid} for example, to > make it work for my case. Yes, we can. => env print commandline bootargs ## Error: "commandline" not defined ## Error: "bootargs" not defined => env set commandline 'This is a %A test for %U replacement.' => setexpr bootargs gsub %A boring "${commandline}" bootargs=This is a boring test for %U replacement. => setexpr bootargs gsub %U '${uuid}' bootargs=This is a boring test for ${uuid} replacement. => env print bootargs bootargs=This is a boring test for ${uuid} replacement. > Or we could go with an idea I previously rejected, to just provide a > simple string replace feature, with any special characters in the > search string. For example: But we have all this already, even with regular expressions and (simple) backreferences. > > Sorry, I don't understand what you mean heare. What is "the same > > env var" versus "anything else"? Maybe you can give a specific example? > > Did you see the 'run regen_scripts' script? > > At present I can do > > setenv uuid blah; setenv partid blah; bootm > > but with your scheme I need the 'run regen_script' to set the > variables correctly, which is a real pain as my example shows. Just insert a "run" there, and you are done with it. > My point here is not about %U, it is about the pain of multiple stages > to get the variables right. WIth bootargs substitution we can just set > the variables and boot. I think this is not so much a bootargs question (or at least it should not be it). bootargs is just another environment variable without any specific properties except that it's content is being passed to the kerenl, so you want to have the needed variable substituion doen before that. It was an intentional decison not to do this automagically, based on the thinking that accorsing to UNNIX philosophy it is not each individual command which implements things like wildcard or variable substitution, but rather this is something the shell does for you, and the commands are dumb in this respect. This works reasonably well, except that we don't pass bootargs as argument to the bootm command - rather it is done internally by default, this lacking the variable substituion. You may call this a design bug, and I will accept the blame for it. To me the obvious fix would be to correct this behavious such that we extend bootm to accept the bootargs argument as a command line parameter, thus enabling variable substitution there, too. As far as I'm concenred, I don't think this is necessary as all it takes to work around this is a single call of a "run" command which can do exactly that, too. But I agree, this is just a workaround, and it would be more consistent to pass bootargs as argument directly. And this is also the direction I see how the zboot stuff should be fixed: import the Linux command line into the environment, modify it as needed (if necessary using regular expression matching / string substituion to fix it up to use common U-Boot variable names), and then pass it like the rest of the world to the kernel. > Can we step past the %U business? I think we can use the ${var} stuff > with some substitution. Don't we have that already? What exactly is it you miss? Best regards, Wolfgang Denk
Hi Wolfgang, On Thu, 22 Oct 2020 at 06:32, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon, > > In message <CAPnjgZ2Vn3oWAHy71smw-+fRo28HUA0K32DP6FPoGtjtK3XoFQ@mail.gmail.com> you wrote: > > > > Well my series does that to a large extent. It is much more like bootm > > now, in that it is properly split into stages. I think it would be > > possible to combine parts of it into bootm as a future step, although > > it is non-trivial, and I think we should build out more tests first. > > Hm... I fear that this is rather cementing the zboot support so we > never get rid of it... > > > > But zboot does make use of an existing command line and does all the > > weird x86 processing, so I don't know how we could get rid of it. > > How about a command that takes this existing command line and > "imports" it into a standard bootargs variable? > > > I don't think the actual mechanism is a big deal. We could do a string > > replace (does U-Boot support that?) of %U with ${uuid} for example, to > > make it work for my case. > > Yes, we can. > > => env print commandline bootargs > ## Error: "commandline" not defined > ## Error: "bootargs" not defined > => env set commandline 'This is a %A test for %U replacement.' > => setexpr bootargs gsub %A boring "${commandline}" > bootargs=This is a boring test for %U replacement. > => setexpr bootargs gsub %U '${uuid}' > bootargs=This is a boring test for ${uuid} replacement. > => env print bootargs > bootargs=This is a boring test for ${uuid} replacement. > > > Or we could go with an idea I previously rejected, to just provide a > > simple string replace feature, with any special characters in the > > search string. For example: > > But we have all this already, even with regular expressions and > (simple) backreferences. > > > > > Sorry, I don't understand what you mean heare. What is "the same > > > env var" versus "anything else"? Maybe you can give a specific example? > > > > Did you see the 'run regen_scripts' script? > > > > At present I can do > > > > setenv uuid blah; setenv partid blah; bootm > > > > but with your scheme I need the 'run regen_script' to set the > > variables correctly, which is a real pain as my example shows. > > Just insert a "run" there, and you are done with it. > > > My point here is not about %U, it is about the pain of multiple stages > > to get the variables right. WIth bootargs substitution we can just set > > the variables and boot. > > I think this is not so much a bootargs question (or at least it > should not be it). bootargs is just another environment variable > without any specific properties except that it's content is being > passed to the kerenl, so you want to have the needed variable > substituion doen before that. > > It was an intentional decison not to do this automagically, based on > the thinking that accorsing to UNNIX philosophy it is not each > individual command which implements things like wildcard or variable > substitution, but rather this is something the shell does for you, > and the commands are dumb in this respect. This works reasonably > well, except that we don't pass bootargs as argument to the bootm > command - rather it is done internally by default, this lacking the > variable substituion. > > You may call this a design bug, and I will accept the blame for it. > > To me the obvious fix would be to correct this behavious such that > we extend bootm to accept the bootargs argument as a command line > parameter, thus enabling variable substitution there, too. > > As far as I'm concenred, I don't think this is necessary as all it > takes to work around this is a single call of a "run" command which > can do exactly that, too. > > But I agree, this is just a workaround, and it would be more > consistent to pass bootargs as argument directly. > > > And this is also the direction I see how the zboot stuff should be > fixed: import the Linux command line into the environment, modify > it as needed (if necessary using regular expression matching / > string substituion to fix it up to use common U-Boot variable > names), and then pass it like the rest of the world to the kernel. > > > Can we step past the %U business? I think we can use the ${var} stuff > > with some substitution. > > Don't we have that already? What exactly is it you miss? I have sent a new version of this series that uses ${var}. Regards, Simon
diff --git a/README b/README index 91c5a1a8fa3..263e31ab7f6 100644 --- a/README +++ b/README @@ -3229,6 +3229,22 @@ List of environment variables (most likely not complete): bootargs - Boot arguments when booting an RTOS image + bootargs_subst - Substitution parameters for bootargs. These are applied after + the commandline has been built. The format is: + + <key>=<value>[!<key>=<value>...] + + where + <key> is a string to replace + <value> is the value to replace it with (either a simple + string or an environment variable starting with $ + + One use for this is to insert the root-disk UUID into the + command line where bootargs contains "root=%U" + + part uuid mmc 2:2 uuid + setenv cmdline_repl %U=$uuid + bootfile - Name of the image to load with TFTP bootm_low - Memory range available for image processing in the bootm diff --git a/arch/Kconfig b/arch/Kconfig index 6caf2338bcf..421ea9a9b51 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -141,6 +141,7 @@ config SANDBOX imply CMD_PMC imply CMD_CLONE imply SILENT_CONSOLE + imply BOOTARGS_SUBST config SH bool "SuperH architecture" diff --git a/common/Kconfig.boot b/common/Kconfig.boot index 4191bfb34df..c5668cf2931 100644 --- a/common/Kconfig.boot +++ b/common/Kconfig.boot @@ -848,6 +848,26 @@ config BOOTARGS CONFIG_BOOTARGS goes into the environment value "bootargs". Note that this value will also override the "chosen" node in FDT blob. +config BOOTARGS_SUBST + bool "Support substituting strings in boot arguments" + help + This allows substituting string values in the boot arguments. These + are applied after the commandline has been built. Set the + 'bootargs_subst' envvar to control this. The format is: + + <key>=<value>[!<key>=<value>...] + + where + <key> is a string to replace + <value> is the value to replace it with (either a simple + string or an environment variable starting with $ + + One use for this is to insert the root-disk UUID into the + command line where bootargs contains "root=%U" + + part uuid mmc 2:2 uuid + setenv cmdline_repl %U=$uuid + config USE_BOOTCOMMAND bool "Enable a default value for bootcmd" help diff --git a/common/bootm.c b/common/bootm.c index 5484c2be900..0a89a72a660 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -540,6 +540,68 @@ static int fixup_silent_linux(char *buf, int maxlen) return 0; } +/** + * process_subst() - Handle substitution of %x fields in the environment + * + * For every "%x" found, it is replaced with the value of envvar "bootargs_x" + * where x is a case-sensitive alphanumeric character. If that environment + * variable does not exist, no substitution is made and the %x remains, to + * allow for plain % characters in the string. + * + * @buf: Buffer containing the string to process + * @maxlen: Maximum length of buffer + * @return 0 if OK, -ENOSPC if @maxlen is too small + */ +static int process_subst(char *buf, int maxlen) +{ + const char *in; + char *cmdline, *out; + bool percent; + int size; + + /* Move to end of buffer */ + size = strlen(buf) + 1; + cmdline = buf + maxlen - size; + if (buf + size > cmdline) + return -ENOSPC; + memmove(cmdline, buf, size); + + /* Replace %c with value of envvar 'bootargs_%c' */ + percent = false; + for (in = cmdline, out = buf; *in; in++) { + /* If we catch up with the input string, we're out of space */ + if (out >= in) + return -ENOSPC; + if (percent) { + char var[12]; + const char *val; + + snprintf(var, sizeof(var), "bootargs_%c", *in); + val = env_get(var); + if (val) { + for (; *val; val++) { + if (out >= in) + return -ENOSPC; + *out++ = *val; + } + percent = false; + continue; + } else { + /* No value, put the % back */ + *out++ = '%'; + } + } else if (*in == '%') { + percent = true; + continue; + } + *out++ = *in; + percent = false; + } + *out++ = '\0'; + + return 0; +} + int bootm_process_cmdline(char *buf, int maxlen, int flags) { int ret; @@ -552,6 +614,11 @@ int bootm_process_cmdline(char *buf, int maxlen, int flags) if (ret) return log_msg_ret("silent", ret); } + if (IS_ENABLED(CONFIG_BOOTARGS_SUBST) && (flags & BOOTM_CL_SUBST)) { + ret = process_subst(buf, maxlen); + if (ret) + return log_msg_ret("silent", ret); + } return 0; } @@ -567,7 +634,7 @@ int bootm_process_cmdline_env(int flags) /* First check if any action is needed */ do_silent = IS_ENABLED(CONFIG_SILENT_CONSOLE) && !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY) && (flags & BOOTM_CL_SILENT); - if (!do_silent) + if (!do_silent && !IS_ENABLED(CONFIG_BOOTARGS_SUBST)) return 0; env = env_get("bootargs"); @@ -700,8 +767,7 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc, if (!ret && (states & BOOTM_STATE_OS_BD_T)) ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images); if (!ret && (states & BOOTM_STATE_OS_PREP)) { - ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ? - BOOTM_CL_SILENT : 0); + ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX); if (ret) { printf("Cmdline setup failed (err=%d)\n", ret); ret = CMD_RET_FAILURE; diff --git a/include/bootm.h b/include/bootm.h index 8d95fb2a90a..7f88ec718b8 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -78,8 +78,9 @@ void switch_to_non_secure_mode(void); /* Flags to control bootm_process_cmdline() */ enum bootm_cmdline_t { BOOTM_CL_SILENT = 1 << 0, /* Do silent console processing */ + BOOTM_CL_SUBST = 1 << 1, /* Do substitution */ - BOOTM_CL_ALL = 1, /* All substitutions */ + BOOTM_CL_ALL = 3, /* All substitutions */ }; /** @@ -95,7 +96,10 @@ void board_preboot_os(void); /* * bootm_process_cmdline() - Process fix-ups for the command line * - * This handles: making Linux boot silently if requested ('silent_linux' envvar) + * This handles: + * + * - making Linux boot silently if requested ('silent_linux' envvar) + * - performing substitutions in the command line ('bootargs_subst' envvar) * * @maxlen must provide enough space for the string being processed plus the * resulting string @@ -111,8 +115,10 @@ int bootm_process_cmdline(char *buf, int maxlen, int flags); /** * bootm_process_cmdline_env() - Process fix-ups for the command line * - * Updates the 'bootargs' envvar as required. This handles making Linux boot - * silently if requested ('silent_linux' envvar) + * Updates the 'bootargs' envvar as required. This handles: + * + * - making Linux boot silently if requested ('silent_linux' envvar) + * - performing substitutions in the command line ('bootargs_subst' envvar) * * @flags: Flags to control what happens (see bootm_cmdline_t) * @return 0 if OK, -ENOMEM if out of memory diff --git a/test/bootm.c b/test/bootm.c index d0b29441d65..8a83be9719f 100644 --- a/test/bootm.c +++ b/test/bootm.c @@ -116,22 +116,122 @@ static int bootm_test_silent(struct unit_test_state *uts) } BOOTM_TEST(bootm_test_silent, 0); +/* Test substitution processing */ +static int bootm_test_subst(struct unit_test_state *uts) +{ + char buf[BUF_SIZE]; + + strcpy(buf, "some%Athing"); + ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST)); + ut_asserteq_str("some%Athing", buf); + + /* Replace with shorter string */ + ut_assertok(env_set("bootargs_A", "a")); + strcpy(buf, "some%Athing"); + ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST)); + ut_asserteq_str("someathing", buf); + + /* Replace with same-length string */ + ut_assertok(env_set("bootargs_A", "ab")); + strcpy(buf, "some%Athing"); + ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST)); + ut_asserteq_str("someabthing", buf); + + /* Replace with longer string */ + ut_assertok(env_set("bootargs_A", "abc")); + strcpy(buf, "some%Athing"); + ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST)); + ut_asserteq_str("someabcthing", buf); + + /* Check it is case sensitive */ + strcpy(buf, "some%athing"); + ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST)); + ut_asserteq_str("some%athing", buf); + + /* Check too long - need 12 bytes for each string */ + strcpy(buf, "some%Athing"); + ut_asserteq(-ENOSPC, + bootm_process_cmdline(buf, 12 * 2 - 1, BOOTM_CL_SUBST)); + + /* Check just enough space */ + strcpy(buf, "some%Athing"); + ut_assertok(bootm_process_cmdline(buf, 12 * 2, BOOTM_CL_SUBST)); + ut_asserteq_str("someabcthing", buf); + + /* + * Check the substition string being too long. This results in a string + * of 12 (13 bytes). Allow one more byte margin. + */ + ut_assertok(env_set("bootargs_A", "1234567890")); + strcpy(buf, "a%Ac"); + ut_asserteq(-ENOSPC, + bootm_process_cmdline(buf, 13, BOOTM_CL_SUBST)); + + strcpy(buf, "a%Ac"); + ut_asserteq(0, bootm_process_cmdline(buf, 14, BOOTM_CL_SUBST)); + + /* Check multiple substitutions */ + ut_assertok(env_set("bootargs_A", "abc")); + strcpy(buf, "some%Athing%Belse"); + ut_asserteq(0, bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST)); + ut_asserteq_str("someabcthing%Belse", buf); + + /* Check multiple substitutions */ + ut_assertok(env_set("bootargs_B", "123")); + strcpy(buf, "some%Athing%Belse"); + ut_asserteq(0, bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST)); + ut_asserteq_str("someabcthing123else", buf); + + return 0; +} +BOOTM_TEST(bootm_test_subst, 0); + /* Test silent processing in the bootargs variable */ static int bootm_test_silent_var(struct unit_test_state *uts) { env_set("bootargs", NULL); - env_set("silent_linux", NULL); - ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT)); + ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SUBST)); ut_assertnull(env_get("bootargs")); - env_set("bootargs", CONSOLE_STR); - env_set("silent_linux", "yes"); + ut_assertok(env_set("bootargs", "some%Athing")); + ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SUBST)); + ut_asserteq_str("some%Athing", env_get("bootargs")); + + return 0; +} +BOOTM_TEST(bootm_test_silent_var, 0); + +/* Test substitution processing in the bootargs variable */ +static int bootm_test_subst_var(struct unit_test_state *uts) +{ + env_set("bootargs", NULL); ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT)); ut_asserteq_str("console=", env_get("bootargs")); + ut_assertok(env_set("bootargs", "some%Athing")); + ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT)); + ut_asserteq_str("some%Athing console=", env_get("bootargs")); + return 0; } -BOOTM_TEST(bootm_test_silent_var, 0); +BOOTM_TEST(bootm_test_subst_var, 0); + +/* Test substitution and silent console processing in the bootargs variable */ +static int bootm_test_subst_both(struct unit_test_state *uts) +{ + ut_assertok(env_set("silent_linux", "yes")); + env_set("bootargs", NULL); + ut_assertok(bootm_process_cmdline_env(BOOTM_CL_ALL)); + ut_asserteq_str("console=", env_get("bootargs")); + + ut_assertok(env_set("bootargs", "some%Athing " CONSOLE_STR)); + ut_assertok(env_set("bootargs_A", "1234567890")); + ut_assertok(bootm_process_cmdline_env(BOOTM_CL_ALL)); + ut_asserteq_str("some1234567890thing console=", env_get("bootargs")); + + return 0; +} +BOOTM_TEST(bootm_test_subst_both, 0); int do_ut_bootm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {
In some cases it is necessary to pass parameters to Linux so that it will boot correctly. For example, the rootdev parameter is often used to specify the root device. However the root device may change depending on whence U-Boot loads the kernel. At present it is necessary to build up the command line by adding device strings to it one by one. It is often more convenient to provide a template for bootargs, with U-Boot doing the substitution from other environment variables. Add a way to substitute strings in the bootargs variable. This allows things like "rootdev=%U" to be used in bootargs, with the %U substitution providing the UUID of the root device. For example, to substitute the GUID of the kernel partition: setenv bootargs "console=/dev/ttyS0 rootdev=%U/PARTNROFF=1 kern_guid=%U" part uuid mmc 2:2 uuid setenv bootargs_U $uuid bootm This is particularly useful when the command line from another place. For example, Chrome OS stores the command line next to the kernel itself. It depends on the kernel version being used as well as the hardware features, so it is extremely difficult to devise a U-Boot script that works on all boards and kernel versions. With this feature, the command line can be read from disk and used directly, with a few substitutions set up. Signed-off-by: Simon Glass <sjg@chromium.org> --- README | 16 +++++++ arch/Kconfig | 1 + common/Kconfig.boot | 20 ++++++++ common/bootm.c | 72 +++++++++++++++++++++++++++-- include/bootm.h | 14 ++++-- test/bootm.c | 110 ++++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 221 insertions(+), 12 deletions(-)