The FDT may contain a short /chosen/bootargs string which we should pass to boot_parse_cmdline. Notably, this allows the use of qemu's -append option to pass things like -s to boot to single user mode.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
arm64's parse_fdt_bootargs deals with loader(8) which will normally handle these for us when used, so we need to copy that approach given https://reviews.freebsd.org/D24912 has landed. My guess is the best place for it is parse_metadata around where we currently call init_static_kenv, to match arm64.
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 1108 ↗ | (On Diff #74007) | This should be at the start of the function. Also, is 256 enough? The arm64 port uses 512 as another arbitrary constant, and as a global; personally I'd be inclined to just make it a 4K global buffer to provide plenty of space and not waste precious stack memory. |
Comment Actions
Also, please avoid filing revisions with CheriBSD tainting the diff; it can be misleading, as well as the fact that we're always a bit behind (and in this case it does matter because of the loader(8) patch landing).
Comment Actions
Revised in light of jrtc27's comments. Unfortunately, I lack a FreeBSD box to build and test this on (sorry, CheriBSD builds fine and I didn't think about our being lagged relative to upstream); perhaps someone who does would be so kind as to let me know if I've broken something?
Comment Actions
Slightly less code motion; tested using qemu "-append -s" on FreeBSD machine I'd forgotten about.
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 846 ↗ | (On Diff #74022) | As was pointed out, if we booted via loader(8) then it has already done this parsing work. There's nothing wrong with doing it again, but we can avoid it. |
| 1108 ↗ | (On Diff #74007) | Our init stack is small (4 pages), but I would expect there to be very little on the stack at this point in boot. Perhaps this buffer should live there since it is a one-time use. Perhaps not, since that could easily come back to bite us one day. |
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 846 ↗ | (On Diff #74022) | While I believe the premise that loader(8) can have handled all this, testing suggests that perhaps that should be if (kern_envp == NULL) parse_fdt_bootargs() But I may be turned around; kern_environment.c is entirely novel to me. :) |
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 846 ↗ | (On Diff #74022) | You are correct. I made a mistake and got the conditional backwards. Sorry about that! |