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

Sun Oct 5 17:41:23 CEST 2025

在 2025年10月5日週日 00:35，george chan <gchan9527 at gmail.com> 寫道：

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

Tr/Dr I wrote a proof of concept code here:

https://github.com/99degree/u-boot/compare/before_tag...reboot-mode?expand=1

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