limine-bootloader / limine-rs Goto Github PK

Why is the address field in framebuffer a Ptr? The original type supplied by the Bootloader is *void why not cast it to a u64 as the limine protocol targets 64bit systems.

It seems weird to me that every structure exposed by the limine crate starts with the Limine prefix. I know it's probably to mimic the limine header file, but I think it just makes the API more verbose.

Is there anything against removing this prefix? (apart from it being a pretty big breaking change x) )

Currently, it is possible to alias mutable references to Limine responses in safe code.

Every request type returns a owned LiminePtr to their associated response. LiminePtr's API provides a get and a get_mut method to access the underlying data. In other words, it is possible to call the *Request::get_response function to get two copies of the same LiminePtr on two separate threads, then use those two copies to create two separate exclusive references to the underlying response.

I'm pretty sure the intent of LiminePtr is to "own" the value without owning the place. The pointer is itself supposed to remain unique, so it seems weird to me that it is returned by the get_response method.

One way to fix that would be to avoid returning the underlying LiminePtr directly, and providing two methods to access the response:

impl *Request {
    pub fn response(&self) -> Option<&*Response> {
        // SAFETY:
        //  The `*Request` is borrowed by reference, and our API never gives out a
        //  mutable reference from a regular `*Request` reference, ensuring that
        //  this dereference is safe.
        unsafe { &*self.response.get() }.get()
    }

    pub fn response_mut(&mut self) -> Option<&mut *Response> {
        // SAFETY: same argument, but for the exclusivity of the pointer.
        unsafe { &mut *self.response.get() }.get_mut()
    }
}

... or we could simply use those methods as accessors to internal LiminePtr, but that would just make the API harder to use.

fn response(&self) -> &LiminePtr<*Response>;
fn response_mut(&mut self) -> &mut LiminePtr<*Resonse>;

With this new API, you need a mutable reference to the request in order to have a mutable reference to the response. And, in order to have a mutable reference to the request... Well.. You need a static mut, which is unsafe to use because it could be accessed from two separate threads.

Also, isn't LiminePtr just a Option<NonNullPtr>? Is there any reason to re-implement all that logic here? Why not define a type alias?

I'd be down to write a PR for this issue!

What I wanted to do in my kernel was that after boot it would read the FramebufferResponse and store the first Framebuffer it contained into a global static, which would in turn be used by my print function.

But I couldn't do that since Framebuffer is not Sync and so can't be put in a global static, and you can't use unsafe to get past this either. In fact FramebufferResponse has unsafe impl Sync on it, but the Framebuffer struct it returns does not.

I think marking it as Sync should be fine, unless I'm missing something

Those two functions are defined as follows:

fn into_slice<'a>(&'a self, len: usize) -> &'a [NonNullPtr<T>] {
    // SAFETY: We have shared reference to the array.
    unsafe { core::slice::from_raw_parts(self.as_ptr(), len) }
}

fn into_slice_mut<'a>(&'a mut self, len: usize) -> &'a mut [NonNullPtr<T>] {
    // SAFETY: We have exculusive access to the array.
    unsafe { core::slice::from_raw_parts_mut(self.as_ptr(), len) }
}

But it is perfectly valid to call the function with a len that's way too large for the data actually referenced by the pointer. The pointer knows it references an array, but it does not know how many items it points to.

The safety doc of the functions should include something like this:

/// # Safety
///
/// The specified `len` must not exceed the number of elements actually referenced by the pointer. 

... or something like that.

I'd be down to implement the pull request for this though.

My entrypoint is defined in inline asm to capture the stack pointer. Because of this I use extern "C" { fn limine_entrypoint() -> !; }, which results in a function signature of unsafe extern "C" fn() -> !, which can't be passed to a function that expects extern "C" fn() -> !. The former function signature is more correct anyway as the entrypoint may depend on getting called with all state in accordance with the limine boot protocol for safety.

I'm making a kernel using the limine-rs template as reference and noticed that the _start() function included in the template doesn't draw anything on the framebuffer:

#[no_mangle]
unsafe extern "C" fn _start() -> ! {
    assert!(BASE_REVISION.is_supported());

    if let Some(framebuffer_response) = FRAMEBUFFER_REQUEST.get_response() {
        if let Some(framebuffer) = framebuffer_response.framebuffers().next() {
            for i in 0..100_u64 {
                // Calculate the pixel offset using the framebuffer information we obtained above.
                // We skip `i` scanlines (pitch is provided in bytes) and add `i * 4` to skip `i` pixels forward.
                let pixel_offset = i * framebuffer.pitch() + i * 4;

                // Write 0xFFFFFFFF to the provided pixel offset to fill it white.
                *(framebuffer.addr().add(pixel_offset as usize) as *mut u32) = 0xFFFFFFFF;
            }
        }
    }

    hcf();
}

Looking at the disassembly:

Disassembly of section .text:

ffffffff80000000 <_start>:
ffffffff80000000:       50                      push   rax
ffffffff80000001:       48 8d 3d f0 10 00 00    lea    rdi,[rip+0x10f0]        # ffffffff800010f8 <_ZN36_$LT$T$u20$as$u20$core..any..Any$GT$7type_id17h654c8c39a6067c34E+0x1018>
ffffffff80000008:       48 8d 15 39 21 00 00    lea    rdx,[rip+0x2139]        # ffffffff80002148 <_ZN36_$LT$T$u20$as$u20$core..any..Any$GT$7type_id17h654c8c39a6067c34E+0x2068>
ffffffff8000000f:       be 2e 00 00 00          mov    esi,0x2e
ffffffff80000014:       e8 77 00 00 00          call   ffffffff80000090 <_ZN4core9panicking5panic17h951cafdfee7cec9cE>
ffffffff80000019:       cc                      int3
ffffffff8000001a:       cc                      int3
ffffffff8000001b:       cc                      int3
ffffffff8000001c:       cc                      int3
ffffffff8000001d:       cc                      int3
ffffffff8000001e:       cc                      int3
ffffffff8000001f:       cc                      int3

it seems that _start() just immediately panics because the compiler thinks that assert!(BASE_REVISION.is_supported()) will always fail. Removing the assertion fixes this.

The problem seems to be that BaseRevision::is_supported() doesn't use a volatile read:

/// Check whether the given revision is supported by the bootloader.
pub fn is_supported(&self) -> bool {
    (unsafe { *self.revision.get() }) == 0 // Should use read_volatile() instead of *
}

and so the compiler assumes that the read will always return 1, because that's whats written in BaseRevision::new(). Making it a volatile read fixes this.

When using limine-rs in a staticlib, and linking that into an executable using ld.lld, the IDs inside the statics somehow get duplicated and cause conflicts:

Source with staticlib changes to reproduce: https://github.com/xdevs23/limine-rust-barebones/tree/staticlib-issue

When using binwalk on the staticlib file, we can see that there are two findings:

$ binwalk -R "\x75\xdd\x81\xd8\xdc\x27\x58\x9d\x1b" kernel/target/x86_64-unknown-none/debug/liblimine_rust_barebones.a

DECIMAL       HEXADECIMAL     DESCRIPTION
--------------------------------------------------------------------------------
155068        0x25DBC         Raw signature (\x75\xdd\x81\xd8\xdc\x27\x58\x9d\x1b)
206148        0x32544         Raw signature (\x75\xdd\x81\xd8\xdc\x27\x58\x9d\x1b)

But when the same command is run on the executable (without my changes), only one occurrence is found:

$ binwalk -R "\x75\xdd\x81\xd8\xdc\x27\x58\x9d\x1b" kernel/target/x86_64-unknown-none/debug/limine-rust-barebones 

DECIMAL       HEXADECIMAL     DESCRIPTION
--------------------------------------------------------------------------------
9320          0x2468          Raw signature (\x75\xdd\x81\xd8\xdc\x27\x58\x9d\x1b)

I tried to define the structs myself in a more simple manner:
xdevs23/limine-rust-barebones@55f3376

If you build this branch, it works: https://github.com/xdevs23/limine-rust-barebones/tree/staticlib-issue-workaround

On the branch I mentioned, the limine crate is not used and instead, the limine_def module contains simpler structs.
Here's the binwalk output:

$ binwalk -R "\x75\xdd\x81\xd8\xdc\x27\x58\x9d\x1b" kernel/target/x86_64-unknown-none/debug/liblimine_rust_barebones.a 

DECIMAL       HEXADECIMAL     DESCRIPTION
--------------------------------------------------------------------------------
136288        0x21460         Raw signature (\x75\xdd\x81\xd8\xdc\x27\x58\x9d\x1b)

And a screenshot:

Currently, (and even when fixing #14), it is possible to call the LimineTerminalWrite function on two separate threads with the same LimineTerminal instance.

That one is pretty simple. The function returns by LimineTerminalResponse::write method returns a function that takes a shared &LimineTerminal, meaning that the function can be called simultaneously with the same aliased terminal.

Using the same exact trick, it's possible to call the write within itself in the terminal callback implementation.

Both of those operations are invalid (as specified in the specification).

An easy way to fix this would be to require the write function to take an exclusive reference to the target terminal.

Going from:

fn write(&self) -> Option<impl Fn(&Terminal, s: &str)>;

to:

fn write(&self) -> Option<impl Fn(&mut Terminal, s: &str)>;

Note that this would require an additional terminals_mut method to be added to the LimineTerminalResponse.

Also: is there any way to remove the Option here? I mean, does it even make sense for this function not to be present?

See the latest CI fails.

In the documentation of the 5-level Paging Feature in the main bootloader repository, it states:

If the response pointer is changed to a valid pointer, 5-level paging is engaged.

However, in the documentation of the Limine5LevelPagingRequest struct it states:

If the response pointer is unchanged, 5-level paging is engaged.

This is a discrepancy that should be fixed to avoid any confusion.

Currently, the LimineEntryPoint type wraps an fn() -> !. Is there a reason why calling this function with the Rust calling convention works as expected? Shouldn't it be extern "C" fn() -> !?

Also, I've got a question:

The LimineEntryPointRequest structure stores a LiminePtr<LimineEntryPoint> internally. Is there a reason for that? I mean, LiminePtr<LimineEntryPoint> basically translates to *mut fn() -> !, and it pretty weird knowing that the "real" type is fn() -> !. In fact, the LiminePtr::<LimineEntryPoint>::new method creates a LiminePtr by casting the input fn() -> ! into a *mut fn() -> !. Why not simply using Option<LimineEntryPoint> in LimineEntryPointRequest?

I tried to compile the code and it does not because the get function is marked as unsafe and also the mmap print uses the wrong capitalization so that would again prevent it from compiling

The fix should be this i think

#[no_mangle]
extern "C" fn x86_64_barebones_main() -> ! {
    println!("Hello, rusty world!\n");

    let bootloader_info = BOOTLOADER_INFO
        .get_response()
        .get()
        .expect("barebones: recieved no bootloader info");

    println!(
        "bootloader: (name={}, version={})",
        bootloader_info.name.to_string().unwrap(),
        bootloader_info.version.to_string().unwrap()
    );

    let mmap_err = "barebones: recieved no mmap";
    let mmap = MMAP
        .get_response()
        .get()
        .expect(mmap_err)
        .mmap()
        .expect(mmap_err);
    println!("mmap: {:#x?}", MMAP);

    loop {}
}

Let me know if everything is ok