Skip to content

feat: Bochs VBE linear framebuffer support - #2523

Open
GloriousAlpaca wants to merge 4 commits into
hermit-os:mainfrom
GloriousAlpaca:pr-bga-clean
Open

feat: Bochs VBE linear framebuffer support#2523
GloriousAlpaca wants to merge 4 commits into
hermit-os:mainfrom
GloriousAlpaca:pr-bga-clean

Conversation

@GloriousAlpaca

Copy link
Copy Markdown

I'm currently working on porting Doom to Hermit and needed a feature to directly write pixel data into the Bochs Graphics Adaptor as Hermit currently only has support for VGA in textmode.

The feature currently adds standard BGA preparation (Link to OSDev) with a hardcoded resolution (640x400), bpp (32) and a systemcall (sys_get_framebuffer) to receive the address of the framebuffer.
The systemcall falls back to 0 if there was a failure in initializing the framebuffer (e.g. if pci is missing, or the bga feature is not enabled).

I have currently only tested this feature with c programs on a mac using Qemu. The feature is also only available on the x86_64 architecture.

@mkroening mkroening self-assigned this Jul 3, 2026
@mkroening
mkroening self-requested a review July 3, 2026 15:40

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Benchmark Results

Details
Benchmark Current: 71aa9fe Previous: 2e23902 Performance Ratio
startup_benchmark Build Time 81.16 s 80.34 s 1.01
startup_benchmark File Size 0.76 MB 0.80 MB 0.96
Startup Time - 1 core 0.77 s (±0.02 s) 0.75 s (±0.02 s) 1.03
Startup Time - 2 cores 0.75 s (±0.02 s) 0.74 s (±0.02 s) 1.02
Startup Time - 4 cores 0.77 s (±0.02 s) 0.74 s (±0.02 s) 1.04
multithreaded_benchmark Build Time 84.00 s 82.11 s 1.02
multithreaded_benchmark File Size 0.87 MB 0.86 MB 1.02
Multithreaded Pi Efficiency - 2 Threads 89.33 % (±5.86 %) 85.89 % (±6.61 %) 1.04
Multithreaded Pi Efficiency - 4 Threads 43.42 % (±2.29 %) 43.43 % (±2.56 %) 1.00
Multithreaded Pi Efficiency - 8 Threads 25.57 % (±1.34 %) 25.76 % (±1.53 %) 0.99
micro_benchmarks Build Time 77.34 s 80.40 s 0.96
micro_benchmarks File Size 0.88 MB 0.86 MB 1.02
Scheduling time - 1 thread 65.09 ticks (±4.18 ticks) 62.65 ticks (±4.06 ticks) 1.04
Scheduling time - 2 threads 37.33 ticks (±5.41 ticks) 34.08 ticks (±4.10 ticks) 1.10
Micro - Time for syscall (getpid) 4.08 ticks (±0.53 ticks) 3.45 ticks (±0.58 ticks) 1.18
Memcpy speed - (built_in) block size 4096 84468.32 MByte/s (±58277.50 MByte/s) 82448.38 MByte/s (±56997.13 MByte/s) 1.02
Memcpy speed - (built_in) block size 1048576 30518.74 MByte/s (±24554.28 MByte/s) 30585.98 MByte/s (±24707.84 MByte/s) 1.00
Memcpy speed - (built_in) block size 16777216 29180.81 MByte/s (±23983.01 MByte/s) 26340.06 MByte/s (±21720.96 MByte/s) 1.11
Memset speed - (built_in) block size 4096 84521.29 MByte/s (±58310.17 MByte/s) 82292.76 MByte/s (±56891.50 MByte/s) 1.03
Memset speed - (built_in) block size 1048576 31240.00 MByte/s (±24980.77 MByte/s) 31323.85 MByte/s (±25145.86 MByte/s) 1.00
Memset speed - (built_in) block size 16777216 29949.76 MByte/s (±24430.47 MByte/s) 27104.68 MByte/s (±22209.94 MByte/s) 1.10
Memcpy speed - (rust) block size 4096 75524.43 MByte/s (±52701.10 MByte/s) 74097.96 MByte/s (±51811.44 MByte/s) 1.02
Memcpy speed - (rust) block size 1048576 30466.83 MByte/s (±24541.43 MByte/s) 30361.60 MByte/s (±24602.37 MByte/s) 1.00
Memcpy speed - (rust) block size 16777216 29285.61 MByte/s (±24083.79 MByte/s) 27625.34 MByte/s (±22806.88 MByte/s) 1.06
Memset speed - (rust) block size 4096 75991.27 MByte/s (±53030.38 MByte/s) 74373.47 MByte/s (±51976.48 MByte/s) 1.02
Memset speed - (rust) block size 1048576 31198.87 MByte/s (±24963.06 MByte/s) 31110.89 MByte/s (±25033.24 MByte/s) 1.00
Memset speed - (rust) block size 16777216 30053.43 MByte/s (±24525.07 MByte/s) 28386.93 MByte/s (±23265.03 MByte/s) 1.06
alloc_benchmarks Build Time 77.06 s 74.76 s 1.03
alloc_benchmarks File Size 0.84 MB 0.87 MB 0.96
Allocations - Allocation success 91.34 % 91.31 % 1.00
Allocations - Deallocation success 100.00 % 100.00 % 1
Allocations - Pre-fail Allocations 61.54 % 61.44 % 1.00
Allocations - Average Allocation time 2600.77 Ticks (±730.09 Ticks) 5860.58 Ticks (±98.43 Ticks) 0.44
Allocations - Average Allocation time (no fail) 3639.43 Ticks (±636.29 Ticks) 6554.81 Ticks (±92.86 Ticks) 0.56
Allocations - Average Deallocation time 702.90 Ticks (±144.85 Ticks) 1805.01 Ticks (±250.35 Ticks) 0.39
mutex_benchmark Build Time 80.43 s 79.82 s 1.01
mutex_benchmark File Size 0.88 MB 0.86 MB 1.02
Mutex Stress Test Average Time per Iteration - 1 Threads 12.06 ns (±0.31 ns) 12.10 ns (±0.41 ns) 1.00
Mutex Stress Test Average Time per Iteration - 2 Threads 40.66 ns (±1.97 ns) 40.26 ns (±1.68 ns) 1.01

This comment was automatically generated by workflow using github-action-benchmark.

@mkroening mkroening left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR! :)

This is great! First I was wondering why we should use BGA instead of UEFI's GOP, which would also work on real machines, but I guess BGA is just easier at the moment. Since it is not that invasive, this is fine with me.

I am a bit confused regarding naming; maybe you could clear that up in a doc comment of the module or even the feature documentation. If I understand correctly, BGA is the Bochs Graphics Adapter, which we talk to via the VESA BIOS Extensions (VBE) API. But instead of talking to the BIOS via interrupts, we can just talk to the VBE API through the special BGA ports, right? In that case, the module names, feature names, and constant names make sense to me now.

Comment thread src/syscalls/system.rs Outdated
Comment thread src/syscalls/system.rs Outdated
Comment on lines +10 to +16
/// Returns the address of the framebuffer, if available. Returns 0 if no framebuffer is available.
#[cfg(all(target_arch = "x86_64", feature = "bga"))]
#[hermit_macro::system]
#[unsafe(no_mangle)]
pub extern "C" fn sys_get_framebuffer() -> u64 {
crate::kernel::bga::get_framebuffer_address()
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The design of system calls needs special scrutiny and discussion.

First, if the user is meant to write into the frame buffer through the return value of this system call, the return type must be a pointer type, not just an integer address.

Second, I am not that deep into graphics, but is the size and dimensions not something that is essential when drawing into a framebuffer? Is that currently just hardcoded? My gut feeling would be to propose something as follows:

pub extern "C" fn sys_framebuffer(framebuffer: *mut Framebuffer) -> c_int;

#[repr(C)]
pub struct Framebuffer {
    height: u32,
    width: u32,
    size: u64,
}

This is just an initial thought, though. It would be useful to look into related existing APIs such as libdrm and UEFI's Graphics Output Protocol (uefi::proto::console::gop::GraphicsOutput).

Another consideration might be multi-monitor support or more properties that a framebuffer has. Of course, the design of this PR does not need to be the last API ever on Hermit. We can replace it if necessary. Still, it would be nice to at least discuss some degree of future-proofing.

@GloriousAlpaca GloriousAlpaca Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes this sounds good, I'd maybe setup the struct in a similar way to UEFI Under EFI_GRAPHICS_OUTPUT_MODE_INFORMATION

#[repr(C)]
pub struct FramebufferInfo {
pub address: *mut u8,
pub width: u32,
pub height: u32,
pub bpp: u32,
}

I'd then change the syscall to take a pointer with which to fill the data and return either 0 or an error code (e.g. if no hardware is present):

sys_get_framebuffer(info: *mut FramebufferInfo, struct_size: usize) -> i32

(Struct Size so we can expand the Syscall later, while legacy code still works as expected)

I don't think BGA has multi-monitor support itself, the driver would probably have to look different for this and it would probably make more sense to do virtio-gpu instead. Virtio could then just expand the same struct downwards (for example with pitch or bitmasks).

Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
const VBE_DISPI_ID5: u16 = 0xb0c5;

pub fn init_device(adapter: &PciDevice<PciConfigRegion>) {
//To Do: Detect Resolution automatically

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you want to do this in this PR or in another, or is this meant for other people in the future?

Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread Cargo.toml
@mkroening

Copy link
Copy Markdown
Member

Ah, please also ensure that your PR does not include merge commits. Please rebase instead.

Comment thread src/arch/x86_64/kernel/bga.rs Outdated
@GloriousAlpaca

Copy link
Copy Markdown
Author

Thanks a lot for the feedback! I have pushed a commit, where I've addressed the feedback you've given. I have actually implemented the systemcall a bit differently than I've written before. Please do have a look if this is a good way to design it. I have also used structs instead of enums to consolidate the constants. Let me know if this is alright with you.

Comment thread src/syscalls/system.rs Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LLM review

The BGA driver implementation has several critical issues that need to be addressed, particularly around unsafe operations, error handling, and potential race conditions.

Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs
Comment thread src/arch/x86_64/kernel/bga.rs
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs
Comment thread src/arch/x86_64/kernel/bga.rs
Comment thread src/syscalls/system.rs
Comment thread src/syscalls/system.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment on lines +29 to +30
unsafe impl Send for BgaInfo {}
unsafe impl Sync for BgaInfo {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really need these impls? I guess Send is ok, since the framebuffer is not thread bound, but the struct isn't Sync in my view.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've gotten rid of this unclean way to circumvent a compiler error and have just changed the type of the address to usize. That way we can just get the pointer with exposed provenance in the systemcall.

Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
Comment thread src/arch/x86_64/kernel/bga.rs Outdated
@GloriousAlpaca
GloriousAlpaca force-pushed the pr-bga-clean branch 2 times, most recently from f8d1600 to c6a806c Compare August 4, 2026 14:45
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.

4 participants