Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Centralize the kernel and module metadata types #1394

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

VexedUXR
Copy link
Contributor

Currently, the way we get the kernel metadata pointer is by calling
preload_search_by_type with one of the following three: "elf kernel",
"elf32 kernel" and "elf64 kernel". Which one(s) we use isn't consistent
though. Sometimes we would only try "elf kernel", and other times we
would try one of the latter two if the first failed. However, the loader
only ever sets "elf kernel" as the kernel type.
See:

const char *__elfN(kerneltype) = "elf kernel";

Instead, make the kmdp a global, preload_kmdp, and initialize it using
preload_initkmdp in machdep.c (or machdep_boot.c on arm/64).
preload_initkmdp takes a single boolean argument that tells us whether
not finding kmdp is fatal or not.

Also, feel free to critique my commit message. I feel like it's not great,
but that's the best I could word it.

@VexedUXR
Copy link
Contributor Author

Fixed some style warnings

@bsdimp
Copy link
Member

bsdimp commented Aug 22, 2024

I'll take a look. @kostikbel and @kevans91 should take a look too.

@kostikbel
Copy link
Member

My opinion is that this is an enormous churn.

Regardless of it, I think even the approach is not taken to the logical completion. All "elf kernel" literals must be replaced by the global const string which is used by reference, both in loader and in kernel.

Ideally, we would have some enumeration value used to identify type, instead of free-typed string, but for this change at least replacing all individual strings with the same reference to constant string is the least required step.

@VexedUXR
Copy link
Contributor Author

My opinion is that this is an enormous churn.

Yes, but it centralizes where we get the metadata at least. It also removes a bunch of unnecessary checks where kmdp can't be NULL. IMO most of the changes are trivial.

Regardless of it, I think even the approach is not taken to the logical completion. All "elf kernel" literals must be replaced by the global const string which is used by reference, both in loader and in kernel.

You're right, they should be. But for the kernel, after this patch, I believe the only reference to "elf kernel" would be in preload_initkmdp.

@bsdimp
Copy link
Member

bsdimp commented Aug 23, 2024

I'll take a closer look at this. It's basically OK, but I think I have some misgivings I need to put into an actionable form.

@VexedUXR
Copy link
Contributor Author

Looking more into what @kostikbel said, I've decided to move "elf kernel" and friends to sys/linker.h as macros. The reason they're macros an no longer globals is so that they can be easily shared across both the loader and kernel code.

Other preload_search_by_type calls either use macros or use a single free-typed string (i.e it only has one reference), so I think this is better.

Also, I noticed that we also check for "elfN module" and "elfN obj module", so I removed those too.

sys/sys/linker.h Outdated Show resolved Hide resolved
@VexedUXR
Copy link
Contributor Author

Alright, settled on modinfo.c at the end
Also, needed to make file_loadraw accept a const char * for the type string instead of a plain char *

@VexedUXR VexedUXR changed the title Make the kernel metadata pointer a global and avoid unnecessary "elfN kernel" checks. Centralize the kernel and module metadata types Aug 31, 2024
Copy link
Member

@kostikbel kostikbel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the patch series, modulo two small identical comments

sys/kern/subr_module.c Outdated Show resolved Hide resolved
stand/common/modinfo.h Outdated Show resolved Hide resolved
The way we got the kernel metadata pointer was by calling
preload_search_by_type with one of the following three: "elf kernel",
"elf32 kernel" and "elf64 kernel". Which one(s) we used wasn't
consistent though. Sometimes we would only try "elf kernel", and other
times we would try one of the latter two if the first failed. However,
the loader only ever sets "elf kernel" as the kernel type.

Now, the kmdp is a global, preload_kmdp, and it's initialized using
preload_initkmdp in machdep.c (or machdep_boot.c on arm/64).
preload_initkmdp takes a single boolean argument that tells us whether
not finding the kmdp is fatal or not.
We never set the kernel type to either "elf64 kernel" nor "elf32
kernel".
Initialize the globals with macros so we can use the same values in the
loader.

Also remove unnecessary "elfN module" checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants