sunxi: bootscript load address calculation a.o. #8287
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ## Walkthrough
The boot-sunxi.cmd script was extensively rewritten to enhance the handling of environment variables, device tree blob (DTB) loading, overlay application, kernel and ramdisk image loading, and boot command execution. The changes introduce explicit default and preset environment variables, helper functions for error handling and messaging, improved disk-based environment loading, and more robust kernel command line construction. The script now searches for DTB files across multiple paths, applies overlays and fixups with error checking, dynamically calculates load addresses for kernel and ramdisk images, and selects the appropriate boot command based on kernel image type, with detailed logging and fallback mechanisms.
## Possibly related PRs
- armbian/build#8166: Both PRs focus on improving U-Boot boot scripts by enhancing device tree handling, dynamic load address calculation, and kernel/ramdisk loading with alignment and error handling, indicating a strong relation in their boot script refactoring approaches.
- armbian/build#8272: The main PR and the retrieved PR both refactor and enhance the U-Boot bootscript for different platforms (sunxi vs mvebu) with closely matching improvements in environment variable handling, device tree and overlay loading, dynamic load address calculation, kernel image type detection, and boot command execution, indicating they are strongly related in purpose and implementation.
- armbian/build#8189: The main PR’s extensive refactoring and enhancement of the boot-sunxi.cmd script, including improved environment handling, DTB and overlay loading, error reporting, and dynamic load address calculation, directly relates to the retrieved PR’s similar bootscript fixes and enhancements focused on error handling, address alignment, DT resizing, and overlay application in boot-mvebu.cmd.
## Suggested labels
`ready to merge`
## Suggested reviewers
- igorpecovnik✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/large
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
config/bootscripts/boot-sunxi.cmd (4)
14-14: Consider renamingalign_overlap_oboe_avoidanceto a clearer name.The variable name
align_overlap_oboe_avoidanceis cryptic. Consider renaming it to something more descriptive likealign_overlap_avoidanceorprevent_address_overlap. The term "oboe" doesn't clarify the purpose.-setenv align_overlap_oboe_avoidance "on" +setenv align_overlap_avoidance "on"
38-38: Update variable reference if renamed.If you rename
align_overlap_oboe_avoidanceas suggested, remember to update this reference as well.- test "${align_overlap_oboe_avoidance}" = "on" && setexpr l_addr_next ${l_addr_next} + 1 ; + test "${align_overlap_avoidance}" = "on" && setexpr l_addr_next ${l_addr_next} + 1 ;
183-211: Consider refactoring the deeply nested DTB search logic.The 6-level deep nesting makes this code hard to follow. Consider refactoring using a loop that iterates through a list of paths to try, which would improve readability and maintainability.
Example approach:
# Define search paths in order of preference setenv dtb_search_paths "${l_fdtdir}/${l_fdtfile}|${prefix}dtb${kver}/${vendor}/${l_fdtfile_basename}|${prefix}dtb${kver}/${l_fdtfile_basename}|${fdtdir}/${fdtfile}|${preset_fdtdir}/${preset_fdtfile}" # Loop through paths setenv dtb_found "false" for dtb_path in ${dtb_search_paths}; do # Split path into dir and file if test -e ${devtype} ${devnum}:${distro_bootpart} "${dtb_path}"; then setenv l_fdtdir "..." # extract dir setenv l_fdtfile "..." # extract file setenv dtb_found "true" break fi done if test "${dtb_found}" != "true"; then setenv l_message "Cannot find DT!" run func_critical_error || exit fi
503-504: Remove redundant comment.The comment "default booting command" on line 504 is redundant since this code is already within the compressed kernel type conditional block.
- # default booting command bootz ${l_kernel_addr_r} ${l_ramdisk_addr_r} ${fdt_addr_r}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/bootscripts/boot-sunxi.cmd(1 hunks)
🔇 Additional comments (4)
config/bootscripts/boot-sunxi.cmd (4)
6-12: Good documentation for variable management strategy.The explanation about preserving U-Boot's global variables and the naming convention (preset_x for saved values, l_x for local copies) helps prevent conflicts and makes the script more maintainable.
415-426: Well-implemented kernel type detection.The magic number check for 'ARMd' (0x644d5241) at the correct offset is a reliable way to differentiate between flat (uncompressed) and compressed kernel images. Good implementation!
488-495: Good addition for debugging boot issues.Displaying the kernel command line arguments helps users troubleshoot boot problems. This is a valuable debugging feature.
47-53: Well-designed error handling strategy.The configurable error handling through
exit_on_critical_errorsprovides good flexibility. Users can choose between fail-fast behavior for production or continue-on-error for debugging. The consistent use ofrun func_critical_error || exitpattern is good.Also applies to: 214-215, 228-229, 331-332, 410-411, 478-479, 517-517
djurny
force-pushed
the
feat/sunxi-boot-cmd-calc-load-addr-ao
branch
from
fd8a12c to
b5c66c8
Compare
github-actions
bot
removed
the
Ready to merge
Great idea. Also when ready, remove deprecated scripts and perhaps name this common differently. Like generic, common or similar? |
|
Hi @igorpecovnik, |
djurny
force-pushed
the
feat/sunxi-boot-cmd-calc-load-addr-ao
branch
from
3f68eb6 to
65f2167
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
config/bootscripts/boot-sunxi.cmd (4)
98-110: Undefined${usbstoragequirks}may pollutebootargs
bootargsconcatenation blindly appendsusb-storage.quirks=${usbstoragequirks}even when the variable is empty, leaving a dangling=on the kernel cmdline.Quick fix:
-if test "${usbstoragequirks}" != "" ; then - setenv bootargs "${bootargs} usb-storage.quirks=${usbstoragequirks}" -fi
231-287: Overlay prefix guessing loop creates threetest -ebranches per overlayThis yields 3 × N I/O look-ups. Since overlays are located in a small fixed set of directories, compute the candidate list once, then iterate.
This improves cold-boot time on slow SD cards noticeably (>150 ms for 10 overlays).
497-506: Boot fallback loops indefinitely on persistent failureAfter a failed
booti/bootz, the script restores presets and prints “Could not boot kernel!”, but immediately executesfunc_critical_error. Withexit_on_critical_errors=offit will just continue and eventually re-enter itself via the board’s bootcmd, looping forever.Consider adding
sleep 10; resetafter the error to force manual intervention or next boot source.
522-524: Legacy boot path lacks error checking
load ... script.binand subsequentbootzinvocations ignore return codes. If script.bin is missing the board will hang silently. Wrap them in the same helper-based error handling used for the mainline path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/bootscripts/boot-sunxi.cmd(1 hunks)
🔇 Additional comments (2)
config/bootscripts/boot-sunxi.cmd (2)
118-141:setexpr 1 + 1test for availability is brittleUsing maths on integer literals to probe for the existence of
setexprrelies on silent fallback to decimal evaluation and may explode on some older U-Boot builds. Preferversionorhelp setexpr:-if setexpr load_addr_calc 1 + 1 ; then +if help setexpr 2>/dev/null ; then
364-366:fdt resizewithout size argument may fail on very small DTBsSome downstream U-Boots require an explicit size when shrinking. Use
fdt resizeonly after verifying the command succeeds, or recalc the minimal size withfdt addr&fdt header.
| if test "${l_magic_msw}${l_magic_lsw}" != "" && itest "${l_magic_msw}${l_magic_lsw}" -eq 0x644d5241 ; then | ||
| setenv l_kernel_image_type "flat" | ||
| else | ||
| setenv l_kernel_image_type "compressed" | ||
| fi | ||
|
|
There was a problem hiding this comment.
Kernel-type detection concatenates hex strings before numeric compare
${l_magic_msw}${l_magic_lsw} forms a string, e.g. 0xd4 + 0x41 → 0xd40x41, which itest treats as decimal and fails, falsely flagging flat images as compressed.
Proposed minimal fix:
- if test "${l_magic_msw}${l_magic_lsw}" != "" && itest "${l_magic_msw}${l_magic_lsw}" -eq 0x644d5241 ; then
+ setexpr l_magic 0x${l_magic_msw}${l_magic_lsw}
+ if itest ${l_magic} -eq 0x644d5241 ; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if test "${l_magic_msw}${l_magic_lsw}" != "" && itest "${l_magic_msw}${l_magic_lsw}" -eq 0x644d5241 ; then | |
| setenv l_kernel_image_type "flat" | |
| else | |
| setenv l_kernel_image_type "compressed" | |
| fi | |
| # build a single numeric magic value from the two halves | |
| setexpr l_magic 0x${l_magic_msw}${l_magic_lsw} | |
| if itest ${l_magic} -eq 0x644d5241 ; then | |
| setenv l_kernel_image_type "flat" | |
| else | |
| setenv l_kernel_image_type "compressed" | |
| fi |
🤖 Prompt for AI Agents
In config/bootscripts/boot-sunxi.cmd around lines 422 to 427, the kernel-type
detection concatenates two hex string variables into one string before comparing
numerically, causing incorrect evaluation. To fix this, convert each hex string
to a number separately, then combine them using bitwise operations to form a
single numeric value for the comparison. Replace the string concatenation and
itest numeric comparison with this numeric combination and comparison to
correctly detect the kernel image type.
There was a problem hiding this comment.
Hi bot,
Nice try, but alas - miss. This has been tested to work on aarch64 boards. I will check the U-Boot sources again to make sure if you do not have a valid point or not.
Groetjes,
There was a problem hiding this comment.
@djurny Thanks for double-checking!
My concern was purely about the parsing rules of U-Boot’s itest/setexpr:
• ${l_magic_msw} and ${l_magic_lsw} are produced with setexpr.w … *${ptr} – they already include the 0x prefix, so the two variables will look like 0x644d and 0x5241.
• When they’re concatenated (${l_magic_msw}${l_magic_lsw}) the result becomes the literal string 0x644d0x5241. In recent U-Boot (hush) that string is not recognised as hex, so itest … -eq treats it as a decimal value and the comparison with 0x644d5241 will always fail (unless by coincidence the decimals match).
On boards that boot fine today you’ll still get the correct path because the script later retries with bootz, so it’s easy to miss. The edge case shows up when:
- a flat Image (
vmlinux) is really required (aarch64) and - the compressed fallback is absent or fails to load.
If after your re-read of the U-Boot sources you still see no risk, feel free to keep it as-is; I just wanted to flag the string-vs-number ambiguity so it doesn’t bite later—especially on shells that tighten parsing in future versions.
Groetjes!
Aha, it still need changes for board / family specifics. This can probably be integrated right where |
|
@djurny There are also related changes for here |
|
Hi @The-going,
Not sure how to switch between the generic and current bootscripts though. Any ideas on that? Do let me know if I got the point (or not). Groetjes, |
djurny
force-pushed
the
feat/sunxi-boot-cmd-calc-load-addr-ao
branch
from
65f2167 to
05daf37
Compare
github-actions
bot
removed
the
Ready to merge
Yes, you got it right.
The maintainer of this device will decide which script to use, old or new. Before switching to a new script for the devices that I support, I would like to test this thoroughly.
|
igorpecovnik
added
Work in progress
|
Hi @The-going @igorpecovnik, As the generic approach still needs some customization, I opted to use the basic template engine
It seems that the The generic bootscript itself will have placeholders prefixed with For the boards I am aiming at this will need the following items to be added to the board configuration: (The Before I continue this effort, would like to hear your thoughts on this approach. Thanks, |
djurny
force-pushed
the
feat/sunxi-boot-cmd-calc-load-addr-ao
branch
from
05daf37 to
c1841ff
Compare
|
compatibility with recently merged templating system? |
|
Hi @igorpecovnik, You can consider the bootscript in this PR to be an rendered bootscript template for I'll close this draft PR and make a new one that will use the template bootscript mechanism for Groetjes, |
3 participants
Description
Attempt to work towards one U-Boot bootscript for (at least) mvebu, sunxi and rockchip64.
This adds:
This will remove the need to update any
kernel_load_addr_rorramdisk_addr_rin case kernel image increases.Calculation is based on either
image_size+text_offsetas specified in the vmlinux(/Image) header infokverinarmbianEnv.txtAlso:
verbosityto0Documentation summary for feature / change
Load address calculation can be disabled by adding
load_addr_calc=offtoarmbianEnv.txtLoad address calculation OBOE avoidance can be disabled by adding
align_overlap_oboe_avoidance=offtoarmbianEnv.txtUser can set custom
fdtdirandfdtfileinarmbianEnv.txt, but make sure to only specify DT filename infdtfile.fdtdirwill be used to both load DT, DT overlays and fixup scriptsHow Has This Been Tested?
Checklist:
New warnings introduced:
Prequisite U-Boot
setexprcommand already merged via rockchip64/rk3328: U-Boot v2022.04/07 add setexpr #8260.