Unobfuscate "KERNLOAD" parameter on amd64
ClosedPublic
Actions

Authored by sobomax on Nov 24 2020, 7:24 PM.

Details

Reviewers

emaste
markj
royger

Commits

rS368041: Unobfuscate "KERNLOAD" parameter on amd64. This change lines-up amd64 with the

Summary

Kernel loading address seems like important parameter that somehow got scrambled up while developing amd64.

This change lines-up amd64 with the i386 and the rest of supported architectures by defining KERNLOAD in the vmparam.h and getting rid of magic constant in the linker script.

Use KERNLOAD to eliminate another (matching) magic constant 200 lines down inside unassuming TU "copy.c" 3 levels deep in the loader tree.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

sobomax created this revision.Nov 24 2020, 7:24 PM

Herald added subscribers: bcran, imp. · View Herald TranscriptNov 24 2020, 7:24 PM

sobomax requested review of this revision.Nov 24 2020, 7:24 PM

Harbormaster completed remote builds in B35006: Diff 79945.Nov 24 2020, 7:24 PM

sobomax edited the summary of this revision. (Show Details)Nov 24 2020, 7:25 PM

sobomax edited the summary of this revision. (Show Details)Nov 24 2020, 7:28 PM

sobomax edited the summary of this revision. (Show Details)Nov 24 2020, 7:31 PM

markj added inline comments.Nov 25 2020, 4:40 PM

stand/efi/loader/copy.c
36 ↗	(On Diff #79945)	Why is the seatbelt needed?

sobomax added inline comments.Nov 25 2020, 5:10 PM

stand/efi/loader/copy.c
36 ↗	(On Diff #79945)	Good question. I was not sure if I am going to break other arches or people building on older systems perhaps? But if that's OK, I'd be glad to get rid of this if you guys think it's not needed.

markj added inline comments.Nov 25 2020, 5:16 PM

stand/efi/loader/copy.c
36 ↗	(On Diff #79945)	I see now that KERNEL_PHYSICAL_BASE is only used if `__i386__` or `__amd64__` is defined, so the definition should move into that block and the warning can be dropped, IMO.

Eliminate KERNEL_PHYSICAL_BASE althogether, move #include <machine/vmparam.h> into i386/amd64-specific section.

Suggested by: markj

Harbormaster completed remote builds in B35043: Diff 80004.Nov 25 2020, 9:43 PM

Updated, thanks @markj !

markj accepted this revision.Nov 25 2020, 10:16 PM

markj added inline comments.

sys/amd64/include/vmparam.h
155 ↗	(On Diff #80004)	I would s/size/alignment/ in the comment. I believe the kernel expects to be loaded on a superpage boundary, it doesn't have to be 0x200000 specifically.