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

Load ramdisk feature #302

Merged
merged 18 commits into from
Jan 9, 2023
Merged

Conversation

jasoncouture
Copy link
Contributor

@jasoncouture jasoncouture commented Dec 30, 2022

Adds support for loading a ramdisk to both bios and UEFI, and passing it to the kernel.

Closes #308
cc #299

bios/stage-2/src/main.rs Outdated Show resolved Hide resolved
@phil-opp
Copy link
Member

phil-opp commented Jan 2, 2023

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 cargo fmt to fix them).

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.

@phil-opp
Copy link
Member

phil-opp commented Jan 2, 2023

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:

  • Place the files in a folder on the FAT partition and let the bootloader load all items of that folder. This would require us to implement FAT directory traversal in our real mode code, though, which might be difficult.
  • Concat all files into a single container file in create_disk_image and unpack that file in the bootloader before doing the virtual memory mapping. For example, we could make the ramdisk a FAT partition too.

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.

@jasoncouture
Copy link
Contributor Author

jasoncouture commented Jan 2, 2023

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:

  • Place the files in a folder on the FAT partition and let the bootloader load all items of that folder. This would require us to implement FAT directory traversal in our real mode code, though, which might be difficult.

I see a few issues with loading multiple files:

  1. It's unique, and less portable (To say, multiboot2)
  2. This, as mentioned is likely to be a challenge for the real mode code (but not impossible)

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.

  • Concat all files into a single container file in create_disk_image and unpack that file in the bootloader before doing the virtual memory mapping. For example, we could make the ramdisk a FAT partition too.

As for building an archive and parsing/loading with the bootloader:

  1. We can build support for archives on top of this change.
  2. The kernel may or may not want the archive parsed ahead of time.
  3. We'd have to pick a specific format, which could increase coupling of kernels to bootloaders
  4. The specific format may also not be appropriate for a particular kernel (IE: newc CPIO, in a kenrel that uses a non-unix like security model)

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.

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.

This would be my preferred option, given the notes about the archive above.
But I only have my use case, and a few other examples.
They may be a product of multiboot features, rather than the "right way" of doing this type of thing.

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

@jasoncouture
Copy link
Contributor Author

The tests hang, but the test kernels boot. I'm not sure why.
I didn't really make any changes to the tests, other than allowing them to add a ramdisk.

And it works with https://github.com/jasoncouture/oxidized with and without this commit

@jasoncouture jasoncouture force-pushed the load_ramdisk_feature branch 4 times, most recently from fc37a18 to 9eb1852 Compare January 3, 2023 11:50
@jasoncouture jasoncouture force-pushed the load_ramdisk_feature branch 2 times, most recently from 55573c7 to a0fb1d4 Compare January 3, 2023 22:21
@jasoncouture
Copy link
Contributor Author

Boot issues have been fixed, and this is finally ready.
This will have conflicts with #304 which is a great PR, but the issues will likely be much easier to resolve in #304 (I'm happy to fix them there after this is merged, or incorporate the changes into this PR)

This PR also blocks #307

@phil-opp
Copy link
Member

phil-opp commented Jan 4, 2023

As noted in #299 (comment), I'm fine with the single-file approach for now. The cpio_reader crate looks very promising indeed!

We still might want to add native support for multiple files in the future because it still has some advantages:

  • It's easier for users to just specify a list of files that should be loaded, without needing to manuall pack/unpack archives.
  • It enables more flexible mappings. For example, kernels could configure specific memory offsets for different files.
  • Compined with ASLR, it would increase security if we map the individual files at different offsets. This way, attackers could no longer deduce the addresses of other files from a file's address.

@jasoncouture
Copy link
Contributor Author

As noted in #299 (comment), I'm fine with the single-file approach for now. The cpio_reader crate looks very promising indeed!

We still might want to add native support for multiple files in the future because it still has some advantages:

  • It's easier for users to just specify a list of files that should be loaded, without needing to manuall pack/unpack archives.
  • It enables more flexible mappings. For example, kernels could configure specific memory offsets for different files.
  • Compined with ASLR, it would increase security if we map the individual files at different offsets. This way, attackers could no longer deduce the addresses of other files from a file's address.

100% agreed. I'll try to implement this as a follow up to this PR, building on top of the generalized file loading.

src/lib.rs Outdated Show resolved Hide resolved
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.

Great PR, thanks a lot!

common/src/lib.rs Outdated Show resolved Hide resolved
common/src/lib.rs Outdated Show resolved Hide resolved
common/src/lib.rs Outdated Show resolved Hide resolved
common/src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Outdated Show resolved Hide resolved
@jasoncouture
Copy link
Contributor Author

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.

@phil-opp
Copy link
Member

phil-opp commented Jan 5, 2023

Looks like there are still some unaddressed comments, that GitHub helpfully hides by default :D

image

common/src/lib.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Outdated Show resolved Hide resolved
@jasoncouture
Copy link
Contributor Author

I've rebased on main @phil-opp , I think this is ready to be merged.

@jasoncouture
Copy link
Contributor Author

In order to resolve the merge conflict resulting from the merge of #304 , I'm going to need to revert it, and instead merge in #313

@jasoncouture jasoncouture force-pushed the load_ramdisk_feature branch 2 times, most recently from 0adaf46 to f2c8bbe Compare January 9, 2023 17:13
@phil-opp
Copy link
Member

phil-opp commented Jan 9, 2023

In order to resolve the merge conflict resulting from the merge of #304 , I'm going to need to revert it, and instead merge in #313

Oh wow, now I'm confused.. This should not be necessary, we just need to fix a few conflicts when merging main into 64faf0f. I can take care of these conflicts if you like, but the reverting, the integration of #313, and the force-pushes make reviewing this PR much more difficult.

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.

@jasoncouture jasoncouture force-pushed the load_ramdisk_feature branch 3 times, most recently from 718c28d to b4b6ed5 Compare January 9, 2023 17:26
@jasoncouture
Copy link
Contributor Author

Understood.

In order to resolve the merge conflict resulting from the merge of #304 , I'm going to need to revert it, and instead merge in #313

Oh wow, now I'm confused.. This should not be necessary, we just need to fix a few conflicts when merging main into 64faf0f. I can take care of these conflicts if you like, but the reverting, the integration of #313, and the force-pushes make reviewing this PR much more difficult.

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.

@jasoncouture
Copy link
Contributor Author

Understood.

In order to resolve the merge conflict resulting from the merge of #304 , I'm going to need to revert it, and instead merge in #313

Oh wow, now I'm confused.. This should not be necessary, we just need to fix a few conflicts when merging main into 64faf0f. I can take care of these conflicts if you like, but the reverting, the integration of #313, and the force-pushes make reviewing this PR much more difficult.
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.

Headed for 0a4b9c7 instead, as that was merged on top of the logging changes.

@jasoncouture
Copy link
Contributor Author

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 a lot for all the updates!

I'll handle the conflicts and merge this PR afterwards.

bios/stage-2/src/main.rs Outdated Show resolved Hide resolved
@phil-opp phil-opp merged commit e15954b into rust-osdev:main Jan 9, 2023
@phil-opp phil-opp mentioned this pull request Mar 12, 2023
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.

Add support for loading ramdisk
3 participants