Development #133
Merged
Development #133
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (12)
fs/BlockDevice.c (1)
41-41: Allow full device name capacityLine 41:
FastStrCopyalready handles the terminator by limiting the loop withi + 1 < max_len(see kernel/etc/StringOps.c Lines 17-23). Passingsizeof(dev->name) - 1therefore truncates one extra character unnecessarily. Please pass the full buffer size so device names can use the entire allotted space.- FastStrCopy(dev->name, name, sizeof(dev->name) - 1); + FastStrCopy(dev->name, name, sizeof(dev->name));fs/EXT/Ext2.h (1)
2-6: Duplicate include of BlockDevice.hBlockDevice.h is included twice. Drop the duplicate to avoid redundant compilation and potential cyclic-include fragility.
-#include "BlockDevice.h" -#include "stdint.h" - -#include "BlockDevice.h" +#include "BlockDevice.h" +#include "stdint.h"fs/MBR.c (2)
21-27: Add null-device guard before dereferencingParseMBR immediately dereferences device->name/id. Add a fast‑fail guard.
void ParseMBR(BlockDevice* device) { + if (!device) { + PrintKernelError("MBR: Null device passed to ParseMBR\n"); + return; + }
91-97: Consider composing nested offsets for robustnessIf partition devices ever stack (e.g., extended/logical), compose offsets rather than assume parent->lba_offset == 0.
- part_dev->lba_offset = p->lba_start; + part_dev->lba_offset = device->lba_offset + p->lba_start;fs/FileSystem.h (1)
12-16: Expose per‑filesystem ops to eliminate string‑based dispatchVFS currently branches on driver->name to call FAT/EXT2 functions. Add an ops table to FileSystemDriver so VFS can call through function pointers uniformly.
typedef struct FileSystemDriver { const char* name; DetectFunc detect; MountFunc mount; + // Optional: filesystem ops (populate per driver) + int (*read_file)(const char* path, void* buffer, uint32_t max_size); + int (*write_file)(const char* path, const void* buffer, uint32_t size); + int (*list_dir)(const char* path); + int (*create_file)(const char* path); + int (*create_dir)(const char* path); + int (*delete_path)(const char* path); + int (*is_file)(const char* path); + int (*is_dir)(const char* path); + uint64_t (*get_size)(const char* path); } FileSystemDriver;This enables VFS to dispatch without brittle name checks and reduces duplication. Update FAT1x/EXT2 drivers to fill these pointers.
fs/VFS.c (2)
122-128: Centralize filesystem dispatch; avoid driver->name string checksRepeated string comparisons for FAT1x/EXT2 increase risk and maintenance cost. After adding ops to FileSystemDriver, VFS can call mount->fs_driver->read_file/write_file/etc directly. Until then, consider a small helper that maps fs_driver->name once per mount to a vtable.
Also applies to: 148-154, 172-178, 192-198, 215-221, 235-241, 257-263, 299-305
18-28: Mount path creation: ignore FsMkdir error on existing pathFsMkdir likely returns -1 if the directory already exists. Consider ignoring that specific error to keep VfsMount idempotent.
- if (FastStrCmp(path, "/") != 0) { - FsMkdir(path); - } + if (FastStrCmp(path, "/") != 0) { + (void)FsMkdir(path); // ok if already exists + }fs/BlockDevice.h (2)
3-4: Use standard header guards or#pragma onceconsistentlyThe file uses
#pragma once(line 1) but also includes custom headers with quotes. Consider using angle brackets for system headers to maintain consistency:-#include "stdint.h" -#include "stdbool.h" +#include <stdint.h> +#include <stdbool.h>If these are project-specific headers, document this convention.
44-48: Add documentation for public API functionsThe BlockDevice API lacks documentation. Consider adding doxygen-style comments describing parameters, return values, and error conditions for each public function. This is particularly important for a core abstraction layer that other components will depend on.
Example documentation:
/** * Register a new block device in the system * @param type Device type identifier * @param block_size Size of each block in bytes * @param total_blocks Total number of blocks on the device * @param name Human-readable device name (max 31 chars + null) * @param driver_data Opaque pointer to driver-specific data * @param read Function pointer for reading blocks * @param write Function pointer for writing blocks * @return Pointer to registered BlockDevice, or NULL on failure */ BlockDevice* BlockDeviceRegister(...);fs/FAT/FAT1x.c (1)
227-227: Add null check after memory allocationWhile the code checks if
cluster_bufferis NULL, it would be clearer to handle this immediately after allocation with an appropriate error message.Consider adding after line 226:
uint8_t* cluster_buffer = KernelMemoryAlloc(cluster_bytes); -if (!cluster_buffer) return NULL; // Critical: Check allocation +if (!cluster_buffer) { + PrintKernel("FAT1x: Failed to allocate cluster buffer\n"); + return NULL; +}fs/EXT/Ext2.c (2)
84-99: Consider extracting common block I/O validation logicBoth
Ext2ReadBlockandExt2WriteBlockduplicate the bounds checking and sector calculation logic. Consider extracting a common validation function.Example refactor:
static int Ext2ValidateAndCalculateSectors(uint32_t block, uint32_t* num_sectors) { if (block >= volume.superblock.s_blocks_count) { PrintKernelF("EXT2: Block %u out of bounds (max: %u)", block, volume.superblock.s_blocks_count - 1); return -1; } *num_sectors = volume.block_size / 512; return 0; }
1039-1039: Add TODO tracking for indirect block handlingThe code has a TODO comment for handling indirect blocks in file deletion. This could lead to orphaned blocks if files larger than 48KB (12 direct blocks × 4KB) are deleted.
Would you like me to create an issue to track implementing indirect block handling for the delete operation? This is important for preventing filesystem corruption with larger files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
arch/x86_64/asm/pxs.asm(1 hunks)drivers/Ide.h(2 hunks)drivers/storage/AHCI.c(5 hunks)fs/BlockDevice.c(1 hunks)fs/BlockDevice.h(1 hunks)fs/EXT/Ext2.c(14 hunks)fs/EXT/Ext2.h(2 hunks)fs/FAT/FAT1x.c(16 hunks)fs/FAT/FAT1x.h(3 hunks)fs/FileSystem.c(1 hunks)fs/FileSystem.h(1 hunks)fs/MBR.c(1 hunks)fs/MBR.h(1 hunks)fs/VFRFS.c(2 hunks)fs/VFS.c(8 hunks)fs/VFS.h(2 hunks)kernel/core/InitRD.c(4 hunks)kernel/core/Kernel.c(4 hunks)kernel/core/Kernel.h(1 hunks)kernel/sched/MLFQ.c(2 hunks)meson.build(2 hunks)
🧰 Additional context used
🧠 Learnings (22)
📚 Learning: 2025-09-14T11:01:02.698Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#130
File: kernel/core/InitRD.c:25-35
Timestamp: 2025-09-14T11:01:02.698Z
Learning: In VoidFrame kernel's InitRD.c, multiboot modules should be mapped using VMemMap() with PAGE_PRESENT | PAGE_WRITABLE flags instead of VMemMapMMIO(), as the latter applies cache-disabling flags (PAGE_NOCACHE | PAGE_WRITETHROUGH) that can corrupt data when reading regular RAM content.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-09-01T02:35:28.353Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: fs/VFS.c:331-335
Timestamp: 2025-09-01T02:35:28.353Z
Learning: VfsIsFile function was initially implemented as a placeholder that only checked for mount existence. The proper implementation should follow the same pattern as VfsIsDir: find mount, strip mount path, then use filesystem-specific functions to check if the path points to a file (FS_FILE for RAMFS, Fat12 functions for FAT12).
Applied to files:
fs/VFS.hfs/FAT/FAT1x.cfs/VFS.c
📚 Learning: 2025-09-02T00:55:58.224Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.224Z
Learning: The VoidFrame MLFQ scheduler has a critical bug where ProcessRuntimePath uses FormatS() which returns a static buffer, causing all processes to share the same path string. This results in process directory cleanup operations affecting the wrong directories - killing one process can delete another process's /Runtime/Processes directory because they all point to the same FormatS static buffer.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-02T00:55:58.224Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.224Z
Learning: The VoidFrame MLFQ ProcessRuntimePath bug occurs because: 1) ProcessRuntimePath is defined as char* (not char array), 2) All processes get assigned the same FormatS static buffer pointer, 3) Directory cleanup via VfsDelete happens during TerminateProcess() not final cleanup, 4) When terminating a process, VfsDelete uses whatever path is currently in the shared static buffer, causing wrong directories to be deleted. Fix requires either changing ProcessRuntimePath to char array or using dynamic allocation.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-02T00:55:58.224Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.224Z
Learning: In VoidFrame's MLFQ.c, line 1076 assigns ProcessRuntimePath using FormatS() which returns a static buffer, causing all processes to share the same path pointer. During cleanup, VfsDelete() can delete the wrong process directory because all ProcessRuntimePath fields point to the same static buffer containing the path of the most recently created process. This causes directory cleanup operations to affect unintended processes.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-01T05:15:45.860Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#104
File: kernel/process/Process.c:1074-1075
Timestamp: 2025-09-01T05:15:45.860Z
Learning: In the VoidFrame kernel Process.c, the ProcessRuntimePath field uses FormatS() which returns a static buffer. While technically this creates a race condition where all processes could share the same path memory, the user has tested alternatives and they cause system failures. The current implementation works reliably in practice because the path is used immediately after assignment before FormatS() is called again.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-08-30T07:32:52.338Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#99
File: meson.build:103-103
Timestamp: 2025-08-30T07:32:52.338Z
Learning: FAT12 filesystem has a hard mathematical limit of 4,086 clusters due to 12-bit cluster addressing. This means FAT12 cannot handle disk sizes larger than approximately 4MB, making 128MB disks incompatible with FAT12 format. The user's meson.build was creating 128M disks with FAT12 format (`mkfs.fat -F 12`), which caused kernel panics when accessing files. The solution is to either use smaller disk sizes (≤4MB, preferably 1440K for traditional floppy compatibility) with FAT12, or use FAT16/FAT32 for larger disk sizes.
Applied to files:
fs/FAT/FAT1x.c
📚 Learning: 2025-08-17T08:16:43.539Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#93
File: kernel/memory/VMem.c:72-79
Timestamp: 2025-08-17T08:16:43.539Z
Learning: In VoidFrame kernel, VMemGetPhysAddr and related page table walking functions were causing panics because they assumed identity mapping existed for physical addresses < IDENTITY_MAP_SIZE (4GB), but the bootloader doesn't provide this 1:1 mapping. The fix is to remove all identity mapping assumptions and always use PHYS_TO_VIRT() for page table access in functions like VMemGetPageTablePhys, VMemMap, VMemMapHuge, VMemGetPhysAddr, VMemUnmap, VMemMapMMIO, and VMemUnmapMMIO.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-19T11:55:43.328Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#132
File: arch/x86_64/syscall/Syscall.c:12-19
Timestamp: 2025-09-19T11:55:43.328Z
Learning: In the VoidFrame kernel project, assembler-0 is implementing basic POSIX-compliant syscalls in ring 0 without user/kernel memory protection, prioritizing getting basic functionality working before adding full security features.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-13T07:26:56.892Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:26:56.892Z
Learning: VoidFrame kernel uses direct interrupt dispatch in arch/x86_64/interrupts/Interrupts.c rather than RegisterInterruptHandler pattern. Hardware interrupts are handled in switch cases by interrupt number (32+ for IRQs), with proper PIC EOI handling required.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-01T01:10:22.123Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.123Z
Learning: The current VoidFrame Registers struct in arch/x86_64/cpu/Cpu.h only saves basic GPRs, segment registers, and exception context (interrupt_number, error_code, rip, cs, rflags, rsp, ss). It's missing critical registers for deep fault tracing: SIMD registers (XMM/YMM/ZMM), control registers (CR0/CR2/CR3/CR4), debug registers (DR0-DR7), and extended FPU state. The struct has a "DO NOT TOUCH" comment indicating it must match interrupt ASM stack layout.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:32.713Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.713Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-31T14:19:11.366Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: docs/ARCHITECHTURE.md:1-3
Timestamp: 2025-08-31T14:19:11.366Z
Learning: assembler-0 wants to implement deep fault tracing for VoidFrame kernel exceptions (Page Fault, General Protection Fault, Invalid Opcode) instead of just panicking. The current interrupt handler has complete register state available in the Registers struct (defined in arch/x86_64/cpu/Cpu.h) including RBP for stack walking, RIP for fault location, and error codes for analysis.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:51.217Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.217Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-09T04:47:56.883Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.883Z
Learning: The VoidFrame kernel memory crash occurs when VIRT_ADDR_SPACE_START is set to 0xFFFF800000000000ULL instead of 0xFFFFFFFF80000000ULL, causing the heap to allocate 128TB away from where the kernel is mapped, resulting in page faults when accessing heap memory.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T02:19:46.948Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.948Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.c
🧬 Code graph analysis (18)
fs/VFRFS.c (1)
kernel/etc/StringOps.c (1)
FastStrCopy(18-24)
fs/MBR.h (1)
fs/MBR.c (1)
ParseMBR(21-106)
fs/BlockDevice.c (3)
kernel/etc/Console.c (2)
PrintKernel(174-192)PrintKernelInt(298-322)kernel/etc/StringOps.c (1)
FastStrCopy(18-24)fs/MBR.c (1)
ParseMBR(21-106)
fs/MBR.c (4)
kernel/etc/Console.c (4)
PrintKernel(174-192)PrintKernelInt(298-322)PrintKernelError(213-218)PrintKernelHex(228-251)mm/KernelHeap.c (2)
KernelMemoryAlloc(357-433)KernelFree(480-525)fs/BlockDevice.c (2)
BlockDeviceRead(64-70)BlockDeviceRegister(19-55)kernel/etc/Format.c (1)
FormatS(288-295)
kernel/core/InitRD.c (2)
kernel/etc/Console.c (2)
PrintKernelWarning(220-225)PrintKernelF(253-260)fs/VFS.c (1)
VfsWriteFile(141-163)
drivers/Ide.h (1)
drivers/Ide.c (2)
IdeReadBlocks(214-266)IdeWriteBlocks(268-318)
fs/FileSystem.h (1)
fs/FileSystem.c (3)
FileSystemInit(10-12)FileSystemRegister(14-23)FileSystemAutoMount(25-119)
fs/FileSystem.c (3)
kernel/etc/Console.c (3)
PrintKernel(174-192)PrintKernelInt(298-322)PrintKernelWarning(220-225)fs/BlockDevice.c (1)
BlockDeviceGet(57-62)kernel/etc/Format.c (1)
FormatA(280-286)
fs/VFS.h (1)
fs/VFS.c (1)
VfsMount(18-34)
fs/FAT/FAT1x.h (1)
fs/FAT/FAT1x.c (2)
Fat1xMount(34-78)Fat1xDetect(14-30)
fs/EXT/Ext2.h (1)
fs/EXT/Ext2.c (2)
Ext2Mount(103-175)Ext2Detect(29-63)
kernel/sched/MLFQ.c (2)
kernel/etc/Format.c (2)
FormatA(280-286)FormatS(288-295)fs/VFS.c (1)
VfsCreateDir(208-226)
fs/FAT/FAT1x.c (4)
fs/BlockDevice.c (2)
BlockDeviceRead(64-70)BlockDeviceWrite(72-78)mm/MemOps.c (2)
FastMemcmp(204-240)FastMemset(10-73)fs/VFS.c (1)
VfsMount(18-34)mm/KernelHeap.c (1)
KernelFree(480-525)
fs/EXT/Ext2.c (7)
kernel/etc/Console.c (6)
PrintKernel(174-192)PrintKernelInt(298-322)PrintKernelHex(228-251)PrintKernelF(253-260)PrintKernelSuccess(206-211)PrintKernelSuccessF(280-287)fs/BlockDevice.c (2)
BlockDeviceRead(64-70)BlockDeviceWrite(72-78)kernel/atomic/Spinlock.h (4)
WriteLock(121-134)WriteUnlock(136-147)ReadLock(100-111)ReadUnlock(113-119)kernel/sched/MLFQ.c (1)
MLFQGetCurrentProcess(1233-1238)mm/MemOps.c (1)
FastMemcpy(75-162)mm/KernelHeap.c (2)
KernelMemoryAlloc(357-433)KernelFree(480-525)fs/VFS.c (3)
VfsIsDir(250-269)VfsCreateDir(208-226)VfsMount(18-34)
kernel/core/Kernel.c (8)
fs/BlockDevice.c (1)
BlockDeviceInit(10-17)fs/FileSystem.c (2)
FileSystemInit(10-12)FileSystemAutoMount(25-119)kernel/etc/Console.c (3)
PrintKernel(174-192)PrintKernelSuccess(206-211)PrintKernelWarning(220-225)drivers/Ide.c (1)
IdeInit(115-212)drivers/storage/AHCI.c (1)
AHCI_Init(261-421)fs/VFS.c (1)
VfsInit(36-60)fs/VFRFS.c (1)
FsInit(77-91)kernel/core/InitRD.c (1)
InitRDLoad(10-69)
drivers/storage/AHCI.c (3)
mm/KernelHeap.c (2)
KernelMemoryAlloc(357-433)KernelFree(480-525)kernel/etc/Format.c (1)
FormatA(280-286)fs/BlockDevice.c (2)
BlockDeviceRegister(19-55)BlockDeviceDetectAndRegisterPartitions(80-86)
fs/VFS.c (6)
kernel/etc/StringOps.c (2)
FastStrCopy(18-24)FastStrCmp(43-53)fs/VFRFS.c (8)
FsMkdir(292-299)FsFind(129-180)FsOpen(185-213)FsWrite(240-261)FsClose(215-220)FsListDir(432-449)FsDeleteRecursive(368-381)FsDelete(321-331)fs/FAT/FAT1x.c (7)
Fat1xMount(34-78)Fat1xReadFile(602-641)Fat1xWriteFile(648-799)Fat1xListDirectory(427-498)Fat1xCreateFile(643-646)Fat1xIsDirectory(411-425)Fat1xGetFileSize(1019-1030)fs/FileSystem.c (1)
FileSystemRegister(14-23)fs/EXT/Ext2.c (6)
Ext2Mount(103-175)Ext2ReadFile(354-405)Ext2WriteFile(407-498)Ext2IsDir(1136-1153)Ext2IsFile(1117-1134)Ext2GetFileSize(1155-1172)mm/KernelHeap.c (2)
KernelMemoryAlloc(357-433)KernelFree(480-525)
fs/BlockDevice.h (1)
fs/BlockDevice.c (6)
BlockDeviceInit(10-17)BlockDeviceRegister(19-55)BlockDeviceGet(57-62)BlockDeviceRead(64-70)BlockDeviceWrite(72-78)BlockDeviceDetectAndRegisterPartitions(80-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
fs/VFS.c (1)
37-50: No need to call FileSystemInit here
FileSystemInit() is already invoked in kernel/core/Kernel.c:721 before any driver registration, so adding it in VFS.c is redundant.Likely an incorrect or invalid review comment.
fs/FAT/FAT1x.c (2)
14-30: LGTM! Clean device detection implementationThe FAT detection logic properly validates the boot sector signature and OEM name, following standard FAT filesystem detection patterns. Good use of early returns for clarity.
846-862: Good defensive programming with cycle detectionExcellent implementation of cycle detection in the cluster chain traversal to prevent infinite loops in corrupted filesystems. The visited clusters tracking with a fixed-size array is a reasonable approach.
fs/EXT/Ext2.c (2)
29-63: Comprehensive filesystem detection with proper loggingExcellent implementation of EXT2 detection with detailed debug output. The function correctly reads the superblock at the standard offset (sectors 2-3 for 1024 bytes starting at byte 1024) and validates the magic number. Good practice in logging both successful and failed detection attempts.
124-129: Good input validation for block sizeExcellent validation of the block size shift value to prevent integer overflow. The check for
s_log_block_size > 10ensures the block size stays within reasonable bounds (max 1MB blocks).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fs/EXT/Ext2.c(14 hunks)fs/MBR.c(1 hunks)kernel/core/Kernel.c(4 hunks)meson.build(5 hunks)
🧰 Additional context used
🧠 Learnings (20)
📚 Learning: 2025-08-17T08:16:43.539Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#93
File: kernel/memory/VMem.c:72-79
Timestamp: 2025-08-17T08:16:43.539Z
Learning: In VoidFrame kernel, VMemGetPhysAddr and related page table walking functions were causing panics because they assumed identity mapping existed for physical addresses < IDENTITY_MAP_SIZE (4GB), but the bootloader doesn't provide this 1:1 mapping. The fix is to remove all identity mapping assumptions and always use PHYS_TO_VIRT() for page table access in functions like VMemGetPageTablePhys, VMemMap, VMemMapHuge, VMemGetPhysAddr, VMemUnmap, VMemMapMMIO, and VMemUnmapMMIO.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-13T07:19:55.274Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:19:55.274Z
Learning: VMemUnmapMMIO in VoidFrame kernel should use the same page table access pattern as the working VMemUnmap function: `uint64_t* pt_table = (uint64_t*)(pt_phys + KERNEL_VIRTUAL_OFFSET);` rather than PHYS_TO_VIRT macro or identity mapping checks.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-31T02:11:29.291Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#100
File: kernel/memory/VMem.h:44-47
Timestamp: 2025-08-31T02:11:29.291Z
Learning: VoidFrame kernel's MAX_PAGES is hardcoded to exactly 4GB (1,048,576 pages), causing false "Memory detected exceeds MAX_PAGES" warnings when memory holes push the highest physical address slightly above 4GB. The solution is to sum only MULTIBOOT2_MEMORY_AVAILABLE regions instead of using max_physical_address for total_pages calculation.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-14T11:01:02.698Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#130
File: kernel/core/InitRD.c:25-35
Timestamp: 2025-09-14T11:01:02.698Z
Learning: In VoidFrame kernel's InitRD.c, multiboot modules should be mapped using VMemMap() with PAGE_PRESENT | PAGE_WRITABLE flags instead of VMemMapMMIO(), as the latter applies cache-disabling flags (PAGE_NOCACHE | PAGE_WRITETHROUGH) that can corrupt data when reading regular RAM content.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-19T11:55:43.328Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#132
File: arch/x86_64/syscall/Syscall.c:12-19
Timestamp: 2025-09-19T11:55:43.328Z
Learning: In the VoidFrame kernel project, assembler-0 is implementing basic POSIX-compliant syscalls in ring 0 without user/kernel memory protection, prioritizing getting basic functionality working before adding full security features.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-13T07:26:56.892Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:26:56.892Z
Learning: VoidFrame kernel uses direct interrupt dispatch in arch/x86_64/interrupts/Interrupts.c rather than RegisterInterruptHandler pattern. Hardware interrupts are handled in switch cases by interrupt number (32+ for IRQs), with proper PIC EOI handling required.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-01T01:10:22.123Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.123Z
Learning: The current VoidFrame Registers struct in arch/x86_64/cpu/Cpu.h only saves basic GPRs, segment registers, and exception context (interrupt_number, error_code, rip, cs, rflags, rsp, ss). It's missing critical registers for deep fault tracing: SIMD registers (XMM/YMM/ZMM), control registers (CR0/CR2/CR3/CR4), debug registers (DR0-DR7), and extended FPU state. The struct has a "DO NOT TOUCH" comment indicating it must match interrupt ASM stack layout.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:32.713Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.713Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-31T14:19:11.366Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: docs/ARCHITECHTURE.md:1-3
Timestamp: 2025-08-31T14:19:11.366Z
Learning: assembler-0 wants to implement deep fault tracing for VoidFrame kernel exceptions (Page Fault, General Protection Fault, Invalid Opcode) instead of just panicking. The current interrupt handler has complete register state available in the Registers struct (defined in arch/x86_64/cpu/Cpu.h) including RBP for stack walking, RIP for fault location, and error codes for analysis.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:51.217Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.217Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-09T04:47:56.883Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.883Z
Learning: The VoidFrame kernel memory crash occurs when VIRT_ADDR_SPACE_START is set to 0xFFFF800000000000ULL instead of 0xFFFFFFFF80000000ULL, causing the heap to allocate 128TB away from where the kernel is mapped, resulting in page faults when accessing heap memory.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T02:19:46.948Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.948Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-01T02:34:45.415Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: kernel/etc/Format.c:384-391
Timestamp: 2025-09-01T02:34:45.415Z
Learning: The user prefers FormatS over FormatA for performance reasons in kernel/etc/Format.c, accepting the thread-safety trade-off when the result is consumed immediately.
Applied to files:
fs/MBR.c
📚 Learning: 2025-09-02T00:55:58.224Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.224Z
Learning: In VoidFrame's MLFQ.c, line 1076 assigns ProcessRuntimePath using FormatS() which returns a static buffer, causing all processes to share the same path pointer. During cleanup, VfsDelete() can delete the wrong process directory because all ProcessRuntimePath fields point to the same static buffer containing the path of the most recently created process. This causes directory cleanup operations to affect unintended processes.
Applied to files:
fs/MBR.c
🧬 Code graph analysis (3)
kernel/core/Kernel.c (7)
fs/BlockDevice.c (1)
BlockDeviceInit(10-17)fs/FileSystem.c (2)
FileSystemInit(10-12)FileSystemAutoMount(25-119)drivers/Ide.c (1)
IdeInit(115-212)drivers/storage/AHCI.c (1)
AHCI_Init(261-421)fs/VFRFS.c (1)
FsInit(77-91)fs/VFS.c (1)
VfsInit(36-60)kernel/core/InitRD.c (1)
InitRDLoad(10-69)
fs/EXT/Ext2.c (7)
kernel/etc/Console.c (6)
PrintKernel(174-192)PrintKernelInt(298-322)PrintKernelHex(228-251)PrintKernelF(253-260)PrintKernelSuccess(206-211)PrintKernelSuccessF(280-287)fs/BlockDevice.c (2)
BlockDeviceRead(64-70)BlockDeviceWrite(72-78)kernel/atomic/Spinlock.h (4)
WriteLock(121-134)WriteUnlock(136-147)ReadLock(100-111)ReadUnlock(113-119)kernel/sched/MLFQ.c (1)
MLFQGetCurrentProcess(1233-1238)mm/MemOps.c (1)
FastMemcpy(75-162)mm/KernelHeap.c (2)
KernelMemoryAlloc(357-433)KernelFree(480-525)fs/VFS.c (2)
VfsCreateDir(208-226)VfsMount(18-34)
fs/MBR.c (4)
kernel/etc/Console.c (4)
PrintKernel(174-192)PrintKernelInt(298-322)PrintKernelError(213-218)PrintKernelHex(228-251)mm/KernelHeap.c (2)
KernelMemoryAlloc(357-433)KernelFree(480-525)fs/BlockDevice.c (2)
BlockDeviceRead(64-70)BlockDeviceRegister(19-55)kernel/etc/Format.c (1)
FormatA(280-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
fs/MBR.c (1)
7-19: Guard partition wrappers against missing parent ops
PartitionReadBlocks/PartitionWriteBlocksstill dereferencedevice->parent->read_blocks/write_blockswithout verifying the callbacks exist, so a read-only or partially initialised parent will crash every partition access. Please extend the guard to cover the function pointers.static int PartitionReadBlocks(BlockDevice* device, uint64_t start_lba, uint32_t count, void* buffer) { - if (!device || !device->parent) { + if (!device || !device->parent || !device->parent->read_blocks) { return -1; } return device->parent->read_blocks(device->parent, device->lba_offset + start_lba, count, buffer); } static int PartitionWriteBlocks(BlockDevice* device, uint64_t start_lba, uint32_t count, const void* buffer) { - if (!device || !device->parent) { + if (!device || !device->parent || !device->parent->write_blocks) { return -1; } return device->parent->write_blocks(device->parent, device->lba_offset + start_lba, count, buffer); }kernel/core/Kernel.c (1)
770-779: Defensive A20 probe must not clobber low memory or run before our page tablesThe new
A20Test()writes directly to physical 0x000000/0x0010 0000 while we’re still on the bootloader’s tables, reintroducing the exact crash/corruption we discussed earlier—many loaders leave page zero unmapped, and even when mapped you’re stomping the IVT and possibly kernel text without restoring it. Please defer the probe until after we switch to our higher-half page tables and make it non-destructive (map viaPHYS_TO_VIRT, save/restore the words).-void A20Test(void) { - volatile uint32_t *low = (uint32_t*)0x000000; - volatile uint32_t *high = (uint32_t*)0x100000; - - *low = 0x12345678; - *high = 0x87654321; - - if (*low == *high) PrintKernelWarning("A20 is disabled - memory is contiguous\n"); - else PrintKernelSuccess("A20 is enabled - memory is not contiguous\n"); -} - -asmlinkage void KernelMain(const uint32_t magic, const uint32_t info) { - ... - A20Test(); - ... -} +void A20Test(void) { + volatile uint32_t* low = (volatile uint32_t*)PHYS_TO_VIRT(0x000000); + volatile uint32_t* high = (volatile uint32_t*)PHYS_TO_VIRT(0x100000); + + uint32_t saved_low = *low; + uint32_t saved_high = *high; + + *low = 0x12345678; + *high = 0x87654321; + + const bool alias = (*low == *high); + + *low = saved_low; + *high = saved_high; + + if (alias) { + PrintKernelWarning("A20 is disabled - memory is contiguous\n"); + } else { + PrintKernelSuccess("A20 is enabled - memory is not contiguous\n"); + } +} + +asmlinkage void KernelMain(const uint32_t magic, const uint32_t info) { + ... + // defer A20Test until after we own the page tables +} + +void KernelMainHigherHalf(void) { + PrintKernelSuccess("System: Successfully jumped to higher half.\n"); + + A20Test(); + ... +}Also applies to: 789-790
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation
Build/Chores