Fixed most major issues but currently triple faulting at boot #13
Conversation
PinkQween
added
bug
|
Caution Review failedThe pull request is closed. WalkthroughThe changes overhaul the build process, bootloader (MBR and VBR), and project documentation. The Makefile is refactored for reliability and error handling. The MBR and VBR bootloaders are rewritten for direct disk access and robust booting. Supporting scripts are removed, and documentation is updated to reflect the new structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant MBR
participant VBR
participant Kernel
User->>Makefile: Run build/install
Makefile->>Makefile: Build boot.bin, kernel.bin, os.bin
Makefile->>os.bin: Copy boot and kernel at fixed offsets
User->>os.bin: Boot in QEMU/hardware
os.bin->>MBR: BIOS loads MBR (LBA 0)
MBR->>MBR: Print message, load VBR from LBA 2048
MBR->>VBR: Jump to VBR (0x7E00)
VBR->>VBR: Print message, set video mode, enter protected mode
VBR->>VBR: Load kernel from disk (ATA LBA)
VBR->>Kernel: Jump to kernel entry
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the objectives in the linked issues. Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/boot/mbr.asm (1)
23-25: Consider using the boot drive from BIOS instead of hard-coding.The drive number is hard-coded as 0x80. The BIOS typically passes the boot drive number in
dlregister at boot time.Consider preserving the boot drive number:
start: cli + ; Save boot drive number from BIOS + mov [boot_drive], dl xor ax, ax mov ds, ax mov es, ax mov ss, ax mov sp, 0x7C00 ; Set stack stiAnd use it for the disk read:
; Set up DAP (Disk Address Packet) for LBA read mov si, dap mov ah, 0x42 ; Extended read - mov dl, 0x80 ; Drive 0 + mov dl, [boot_drive]; Use saved boot drive int 0x13 jc failAdd at the end before the boot signature:
+boot_drive: db 0 + dw 0xAA55utilities/fat32_init.py (2)
17-27: Moveverify_vbr_writtenmethod after__init__for better code organization.Methods should be defined after
__init__following Python conventions.Move the
verify_vbr_writtenmethod after the__init__method:class FAT32Initializer: - def verify_vbr_written(self, partition_offset, vbr_sector): - """Verify that the VBR sector in the disk image matches the assembled vbr.bin""" - with open(self.image_path, 'rb') as f: - f.seek(partition_offset) - disk_vbr = f.read(512) - if disk_vbr != vbr_sector: - print("WARNING: VBR sector in disk image does not match assembled vbr.bin!") - else: - print("✓ Verified VBR sector matches assembled vbr.bin") def __init__(self, image_path, partition_start_lba=2048): self.image_path = image_path # ... rest of __init__ ... + + def verify_vbr_written(self, partition_offset, vbr_sector): + """Verify that the VBR sector in the disk image matches the assembled vbr.bin""" + with open(self.image_path, 'rb') as f: + f.seek(partition_offset) + disk_vbr = f.read(512) + if disk_vbr != vbr_sector: + print("WARNING: VBR sector in disk image does not match assembled vbr.bin!") + else: + print("✓ Verified VBR sector matches assembled vbr.bin")
36-36: Consider making sectors_per_fat configurable based on disk size.The hard-coded value of 512 sectors per FAT might not be optimal for all disk sizes.
The sectors per FAT should be calculated based on the total disk size and cluster count. Would you like me to provide a calculation method that determines the appropriate FAT size dynamically?
src/boot/vbr.asm (1)
77-82: Consider implementing VBE mode fallback for better compatibility.VBE mode 0x117 (1024x768x16) may not be supported on all hardware or emulators. Consider implementing a fallback mechanism to try multiple modes or skip graphics initialization if VBE fails.
start: - ; Try VBE Mode 0x117 (1024x768x16) - mov ax, 0x4F02 - mov bx, 0x117 - int 0x10 - cmp ax, 0x004F - jne vbe_error + ; Try VBE modes in order of preference + mov si, vbe_modes +.try_mode: + mov bx, [si] + test bx, bx + jz .vbe_skip ; No more modes, skip VBE + mov ax, 0x4F02 + int 0x10 + cmp ax, 0x004F + je .vbe_success + add si, 2 + jmp .try_mode +.vbe_skip: + ; Continue without VBE +.vbe_success:Add this data section:
vbe_modes: dw 0x117 ; 1024x768x16 dw 0x114 ; 800x600x16 dw 0x111 ; 640x480x16 dw 0 ; End marker
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
qemu.logis excluded by!**/*.log
📒 Files selected for processing (9)
Makefile(3 hunks)README.md(2 hunks)src/boot/mbr.asm(1 hunks)src/boot/vbr.asm(1 hunks)src/linker.ld(1 hunks)src/task/task.asm(2 hunks)updateBoot.sh(0 hunks)utilities/fat32_init.py(1 hunks)utilities/make_mbr_partition.py(0 hunks)
💤 Files with no reviewable changes (2)
- updateBoot.sh
- utilities/make_mbr_partition.py
🧰 Additional context used
🪛 GitHub Actions: Build OS
Makefile
[error] 1-1: make[1]: *** assets/programs/*/: No such file or directory. Stop.
[error] 29-29: make: i686-elf-gcc: No such file or directory. Failed to build ViOS standard library (Error 127).
🔇 Additional comments (8)
src/task/task.asm (2)
11-21: LGTM! Correct implementation of port-based serial output.The change from memory-mapped I/O to port I/O for COM1 serial output is properly implemented. Preserving
edxis necessary since it holds the port address.
60-70: Consistent implementation of port I/O for debug output.The second debug output section correctly mirrors the first implementation, maintaining consistency in the debugging mechanism.
src/linker.ld (1)
8-8: .text.start section usage verified
- Found
section .text.startinsrc/kernel.asm, confirming startup code is placed correctly.Approving the linker script change.
README.md (1)
193-193: Documentation correctly updated to reflect project structure changes.The README accurately reflects the addition of
qemu.logand the replacement ofmake_mbr_partition.pywithfat32_init.pyin the utilities directory.Also applies to: 322-329
src/boot/mbr.asm (1)
77-78: Verify the partition size calculation.The partition size is calculated as
2048 * 128sectors, which equals 262,144 sectors or 128 MB. Please confirm this is the intended partition size.src/boot/vbr.asm (1)
121-144: GDT setup looks correct.The GDT configuration with null, code, and data segments is properly structured for 32-bit protected mode operation.
Makefile (2)
126-146: Boot image creation process is well-structured.The build process correctly:
- Places VBR at LBA 2048 matching the MBR partition table
- Initializes FAT32 filesystem with Python utility
- Places kernel at LBA 3104 matching the VBR's read location
79-120: Excellent error handling in install targets.The install target improvements demonstrate good practices:
- Proper mount/unmount with error checking
- Cleanup on failure prevents resource leaks
- Clear error messages for debugging
- macOS script is particularly robust
utilities/fat32_init.py
Outdated
| def initialize_filesystem(self): | ||
| """Initialize the complete FAT32 filesystem, using assembled VBR binary""" | ||
| print(f"Initializing FAT32 filesystem in {self.image_path}") | ||
| vbr_bin_path = os.path.join(os.path.dirname(__file__), '../bin/vbr.bin') | ||
| vbr_sector = self.load_vbr_bin(vbr_bin_path) | ||
|
|
||
| # Open the disk image | ||
| with open(self.image_path, 'r+b') as f: | ||
| # Calculate partition offset | ||
| partition_offset = self.partition_start_lba * self.bytes_per_sector | ||
|
|
||
| # 1. Write VBR sector (sector 0 of partition) | ||
| f.seek(partition_offset) | ||
| f.write(vbr_sector) | ||
| print("✓ VBR sector written (from assembled vbr.bin)") | ||
| # Verify VBR sector | ||
| self.verify_vbr_written(partition_offset, vbr_sector) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations.
The file operations lack error handling which could lead to unclear failures.
Wrap file operations in try-except blocks:
def initialize_filesystem(self):
"""Initialize the complete FAT32 filesystem, using assembled VBR binary"""
print(f"Initializing FAT32 filesystem in {self.image_path}")
vbr_bin_path = os.path.join(os.path.dirname(__file__), '../bin/vbr.bin')
- vbr_sector = self.load_vbr_bin(vbr_bin_path)
+ try:
+ vbr_sector = self.load_vbr_bin(vbr_bin_path)
+ except (IOError, ValueError) as e:
+ print(f"Error loading VBR binary: {e}")
+ sys.exit(1)
# Open the disk image
- with open(self.image_path, 'r+b') as f:
+ try:
+ with open(self.image_path, 'r+b') as f:
+ # ... rest of the code ...
+ except IOError as e:
+ print(f"Error accessing disk image: {e}")
+ sys.exit(1)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In utilities/fat32_init.py around lines 111 to 128, the file operations for
opening and writing to the disk image lack error handling, which can cause
unclear failures. Wrap the entire file operation block, including opening the
file, seeking, writing, and verification, in a try-except block to catch
exceptions like IOError or OSError. In the except block, log or print a clear
error message indicating the failure to perform file operations on the disk
image, and optionally re-raise or handle the exception as needed.
src/boot/vbr.asm
Outdated
| mov ebx, eax | ||
|
|
||
| shr eax, 24 | ||
| or eax, 0xE0 | ||
| mov dx, 0x1F6 | ||
| out dx, al | ||
|
|
||
| ; === Kernel loading with INT 13h Extensions === | ||
| load_kernel: | ||
| mov edi, 0x100000 | ||
| mov eax, [kernel_start_sector] | ||
| mov cx, [kernel_size_sectors] | ||
|
|
||
| .read: | ||
| pusha | ||
| mov si, dap | ||
| mov dl, [boot_drive] | ||
| mov [dap+4], di | ||
| mov [dap+6], word 0 | ||
| mov [dap+8], eax | ||
| mov [dap+2], word 1 | ||
| mov ah, 0x42 | ||
| int 0x13 | ||
| popa | ||
| add edi, 512 | ||
| inc eax | ||
| dec cx | ||
| jnz .read | ||
| ret | ||
| mov eax, ecx | ||
| mov dx, 0x1F2 | ||
| out dx, al | ||
|
|
||
| ; === Enter protected mode === | ||
| enter_protected: | ||
| cli | ||
| mov eax, ebx | ||
| mov dx, 0x1F3 | ||
| out dx, al | ||
|
|
||
| ; Copy GDT to known address (0x0500) | ||
| lea si, [gdt_start] | ||
| mov di, 0x0500 | ||
| mov cx, gdt_end - gdt_start | ||
| rep movsb | ||
| mov dx, 0x1F4 | ||
| shr eax, 8 | ||
| out dx, al | ||
|
|
||
| ; Setup descriptor | ||
| mov word [gdtr], gdt_end - gdt_start - 1 | ||
| mov dword [gdtr + 2], 0x00000500 | ||
| mov dx, 0x1F5 | ||
| shr eax, 8 | ||
| out dx, al | ||
|
|
||
| lgdt [gdtr] | ||
| mov dx, 0x1F7 | ||
| mov al, 0x20 | ||
| out dx, al | ||
|
|
||
| call serial_send_G | ||
| .next_sector: | ||
| push ecx | ||
| .try_again: | ||
| mov dx, 0x1F7 | ||
| in al, dx | ||
| test al, 8 | ||
| jz .try_again | ||
|
|
||
| ; Enable protected mode | ||
| mov eax, cr0 | ||
| or eax, 1 | ||
| mov cr0, eax | ||
| mov ecx, 256 | ||
| mov dx, 0x1F0 | ||
| rep insw | ||
|
|
||
| jmp 0x08:protected_entry ; far jump | ||
| pop ecx | ||
| loop .next_sector | ||
| ret | ||
|
|
There was a problem hiding this comment.
Add proper ATA status checking to prevent read failures.
The ATA read routine lacks critical error checking that could cause boot failures:
- No BSY (busy) wait before sending commands
- No error bit checking after commands
- No drive ready verification
Add status checking:
ata_lba_read:
mov ebx, eax
+
+ ; Wait for drive to be ready
+ mov dx, 0x1F7
+.wait_ready:
+ in al, dx
+ test al, 0x80 ; Check BSY bit
+ jnz .wait_ready
shr eax, 24
or eax, 0xE0
mov dx, 0x1F6
out dx, al
+
+ ; Small delay for drive select
+ mov ecx, 4
+.delay:
+ in al, dx
+ loop .delayAnd after sending the read command:
mov dx, 0x1F7
mov al, 0x20
out dx, al
+
+ ; Check for errors
+ in al, dx
+ test al, 0x01 ; Check ERR bit
+ jnz .read_errorCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/boot/vbr.asm between lines 181 and 223, the ATA read routine is missing
essential status checks such as waiting for BSY to clear before issuing
commands, verifying the drive is ready, and checking for errors after commands.
To fix this, add a loop to wait until the BSY bit in the ATA status register
clears before sending commands, check the DRDY bit to confirm the drive is
ready, and after issuing the read command, verify the ERR bit is not set to
detect errors. Implement these status checks using appropriate in and test
instructions on port 0x1F7 before proceeding with data reads.
src/boot/vbr.asm
Outdated
| KernelSizeSectors dw 0x00F0 ; Kernel size in sectors (240 sectors = 120KB) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify kernel size is properly updated in the VBR.
The code uses a hardcoded KernelSizeSectors value of 240 sectors. The removed updateBoot.sh script suggests there was a mechanism to update this value, but it's unclear if this is now handled correctly.
Also applies to: 172-172
🏁 Script executed:
#!/bin/bash
# Check if there's a mechanism to update kernel size in VBR
# Look for scripts or utilities that might update the kernel size fields
echo "Searching for kernel size update mechanisms..."
fd -t f -e py -e sh | xargs rg -l "KernelSize|kernel.*size" -A 3
echo -e "\nChecking fat32_init.py for kernel size handling..."
cat utilities/fat32_init.py | rg -A 5 -B 5 "kernel|size"
echo -e "\nChecking if kernel size is calculated in Makefile..."
rg -A 3 -B 3 "kernel.*size|boot_with_size" MakefileLength of output: 2901
Implement dynamic kernel size patch in the VBR
The boot_with_size.bin target in the Makefile currently just copies boot.bin, leaving the fields in src/boot/vbr.asm at lines 53–54 hardcoded to zero/240 sectors. As a result, the VBR will not reflect the actual size of kernel.bin:
• src/boot/vbr.asm, lines 53–54
KernelSizeBytes dd 0x00000000 ; Kernel size in bytes
KernelSizeSectors dw 0x00F0 ; Kernel size in sectors• Makefile target for ./bin/boot_with_size.bin
./bin/boot_with_size.bin: ./bin/boot.bin ./bin/kernel.bin
@echo "Calculating kernel size and updating boot sector..."
cp ./bin/boot.bin ./bin/boot_with_size.bin
Action items to fix this:
- Calculate the actual kernel size at build time:
KERNEL_BYTES := $(shell wc -c < bin/kernel.bin) KERNEL_SECTORS := $(shell printf "%d" $$(( (KERNEL_BYTES + 511) / 512 )))
- Patch those values into
boot_with_size.binat the correct byte offsets (whereKernelSizeBytesandKernelSizeSectorsare emitted). You can use:
• a small shell/dd snippet in the Makefile
• a Python helper script (re-add or replace the oldupdateBoot.sh)
• NASM defines (-D KERNEL_SIZE_BYTES=… -D KERNEL_SIZE_SECTORS=…) when assemblingvbr.asm - Verify offsets against the assembled
vbr.binlayout and update the Makefile accordingly so that the VBR always contains the true kernel size.
This ensures the bootloader will read the correct sector count and avoid mismatches between the hardcoded 240 sectors and your actual kernel image.
🤖 Prompt for AI Agents
In src/boot/vbr.asm at lines 53-54, the KernelSizeBytes and KernelSizeSectors
fields are hardcoded and do not reflect the actual kernel.bin size. To fix this,
modify the Makefile target for boot_with_size.bin to calculate the kernel size
in bytes and sectors at build time using shell commands. Then patch these values
into the copied boot_with_size.bin at the correct byte offsets corresponding to
KernelSizeBytes and KernelSizeSectors, either via a shell/dd command, a Python
script, or by passing NASM defines during assembly. Ensure the offsets match the
assembled vbr.bin layout so the VBR contains the true kernel size for correct
bootloader operation.
|
|
||
|
|
||
| ./bin/kernel.bin: prepare_dirs $(FILES) | ||
| i686-elf-gcc $(FLAGS) -T ./src/linker.ld -o ./bin/kernel.bin -ffreestanding -O0 -nostdlib $(FILES) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document cross-compiler requirement and ensure CI has it installed.
The build requires i686-elf-gcc cross-compiler which is missing in the CI environment, causing pipeline failures.
Consider:
- Adding a prerequisites check target to the Makefile
- Documenting the cross-compiler requirement in README
- Ensuring the CI workflow installs the cross-compiler
Also applies to: 194-194
🏁 Script executed:
#!/bin/bash
# Check if CI workflow installs cross-compiler
echo "Checking for GitHub Actions workflow files..."
fd -t f -e yml -e yaml . .github/workflows | xargs cat | rg -A 5 -B 5 "i686-elf|cross-compiler|gcc"
echo -e "\nChecking for build documentation..."
fd README -t f | xargs rg -i "build|compiler|prerequisite|dependency" -A 3Length of output: 3605
Add CI installation for i686-elf-gcc and a prereqs check
- The README already documents the
i686-elf-gccrequirement under Build Requirements. - Update your GitHub Actions workflow (e.g.
.github/workflows/build.yml) to install the cross-compiler before runningmake. For example:- name: Install build dependencies run: | sudo apt-get update sudo apt-get install -y \ build-essential \ nasm \ qemu \ gcc-multilib \ binutils-i686-elf \ grub-pc-bin \ xorriso
- Add a
check-prereqstarget in theMakefileto fail early ifi686-elf-gccis missing:.PHONY: check-prereqs check-prereqs: @command -v i686-elf-gcc >/dev/null 2>&1 \ || { echo >&2 "Error: i686-elf-gcc not found. See README.md #build-requirements."; exit 1; } # Then invoke this at the top of your default target: all: check-prereqs kernel.bin
These changes will ensure the CI environment installs the cross-compiler and that local builds fail fast with a clear error.
🤖 Prompt for AI Agents
In the Makefile at line 124, add a new target named check-prereqs that checks if
i686-elf-gcc is installed by using the command -v i686-elf-gcc and exits with an
error message if not found. Then modify the default all target to depend on
check-prereqs so it runs this check before building kernel.bin. Additionally,
update the GitHub Actions workflow file (e.g., .github/workflows/build.yml) to
include a step that installs i686-elf-gcc and other required dependencies using
apt-get before running make, ensuring the CI environment has the necessary
cross-compiler installed.
| user_programs_clean: | ||
| @for dir in ./assets/etc/default/user/programs/*/ ; do \ | ||
| @for dir in ./assets/programs/*/ ; do \ | ||
| $(MAKE) -C $$dir clean || true; \ | ||
| done | ||
|
|
There was a problem hiding this comment.
Fix pipeline failure: Add directory existence check to user_programs_clean.
The wildcard pattern ./assets/programs/*/ fails when the directory doesn't exist, causing the pipeline error.
user_programs_clean:
- @for dir in ./assets/programs/*/ ; do \
- $(MAKE) -C $$dir clean || true; \
- done
+ @if [ -d "./assets/programs" ]; then \
+ for dir in ./assets/programs/*/ ; do \
+ if [ -d "$$dir" ]; then \
+ $(MAKE) -C $$dir clean || true; \
+ fi; \
+ done; \
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| user_programs_clean: | |
| @for dir in ./assets/etc/default/user/programs/*/ ; do \ | |
| @for dir in ./assets/programs/*/ ; do \ | |
| $(MAKE) -C $$dir clean || true; \ | |
| done | |
| user_programs_clean: | |
| @if [ -d "./assets/programs" ]; then \ | |
| for dir in ./assets/programs/*/ ; do \ | |
| if [ -d "$$dir" ]; then \ | |
| $(MAKE) -C $$dir clean || true; \ | |
| fi; \ | |
| done; \ | |
| fi |
🤖 Prompt for AI Agents
In the Makefile at lines 166 to 169, the user_programs_clean target uses a
wildcard pattern that fails if the directory ./assets/programs/ does not exist.
To fix this, add a check before the for loop to verify that the directory
exists. Only run the loop if the directory is present to prevent pipeline
failures caused by the missing directory.
- Updated the Makefile to change the directory structure for user programs, enhancing the build process. - Adjusted the LBA start value in mbr.asm for correct partitioning. - Modified kernel size in vbr.asm to reflect accurate sector allocation. - Added a simple BIOS print function in vbr.asm for debugging purposes. - Removed the obsolete fat32_init.py script to streamline the project structure.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/boot/vbr.asm (5)
53-54: The kernel size fields remain hardcoded and require dynamic patching.The
KernelSizeBytesandKernelSizeSectorsfields are still hardcoded (0x00000000 and 0x00E6 respectively) and don't reflect the actual kernel size at build time. This was identified in a previous review as needing dynamic patching.
193-200: Critical: Stack pointer (ESP) is still not initialized in 32-bit mode.The code sets all segment registers including SS but doesn't initialize ESP. This is the same critical issue identified in the previous review and will cause a triple fault when the first stack operation occurs.
Add ESP initialization after setting segment registers:
mov gs, ax mov ss, ax + mov esp, 0x90000 ; Set stack pointer to a safe location
207-211: Critical: Still copying from potentially uninitialized memory at 0x90000.The code continues to copy 512 bytes from 0x90000 to 0x20000 without any evidence that valid data was loaded at 0x90000. This was flagged in the previous review as potentially copying garbage data.
Either remove this copy if it's not needed, or ensure the data at 0x90000 is properly initialized before this point and document its purpose.
221-264: Critical: ATA read routine still lacks essential error checking.The ATA LBA read routine is missing the same critical status checks identified in the previous review:
- No BSY (busy) wait before sending commands
- No error bit checking after commands
- No drive ready verification
Add proper status checking as suggested in the previous review to prevent read failures that could contribute to the triple fault.
246-256: ATA command completion check is insufficient.The code only checks bit 3 (DRQ - Data Request) in the status register but doesn't verify that the command completed successfully or check for errors.
Add proper command completion and error checking:
.try_again: mov dx, 0x1F7 in al, dx + test al, 0x01 ; Check ERR bit + jnz .read_error + test al, 0x20 ; Check DF (Drive Fault) bit + jnz .read_error test al, 8 jz .try_againAnd add an error handler:
+.read_error: + ; Handle read error - could halt or retry + hlt
🧹 Nitpick comments (1)
src/boot/vbr.asm (1)
92-107: VBE mode setup may leave ES register in undefined state.The VBE mode query at line 94-96 sets ES to 0x090 but if the mode query fails and execution continues to the fallback, ES may not be properly reset for subsequent operations.
Consider saving and restoring ES or explicitly resetting it in the fallback path:
try_117_query: ; --- Fallback to VBE Mode 0x117 --- + xor ax, ax + mov es, ax mov ax, 0x4F01
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(3 hunks)README.md(2 hunks)src/boot/mbr.asm(1 hunks)src/boot/vbr.asm(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/boot/mbr.asm
- Makefile
…debugging - Modified the Makefile to ensure proper creation of the OS binary. - Updated mbr.asm to allocate two sectors for the VBR, correcting partitioning. - Enhanced vbr.asm with serial port initialization and debugging output to track boot progress. - Added screen clearing functionality in kernel.asm to indicate successful kernel loading. - Improved debugging capabilities by adding serial print routines in vbr.asm.
…d debugging - Updated the Makefile to streamline the creation of boot and OS binaries. - Adjusted the MBR and VBR assembly files for correct sector allocation and improved error handling. - Enhanced debugging output in the MBR and VBR to provide clearer boot progress messages. - Refined the kernel loading process in the VBR to ensure proper memory addressing and initialization.
… and debugging - Modified the Makefile to adjust sector allocation for boot binaries, ensuring correct installation of the custom boot sector. - Updated mbr.asm to clarify comments and correct LBA values for partitioning. - Enhanced vbr.asm with updated FAT32 parameters and improved debugging output for kernel loading. - Added debug messages in fat32.c to track partition access and FAT32 header reading, improving traceability during boot.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/boot/vbr.asm (2)
38-41: Kernel metadata fields need dynamic updating.The kernel size fields are hardcoded but should be dynamically updated by the build process. Based on past review comments, this was identified as an issue.
These hardcoded values need to be updated by the build process to reflect the actual kernel size, as noted in previous reviews.
197-240: ATA LBA read routine needs status checking.The ATA read routine is missing the critical status checks that were identified in past review comments. This could contribute to the triple fault issue.
The ATA read routine lacks essential status checks for BSY, DRDY, and ERR bits as noted in previous reviews. This omission could cause boot failures.
Makefile (2)
137-140: Boot size calculation target needs implementation.The
boot_with_size.bintarget currently just copiesboot.binwithout updating the kernel size fields. This was identified as an issue in past reviews.The boot_with_size.bin target needs to calculate and patch the actual kernel size into the boot sector, as noted in previous reviews.
125-126: Pipeline failure: Missing i686-elf-gcc cross-compiler.The build is failing because the CI environment doesn't have the i686-elf-gcc cross-compiler installed. This was identified in past reviews.
The CI environment needs i686-elf-gcc installed and a prerequisites check should be added to the Makefile, as noted in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)Makefile(3 hunks)README.md(2 hunks)src/boot/fsinfo.asm(1 hunks)src/boot/mbr.asm(1 hunks)src/boot/vbr.asm(1 hunks)src/fs/fat/fat32.c(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- README.md
- src/boot/fsinfo.asm
🚧 Files skipped from review as they are similar to previous changes (1)
- src/boot/mbr.asm
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/fs/fat/fat32.c (2)
src/debug/simple_serial.c (1)
simple_serial_puts(38-48)src/disk/streamer.c (2)
diskstreamer_seek(21-25)diskstreamer_read(27-59)
🪛 GitHub Actions: Build OS
Makefile
[error] 29-29: Build failed: 'i686-elf-gcc' command not found. Makefile target 'build/src/crt0.o' failed with error 127.
🔇 Additional comments (15)
src/fs/fat/fat32.c (3)
131-131: Good debugging addition for troubleshooting the boot process.Adding debug output at the entry point of
fat32_resolvewill help identify when the function is called and aid in debugging the triple fault issue mentioned in the PR.
164-181: Excellent debug output for signature verification.The hex conversion and signature comparison debug output is well-implemented and will be very helpful for diagnosing FAT32 detection issues. The manual hex conversion is appropriate given the simple serial output constraints.
193-194: Good debug output for function exit tracking.Adding debug output at the function exit point will help track the function's completion and return status during boot troubleshooting.
src/boot/vbr.asm (6)
7-36: Comprehensive FAT32 BPB implementation looks correct.The FAT32 BPB structure is well-implemented with proper field sizes and standard values. The OEM identifier "MSWIN4.1" is a common choice for compatibility. The partition start at LBA 2048 (HiddenSectors) aligns with the fat32.c changes.
62-104: VBE video mode setup is well-structured.The VBE setup with fallback from mode 0x17E to 0x117 and proper error handling is well-implemented. The clear screen functionality and proper segment register initialization are good practices.
118-136: Protected mode setup looks correct.The protected mode setup sequence is properly implemented with CLI/STI around critical sections, GDT loading, and CR0 manipulation. The far jump to 32-bit code is correctly structured.
138-165: GDT definition is properly structured.The GDT with null, code, and data segments is correctly defined with proper access rights and flags. The descriptor and segment constants are appropriately calculated.
1-4: VBR origin change correctly aligned with MBR loading
Verified that src/boot/mbr.asm uses a Disk Address Packet with offset 0x7E00 and far-jumps to 0x0000:0x7E00 after the INT 0x13 AH=0x42 call. TheORG 0x7E00andjmp near startin vbr.asm therefore match the MBR loader’s behavior—no changes required.
167-195: Verified: Kernel placement at LBA 2050The Makefile’s disk‐image recipe uses
dd if=./bin/kernel.bin of=./bin/os.bin bs=512 seek=2050 conv=notruncto write the kernel at LBA 2050, matching the bootloader’s
mov eax, 2050setup. No further changes required.Makefile (6)
72-72: Build target dependencies are correctly structured.The build target now properly depends on both
boot_with_size.binandkernel.bin, ensuring the complete boot image is built before proceeding.
84-87: Improved Linux install with better error handling.The Linux install target now includes proper error handling with conditional directory checks and cleanup on failure. The mount failure detection and umount on error are good practices.
90-121: Comprehensive macOS install script with robust error handling.The macOS install implementation using a bash script with proper error handling, disk attachment/detachment, and conditional file copying is well-structured. The use of
set -eensures the script fails fast on errors.
128-135: Boot binary creation process is well-structured.The new
boot.bintarget properly assembles MBR, VBR, and FSInfo components and combines them usingddat the correct offsets. This aligns with the bootloader changes.
142-147: OS binary creation aligns with bootloader changes.The OS binary creation properly places the kernel at sector 2050, which aligns with the VBR's kernel loading address. The padding with zeros creates a proper disk image size.
158-165: Conditional user programs building is well-implemented.The conditional check for the user programs directory prevents build failures when the directory doesn't exist. The error handling and informative messages are appropriate.
| // Seek to partition start (LBA 2048) instead of disk start (LBA 0) | ||
| // The partition starts at LBA 2048 according to the MBR | ||
| simple_serial_puts("DEBUG: Seeking to partition start at LBA 2048\n"); | ||
| if (diskstreamer_seek(stream, 2048 * 512) != VIOS_ALL_OK) | ||
| { | ||
| simple_serial_puts("DEBUG: Failed to seek to partition start\n"); | ||
| res = -EIO; | ||
| goto out; | ||
| } |
There was a problem hiding this comment.
Correct partition offset seek but verify error handling consistency.
The seek to LBA 2048 (partition start) correctly aligns with the bootloader changes that place the FAT32 partition at this location. However, there's an inconsistency in error handling - the function returns -EIO on seek failure but should return res for consistency with the rest of the function.
Apply this fix for consistent error handling:
- if (diskstreamer_seek(stream, 2048 * 512) != VIOS_ALL_OK)
- {
- simple_serial_puts("DEBUG: Failed to seek to partition start\n");
- res = -EIO;
- goto out;
- }
+ res = diskstreamer_seek(stream, 2048 * 512);
+ if (res != VIOS_ALL_OK)
+ {
+ simple_serial_puts("DEBUG: Failed to seek to partition start\n");
+ goto out;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Seek to partition start (LBA 2048) instead of disk start (LBA 0) | |
| // The partition starts at LBA 2048 according to the MBR | |
| simple_serial_puts("DEBUG: Seeking to partition start at LBA 2048\n"); | |
| if (diskstreamer_seek(stream, 2048 * 512) != VIOS_ALL_OK) | |
| { | |
| simple_serial_puts("DEBUG: Failed to seek to partition start\n"); | |
| res = -EIO; | |
| goto out; | |
| } | |
| // Seek to partition start (LBA 2048) instead of disk start (LBA 0) | |
| // The partition starts at LBA 2048 according to the MBR | |
| simple_serial_puts("DEBUG: Seeking to partition start at LBA 2048\n"); | |
| res = diskstreamer_seek(stream, 2048 * 512); | |
| if (res != VIOS_ALL_OK) | |
| { | |
| simple_serial_puts("DEBUG: Failed to seek to partition start\n"); | |
| goto out; | |
| } |
🤖 Prompt for AI Agents
In src/fs/fat/fat32.c around lines 146 to 154, the error handling after failing
to seek to the partition start currently returns -EIO directly, which is
inconsistent with the rest of the function. Modify the code to return the
variable res instead of -EIO to maintain consistent error handling throughout
the function.
| simple_serial_puts("DEBUG: Reading FAT32 header from partition start\n"); | ||
| if (diskstreamer_read(stream, &fat_private->header, sizeof(fat_private->header)) != VIOS_ALL_OK) | ||
| { | ||
| simple_serial_puts("DEBUG: Failed to read FAT32 header\n"); | ||
| res = -EIO; | ||
| goto out; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling consistency for header read.
The debug output is helpful, but the error handling should be consistent with the seek operation above.
Apply this fix to maintain consistent error handling:
- if (diskstreamer_read(stream, &fat_private->header, sizeof(fat_private->header)) != VIOS_ALL_OK)
- {
- simple_serial_puts("DEBUG: Failed to read FAT32 header\n");
- res = -EIO;
- goto out;
- }
+ res = diskstreamer_read(stream, &fat_private->header, sizeof(fat_private->header));
+ if (res != VIOS_ALL_OK)
+ {
+ simple_serial_puts("DEBUG: Failed to read FAT32 header\n");
+ goto out;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| simple_serial_puts("DEBUG: Reading FAT32 header from partition start\n"); | |
| if (diskstreamer_read(stream, &fat_private->header, sizeof(fat_private->header)) != VIOS_ALL_OK) | |
| { | |
| simple_serial_puts("DEBUG: Failed to read FAT32 header\n"); | |
| res = -EIO; | |
| goto out; | |
| } | |
| simple_serial_puts("DEBUG: Reading FAT32 header from partition start\n"); | |
| res = diskstreamer_read(stream, &fat_private->header, sizeof(fat_private->header)); | |
| if (res != VIOS_ALL_OK) | |
| { | |
| simple_serial_puts("DEBUG: Failed to read FAT32 header\n"); | |
| goto out; | |
| } |
🤖 Prompt for AI Agents
In src/fs/fat/fat32.c around lines 156 to 162, the error handling after failing
to read the FAT32 header should be consistent with the seek operation above.
Modify the error handling to include any necessary cleanup or state reset as
done in the seek operation, and ensure the error code and logging follow the
same pattern for consistency. Adjust the code to handle the error uniformly,
possibly by adding a cleanup label or resetting stream state before returning
the error.
- Adjusted whitespace for consistency and readability in the assembly code. - Reintroduced the call to clear the screen before VBE mode initialization to enhance visual feedback during boot. - Cleaned up unnecessary whitespace in the load_protected label for better code organization.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/boot/vbr.asm (6)
182-187: Remove or document the memory copy operation.This code copies 512 bytes from 0x90000 to 0x20000, but the comment mentions it's "temp kernel sector (VBR data)". Based on past review comments, this was identified as copying from potentially uninitialized memory.
The previous review comment about copying from potentially uninitialized memory at 0x90000 still applies. Either remove this operation if it's not needed, or ensure the data at 0x90000 is properly loaded and document its purpose.
190-195: Fix kernel LBA address to match disk layout.The kernel loading uses LBA 2050, but based on past review comments, this should match the disk image placement. The Makefile uses
seek=2049for kernel placement, creating a mismatch.The kernel LBA start should be 2049 to match the disk image placement (
dd ... seek=2049), not 2050. This mismatch could cause the bootloader to read the wrong sectors.
198-240: Add comprehensive ATA status checking to prevent read failures.The ATA read routine lacks critical error checking that could contribute to triple faults. It doesn't wait for drive readiness, check for errors, or handle timeout conditions.
As identified in previous reviews, the ATA read routine needs:
- BSY (busy) wait before sending commands
- Error bit checking after commands
- Drive ready verification
- Timeout handling for infinite loops
Add proper status checking before and after ATA commands to prevent system hangs and triple faults.
175-175: Initialize ESP properly in 32-bit mode.The code sets SS but doesn't initialize ESP in 32-bit mode, which was identified as a critical issue in past reviews that could cause triple faults.
Add ESP initialization after setting segment registers:
mov ss, ax + mov esp, 0x90000 ; Set stack pointer to a safe location
130-136: Add stack pointer initialization before protected mode entry.The code sets up segment registers but doesn't initialize ESP before entering protected mode. This creates a potential triple fault scenario when the first stack operation occurs in 32-bit mode.
Add ESP initialization after setting segment registers:
mov ss, ax mov sp, 0x7C00 + mov esp, 0x90000 ; Set 32-bit stack pointer sti
39-40: Ensure the Makefile’sboot_with_sizetarget actually patches the VBR kernel size fieldsThe
boot_with_sizerule (Makefile, lines 137–140) currently only copiesboot.bintoboot_with_size.binand never updates theKernelSizeBytes/KernelSizeSectorsentries in the VBR. You need to:
- Calculate the actual size of
bin/kernel.binin bytes and in 512-byte sectors.- Inject those values into
boot_with_size.binat the offsets defined by theKernelSizeBytesandKernelSizeSectorsdirectives insrc/boot/vbr.asm.Example patch in Makefile:
./bin/boot_with_size.bin: ./bin/boot.bin ./bin/kernel.bin @echo "Calculating kernel size and updating boot sector..." cp ./bin/boot.bin ./bin/boot_with_size.bin + @echo "Patching kernel size fields in boot_with_size.bin" + KERNEL_BYTES=$(wc -c < bin/kernel.bin) + KERNEL_SECTORS=$$(( (KERNEL_BYTES + 511) / 512 )) + # Replace <BYTE_OFFSET> and <SECTOR_OFFSET> with the byte offsets of + # KernelSizeBytes and KernelSizeSectors in the VBR + printf "%08x" $$KERNEL_BYTES \ + | xxd -r -p \ + | dd of=./bin/boot_with_size.bin bs=1 seek=<BYTE_OFFSET> conv=notrunc status=none + printf "%04x" $$KERNEL_SECTORS \ + | xxd -r -p \ + | dd of=./bin/boot_with_size.bin bs=1 seek=<SECTOR_OFFSET> conv=notrunc status=none
- Locate the correct
<BYTE_OFFSET>and<SECTOR_OFFSET>by examining whereKernelSizeBytes dd …andKernelSizeSectors dw …reside in the assembled VBR (e.g., usingobjdump -sorxxd).- After this change, builds will embed the true kernel size instead of relying on hardcoded defaults.
| ; Try VBE Mode 0x17E | ||
| mov ax, 0x4F01 | ||
| mov cx, 0x17E | ||
| mov bx, 0x090 | ||
| mov es, bx | ||
| xor di, di | ||
| int 0x10 | ||
| cmp ax, 0x004F | ||
| jne try_117_query | ||
|
|
||
| mov ax, 0x4F02 | ||
| mov bx, 0x17E | ||
| int 0x10 | ||
| cmp ax, 0x004F | ||
| jne try_117_query | ||
|
|
||
| jmp vbe_success |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for VBE mode information retrieval.
The VBE mode setup queries mode information at 0x090:0x0000 but doesn't validate the returned data structure. A malformed or missing VBE response could cause issues during mode setting.
Add validation after the mode info query:
int 0x10
cmp ax, 0x004F
jne try_117_query
+
+ ; Validate mode info structure
+ mov ax, 0x090
+ mov es, ax
+ mov bx, [es:0x00] ; Check mode attributes
+ test bx, 0x01 ; Check if mode supported
+ jz try_117_query📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ; Try VBE Mode 0x17E | |
| mov ax, 0x4F01 | |
| mov cx, 0x17E | |
| mov bx, 0x090 | |
| mov es, bx | |
| xor di, di | |
| int 0x10 | |
| cmp ax, 0x004F | |
| jne try_117_query | |
| mov ax, 0x4F02 | |
| mov bx, 0x17E | |
| int 0x10 | |
| cmp ax, 0x004F | |
| jne try_117_query | |
| jmp vbe_success | |
| ; Try VBE Mode 0x17E | |
| mov ax, 0x4F01 | |
| mov cx, 0x17E | |
| mov bx, 0x090 | |
| mov es, bx | |
| xor di, di | |
| int 0x10 | |
| cmp ax, 0x004F | |
| jne try_117_query | |
| ; Validate mode info structure | |
| mov ax, 0x090 | |
| mov es, ax | |
| mov bx, [es:0x00] ; Check mode attributes | |
| test bx, 0x01 ; Bit0 = mode supported? | |
| jz try_117_query | |
| mov ax, 0x4F02 | |
| mov bx, 0x17E | |
| int 0x10 | |
| cmp ax, 0x004F | |
| jne try_117_query | |
| jmp vbe_success |
🤖 Prompt for AI Agents
In src/boot/vbr.asm around lines 63 to 79, after calling the VBE mode info
function (int 0x10 with ax=0x4F01), add checks to validate the returned data
structure at ES:DI (0x090:0x0000). Verify that the returned mode info block has
the expected signature and reasonable values before proceeding. If validation
fails, jump to the error handling label (try_117_query) to avoid using invalid
mode information.
| push ecx | ||
| .try_again: | ||
| mov dx, 0x1F7 | ||
| in al, dx | ||
| test al, 8 | ||
| jz .try_again | ||
|
|
||
| ; Enable protected mode | ||
| mov eax, cr0 | ||
| or eax, 1 | ||
| mov cr0, eax | ||
| mov ecx, 256 | ||
| mov dx, 0x1F0 | ||
| rep insw | ||
|
|
||
| jmp 0x08:protected_entry ; far jump | ||
| pop ecx | ||
| loop .next_sector | ||
| ret | ||
|
|
There was a problem hiding this comment.
Add timeout protection to prevent infinite loops.
The .try_again loop at line 228-232 can hang indefinitely if the drive never becomes ready, potentially contributing to the triple fault issue.
Add timeout protection to prevent infinite loops:
.next_sector:
push ecx
+ mov ebx, 0x10000 ; Timeout counter
.try_again:
mov dx, 0x1F7
in al, dx
test al, 8
jz .check_timeout
+ jmp .data_ready
+.check_timeout:
+ dec ebx
+ jnz .try_again
+ ; Timeout - could add error handling here
+ jmp $ ; Hang for now
+.data_ready:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .next_sector: | |
| push ecx | |
| .try_again: | |
| mov dx, 0x1F7 | |
| in al, dx | |
| test al, 8 | |
| jz .try_again | |
| ; Enable protected mode | |
| mov eax, cr0 | |
| or eax, 1 | |
| mov cr0, eax | |
| mov ecx, 256 | |
| mov dx, 0x1F0 | |
| rep insw | |
| jmp 0x08:protected_entry ; far jump | |
| pop ecx | |
| loop .next_sector | |
| ret | |
| .next_sector: | |
| push ecx | |
| mov ebx, 0x10000 ; Timeout counter | |
| .try_again: | |
| mov dx, 0x1F7 | |
| in al, dx | |
| test al, 8 | |
| jz .check_timeout | |
| jmp .data_ready | |
| .check_timeout: | |
| dec ebx | |
| jnz .try_again | |
| ; Timeout expired – add real error handling if desired | |
| jmp $ ; Hang for now on timeout | |
| .data_ready: | |
| mov ecx, 256 | |
| mov dx, 0x1F0 | |
| rep insw | |
| pop ecx | |
| loop .next_sector | |
| ret |
🤖 Prompt for AI Agents
In src/boot/vbr.asm around lines 226 to 240, the .try_again loop waits
indefinitely for the drive to become ready, risking an infinite loop. Introduce
a timeout counter initialized before the loop and decrement it each iteration;
if the counter reaches zero, exit the loop or handle the timeout error to
prevent hanging. This ensures the loop will not run forever if the drive never
signals readiness.
…se help: issue right now is the jumping to the load32 to jump to kernel
Fixes #12
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
.text.startsections in the final binary.Documentation
Chores