Hi Casey, Thx for reply. Sorry for a bit long message for explain the problem.
在 2025年4月28日週一 21:53，Casey Connolly <casey.conno...@linaro.org> 寫道： > Hi George, > > Thanks a lot for the series, it's super exciting to see support for > booting Android on top of U-Boot :D > > On 4/27/25 13:25, George Chan via B4 Relay wrote: > > This is a series of patches to enable chainloading LineageOS on qcom SOC. > > > > First patch is to workaround kernel/ramdisk invalid addr by identify > > its physical memory address out-of-range. Since qcom SOC usually have > > 0x80000000 as start/base/real memory address but androidboot img > > specified to around 0x0. If other vendor bootloader behave similar then > > this patch can also workaround it as well. > > I'm curious to know what values are there, tbh I think U-Boot should I don't know either. I figured it out a failure memcpy kernel to addr 0, and failure at right after first 16k succes. Since kernel is relocatable so I ignored it. I know you are dealing with memory mapping on xbl and chain loading case with some kind of smem value. Good job! > entirely ignore the values in the boot image but I guess that could > break some other platforms. I agreed your below suggested alternative approach apply can ease for maintaining, and less concern. I am willing to help draft a patch worked as your mentioned approach. Maybe it is more optimistic than what you see here. Below are some reasons might be worth to rethink. (1) This is specific to bootm dealing androidboot img only. (2)A similar approach applied to check specific kernel address already. (3)The proposed logic is to cater extra case that is never works. (4)unless there is a need for soc that mmap() high (physical) memory addr to low (nonexistent) addr, aka 0, and use low address as kernel data. Sounds like hyperizer iommu work. Looks redundance. (5)if checking gd->base_mem is not safe then we can add extra checking for gd null. The logic only affect valid gd and base_mem value. Either null or 0 won't have any side effect. So it less likely breaks. In the end, if apply your suggested approach, then mine and existing one are redundance and could be removed for clean up. I am not sure what happened and concluded for existing code but the code exists. Maybe some senior contributor knows more about it. Removing code would be a big behavioral change require broader acknowledgement. Or just leave existing logic and apply only your suggested approach anyway. Both are fine for me. I prefer to wait for some feedback before new revision roll. > How about adding a config option CONFIG_ANDROID_BOOTIMG_IGNORE_ADDRS and > just ORing it in all the places that have checks like > "img_data->kernel_addr == 0". Then you can enable this for > mach-snapdragon by selecting it in the ARCH_SNAPDRAGON config definition. > As stated as above. > > > Second patch is enable bootmeth-android to have chance for extra mem > block. > > Usually the fastboot mem block, to hold androidboot img, and loadaddr for > > unzipped kernel. If other SOC have extra mem block available but > > fastboot block, we can add extra logic to take care of it. > > There is a small hack in mach-snapdragon that we put kernel_addr_r and > loadaddr at the same address to avoid using up too much memory on > devices that only have 1GB which explains this crash because we try to > decompress the kernel to the same address it got loaded too. > Reasonable. I think we can get away with giving loadaddr its own allocation of 64M > or so, maybe 96. This would make your second patch redundant (and feels > like the right fix to me) > I don't know how memory constrain affected, and have virtually no knowledge on memory arrangement. So I can't comment on this. There is a third way. Add one more env_var like androidboot_addr_r and pointed to same as fastboot_addr_r then use it in bootmeth-android if this new env exists. If var not exists, fallback to existing logic. That balance with minimal logic change and readability. > > > > Third patch is optional to enable snapdragon board to have extra cmdline > > found from original bootloader that is required for LineageOS boot init. > > An alternate method is to put the append string into a dummy device-tree > > file, as /chosen/bootarg ofnode porperty, that also contain msm-id. Then > > encapsulate as androidboot img and let u-boot as kernel binary. Below is > > an example for Xiaomi Miatoll device. > > I assume you're chainloading U-Boot on this device? If so, isn't it > enough to just copy the boot args that ABL gives us and maybe tweak a few? > Yes, I am doing chain loading u-boot. And yes, patch 3 is doing exactly you mentioned. I mentioned here is 2nd approach that I feels cleaner for chain loading case. I really have no idea on how prev_blob and target_blob works all the way fdt handling (difference of replace xbl and chain loading). If non-chain-loading case there have no prev_blob, then 1st approach is preferable. > I'd also rather keep this constrained, maybe an android specific board > callback to set boot args (if there isn't one already) and have the FDT > fixup code be alongside some other generic FDT fixup stuff. I'd rather > not have this stuff in board specific code. > Reasonably. I can't figure out the difference board specific fixup and android specific fixup. My limited knowledge is fdt_support.c have weak board_fdt_chosen_bootargs func to board.c as my 1st approach do as patch 3. I wonder if you are talking about adding one more fdt-support.c weak func like board_fdt_chosen_bootargs_android. And leave board_fdt_chosen_bootargs empty for further use case on purpose. I also curious if you are targeting to make it generic enough for mach-* and/or enhanced API with full fdt for fixup because the existing one is incapable. And this is reasonable too. I wanna to express some though about additional fixups here. As you knows those androidboot param are half baked and soc/machine/device/os dependent. So it can't be one-sized-fit-all. Currently we have some approaches to put it in. (1) In upstream dts, block the merge. (2) In a dummy dts, might not work for xbl case. (3) In board.c/Kconfig compile-time config. Not flexible. Can't do it with constrained way unless a string parser is needed (I wrote one similar thing but found useless https://github.com/99degree/u-boot/commit/3240054aa85f4cb70daa8671150bf4a9ca885a61 ) Since all added Androidboot param are android specific, my approach is very simple. Let the android init to get rid of it. Here is how to do so. Android Init treat Androidboot.* param as ro.boot.* and set-once-read-only and first-come-first-serve so any taylor-made param we put in the head will mandate the value even the same appears in the tail. In other words, params in tail are failsafe as result. This behavior is good enough to make an approach to balance flexible and function. We can have wanted param in chainloaded Androidboot (boot.) img dtb, and 'default' param live in board specific fixup logic. Since env_bootargs have param from Android boot img and board can do fixup by append android specific param to end of env_bootargs. Then we do not need to have constrained param (to append env_bootargs, my own understanding) unless overflow bootargs length. Well, this is the core idea of my impl in patch 3. Also patch 3 have Kconfig string to override SOC specific fixup so param value can set by config file. One more thing worth mentioning, I made boot.img repacked with dtb from /sys/firmware/fdt and do u-boot chain load because of well known dtbo issue. Maybe u-boot support dtbo partition but I have not tried. I cant assumed people get used to familiar with android param so a dummy way is do less-is-more. It is easy to repack boot.img with new fdt (by tool like e.g. magisk) but not easy to figure which param to append bootargs is needed. Even duplicated param as result. > I hope this makes sense, feel free to ask for clarifications on anything. > Yes your suggestions are valuable > Thanks and kind regards, > kind regards, George > > > > /dts-v1/; > > > > / { > > qcom,msm-id = <443 0x0>; > > qcom,board-id = <0 0>, > > <0x10022 1>, > > <0x20022 1>, > > <0x30022 1>, > > <0x40022 1>, > > <0x50022 1>; > > > > #address-cells = <2>; > > #size-cells = <2>; > > > > memory { > > ddr_device_type = <0x07>; > > /* We expect the bootloader to fill in the size */ > > reg = <0 0 0 0>; > > }; > > > > chosen { > > bootargs = ""; > > }; > > }; > > > > Signed-off-by: George Chan <gchan9...@gmail.com> > > --- > > George Chan (3): > > boot/image-android.c: Workaround androidboot kernel/ramdisk addr > > boot/bootmeth-android.c: Reuse fastboot memory block for unzip > kernel > > mach-snapdragon: Add support to append string to kernel cmdline > > > > arch/arm/mach-snapdragon/Kconfig | 11 +++++ > > arch/arm/mach-snapdragon/board.c | 97 > ++++++++++++++++++++++++++++++++++++++++ > > boot/bootmeth_android.c | 12 +++++ > > boot/image-android.c | 10 +++++ > > 4 files changed, 130 insertions(+) > > --- > > base-commit: 5a0a93a768487e55ebe50a34cc90d751bf99cc56 > > change-id: 20250427-android-boot-ecbb768cda72 > > > > Best regards, > -- > Casey (she/they) > >
