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

Simon Glass sjg at chromium.org
Mon Oct 6 22:38:19 CEST 2025 

Hi George,

On Sun, 5 Oct 2025 at 09:41, george chan <gchan9527 at gmail.com> wrote:
>
>
>
> 在 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

This thread is a bit confusing now, but it seems you have written a
new function and that is fine with me, as it won't affect ChromeOS.


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

More information about the U-Boot mailing list