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

Mattijs Korpershoek mkorpershoek at kernel.org
Thu Oct 9 14:46:26 CEST 2025 

Hi George,

On Wed, Oct 08, 2025 at 00:03, george chan <gchan9527 at gmail.com> wrote:

> Hi Simon,
>
> Thx for feedback.
>
>
> 在 2025年10月7日週二 04:38，Simon Glass <sjg at chromium.org> 寫道：
>
>> 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
>>
>
> I am sorry but I had to prove my patch is necessary and correctnessly and
> also limit the scope of the patch. Thanks for your understand.
>
> new function and that is fine with me, as it won't affect ChromeOS.
>>
>
> There is no new function at all. I am sorry to put more info here again to
> let related code to be clear for every audience here.
>
> As you may notice function bootm_modify_bootargs_env() is a renamed and
> move to bootm.c which is split/extracted from existing function
> android_image_get_kernel()
>
> And bootm_boot_start_ex() is split from
> bootm_boot_start() with your suggested enable param.
>
> I am afraid if you or other reviewer might wonder why move to bootm.c and
> here is the reason.
>
> Supposed with below 2 code paths:
>
> Boot->bootflow->bootmeth_android->bootm
> Boot->fastboot->do_bootm->bootm
>
> I believe android_image_get_kernel is living around within bootmeth_android
> stage so first two patch has taken care of. But fastboot path is not. that
> might be the reason above two path need separate special handling for.
>
> So as to code reuse with fastboot path then those code had to move to bootm
> layer.

Initially, I did not understand that you were wanting to support both
"fastboot boot" and booting via bootmeth_android.

Things make more sense now. Please send a new series so that Simon and I
can review. Thanks again for your patience!

>
> And my understanding each stage is isolated, and depends on env as a kind
> of api to link up each stage. Then I believe the patch is now a clean
> solution. Anyway welcome to jot a mail if there are suggestions, if any
> doubt, please jot a mail too.
>
> Thanks,
> George
>
>
>>
>> >
>> >
>> >>
>> >> 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