Hi Simon and Mattijs,
在 2025年10月9日週四 20:46，Mattijs Korpershoek <[email protected]> 寫道： > Hi George, > > On Wed, Oct 08, 2025 at 00:03, george chan <[email protected]> wrote: > > > Hi Simon, > > > > Thx for feedback. > > > > > > 在 2025年10月7日週二 04:38，Simon Glass <[email protected]> 寫道： > > > >> 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 > >> > > > > 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! > Thanks for feedback. if you have other detail wanted to share or have in next round, please let me know. I planed to have next submission mid/end next week. If you feel comfortable with GitHub code diff, please take a closer look and I can make modification accordingly. Regards, George > > > > > > 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 > >> >
