Conversation
bbff1fb to
f0e52b0
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Updates DTB bootargs handling so wolfBoot preserves FIT DTB /chosen/bootargs by default, only overriding when explicitly configured (or when forced), to avoid breaking validated kernel command lines.
Changes:
- Added
fdt_fixup_bootargs()helper to keep/override DTB bootargs based on aforceflag, with logging. - Updated multiple HALs to use the new helper and introduced
LINUX_BOOTARGS_OVERRIDEdefaulting based on build-time config. - Added unit tests and updated documentation to cover keep/replace/empty/malformed boundaries and configuration behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/unit-tests/unit-fdt.c | Adds unit test coverage for the new bootargs fixup semantics. |
| src/fdt.c | Implements fdt_fixup_bootargs() logic (keep vs override + logging). |
| include/fdt.h | Exposes the new fdt_fixup_bootargs() API. |
| hal/zynq.c | Switches DTB fixup to fdt_fixup_bootargs() and adds override macro defaulting. |
| hal/versal.c | Same as zynq: new override macro defaulting and bootargs fixup call. |
| hal/mpfs250.c | Same bootargs fixup change, but logs and continues on failure to preserve later fixups. |
| docs/Targets.md | Documents new “keep unless explicitly overridden” bootargs behavior and configuration knobs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f0e52b0 to
14982ec
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical compilation and raw-DTB bootargs trust issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
| fdt_fixup_str(&ctx, off, "chosen", "bootargs", LINUX_BOOTARGS); | ||
| /* Later fixups (watchdog disable, serial number) must run even if the | ||
| * bootargs fixup fails, so log and continue rather than return. */ | ||
| ret = fdt_fixup_bootargs(&ctx, LINUX_BOOTARGS, LINUX_BOOTARGS_OVERRIDE); |
| #ifndef LINUX_BOOTARGS_OVERRIDE | ||
| #if defined(LINUX_BOOTARGS) || defined(LINUX_BOOTARGS_ROOT) | ||
| #define LINUX_BOOTARGS_OVERRIDE 1 | ||
| #else | ||
| #define LINUX_BOOTARGS_OVERRIDE 0 |
Found on a VMK180 booting the stock PetaLinux 2022.2 image.ub:
hal_dts_fixup()unconditionally replaced the FIT DTB's/chosen/bootargswith the compiled-inLINUX_BOOTARGS, silently discarding theconsole=/earlycon=arguments the image's kernel was validated with -- the kernel then panicked (Asynchronous SError) with no console attached, presenting as a dead hang after the EL2->EL1 handoff. wolfBoot now keeps a DTB's own non-empty bootargs by default and only replaces them when the build explicitly setsLINUX_BOOTARGSorLINUX_BOOTARGS_ROOT(logging the replaced value);LINUX_BOOTARGS_OVERRIDE=0demotes an explicit value to a fallback used only when the DTB carries none. Hardware-validated on a VMK180: the 2022.2 image boots to userspace with a stock config, the explicit-override path still boots and logs the replacement, and the newfdt_fixup_bootargs()unit tests cover the keep, replace, and empty-value boundaries.