Conversation
- changed workspace architecture: separated the kernel and builder - explicitly refresh the memory segments to prevent General Protection Faults on `iretq`
- Configured rust-analyzer target and resolved clippy warnings - Upgraded panic_handler to dump registers and stack traces - Moved BitmapFrameAllocator to .bss to resolve triple fault stack overflow - Implemented AcpiHandler to parse MADT via the acpi crate - Masked legacy 8259 PIC and enabled Local APIC via MMIO - Routed Keyboard (IRQ 1) through I/O APIC indirect registers - Rewrote interrupts.rs to send APIC-specific EOI signals - Refactored boot sequence into main.rs, lib.rs, fs/filesystem.rs and new apic.rs module
we can actually see the stack trace now:
[02:18] zen-zap@apeiron:debug (shikimori) | llvm-addr2line -e creo -fCi \
0x22d23 0x9708c 0x97052 0x24510 0x245e9 0x77474 0x7751d 0x773b2 0x255ad 0x245bd 0x52a6b 0x22986 0x23214
__rustc::rust_begin_unwind
/home/zen-zap/Code/blog_os/kernel/src/main.rs:213
??
??:0
??
??:0
<creo::framebuffer::GraphicsWriter>::new
/home/zen-zap/Code/blog_os/kernel/src/framebuffer.rs:33
<creo::framebuffer::GraphicsWriter as core::fmt::Write>::write_str
/home/zen-zap/Code/blog_os/kernel/src/framebuffer.rs:147
<&mut creo::framebuffer::GraphicsWriter as core::fmt::Write::write_fmt::SpecWriteFmt>::spec_write_fmt
/home/zen-zap/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:236
<creo::framebuffer::GraphicsWriter as core::fmt::Write>::write_fmt
/home/zen-zap/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:242
creo::framebuffer::_print::{closure#0}
/home/zen-zap/Code/blog_os/kernel/src/framebuffer.rs:165
x86_64::instructions::interrupts::without_interrupts::<creo::framebuffer::_print::{closure#0}, ()>
/home/zen-zap/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/x86_64-0.14.13/src/instructions/interrupts.rs:0
creo::framebuffer::_print
/home/zen-zap/Code/blog_os/kernel/src/framebuffer.rs:169
creo::apic::init
/home/zen-zap/Code/blog_os/kernel/src/print.rs:3
creo::kernel_main
/home/zen-zap/Code/blog_os/kernel/src/main.rs:118
??
??:0
the first ?? is probably from core library's formatting logic
the last ?? is before the main is called so there is nothing above it .. dead-end
|
Let me request a review from copilot .. let's see what it says. |
…n permissions okay this is nice Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR performs a major repo restructure and platform migration for the OS: it splits the project into a workspace (kernel + builder), migrates to bootloader_api v0.11.15, replaces legacy PIC-based interrupts with ACPI/APIC wiring, moves output from VGA text mode to a framebuffer-backed console, and introduces a bitmap-based physical frame allocator.
Changes:
- Converted the repo into a workspace with a new
kernelcrate and abuilderorchestration binary (replacing the prior single-crate layout and custom target JSON). - Migrated boot/runtime integration to
bootloader_apiv0.11.15, added framebuffer printing, and updated build/CI configuration forx86_64-unknown-none. - Reworked low-level subsystems: ACPI/APIC interrupt path, bitmap frame allocator, and filesystem init refactor.
Reviewed changes
Copilot reviewed 52 out of 63 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| x86_64-blog_os.json | Removed custom target JSON (now using x86_64-unknown-none). |
| rust-toolchain.toml | Fixed toolchain component name (rustfmt). |
| rust-analyzer.toml | Updated rust-analyzer target to x86_64-unknown-none. |
| README.md | Updated project overview and build/run instructions for the new workspace/builder flow. |
| Cargo.toml | Converted root crate into a workspace (kernel, builder) and adjusted profiles. |
| Cargo.lock | Updated lockfile for new workspace crates and dependencies (bootloader_api, acpi, etc.). |
| .cargo/config.toml | Set force-frame-pointers rustflags for target_os = none builds. |
| .github/workflows/build.yml | Reworked CI to build kernel + builder and do a basic boot run in QEMU. |
| .vscode/settings.json | Updated rust-analyzer settings and added cSpell words. |
| .idea/vcs.xml | Updated IntelliJ VCS mapping (currently includes user-specific path). |
| .idea/modules.xml | Updated IntelliJ module reference (now points to creo.iml). |
| .gitignore | Simplified ignore list (removed some previously ignored local files). |
| builder/Cargo.toml | Added new builder crate depending on bootloader 0.11.15. |
| builder/src/main.rs | Added orchestrator: builds kernel, produces BIOS/UEFI images, runs QEMU. |
| kernel/Cargo.toml | Added creo kernel crate with new dependencies (bootloader_api, acpi, framebuffer font). |
| kernel/.cargo/config.toml | Kernel-local target + build-std configuration for x86_64-unknown-none. |
| kernel/src/main.rs | New kernel entrypoint using bootloader_api, framebuffer init, APIC init, FS init, executor run. |
| kernel/src/lib.rs | New kernel library root, module exports, globals, init/hlt/exit-qemu. |
| kernel/src/acpi.rs | Added ACPI handler implementation for mapping physical tables via the bootloader offset. |
| kernel/src/apic.rs | Added ACPI/APIC init (disables PIC, enables Local APIC, programs I/O APIC for IRQ1). |
| kernel/src/interrupts.rs | New IDT + APIC EOI logic; IRQ vectors defined without legacy PIC. |
| kernel/src/gdt.rs | Updated GDT/TSS with ring0/ring3 selectors + RSP0 and IST setup. |
| kernel/src/memory.rs | Migrated memory map plumbing to bootloader_api and added bitmap frame allocator + user page mapping helper. |
| kernel/src/allocator.rs | Switched to fixed-size block allocator as global allocator (plus refactors/comments cleanup). |
| kernel/src/allocator/bump.rs | Added bump allocator implementation. |
| kernel/src/allocator/linked_list.rs | Added linked-list allocator implementation. |
| kernel/src/allocator/fixed_size_block.rs | Added fixed-size block allocator implementation with fallback heap. |
| kernel/src/framebuffer.rs | Added framebuffer-backed text rendering and integrated it into printing path. |
| kernel/src/print.rs | Added exported print!/println! macros targeting the framebuffer printer. |
| kernel/src/serial.rs | Formatting/cleanup of serial printing implementation. |
| kernel/src/scanc.rs | Added scancode-to-string helper (legacy/testing). |
| kernel/src/task/mod.rs | Added user task module and updated task module exports. |
| kernel/src/task/executor.rs | Updated executor: added idle sleep and waker constructor rename. |
| kernel/src/task/simple_executor.rs | Added a minimal FIFO executor implementation. |
| kernel/src/task/keyboard.rs | Added async scancode stream + helpers for shell/key tasks. |
| kernel/src/task/user.rs | Added inline-asm transition helper for entering ring 3. |
| kernel/src/shell/mod.rs | Reordered/updated shell module exports. |
| kernel/src/shell/shell_task.rs | Added async shell task with basic commands and FS command dispatch. |
| kernel/src/shell/fs_com.rs | Added async filesystem command helpers that release locks before printing. |
| kernel/src/shell/basic_com.rs | Added placeholder basic command module. |
| kernel/src/fs/mod.rs | Added filesystem module and updated FS module exports. |
| kernel/src/fs/filesystem.rs | Added filesystem initialization helper (VirtIO mount/format + optional self-check). |
| kernel/src/fs/block_dev.rs | Minor import reordering. |
| kernel/src/fs/simple_fs.rs | Multiple cleanups/refactors, minor behavior changes, and formatting improvements. |
| kernel/src/fs/layout.rs | Formatting/comment tweaks in on-disk layout definitions. |
| kernel/src/utils/tracing.rs | Updated crate naming in tracing docs. |
| kernel/src/utils/mod.rs | Reintroduced utils::tracing module for the kernel crate. |
| kernel/src/virtio/mod.rs | Switched VirtIO to use bitmap frame allocator and updated global allocator plumbing. |
| kernel/src/virtio/pci.rs | Reduced/cleaned PCI scan logging and kept VirtIO vendor scan. |
| kernel/src/vga_buffer.rs | Redirected print!/println! away from VGA text mode toward serial output (VGA tests removed). |
| src/main.rs | Removed legacy single-crate kernel entrypoint (now kernel/src/main.rs). |
| src/lib.rs | Removed legacy library root (now kernel/src/lib.rs). |
| src/interrupts.rs | Removed legacy PIC-based interrupt code (replaced by APIC-based kernel module). |
| src/gdt.rs | Removed legacy GDT implementation (replaced by kernel’s updated GDT). |
| src/task/keyboard.rs | Removed legacy async keyboard module (moved into kernel crate). |
| src/shell/shell_task.rs | Removed legacy shell task (moved into kernel crate). |
| src/shell/fs_com.rs | Removed legacy FS command module (replaced with async version in kernel crate). |
| src/utils/mod.rs | Removed legacy utils module stub (now kernel/src/utils/mod.rs). |
| tests/basic_boot.rs | Removed integration test (test harness removed per PR description). |
| tests/heap_allocation.rs | Removed integration test (test harness removed per PR description). |
| tests/should_panic.rs | Removed integration test (test harness removed per PR description). |
| tests/stack_overflow.rs | Removed integration test (test harness removed per PR description). |
Files not reviewed (2)
- .idea/modules.xml: Language not supported
- .idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn notify_apic_eoi() { | ||
| unsafe { | ||
| let phys_offset = PHYSICAL_MEMORY_OFFSET; | ||
|
|
||
| let local_apic_vaddr = 0xfee00000 + phys_offset; | ||
| // The EOI register is located at offset 0xB0 from the APIC base. | ||
| let eoi_ptr = (local_apic_vaddr + 0xB0) as *mut u32; | ||
|
|
||
| core::ptr::write_volatile(eoi_ptr, 0); |
There was a problem hiding this comment.
notify_apic_eoi hard-codes the Local APIC base to 0xfee00000, but apic::init reads the actual local_apic_address from ACPI. If the Local APIC isn’t at the default address, EOI will be written to the wrong MMIO location and interrupts can stop working. Consider storing the detected Local APIC physical address (or virtual base) in a global and using it here instead of a constant.
| //! in kernel/src/shell_task.rs | ||
| //! | ||
| //! File System Commands (FSC) | ||
| //! | ||
| //! This module provides a stateless utility for executing standard file system | ||
| //! commands (like `ls`, `cat`, `rm`). It acts as a bridge between the user shell | ||
| //! and the underlying Simple File System (SFS). |
There was a problem hiding this comment.
File header says in kernel/src/shell_task.rs, but this is kernel/src/shell/fs_com.rs. This is misleading when navigating the code; update the module-level doc comment to match the actual path/module.
| use crate::{GLOBALFSType, error, info, println}; | ||
| use alloc::vec::Vec; | ||
| use no_std_async::Mutex as AsyncMutex; | ||
| use spin::Mutex; |
There was a problem hiding this comment.
Unused imports (alloc::vec::Vec and spin::Mutex) appear to be left over here. Removing them will keep this module tidy and avoid relying on crate-level #![allow(unused)] to hide warnings.
| if BOOT_UEFI { | ||
| println!("Booting in UEFI mode"); | ||
| // uefi boot requires the ovmf firmware | ||
| cmd.arg("-bios").arg("OVMF.fd"); | ||
| cmd.arg("-drive").arg(format!("format=raw,file={}", uefi_path.display())); | ||
| } else { |
There was a problem hiding this comment.
UEFI boot uses -bios OVMF.fd, but this file is not in the repository and likely won’t exist on CI/other dev machines. This makes cargo run --bin builder fail in UEFI mode by default. Consider either (1) using the distro OVMF path (e.g., /usr/share/OVMF/OVMF_CODE.fd on Ubuntu), (2) making the path configurable via env/CLI flag, or (3) defaulting to BIOS unless an OVMF path is provided.
| - name: Cache Cargo Registry and Build | ||
| uses: Swatinem/rust-cache@v2 | ||
|
|
There was a problem hiding this comment.
The Swatinem/rust-cache@v2 step is duplicated (same name and action at lines 28-30 and 38-39). This is redundant and can be removed to shorten CI time and reduce confusion.
| - name: Cache Cargo Registry and Build | |
| uses: Swatinem/rust-cache@v2 |
| <component name="VcsDirectoryMappings"> | ||
| <mapping directory="" vcs="Git" /> | ||
| <mapping directory="$USER_HOME$/Code/blog_os" vcs="Git" /> | ||
| <mapping directory="$USER_HOME$/Code/creo" vcs="Git" /> |
There was a problem hiding this comment.
This adds a user-specific IntelliJ VCS mapping ($USER_HOME$/Code/creo). Committing machine-specific IDE paths makes the repo noisy and can confuse other developers. Consider removing this mapping (and ideally excluding .idea/ from version control) or keeping only project-relative mappings.
| <mapping directory="$USER_HOME$/Code/creo" vcs="Git" /> |
| // building the iretq stack frame (pushed in reverse order) | ||
| "push rcx", // SS (user data segment) | ||
| "push rdx", // RSP (user stack pointer) | ||
|
|
||
| // pushing RFLAGS, make sure interrupts are enabled (set bit 9) | ||
| "pushf", | ||
| "pop rax", | ||
| "or rax, 0x200", | ||
| "push rax", // RFLAGS | ||
| "push rdi", // CS (user code segment) | ||
| "push rsi", // RIP (user entry point) | ||
|
|
||
| "iretq", | ||
|
|
||
| in("rdi") code_selector, | ||
| in("rsi") entry_point, | ||
| in("rdx") stack_pointer, | ||
| in("cx") data_selector, |
There was a problem hiding this comment.
enter_user_mode binds code_selector/data_selector as u16 into 64-bit registers (rdi/cx) and then pushes rcx/rdi. With u16 operands the compiler may only write di/cx, leaving the upper bits of rdi/rcx undefined when doing 64-bit push instructions. It’s safer to explicitly zero-extend to 64-bit before pushing (e.g., pass selectors as u64 into rcx/rdi, or add movzx in the asm).
|
|
||
| unsafe { | ||
| let mut sivr = core::ptr::read_volatile(sivr_ptr); | ||
| sivr |= 0x100 | 0xFF; |
There was a problem hiding this comment.
sivr |= 0x100 | 0xFF sets the Local APIC spurious interrupt vector to 0xFF, but there is no IDT handler installed for vector 0xFF. If a spurious interrupt is delivered, this can lead to an unhandled interrupt (and potentially a double/triple fault). Consider picking a dedicated vector that has an IDT entry (and implementing a minimal spurious handler that just EOIs/returns).
| unsafe { | |
| let mut sivr = core::ptr::read_volatile(sivr_ptr); | |
| sivr |= 0x100 | 0xFF; | |
| const APIC_SIVR_SOFTWARE_ENABLE: u32 = 0x100; | |
| unsafe { | |
| let mut sivr = core::ptr::read_volatile(sivr_ptr); | |
| // Preserve the currently configured spurious interrupt vector here. | |
| // A dedicated vector should only be programmed once an IDT entry exists for it. | |
| sivr |= APIC_SIVR_SOFTWARE_ENABLE; |
| if offset + 2 < self.framebuffer.len() { | ||
| // offset + 2 since we writing 3 separate bytes into the 1D array of framebuffer | ||
| match self.info.pixel_format { | ||
| PixelFormat::Rgb => { | ||
| self.framebuffer[offset] = r; | ||
| self.framebuffer[offset + 1] = g; | ||
| self.framebuffer[offset + 2] = b; | ||
| }, | ||
| PixelFormat::Bgr => { | ||
| self.framebuffer[offset] = b; | ||
| self.framebuffer[offset + 1] = g; | ||
| self.framebuffer[offset + 2] = r; | ||
| }, | ||
| PixelFormat::U8 => { | ||
| // Grayscale fallback | ||
| let gray = (r as u32 + g as u32 + b as u32) / 3; | ||
| self.framebuffer[offset] = gray as u8; | ||
| }, | ||
| _ => {}, | ||
| } |
There was a problem hiding this comment.
set_pixel uses if offset + 2 < framebuffer.len() for all pixel formats, but for PixelFormat::U8 you only write a single byte at offset. This overly-strict bounds check can skip valid pixels near the end of the buffer in U8 mode. Consider making the bounds check depend on bytes_per_pixel/pixel format (e.g., require offset < len for U8, offset + 2 < len for RGB/BGR, etc.).
| if offset + 2 < self.framebuffer.len() { | |
| // offset + 2 since we writing 3 separate bytes into the 1D array of framebuffer | |
| match self.info.pixel_format { | |
| PixelFormat::Rgb => { | |
| self.framebuffer[offset] = r; | |
| self.framebuffer[offset + 1] = g; | |
| self.framebuffer[offset + 2] = b; | |
| }, | |
| PixelFormat::Bgr => { | |
| self.framebuffer[offset] = b; | |
| self.framebuffer[offset + 1] = g; | |
| self.framebuffer[offset + 2] = r; | |
| }, | |
| PixelFormat::U8 => { | |
| // Grayscale fallback | |
| let gray = (r as u32 + g as u32 + b as u32) / 3; | |
| self.framebuffer[offset] = gray as u8; | |
| }, | |
| _ => {}, | |
| } | |
| match self.info.pixel_format { | |
| PixelFormat::Rgb => { | |
| if offset + 2 < self.framebuffer.len() { | |
| // offset + 2 since we writing 3 separate bytes into the 1D array of framebuffer | |
| self.framebuffer[offset] = r; | |
| self.framebuffer[offset + 1] = g; | |
| self.framebuffer[offset + 2] = b; | |
| } | |
| }, | |
| PixelFormat::Bgr => { | |
| if offset + 2 < self.framebuffer.len() { | |
| // offset + 2 since we writing 3 separate bytes into the 1D array of framebuffer | |
| self.framebuffer[offset] = b; | |
| self.framebuffer[offset + 1] = g; | |
| self.framebuffer[offset + 2] = r; | |
| } | |
| }, | |
| PixelFormat::U8 => { | |
| if offset < self.framebuffer.len() { | |
| // Grayscale fallback | |
| let gray = (r as u32 + g as u32 + b as u32) / 3; | |
| self.framebuffer[offset] = gray as u8; | |
| } | |
| }, | |
| _ => {}, |
There was a problem hiding this comment.
our output is bgr if I'm correct ... not tested with other things
| <component name="ProjectModuleManager"> | ||
| <modules> | ||
| <module fileurl="file://$PROJECT_DIR$/.idea/blog_os.iml" filepath="$PROJECT_DIR$/.idea/blog_os.iml" /> | ||
| <module fileurl="file://$PROJECT_DIR$/.idea/creo.iml" filepath="$PROJECT_DIR$/.idea/creo.iml" /> |
There was a problem hiding this comment.
.idea/modules.xml now references .idea/creo.iml, but the repo still contains .idea/blog_os.iml and no creo.iml. This mismatch can break IntelliJ project loading for others. Either add/rename the .iml accordingly or avoid committing IDE module files.
| <module fileurl="file://$PROJECT_DIR$/.idea/creo.iml" filepath="$PROJECT_DIR$/.idea/creo.iml" /> | |
| <module fileurl="file://$PROJECT_DIR$/.idea/blog_os.iml" filepath="$PROJECT_DIR$/.idea/blog_os.iml" /> |
So this one includes some significant changes.
The first would be the separation of kernel and builder. Besides this, there are additional parameters added to the GDT along with a change in text mode.
Major Changes:
Minor changes:
x86_64-unknown-nonellvm-addr2lineAlso planned to improve virtio operations in this one. But, I found some things to do in my filesystem as well so interrupt driven virtio and filesystem changes will be for another time.
Also planning syscalls for that privilege separation thingy. Let's see. Whichever one I learn first goes in the next PR.
This is how it looks now: