-
Notifications
You must be signed in to change notification settings - Fork 215
Specify kernel-code virt addr in BootloaderConfig #494
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
I'd like to point out that it's also possible to set the kernel base address to be a static address by changing the relocation-model to be a static one. That said, I think there are enough differences between static and dynamic executables for it to be worthwhile making debugging dynamic executables easier.
common/src/load_kernel.rs
Outdated
if used_entries.try_mark_range_as_used(address, size).is_err() { | ||
return Err("kernel_code mapping is already in use"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should fail if that happens. p4 entries cover quite a large amount of address space (512 GiB), so it's perfectly possible to map multiple things into the same p4 entry.
I think we might want to reevaluate that once we start tracking memory mappings on a more fine-grained level, but that's certainly out of the scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your right. However that would leave the code in a bit of a fragile state. For all other fixed-address mappings we mark the p4 entries as used when used_entries
is constructed. That way we don't accidentally overlap any dynamic allocations (they are all placed in separate p4 entries, although I think they are later mapped as 4K pages).
The reason I can't mark the kernel code as used the same way, is that when used_entries
is constructed, I don't know the size of the kernel.
Skipping this test for now is fine, because the kernel is the first thing "allocated", however it might lead to potential errors in the future.
Let me think about this some more. I think I might have a way to calculate the kernel size/alignment a bit earlier without having to completely refactor everything. That should allow us to treat a fixed kernel code base address the same way we handle every other fixed address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already parsed the kernel elf file by the time UsedLevel4Entries::new
is called, so I think we can just use that to mark the kernel's address space as used.
That said if the kernel is the first thing that's allocated, then we can also just do it here. We've already been doing that for statically-relocated binaries after all.
Also, I just noticed that we already mark the kernel's address space as used in this line here:
bootloader/common/src/load_kernel.rs
Line 92 in 170320b
used_entries.mark_segments(elf_file.program_iter(), virtual_address_offset); |
So we might not even have to do anything about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we might not even have to do anything about this.
True. However I think it is still preferable to mark the memory range as used as early as possible to ensure that any dynamic allocations don't overlap it.
We already parsed the kernel elf file by the time UsedLevel4Entries::new is called, so I think we can just use that to mark the kernel's address space as used.
Also true. Maybe I am doing more work than is actually required here. I moved the size calculation into it's own function and used it to mark the range as used where all the other fixed ranges are also marked.
That said if the kernel is the first thing that's allocated, then we can also just do it here. We've already been doing that for statically-relocated binaries after all.
Should I also add that for statically-relocated binaries? Right now it does not really matter because mark_segments
happens to be called before we do any other dynamic allocations but that might change in the future.
Now that I refactored the size calculations this should be a trivial addition that might prevent future bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called earlier when used_entries
is initialized. At this point we also have all the necessary information to mark the range for statically-relocated binaries.
bootloader/common/src/level_4_entries.rs
Lines 80 to 89 in dc41a5e
if let config::Mapping::FixedAddress(kernel_base) = config.mappings.kernel_base { | |
let ElfMemoryRequirements { size, align, .. } = | |
calc_elf_memory_requirements(kernel_elf); | |
if !VirtAddr::new(kernel_base).is_aligned(align) { | |
return Err("kernel_code mapping alignment does not match elf file"); | |
} | |
used.mark_range_as_used(kernel_base, size); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also add that for statically-relocated binaries? Right now it does not really matter because
mark_segments
happens to be called before we do any other dynamic allocations but that might change in the future. Now that I refactored the size calculations this should be a trivial addition that might prevent future bugs.
That'd be great, thanks!
tests/test_kernels/fixed_kernel_address/src/bin/verify_kernel_address.rs
Outdated
Show resolved
Hide resolved
common/src/load_kernel.rs
Outdated
if used_entries.try_mark_range_as_used(address, size).is_err() { | ||
return Err("kernel_code mapping is already in use"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already parsed the kernel elf file by the time UsedLevel4Entries::new
is called, so I think we can just use that to mark the kernel's address space as used.
That said if the kernel is the first thing that's allocated, then we can also just do it here. We've already been doing that for statically-relocated binaries after all.
Also, I just noticed that we already mark the kernel's address space as used in this line here:
bootloader/common/src/load_kernel.rs
Line 92 in 170320b
used_entries.mark_segments(elf_file.program_iter(), virtual_address_offset); |
So we might not even have to do anything about this.
9c88a9c
to
dc41a5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost ready, thank you for your great work!
common/src/load_kernel.rs
Outdated
if used_entries.try_mark_range_as_used(address, size).is_err() { | ||
return Err("kernel_code mapping is already in use"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also add that for statically-relocated binaries? Right now it does not really matter because
mark_segments
happens to be called before we do any other dynamic allocations but that might change in the future. Now that I refactored the size calculations this should be a trivial addition that might prevent future bugs.
That'd be great, thanks!
dc41a5e
to
fa4be39
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work!
This PR adds a way for the kernel to specify where the bootloader should write the kernel_code in memory.
This is useful for debugging purposes as it ensures that it is easy to specify an offset to gdb.
I also incremented the minor version as this is a breaking change in the way the BootloaderConfig is serialized.