-
Notifications
You must be signed in to change notification settings - Fork 214
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
Load ramdisk feature #302
Load ramdisk feature #302
Conversation
Thanks a lot for submitting, this is quite a useful feature! I left some review comments on #301, but I think we can apply them directly here instead (and close #301). There are some build errors on the CI that we should fix. Looks like they're related to the config parsing and some formatting errors (run To ensure that the ramdisk loading works as expected, we should also add some tests for it. For example, a test kernel could load some dummy file with some random data and compare its contents after loading. |
Another idea came up in #299: Maybe we should support loading a list of files, instead of just a single ramdisk? We could implement this in different ways:
What do you think about this? Edit: As a third option, we could also keep a single ramdisk file for now and let the kernel (and its build script) handle the packing and unpacking itself. |
9085b09
to
95e02d5
Compare
I see a few issues with loading multiple files:
Do we want to add the complication of this, and increase bootloader coupling? For my use, I don't believe this is a good path, but I wonder if there's a use case I'm missing.
As for building an archive and parsing/loading with the bootloader:
Having the bootloader parse it though, also increases coupling. mutliboot2 bootloaders will not do this for you, and will pass the blob to the kernel as initrd. There are no_std crates for parsing CPIO archives, and building one and using it as a ramdisk could easily be done with this change, without adding any complexity or coupling to the bootloader.
This would be my preferred option, given the notes about the archive above. The archive method would perhaps be a good choice, but can easily be built as a separate, optional feature on top of this change. It can also be implemented, as mentioned, quite easily using crates like cpio_reader in the kernel, and cpio to build the image in build.rs |
95e02d5
to
06b4881
Compare
The tests hang, but the test kernels boot. I'm not sure why. And it works with /~https://github.com/jasoncouture/oxidized with and without this commit |
fc37a18
to
9eb1852
Compare
55573c7
to
a0fb1d4
Compare
As noted in #299 (comment), I'm fine with the single-file approach for now. The We still might want to add native support for multiple files in the future because it still has some advantages:
|
100% agreed. I'll try to implement this as a follow up to this PR, building on top of the generalized file loading. |
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.
Great PR, thanks a lot!
I'll fix the build errors when I rebase this later. I'm at work and wanted to get them in so I don't forget. |
e009c51
to
b86fb0c
Compare
…or the ramdisk path Co-authored-by: Philipp Oppermann <dev@phil-opp.com>
16fd0bb
to
0a4b9c7
Compare
I've rebased on main @phil-opp , I think this is ready to be merged. |
0adaf46
to
f2c8bbe
Compare
Oh wow, now I'm confused.. This should not be necessary, we just need to fix a few conflicts when merging So my preferred approach would be to force-push this PR back to 64faf0f. Then I can give it another round of review and resolve the conflicts. |
718c28d
to
b4b6ed5
Compare
Understood.
|
Headed for 0a4b9c7 instead, as that was merged on top of the logging changes. |
b4b6ed5
to
0a4b9c7
Compare
Changes from the logger merge are here: /~https://github.com/rust-osdev/bootloader/compare/16fd0bb04da7975e0ea2438db6885df9064444a5..0a4b9c719506774cbbbe2a4379589b0d0445aa9b |
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.
Looks good to me, thanks a lot for all the updates!
I'll handle the conflicts and merge this PR afterwards.
f6f0eb1
to
3fa940b
Compare
Adds support for loading a ramdisk to both bios and UEFI, and passing it to the kernel.
Closes #308
cc #299