Page MenuHomeFreeBSD

build: Rewrite arm_kernel_boothdr in Lua and add RISC-V support.
Needs ReviewPublic

Authored by jceel on Jul 15 2021, 2:04 PM.

Details

Reviewers
imp
manu
ian
mmel
Group Reviewers
ARM
riscv
Summary

awk version of the script has an inherent issue when dealing with 64-bit
addresses: all numeric values are internally stored as doubles by awk. This
causes severe loss of precision with high 64-bit values.

Rewrite it in Lua which is much better suited for dealing with binary data
and change the name to a more generic one (kernel_boothdr.lua) because of
the newly added RISC-V booti header generation support.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningsys/tools/kernel_boothdr.lua:CHMOD1Invalid Executable
Unit
No Unit Test Coverage
Build Status
Buildable 40485
Build 37374: arc lint + arc unit

Event Timeline

jceel requested review of this revision.Jul 15 2021, 2:04 PM
sys/conf/Makefile.riscv
44

ARM? In a riscv kernel?

85

'arm' references.

mmel requested changes to this revision.Jul 15 2021, 2:48 PM
mmel added a subscriber: mmel.

In final commit, the original arm_kernel_boothdr.awk should be deleted. Otherwise look good to me

sys/conf/Makefile.riscv
41

Unless it's a forgotten fragment, it's unnecessary and imho a bad move. You should follow arm64 - the kernel.bin created this way has all symbols shifted from kernel.full -> so all debug symbols are wrong.

This revision now requires changes to proceed.Jul 15 2021, 2:48 PM
sys/conf/Makefile.riscv
41

Unless it's a forgotten fragment, it's unnecessary and imho a bad move. You should follow arm64 - the kernel.bin created this way has all symbols shifted from kernel.full -> so all debug symbols are wrong.

Can you be more specific? This particular fragment is directly copied from arm64 Makefile, so I'm not sure what "follow arm64" means.

Added riscv linker script change missing in previous revision.

Fix kernel_boothdr.lua file mode.

sys/conf/Makefile.riscv
40–58

Oops, I take it back, sorry for the noise. I shouldn't write a review under time pressure, never... Ignore this, please.

mhorne added inline comments.
sys/conf/Makefile.riscv
42

Unless I'm mistaken, these mapping symbols don't exist at all on riscv. The attempts to strip them are not required.

46

Is there a reason to introduce this offset to the .text section, or it is just a result of the copy+paste? It breaks existing functionality, see my comment below.

sys/conf/ldscript.riscv
11 ↗(On Diff #92231)

This will break the functionality introduced by D23436.

sys/tools/kernel_boothdr.lua
45

This may complicate things, but for booti on riscv you almost certainly want the _alt_start entry, which appears first in the .text section. See the comments in locore.S for a slight elaboration.

So, my personal view for RISC-V is that booti is redundant. All U-Boots for RISC-V are new enough to properly support UEFI, so you should be using loader(8) (which can be loader_simp). If you're in a really constrained embedded environment then you should probably skip U-Boot entirely and use plain SBI booting. The booti U-Boot stuff is a weird U-Boot-specific thing that predates U-Boot's UEFI support, should never have been added for RISC-V anywhere in my opinion, and is in a weird middle ground that doesn't seem to offer much benefit to me.

sys/conf/Makefile.riscv
42

There is a proposal to add them, primarily because with IFUNCs and conflicting extensions we need a way to be able to control disassembly. But no, they don't exist currently, and there certainly won't be a and t symbols (A32 and T32).

sys/conf/Makefile.riscv
53–56

Even if it's decided that we should support RISC-V booti, I do not think we should produce these files by default. No user should be using it unless they really know what they're doing, so building the files is a waste of space and risks users stumbling across them and using booti rather than using UEFI as the primary supported method. I'd want to see this behind an off-by-default option.