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

[Simon Glass]

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.

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.

If you are wanting to append, I suggest writing a new function, or
perhaps adding a 'bool append' arg to bootm_boot_start(), if all the
other code is useful for Android.

>
> ... 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