[PATCH v4 3/6] bootm: Append bootargs value when bootmeth_android provide cmdline

[george chan]

Hi Simon and Mattijs,

Thx for feedback. Just in case the discussion drift to other direction, do
let me have a chance to put some background info here.

there are ways for pre-set env variable and its value. Firstly external env
file and loaded by uboot, like, runtime handling; and another type is build
time embed env file into uboot body.

maybe in chromeos case first stage boot loader will pass along bootinfo
struct to uboot and treat as preset defaults.

so in either case uboot can have right to choose the preset from (by
disable external env file, secured/trusted boot?), and then either follow
the bootinfo struct preset value and/or concat and/or overrite with uboot’s
own embedded env value at boot time, and then runtime modify.

This behavior maybe enable or disable by kconfig. but in secured boot
context point of view, env file kind of thing, might have potential design
wise conflict. And this is not within my scope.

in some other use case like simulation of vendor bootloader behavior to
have a build time preset default value, this is within my scope. and thats
why i decided to put those preset into env file and embeded.

let me reply below inline.

在 2025年10月2日週四 00:22，Simon Glass <sjg at chromium.org> 寫道：

> Hi Mattijs, George,
>
> On Tue, 30 Sept 2025 at 01:29, Mattijs Korpershoek
> <mkorpershoek at kernel.org> wrote:
> >
> > Hi Simon,
> >
> > Now that you are back, could you please have a look at ...
> >
> > On Fri, Jul 18, 2025 at 14:03, Mattijs Korpershoek <
> mkorpershoek at kernel.org> wrote:
> >
> > > Hi George,
> > >
> > > On Wed, Jul 16, 2025 at 19:55, george chan <gchan9527 at gmail.com>
> wrote:
> > >
> > >> Hi Mattijs,
> > >>
> > >> Thx for reply.
> > >>
> > >> 在 2025年7月16日週三 17:05，Mattijs Korpershoek <mkorpershoek at kernel.org>
> 寫道：
> > >>
> > >>> Hi George,
> > >>>
> > >>> Thank you for the patch.
> > >>>
> > >> ...
> > >>
> > >>>
> > >>> > -     ret = env_set("bootargs", cmdline);
> > >>> > +     ret = android_image_modify_bootargs_env(cmdline, NULL);
> > >>>
> > >>> I don't think it can be done this way. bootm_boot_start() is used in
> the
> > >>> ChromeOS bootmethod as well (boot/bootmeth_cros.c)
> > >>>
> > >>
> > >> I got your point. I have 3 ideas come out.
> > >>
> > >> (1) The logic purposely empty the env bootargs, either developer
> don't use
> > >> env bootargs or those use cases touched bootargs in some early step.
> And
> > >> then wanna disregard previous u-boot bootmeth operation, and empty
> bootargs
> > >> for that ongoing bootmeth stage...well that's not congruent behavior;
> I
> > >> have a gut feeling this is a bug or band-aid anyway, it closed the
> door for
> > >
> > > Simon, is it *intentional* that override the bootargs variable in
> > > commit daffb0be2c83 ("bootstd: cros: Add ARM support") ?
> > >
> > > Shouldn't we get the bootargs from the environment, and append cmdline
> > > to the existing bootargs instead ?
>
> The way U-Boot operates today, bootargs defines the entire cmdline,
> not just part of it. So if you use 'env set bootargs ..' you are
> changing the entire cmdline passed to Linux, etc.
>

This is runtime change. My test case in fastboot path, it get bootargs
wiped and filling in with something from boot img. That might share same
procedure with other boot method like chromeos do. Wipe env default each
time in fastboot (when fail) might be a secure-wise handling.

And I feel it is a critical entry point logic like a central hub, and
looking good to decide uboot behavior here. By kconfig whatever? Again, out
of my scope.


> With ChromeOS, after 'bootflow read', the cmdline can be edited using
> 'bootflow cmdline set' etc., which also changes bootargs. But in
> general ChromeOS provides the whole Linux cmdline to U-Boot.
>

this is also runtime change. And as a side note, if by doing bootflow
select, should env bootargs gets overrided or appended? This is a bigger
arch-wise question worth at least a public paper. and I might not go into
this depth.


> If you are wanting to append, I suggest writing a new function, or
>

yeah agreed. I do more appreciate and feel it really lower project
management/test cost.

perhaps adding a 'bool append' arg to bootm_boot_start(), if all the
> other code is useful for Android.
>

A more simpler method is to create bootm_boot_start_ex(kernel_cmd_line,
vendor_cmd_line) as
 I proposed to complement with
 bootm_boot_start(cmdline) or even add one layer of abstract, called from
bootm_boot_start(), make vendor_cmd_line param as null. As can preserve the
old logic and behavior.

So build time env preset value, and runtime value override decision can
live in this new function, or elsewhere; but is not within my scope.

So I leave it as a open-end question here. Once you had decided then feel
free to let me know and I can do one extra round patch. Thank again for
feedback.

George


> >
> > ... this question ?
> >
> > We are wondering why bootm_boot_start() overrides the bootargs variable
> > instead of appending cmdline to the existing bootargs instead.
> >
> > >
> > >
> > >> people (potentially abootimg) inject needed boot param between
> bootmeth
> > >> stage. Perhaps, either clean up bootargs before bootmeth load stage,
> or add
> > >> kconfig to check and enable this logic, a better representation.
> > >>
> > >> (2) One more thing, the vendor_boot cmdline is not handle neither in
> > >> original logic nor in url from you provided. So I can say the url one
> is
> > >> not capable for extended with Android boot img version > 2 since
> > >> vendor_boot cmdline not handled.
> > >
> > > What do you mean by the vendor_boot cmdline is not handled?
> > >
> > > When parsing the vendor_boot image, we go through
> > > android_vendor_boot_image_v3_v4_parse_hdr()
> > >
> > > That function copies the vendor_boot cmdline with:
> > >
> > >       data->kcmdline_extra = hdr->cmdline;
> > >
> > > (to the struct andr_image_data).
> > >
> > > Later on, in android_image_get_kernel(), this gets copied to the
> > > bootargs:
> > >
> > >       if (img_data.kcmdline_extra && *img_data.kcmdline_extra) {
> > >               if (*newbootargs) /* If there is something in
> newbootargs, a space is needed */
> > >                       strcat(newbootargs, " ");
> > >               strcat(newbootargs, img_data.kcmdline_extra);
> > >       }
> > >
> > >       env_set("bootargs", newbootargs);
> > >
> > >>
> > >> (3) I don't think it is very much different between your mentioned
> url with
> > >> my patch. There is nothing advanced, but just existing code logic
> reuse and
> > >> that logic handled vendor_cmdline in other path. I prefer code logic
> reuse.
> > >
> > > The difference is that in the patch I've linked is that we only change
> > > Android specific boot behaviour. So less risk to impact ChromeOS.
> > >
> > >>
> > >> And I believe above are not the important thing....
> > >>
> > >>>
> > >>> Changing this would potentially break ChromeOS boot behaviour so I'd
> > >>> prefer to find another solution.
> > >>>
> > >>> I know that TI has a downstream patch that changes bootmeth_android.c
> > >>> instead:
> > >>>
> > >>>
> > >>>
> https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04-next&id=9d802d798ac143e06ffe545606aa657a6c32cc63
> > >>
> > >>
> > >> ...
> > >>
> > >>
> > >>> Would that work for you?
> > >>>
> > >>>
> > >> Well, the one from url also fine with me.
> > >>
> > >> The important thing is here. So as to ease the change with "minimal
> impact"
> > >> gets in. I can add one extra check to maintain original behavior in
> case
> > >> people have concern of breakage. Code example as below:
> > >>
> > >> + If (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_PREPEND_ENV_BOOTARGS))
> > >> +   ret = android_image_modify_bootargs_env(cmdline, NULL);
> > >> + else
> > >>       ret = env_set("bootargs", cmdline);
> > >>
> > >> This logic eliminated the concern, but it also limited those
> potential use
> > >> cases for Android boot header version > 2, unless the newly introduced
> > >> CONFIG_ANDROID_BOOT_IMAGE_PREPEND_ENV_BOOTARGS is defined.
> > >
> > > I'm not sure about why this is better, as from what I understand we
> > > handle vendor_boot cmdline properly already.
> > >
> > > Can you provide me with a test case or some example commands that show
> > > that vendor_boot cmdline is not handled properly?
> > >
> > >>
> > >> Or this one with good extendible.
> > >> + /* Clean up bootargs purposely */
> > >> + if (IS_ENABLED(SOME_FLAG))
> > >> +    env_set("bootargs", "");
> > >> + ret = android_image_modify_bootargs_env(cmdline, NULL);
> > >>
> > >> Either way. I prefer first one and can blindly apply without afford. I
> > >> leave the idea above to people more concern with it. Please let me
> know
> > >> your decision, I can provide one more revision later.
> > >
> > > I'm sorry there is so much back and forth on this series. I hope we can
> > > come to a common agreement and get something merged.
>
> Regards,
> Simon
>