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

copy more PML4 entries #466

Merged
merged 2 commits into from
Nov 14, 2024
Merged

copy more PML4 entries #466

merged 2 commits into from
Nov 14, 2024

Conversation

Freax13
Copy link
Member

@Freax13 Freax13 commented Oct 26, 2024

If there is more than 512 GiB of memory, we need to copy more than the first PML4 entry to access it. Similarly if the frame buffer is not in the address range covered by the first PML4 entry, we need to copy more entries in order to access it. We also need to mark these PML4 entries as unusable when allocating virtual addresses for the kernel because the bootloader maps some of them into its own address space.

This is a much simpler solution to #462 than #465. The downside is that we mark some of the kernel's virtual address space as unusable when it doesn't technically need to be.

@Colepng Could you please also test this PR?

Supersedes #464
Closes #462

If there is more than 512 GiB of memory, we need to copy more than the
first PML4 entry to access it. Similarly if the frame buffer is not in
the address range covered by the first PML4 entry, we need to copy more
entries in order to access it.
The bootloader maps some of the memory used by the kernel into its own
address space as well. In order for that to work we must ensure that
the bootloader doesn't already have memory mapped there. Mark regions
that are likely to be used by the bootloader as unusable.
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! And sorry again for the long delay!

@phil-opp
Copy link
Member

The downside is that we mark some of the kernel's virtual address space as unusable when it doesn't technically need to be.

Just to clarify: Do you mean unusable for the bootloader or unusable in the memory map (i.e. affecting the kernel)?

@Freax13
Copy link
Member Author

Freax13 commented Nov 14, 2024

Looks good to me, thanks! And sorry again for the long delay!

Thank you for the review!

The downside is that we mark some of the kernel's virtual address space as unusable when it doesn't technically need to be.

Just to clarify: Do you mean unusable for the bootloader or unusable in the memory map (i.e. affecting the kernel)?

We mark it as unusable for dynamic virtual memory space allocations for the kernel i.e. for mappings configured with Mapping::Dynamic. IIRC mappings with statically configured addresses are not affected because we never do any checks for those.

@phil-opp
Copy link
Member

Ah right. So when the UEFI firmware puts the framebuffer somewhere into the second PML4 entry, we won't use any address within that PML4 entry for dynamic kernel mappings?

@Freax13
Copy link
Member Author

Freax13 commented Nov 14, 2024

Ah right. So when the UEFI firmware puts the framebuffer somewhere into the second PML4 entry, we won't use any address within that PML4 entry for dynamic kernel mappings?

Yes, exactly.

@phil-opp
Copy link
Member

Ok, thanks for clarifying! Personally, I'm fine with this limitation for now. We can then try to make the used-tracking more precise in some future PR.

I'm also looking into #465 currently. Which of the two approaches do you prefer?

@Freax13
Copy link
Member Author

Freax13 commented Nov 14, 2024

Ok, thanks for clarifying! Personally, I'm fine with this limitation for now. We can then try to make the used-tracking more precise in some future PR.

I'm also looking into #465 currently. Which of the two approaches do you prefer?

I like how #465 solves this problem without any limitations for the loaded kernel, but I think the code is much uglier/hackier. I agree that losing a pml4 is acceptable for now. I like your idea of making the used tracking more precise, this would allow us to use the less-hacky from this PR code but also minimize wasting virtual address space.

@phil-opp
Copy link
Member

Ok, then let's proceed with this PR.

I don't mind the indirect memory access in #465, but I agree that the code seems a bit hacky in places (e.g. a separate translation step for each individual byte or the construction of an invalid &mut reference).

@Freax13 Freax13 merged commit af99f0f into main Nov 14, 2024
18 checks passed
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.

Hang after loading first level 4 table on uefi
2 participants