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

avoid 32-bit relocation to __BOOTLOADER_CONFIG #428

Merged

Conversation

Freax13
Copy link
Contributor

@Freax13 Freax13 commented Feb 21, 2024

As explained in my comment in #427, the compiler seems to have trouble emitting relocations for references to global variables in custom sections. For custom sections, it always emits 32-bit relocations even when a 64-bit relocation would be required. This patch works around that by never referencing the global in the custom section directly from code, but only through a pointer from another global variable in the non-custom .data section. The relocation used for the pointer in the global variable will always use a 64-bit relocation.

I tested the patch with these changes applied on top of this repo:

diff --git a/Cargo.toml b/Cargo.toml
index 94d582c..76e6832 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -113,9 +113,9 @@ lto = true
 [profile.test.package.test_kernel_higher_half]
 rustflags = [
     "-C",
-    "link-args=--image-base 0xFFFF800000000000",
+    "link-args=--image-base 0x00001eecb3620000",
     "-C",
-    "relocation-model=pic",
+    "relocation-model=static",
     "-C",
     "code-model=large",
 ]

Considering that there aren't many people using such an odd image base, I didn't add a test for this, but feel free to tell me if you want one.

Closes #427

As explained in my comment in rust-osdev#427, the compiler seems to have trouble
emitting relocations for references to global variables in custom
sections. For custom sections, it always emits 32-bit relocations even
when a 64-bit relocation would be required. This patch works around
that by never referencing the global in the custom section directly
from code, but only through a pointer from another global variable in
the non-custom .data section. The relocation used for the pointer in
the global variable will always use a 64-bit relocation.
@jxors
Copy link

jxors commented Feb 22, 2024

Unfortunately this fix only works for me in debug mode (cargo 1.78.0-nightly (194a60b29 2024-02-21) & rustc 1.78.0-nightly (3406ada96 2024-02-21)). In release mode I still get the error. Probably because the extra reference is optimized away.

@jxors
Copy link

jxors commented Feb 22, 2024

Maybe the section could be renamed to .note.bootloader-config. The linker doesn't seem to remove sections starting with .note regardless of whether they are used or not.

@bjorn3
Copy link
Contributor

bjorn3 commented Feb 22, 2024

That doesn't help if the compiler already optimized the indirection away. In that case the compiler will still emit the relocation in question.

@bjorn3
Copy link
Contributor

bjorn3 commented Feb 22, 2024

By the way, why are you using -Ccode-model=large rather than -Ccode-model=kernel (higher half kernel) or -Ccode-model=medium (lower half kernel)? Your kernel is unlikely to be larger than 2GB.

@jxors
Copy link

jxors commented Feb 22, 2024

That doesn't help if the compiler already optimized the indirection away. In that case the compiler will still emit the relocation in question.

If I understand correctly, the reference to the section could be removed entirely if it's renamed to .note.bootloader-config. The reference is only there to prevent the linker from removing the section from the final ELF.

By the way, why are you using -Ccode-model=large rather than -Ccode-model=kernel (higher half kernel) or -Ccode-model=medium (lower half kernel)? Your kernel is unlikely to be larger than 2GB.

I'd like to use -Ccode-model=medium, but it produces relocation errors for everything in the kernel. My entire kernel fits within 0x1eecb3600000-0x1eecb36b0000 (0xb0000 bytes) so I'm not sure why it errors.

This prevents the compiler from optimzing out __BOOTLOADER_CONFIG_REF
in favor of accessing __BOOTLOADER_CONFIG directly.
@Freax13
Copy link
Contributor Author

Freax13 commented Feb 23, 2024

That doesn't help if the compiler already optimized the indirection away. In that case the compiler will still emit the relocation in question.

I fixed this by adding another level of indirection.

@Freax13
Copy link
Contributor Author

Freax13 commented Feb 23, 2024

By the way, why are you using -Ccode-model=large rather than -Ccode-model=kernel (higher half kernel) or -Ccode-model=medium (lower half kernel)? Your kernel is unlikely to be larger than 2GB.

I'd like to use -Ccode-model=medium, but it produces relocation errors for everything in the kernel. My entire kernel fits within 0x1eecb3600000-0x1eecb36b0000 (0xb0000 bytes) so I'm not sure why it errors.

The medium code model still uses the small code model for addresses inside the .text section. This won't work because you're base address 0x1eecb3600000 doesn't fit in the small code model's address range from 0 to 0x7eff_ffff. The limitations of the small/medium code model are not just about the size of the kernel, but also about the location i.e. base address.

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.

Thanks a lot for debugging and fixing this!

@phil-opp
Copy link
Member

phil-opp commented Mar 4, 2024

@jxors Could you try whether the latest commit works for you in release mode?

@Freax13 I just added you to the @rust-osdev/bootloader team, so you should have merge permissions on this repo now. Feel free to merge once @jxors confirms, just in case I'm too slow :).

@Freax13
Copy link
Contributor Author

Freax13 commented Mar 4, 2024

@Freax13 I just added you to the @rust-osdev/bootloader team, so you should have merge permissions on this repo now. Feel free to merge once @jxors confirms, just in case I'm too slow :).

Alright, thanks, will do!

@jxors
Copy link

jxors commented Mar 7, 2024

Unfortunately the second fix still doesn't work for me. The error can be reproduced using the following code:

// Cargo.toml:
[package]
name = "bootloader-repro"
version = "0.1.0"
edition = "2021"

[dependencies]
bootloader_api = { path = "../bootloader/api" }
x86_64 = "0.14.2"

// src/main.rs:
#![no_std]
#![no_main]

use bootloader_api::{entry_point, BootInfo};

#[panic_handler]
fn panic(info: &core::panic::PanicInfo) -> ! {
    loop {
        x86_64::instructions::hlt();
    }
}

entry_point!(kernel_main);
fn kernel_main(boot_info: &'static mut BootInfo) -> ! {
    todo!()
}

I'm compiling with RUSTFLAGS="-Ccode-model=large -Crelocation-model=static -Clink-arg=--image-base=0x00001eecb3620000" cargo build --release --target x86_64-unknown-none -Z build-std=core,alloc -Z bindeps on rustc 1.78.0-nightly (7d3702e47 2024-03-06).

@jxors
Copy link

jxors commented Mar 7, 2024

Here's the workaround I've been using in the meantime: jxors@b16e341

We want to avoid directly accessing the data in the .bootloader-config
section and slice::as_ptr gets a pointer to that data. Instead we cast
a reference to a reference to the data in .bootloader-config to an
address and pass that to the asm! block. The problem with as_ptr was
that it's a method on the slice and not the reference and so by
calling slice::as_ptr, we once again directly accessed the slice when
we only meant to access the reference to the reference to the data.
@Freax13
Copy link
Contributor Author

Freax13 commented Mar 7, 2024

Unfortunately the second fix still doesn't work for me. The error can be reproduced using the following code:

// Cargo.toml:
[package]
name = "bootloader-repro"
version = "0.1.0"
edition = "2021"

[dependencies]
bootloader_api = { path = "../bootloader/api" }
x86_64 = "0.14.2"

// src/main.rs:
#![no_std]
#![no_main]

use bootloader_api::{entry_point, BootInfo};

#[panic_handler]
fn panic(info: &core::panic::PanicInfo) -> ! {
    loop {
        x86_64::instructions::hlt();
    }
}

entry_point!(kernel_main);
fn kernel_main(boot_info: &'static mut BootInfo) -> ! {
    todo!()
}

I'm compiling with RUSTFLAGS="-Ccode-model=large -Crelocation-model=static -Clink-arg=--image-base=0x00001eecb3620000" cargo build --release --target x86_64-unknown-none -Z build-std=core,alloc -Z bindeps on rustc 1.78.0-nightly (7d3702e47 2024-03-06).

Thanks for testing and thanks for providing the code for reproducing!

I just pushed another commit that should fix the issue.

@jxors
Copy link

jxors commented Mar 8, 2024

It is now working for me. Thanks!

@Freax13 Freax13 merged commit 4bd1ba4 into rust-osdev:main Mar 14, 2024
8 checks passed
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.

Load kernel at specific address in v0.11
4 participants