Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: initial UEFI support for x86-64 systems #285

Closed
wants to merge 10 commits into from

Conversation

sarahspberrypi
Copy link
Contributor

@sarahspberrypi sarahspberrypi commented Jan 19, 2024

No description provided.

@mkroening mkroening self-assigned this Jan 19, 2024
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Thanks a lot! :)

This looks great, just some minor remarks.

I'll hopefully merge and distribute hermit-os/hermit-entry#29 soon. :)

Cargo.toml Outdated
@@ -34,7 +34,7 @@ exclusive_cell = "0.1"
spinning_top = "0.3"

[target.'cfg(target_os = "uefi")'.dependencies]
uefi = "0.26"
uefi = "0.24"
Copy link
Member

Choose a reason for hiding this comment

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

What is preventing us from using version 0.26?

Comment on lines +12 to +15
QEMU does not support UEFI as firmware interface per se. You'll need to install [OVMF](https://github.com/tianocore/tianocore.github.io/wiki/OVMF-FAQ#what-is-open-virtual-machine-firmware-ovmf) (an open source implementation of UEFI for virtual machines).
You can do so, e.g., via terminal:
```bash
$ sudo apt install ovmf
```
Copy link
Member

Choose a reason for hiding this comment

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

Can you also put a pointer to the pre-built RPMs? :)

https://www.kraxel.org/repos/jenkins/edk2/

Extracting works with rpm2cpio:

rpm2cpio edk2.git-ovmf-x64-*.rpm | cpio -idmv

Copy link
Member

Choose a reason for hiding this comment

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

This one liner works:

wget -O - "http://ftp.debian.org/debian/pool/main/e/edk2/ovmf_2023.11-6_all.deb" | tar xvf - -O data.tar.xz | tar xvf - --strip-components=4 ./usr/share/OVMF/OVMF_CODE_4M.fd ./usr/share/OVMF/OVMF_VARS_4M.fd && mv OVMF_CODE_4M.fd OVMF_CODE.fd && mv OVMF_VARS_4M.fd OVMF_VARS.fd

```bash
$ mkdir -p bootloader/efi/boot

$ cp -f target/x86_64-uefi/debug/BootX64.efi bootloader/efi/boot/
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the scary -f here?

Suggested change
$ cp -f target/x86_64-uefi/debug/BootX64.efi bootloader/efi/boot/
$ cp target/x86_64-uefi/debug/BootX64.efi bootloader/efi/boot/

#[cfg(target_os = "uefi")]
use uefi::prelude::*;
#[cfg(target_os = "uefi")]
use uefi::table::{boot::*, cfg};
Copy link
Member

Choose a reason for hiding this comment

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

Except for preludes, we should not use glob imports.

src/uefi.rs Outdated
Comment on lines 94 to 151
#[panic_handler]
fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
// We can't use `println!` or related macros, because `_print` unwraps a result and might panic again
writeln!(unsafe { &mut console::CONSOLE }, "[LOADER] {info}").ok();

loop {}
}
Copy link
Member

Choose a reason for hiding this comment

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

A panic handler is already provided by uefi-services, so this should not be needed: https://github.com/rust-osdev/uefi-rs/blob/uefi-services-v0.23.0/uefi-services/src/lib.rs#L194-L196

## Building
### UEFI Boot (x86_64 only)

QEMU does not support UEFI as firmware interface per se. You'll need to install [OVMF](https://github.com/tianocore/tianocore.github.io/wiki/OVMF-FAQ#what-is-open-virtual-machine-firmware-ovmf) (an open source implementation of UEFI for virtual machines).
Copy link
Member

Choose a reason for hiding this comment

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

Nit: One line per sentence.

Comment on lines 89 to 94
// Right now, the kernel binary has to be hardcoded into the loader.
// The binary has to be in the same directory (or its whereabouts have to specified in the "include_bytes!" statement).
#[cfg(target_os = "uefi")]
pub unsafe fn find_kernel() -> &'static [u8] {
&[1, 2, 3]
include_bytes!("hermit-rs-template")
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this as we did with AArch64 before?
Also, this does not have to be unsafe.

pub fn find_kernel() -> &'static [u8] {
#[repr(align(8))]
struct Align8;
align_data::include_aligned!(Align8, env!("HERMIT_APP"))
}

Copy link
Member

Choose a reason for hiding this comment

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

Or, better yet, use uefi::fs.

Comment on lines 96 to 114
/// This is the actual boot function.
/// The bootstack is cleared and provided/calculated BOOT_INFO is written into before the actual call to the assembly code to jump into the kernel.
#[cfg(target_os = "uefi")]
pub unsafe fn boot_kernel(
_elf_address: Option<u64>,
_virtual_address: u64,
_mem_size: u64,
_entry_point: u64,
rsdp_addr: u64,
kernel_addr: u64,
filesize: usize,
kernel_info: LoadedKernel,
runtime_system_table: uefi::table::SystemTable<uefi::table::Runtime>,
start_address: usize,
end_address: usize,
) -> ! {
Copy link
Member

Choose a reason for hiding this comment

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

We might want to move this and the target_os = "none" one into separate modules. Let's coordinate on this. :)

@mkroening mkroening changed the title Add inital UEFI support for x86_64 systems feat: initial UEFI support for x86_64 systems Mar 15, 2024
@mkroening mkroening changed the title feat: initial UEFI support for x86_64 systems feat: initial UEFI support for x86-64 systems Mar 15, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants