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