-
Notifications
You must be signed in to change notification settings - Fork 117
feat(fs/mem): Tarfs for Hermit images implementation #2390
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ mod page_range_alloc; | |
| mod physicalmem; | ||
| mod virtualmem; | ||
|
|
||
| use core::cmp; | ||
| use core::ops::Range; | ||
|
|
||
| use align_address::Align; | ||
|
|
@@ -90,22 +91,86 @@ pub(crate) fn kernel_end_address() -> VirtAddr { | |
| KERNEL_ADDR_RANGE.end | ||
| } | ||
|
|
||
| /// Physical and virtual address range of the Hermit image, in case it is present | ||
| /// (indicated via FDT). | ||
| static HERMIT_IMAGE_START_AND_LEN: Lazy<Option<(VirtAddr, usize)>> = Lazy::new(|| { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is having a lazy static for this worth it? Or would a function suffice?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to avoid parsing this information multiple times; Normally, we need it twice: once during the memory setup (to ensure that memory is reserved for it, even tho the FDT reserved memory regions should take care of that), and once during filesystem setup.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you measure any benefit here? I expect the parsing overhead to be minimal.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably; but I prefer an implementation that makes it harder to find out when stuff can panic and when not. And in general, it might be possible to "thread" this information through the call-stack via argument passing, but |
||
| let fdt = env::fdt()?; | ||
|
|
||
| // per FDT specification, /chosen always exists | ||
| let chosen = fdt.find_node("/chosen").unwrap(); | ||
|
|
||
| let fdt::node::NodeProperty { | ||
| value: image_reg, .. | ||
| } = chosen.property("image_reg")?; | ||
|
Comment on lines
+102
to
+104
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a spec on the design of this FDT node? Why is it so much more complicated to parse than a Linux initrd (https://github.com/hermit-os/loader/blob/v0.5.6/src/arch/riscv64/mod.rs#L21-L31)? Should we model it like a Linux initrd instead? We could do either straight up
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I modeled it directly after the usually used
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Doing it the way I described above would make sense, I think.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, hermit-specific naming would make sense. |
||
|
|
||
| let cell_sizes = fdt.root().cell_sizes(); | ||
| let split_point = cell_sizes.address_cells * 4; | ||
| let end_point = split_point + cell_sizes.size_cells * 4; | ||
|
|
||
| if image_reg.len() != end_point { | ||
| return None; | ||
| } | ||
|
|
||
| let (addr, len) = image_reg.split_at(split_point); | ||
|
|
||
| if addr.len() == size_of::<*const u8>() && len.len() == size_of::<usize>() { | ||
| let addr = usize::from_be_bytes(addr.try_into().unwrap()); | ||
| let len = usize::from_be_bytes(len.try_into().unwrap()); | ||
| info!("Hermit image at {addr:x} with length {len:x}"); | ||
| Some(( | ||
| VirtAddr::from_ptr(core::ptr::with_exposed_provenance::<u8>(addr)), | ||
| len, | ||
| )) | ||
| } else { | ||
| error!( | ||
| "Hermit image supplied with invalid address range (#addr = {}, #len = {})", | ||
| addr.len(), | ||
| len.len(), | ||
| ); | ||
| None | ||
| } | ||
| }); | ||
|
|
||
| pub(crate) fn hermit_tar_image() -> Option<&'static [u8]> { | ||
| // technically, the following is UB, because the kernel might be contained within... | ||
| HERMIT_IMAGE_START_AND_LEN | ||
| .map(|(addr, len)| unsafe { core::slice::from_raw_parts(addr.as_ptr(), len) }) | ||
|
Comment on lines
+135
to
+137
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does the FDT node include the kernel? Why not only put the initrd into the node?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The kernel doesn't know if it is contained in it or not, it is simply possible that the ranges overlap (at least currently we don't outlaw that because it might provide memory usage savings in case of using the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand. How can they ever overlap? Sure, the image could contain the kernel ELF disk image, but not the loaded image, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The loader works very similarly to Uhyve. It's just happening from inside the VM. We get an ELF image and load the ELF image to memory by copying. The running executable is not running straight from the ELF image.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, then normally, no overlap should be possible (except by loader error). |
||
| } | ||
|
|
||
| #[cfg(target_os = "none")] | ||
| pub(crate) fn init() { | ||
| use crate::arch::mm::paging; | ||
| use arch::mm::paging; | ||
|
|
||
| Lazy::force(&KERNEL_ADDR_RANGE); | ||
|
|
||
| unsafe { | ||
| arch::mm::init(); | ||
| } | ||
|
|
||
| Lazy::force(&HERMIT_IMAGE_START_AND_LEN); | ||
|
|
||
| let total_mem = physicalmem::total_memory_size(); | ||
| let kernel_addr_range = KERNEL_ADDR_RANGE.clone(); | ||
| let reserved_addr_range = if let Some((image_start, image_len)) = *HERMIT_IMAGE_START_AND_LEN { | ||
| cmp::min( | ||
| kernel_addr_range.start, | ||
| image_start.align_down(LargePageSize::SIZE), | ||
| ) | ||
| ..cmp::max( | ||
| kernel_addr_range.end, | ||
| (image_start + image_len).align_up(LargePageSize::SIZE), | ||
| ) | ||
| } else { | ||
| kernel_addr_range.clone() | ||
| }; | ||
| info!("Total memory size: {} MiB", total_mem >> 20); | ||
| info!( | ||
| "Kernel region: {:p}..{:p}", | ||
| kernel_addr_range.start, kernel_addr_range.end | ||
| kernel_addr_range.start, kernel_addr_range.end, | ||
| ); | ||
| info!( | ||
| "Locally reserved region: {:p}..{:p}", | ||
| reserved_addr_range.start, reserved_addr_range.end, | ||
| ); | ||
|
|
||
| // we reserve physical memory for the required page tables | ||
|
|
@@ -126,7 +191,7 @@ pub(crate) fn init() { | |
| // On UEFI, the given memory is guaranteed free memory and the kernel is located before the given memory | ||
| reserved_space | ||
| } else { | ||
| (kernel_addr_range.end.as_u64() - env::get_ram_address().as_u64() + reserved_space as u64) | ||
| (reserved_addr_range.end.as_u64() - env::get_ram_address().as_u64() + reserved_space as u64) | ||
| as usize | ||
| }; | ||
| info!("Minimum memory size: {} MiB", min_mem >> 20); | ||
|
|
@@ -146,7 +211,7 @@ pub(crate) fn init() { | |
| // we reserve at least 75% of the memory for the user space | ||
| let reserve: usize = (avail_mem * 75) / 100; | ||
| // 64 MB is enough as kernel heap | ||
| let reserve = core::cmp::min(reserve, 0x0400_0000); | ||
| let reserve = cmp::min(reserve, 0x0400_0000); | ||
|
|
||
| let virt_size: usize = reserve.align_down(LargePageSize::SIZE as usize); | ||
| let layout = PageLayout::from_size_align(virt_size, LargePageSize::SIZE as usize).unwrap(); | ||
|
|
||
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 we not print an error instead of panicking and continue?
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.
It doesn't really make sense to boot a kernel that expects a very specific file system environment, and letting the kernel later run into errors when the image loading fails.