Hi George,
On Sun, 5 Oct 2025 at 09:41, george chan <[email protected]> wrote:
>
>
>
> 在 2025年10月5日週日 00:35，george chan <[email protected]> 寫道：
>>
>> 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 <[email protected]> 寫道：
>>>
>>> Hi Mattijs, George,
>>>
>>> On Tue, 30 Sept 2025 at 01:29, Mattijs Korpershoek
>>> <[email protected]> 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
>>> > <[email protected]> wrote:
>>> >
>>> > > Hi George,
>>> > >
>>> > > On Wed, Jul 16, 2025 at 19:55, george chan <[email protected]> wrote:
>>> > >
>>> > >> Hi Mattijs,
>>> > >>
>>> > >> Thx for reply.
>>> > >>
>>> > >> 在 2025年7月16日週三 17:05，Mattijs Korpershoek <[email protected]> 寫道：
>>> > >>
>>> > >>> 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
