Page MenuHomeFreeBSD

riscv: look for bootargs in FDT
ClosedPublic

Authored by freebsdphab-AX9_cmx.ietfng.org on Jul 2 2020, 2:13 AM.

Details

Summary

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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.

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).

freebsdphab-AX9_cmx.ietfng.org edited the test plan for this revision. (Show Details)

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?

Slightly less code motion; tested using qemu "-append -s" on FreeBSD machine I'd forgotten about.

Sorry, newbie learning way around arc. Seems it grabbed an unintended commit.

sys/riscv/riscv/machdep.c
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.

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.

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!

Thanks, looks good to me, and will be quite useful! I gave it a quick test as well.

This revision is now accepted and ready to land.Jul 17 2020, 2:06 PM
This revision was automatically updated to reflect the committed changes.