From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4CF75CCD185 for ; Thu, 9 Oct 2025 12:46:36 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id BBCE783A48; Thu, 9 Oct 2025 14:46:34 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="Es6nu8Gg"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 06C5983C6D; Thu, 9 Oct 2025 14:46:34 +0200 (CEST) Received: from tor.source.kernel.org (tor.source.kernel.org [172.105.4.254]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 4B32883A3A for ; Thu, 9 Oct 2025 14:46:30 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=mkorpershoek@kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 85E80622CB; Thu, 9 Oct 2025 12:46:29 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DD8D6C4CEE7; Thu, 9 Oct 2025 12:46:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1760013989; bh=JWkWcdv5/jXMA19PKcgiyNIFs6Cu6VYVJqwOnpa92NY=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=Es6nu8Gg8Li/spE18cCxxvdSA5TRCscwdBCr08ljQVArtPFslyyyhPC+ZaZDUy6LC xxFSsbDjK+EOiBPq3/6uABg7fEqPins7MBbwuRjMwL2RNnkbCbAQrD/BJgVfpYFbFZ CiOmMq9dcF836e/zf6lUn+vwqgZ7qZAR8+1EA+zKqVa1XYPinUXgIKOGThPdtdK9Ee VzUpncBoFg2mFKhpCRxj4nTAOUUWhVOK2SchbfkDK+1LgquoqT3/FiBcZ8JpS+LAcq RZFhOIsnn6P4WD83kdL/G2l33tj+X8gL21EsaJR38KFZ6zjFyI6ZU6lC6dwgUQHKgS iSIaYZhX5CPKw== From: Mattijs Korpershoek To: george chan , Simon Glass Cc: Mattijs Korpershoek , George Chan via B4 Relay , Tom Rini , Casey Connolly , Neil Armstrong , Sumit Garg , Lukasz Majewski , Marek Vasut , u-boot@lists.denx.de, u-boot-qcom@groups.io Subject: Re: [PATCH v4 3/6] bootm: Append bootargs value when bootmeth_android provide cmdline In-Reply-To: References: <20250630-sc7180-android-boot-v4-0-24cdb50e860f@gmail.com> <20250630-sc7180-android-boot-v4-3-24cdb50e860f@gmail.com> <871pqg4iar.fsf@kernel.org> <871pqdaepm.fsf@kernel.org> <873484jt3u.fsf@kernel.org> Date: Thu, 09 Oct 2025 14:46:26 +0200 Message-ID: <875xcow8d9.fsf@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Hi George, On Wed, Oct 08, 2025 at 00:03, george chan wrote: > Hi Simon, > > Thx for feedback. > > > =E5=9C=A8 2025=E5=B9=B410=E6=9C=887=E6=97=A5=E9=80=B1=E4=BA=8C 04:38=EF= =BC=8CSimon Glass =E5=AF=AB=E9=81=93=EF=BC=9A > >> Hi George, >> >> On Sun, 5 Oct 2025 at 09:41, george chan wrote: >> > >> > >> > >> > =E5=9C=A8 2025=E5=B9=B410=E6=9C=885=E6=97=A5=E9=80=B1=E6=97=A5 00:35= =EF=BC=8Cgeorge chan =E5=AF=AB=E9=81=93=EF=BC=9A >> >> >> >> Hi Simon and Mattijs, >> >> >> >> Thx for feedback. Just in case the discussion drift to other directio= n, >> do let me have a chance to put some background info here. >> >> >> >> there are ways for pre-set env variable and its value. Firstly extern= al >> 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 bootin= fo >> 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 uboo= t=E2=80=99s >> 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 desi= gn >> 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 tha= ts >> why i decided to put those preset into env file and embeded. >> >> >> >> let me reply below inline. >> >> >> >> =E5=9C=A8 2025=E5=B9=B410=E6=9C=882=E6=97=A5=E9=80=B1=E5=9B=9B 00:22= =EF=BC=8CSimon Glass =E5=AF=AB=E9=81=93=EF=BC=9A >> >>> >> >>> Hi Mattijs, George, >> >>> >> >>> On Tue, 30 Sept 2025 at 01:29, Mattijs Korpershoek >> >>> 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@kernel.org> wrote: >> >>> > >> >>> > > Hi George, >> >>> > > >> >>> > > On Wed, Jul 16, 2025 at 19:55, george chan >> wrote: >> >>> > > >> >>> > >> Hi Mattijs, >> >>> > >> >> >>> > >> Thx for reply. >> >>> > >> >> >>> > >> =E5=9C=A8 2025=E5=B9=B47=E6=9C=8816=E6=97=A5=E9=80=B1=E4=B8=89 = 17:05=EF=BC=8CMattijs Korpershoek >> =E5=AF=AB=E9=81=93=EF=BC=9A >> >>> > >> >> >>> > >>> Hi George, >> >>> > >>> >> >>> > >>> Thank you for the patch. >> >>> > >>> >> >>> > >> ... >> >>> > >> >> >>> > >>> >> >>> > >>> > - ret =3D env_set("bootargs", cmdline); >> >>> > >>> > + ret =3D android_image_modify_bootargs_env(cmdline, NUL= L); >> >>> > >>> >> >>> > >>> 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_lin= e, >> 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 c= an >> 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?expa= nd=3D1 >> >> 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_andro= id > 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 boo= tm > 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 neith= er >> in >> >>> > >> original logic nor in url from you provided. So I can say the u= rl >> 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 =3D 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 log= ic >> 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=3Dti-u-boot-2024.0= 4-next&id=3D9d802d798ac143e06ffe545606aa657a6c32cc63 >> >>> > >> >> >>> > >> >> >>> > >> ... >> >>> > >> >> >>> > >> >> >>> > >>> 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 =3D android_image_modify_bootargs_env(cmdline, NULL); >> >>> > >> + else >> >>> > >> ret =3D 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 =3D 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 >>