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

EFI Prekernel (1/N): Kernel: Move boot info variables into a shared struct #24943

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

spholz
Copy link
Collaborator

@spholz spholz commented Aug 18, 2024

Instead of having multiple boot info variables, move them all into a common global BootInfo struct 'g_boot_info'.
Also make BootInfo more boot method-agnostic by moving multiboot-specific data into Multiboot1BootInfo, which is a member of the BootInfo::boot_method_specific union.

This depends on #24920, therefore draft.

*adjust_by_mapping_base(&kernel_mapping_base) = KERNEL_MAPPING_BASE;
*adjust_by_mapping_base(&kernel_load_base) = KERNEL_MAPPING_BASE;
*adjust_by_mapping_base(&g_boot_info.boot_method) = BootMethod::Multiboot1;
adjust_by_mapping_base(&g_boot_info.boot_method_specific)->pre_init.~PreInitBootInfo();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(copied from #23664 (comment))
I am unsure if calling the destructor of the default union member is needed here (and in all other places where boot_method_specific is set to a non-default union member).

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we static_assert that g_boot_info.boot_method is_trivial? Then this dtor call does nothing and isn't needed, and the placement new isn't needed either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

static_assert<IsTrivial<BootMethodSpecificBootInfo>> fails if that is what you meant. I think it's not trivial due to the initializers in Multiboot1BootInfo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But static_assert<IsTrivial<PreInitBootInfo>> succeeds. Does that mean the ~PreInitBootInfo isn't needed?

Why do you think the placement new is unnecessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

It basically just memcpy()s, right? Not super important I suppose.

@spholz spholz marked this pull request as ready for review August 21, 2024 16:09
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Aug 21, 2024
Copy link

stale bot commented Sep 18, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Sep 18, 2024
@stale stale bot removed the stale label Sep 18, 2024
@spholz spholz force-pushed the efi-prekernel-1 branch 3 times, most recently from 2db2abd to d350c64 Compare September 19, 2024 19:26
This commit reorganizes the BootInfo struct definition so it can be
shared for all architectures.

The existing free extern "C" boot info variables have been removed and
replaced with a global BootInfo struct, 'g_boot_info'.

On x86-64, the BootInfo is directly copied from the Prekernel-provided
struct.
On AArch64 and RISC-V, BootInfo is populated during pre_init.
@nico nico merged commit eacfb94 into SerenityOS:master Oct 30, 2024
13 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Oct 30, 2024
@spholz spholz deleted the efi-prekernel-1 branch October 30, 2024 23:08
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