Development #116
Development #116
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughMoves memory-protection initialization earlier in boot, fixes rwlock WriteUnlock underflow, calls Unsnooze earlier in panic handling, improves VFS copy error logging and zero-length handling, converts dev image to qcow2/ext2, adds E1000 and AHCI drivers, extends PCI helpers and BAR0, and introduces Generic PC‑speaker and Shell/help changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Boot as Boot
participant Kernel as Kernel (PXS2)
participant PIC as PIC
participant PIT as PIT
participant Mem as MemoryMgr
participant Guard as MemProtect
Boot->>Kernel: start()
Kernel->>PIC: Init()
PIC-->>Kernel: done
Kernel->>PIT: Init()
PIT-->>Kernel: done
alt VF_CONFIG_ENFORCE_MEMORY_PROTECTION
Kernel->>Mem: GetDetailedMemoryStats()
Mem-->>Kernel: stats
Kernel->>Kernel: Print memory health
Kernel->>Guard: StackGuardInit()
Kernel->>Guard: SetupMemoryProtection()
end
Kernel->>Kernel: continue device/subsystem init (PCI, NICs, AHCI, sound, filesystems)
sequenceDiagram
autonumber
participant Caller
participant VFS as VFS
participant FS as Filesystem
Caller->>VFS: VfsCopyFile(src, dst)
alt src invalid / not file
VFS-->>Caller: error (logged)
else src exists
alt src size == 0
VFS->>VFS: create destination
VFS-->>Caller: result (with log if failed)
else normal flow
VFS->>FS: read src (alloc buffer)
FS-->>VFS: data / error (logged)
VFS->>FS: write dst
FS-->>VFS: success / error (logged)
VFS-->>Caller: result
end
end
sequenceDiagram
autonumber
participant Kernel
participant PCI
participant E1000
Kernel->>PCI: scan bus
PCI->>E1000: detect E1000 device
E1000->>E1000: map MMIO, enable bus mastering, reset
E1000->>E1000: init RX/TX rings, read MAC, enable RX/TX
E1000-->>Kernel: initialized
Kernel->>E1000: E1000_SendPacket(data)
E1000->>E1000: wait TX desc, copy data, update tail
E1000-->>Kernel: success/fail
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel/atomic/Spinlock.h (1)
137-147: Silent unlock on recursion underflow can corrupt lock stateReleasing the writer lock when recursion <= 0 can free a lock owned by another writer or mask double-unlock bugs, leading to data races. Underflow should be treated as a hard error.
Apply:
static inline void WriteUnlock(rwlock_t* lock) { - if (lock->recursion <= 0) { - lock->recursion = 0; - lock->owner = 0; - __sync_lock_release(&lock->writer); - return; - } + if (__builtin_expect(lock->recursion <= 0 || !lock->writer, 0)) { + PrintKernelError("rwlock: WriteUnlock underflow or not held\n"); + PANIC("WriteUnlock underflow"); + } if (--lock->recursion == 0) { lock->owner = 0; __sync_lock_release(&lock->writer); } }If you prefer non-fatal in release builds, gate PANIC behind a debug macro and at minimum avoid changing lock state on underflow.
🧹 Nitpick comments (1)
meson.build (1)
17-17: mkfs.ext2 may be absent on some distros; add fallback or make non-requiredMany systems ship only mke2fs; prefer a fallback to mke2fs or mark mkfs.ext2 as required: false and resolve at use site.
Apply:
-mkfs_ext2 = find_program('mkfs.ext2') +mkfs_ext2 = find_program('mkfs.ext2', required : false) +if not mkfs_ext2.found() + mkfs_ext2 = find_program('mke2fs', required : true) +endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kernel/atomic/Spinlock.h(1 hunks)kernel/core/Kernel.c(2 hunks)kernel/core/Panic.c(1 hunks)meson.build(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
📚 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/Panic.ckernel/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/Panic.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/Panic.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/Panic.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 disable interrupts with "cli" as the very first operation, before any other code executes. If interrupts are enabled (after "sti"), an interrupt can fire between the panic call and the cli instruction in KernelPanicHandler, causing the panic to fail or behave unpredictably.
Applied to files:
kernel/core/Panic.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-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-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-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-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-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-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 (2)
kernel/core/Panic.c (1)
kernel/etc/Console.c (1)
Unsnooze(59-61)
kernel/core/Kernel.c (3)
kernel/etc/Console.c (2)
PrintKernel(181-199)PrintKernelWarning(227-232)mm/PMem.c (1)
GetDetailedMemoryStats(320-370)mm/StackGuard.c (1)
StackGuardInit(10-15)
🔇 Additional comments (3)
kernel/core/Panic.c (1)
66-66: Unsnooze earlier in panic path looks goodThis ensures console output isn't suppressed during panics when snooze was enabled; Unsnooze() is side‑effect free beyond a flag flip, so calling it pre‑guard is safe.
kernel/core/Kernel.c (2)
3-3: Include path adjustment is finePath aligns with existing inc_dirs that already include fs/FAT. No further action.
631-640: Enabling memory protection earlier is a good moveOrdering (after GDT/IDT/PIC/PIT init and before sti) is safe. StackGuardInit and CR0/CR4/EFER tweaks occur with interrupts still disabled later until KernelMainHigherHalf calls sti().
Please boot once with VF_CONFIG_ENFORCE_MEMORY_PROTECTION enabled on a CPU lacking SMAP/SMEP to confirm the “no features available” path stays stable and that IDE/PS2 init remain unaffected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
fs/VFS.c (6)
18-18: Name the lock for its scope.This lock serializes copy operations only; rename to vfs_copy_lock to avoid implying it protects the whole VFS.
479-483: Zero-length source: ensure destination semantics (create vs truncate).If dest exists, VfsCreateFile may not truncate across all backends. If the intended behavior is “dest becomes empty file,” consider truncating explicitly (if available) or writing zero bytes via VfsWriteFile to normalize behavior.
492-494: Augment allocation failure logs with size/context.Include the requested size to aid debugging (device, path, size). If convenient, prefer SerialWriteF for structured output here.
496-502: Fix log wording and keep consistent error messaging.Minor typos and clarity.
Apply:
- PrintKernelError("Failed to read sources\n"); + PrintKernelError("Failed to read source\n");
505-509: Clean up partial destination on write failure.Best-effort delete avoids leaving corrupt/partial files.
Apply:
if (bytes_written <= 0) { - PrintKernelError("Failed to write destination\n"); - WriteUnlock(&vfs_lock); + PrintKernelError("Failed to write destination file\n"); + VfsDelete(dest_path, false); // best-effort cleanup + WriteUnlock(&vfs_lock); return -1; // Failed to write destination file }
12-13: Decouple VFS from Scheduler: remove the#include "../kernel/sched/MLFQ.h"from fs/VFS.c and introduce aKernelGetCurrentOwnerId()helper (or a VFS-local stub returning 0 before the scheduler is up) instead of calling MLFQGetCurrentProcess() directly. Verified that VfsCopyFile is only ever called after VFS is initialized (in fs/VFS.c’s VfsMoveFile and in kernel/etc/Shell.c), so there’s no path invoking MLFQGetCurrentProcess() before scheduler startup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fs/VFS.c(2 hunks)meson.build(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- meson.build
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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.c
🧬 Code graph analysis (1)
fs/VFS.c (4)
kernel/atomic/Spinlock.h (2)
WriteLock(121-134)WriteUnlock(136-147)kernel/sched/MLFQ.c (1)
MLFQGetCurrentProcess(1232-1237)mm/KernelHeap.c (2)
KernelMemoryAlloc(272-330)KernelFree(377-422)kernel/etc/Console.c (1)
PrintKernelError(220-225)
⏰ 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/VFS.c (2)
470-475: Lock/unlock discipline on early-exit looks solid.WriteLock with PID-based recursion plus explicit WriteUnlock on the non-file path is correct and prevents underflow with the updated WriteUnlock.
510-511: LGTM on unlock-on-success.The happy path releases the lock exactly once; recursion remains balanced.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
kernel/etc/Shell.c (2)
671-690: Memory leak: recursive flag from GetArg(args, 2) is never freed in rmrecursive is allocated but not freed on any path.
static void RmHandler(const char * args) { char* name = GetArg(args, 1); char* recursive = GetArg(args, 2); if (name) { char full_path[256]; ResolvePath(name, full_path, 256); if (recursive && FastStrCmp(recursive, "-r") == 0) { if (VfsDelete(full_path, true) == 0) PrintKernel("Removed\n"); else PrintKernel("Failed to remove (file not found or directory not empty)\n"); - KernelFree(name); + KernelFree(name); + KernelFree(recursive); return; } if (VfsDelete(full_path, false) == 0) PrintKernel("Removed\n"); else PrintKernel("Failed to remove (file not found or directory not empty)\n"); - KernelFree(name); + KernelFree(name); + if (recursive) KernelFree(recursive); } else { PrintKernel("Usage: rm <filename>\n"); - KernelFree(name); + KernelFree(name); + if (recursive) KernelFree(recursive); } }
884-909: Implement subleaf-aware CPUID calls in PrintCacheInfoYou’re calling
cpuid(4,…)with ECX=0 and then overwriting EAX with the loop index, so you only ever query subleaf 0. Introduce (or implement) a helper that accepts both leaf and subleaf (ECX) and update the loop accordingly:void PrintCacheInfo() { uint32_t eax, ebx, ecx, edx; - for (int i = 0; ; ++i) { - cpuid(4, &eax, &ebx, &ecx, &edx); - eax = i; // Subleaf - uint32_t cache_type = eax & 0x1F; + for (uint32_t i = 0; ; ++i) { + cpuid_ex(4, i, &eax, &ebx, &ecx, &edx); // ECX = subleaf = i + const uint32_t cache_type = eax & 0x1F; if (cache_type == 0) { break; } @@ - PrintKernelF("L%u cache (%s): %u KB\n", - cache_level, type_str, cache_size / 1024); + PrintKernelF("L%u cache (%s): %u KB\n", + cache_level, type_str, cache_size / 1024); } }Implement
cpuid_ex(uint32_t leaf, uint32_t subleaf, uint32_t* eax, uint32_t* ebx, uint32_t* ecx, uint32_t* edx)if it doesn’t already exist.drivers/ISA/ISA.c (2)
88-96: Also guard against bitmap overflow when validating I/O rangesGood call checking the I/O bitmap before probing. Add an upper-bound check to prevent indexing past
g_isa_bus.io_bitmapifio_base + io_sizeexceedsISA_IO_END(watch 16-bit overflow on the sum).- if (!IsaCheckPortRange(io_base, io_size)) { + // Bounds + conflict checks + if ((uint32_t)io_base + (uint32_t)io_size > ISA_IO_END) { + PrintKernelError("ISA: I/O range out of bounds\n"); + return -2; + } + if (!IsaCheckPortRange(io_base, io_size)) { PrintKernelWarningF("ISA: I/O conflict for %s at 0x%X\n", name, io_base); return -2; }
80-119: Track IRQ/DMA ownership like I/O portsYou now prevent I/O contention; do the same for IRQ/DMA (simple bitmaps + conflict checks) to avoid hard-to-debug spurious interrupts or broken DMA transfers when multiple ISA devices are enabled.
Happy to draft minimal
irq_bitmap/dma8_bitmap/dma16_bitmaphelpers analogous toIsaAllocatePortRange/IsaFreePortRangeif you want.
🧹 Nitpick comments (9)
kernel/etc/Shell.c (6)
152-156: Prefer size_t for sizes/counts in HelpEntry/HelpCategoryUse size_t for fields that represent sizes to avoid signed overflow and match sizeof semantics.
Apply:
-typedef struct { - const char* name; - const char* desc; -} HelpEntry; +typedef struct { + const char* name; + const char* desc; +} HelpEntry; -typedef struct { - const char* category; - const HelpEntry* entries; - int count; -} HelpCategory; +typedef struct { + const char* category; + const HelpEntry* entries; + size_t count; +} HelpCategory;Also applies to: 157-162
176-189: Include the existing 'filesize' command in help to avoid confusionThere's both size and filesize handlers; help lists only size. Add filesize alias to the Files section.
static const HelpEntry file_cmds[] = { {"ls [path]", "List directory contents"}, {"cd <dir>", "Change directory"}, {"pwd", "Print working directory"}, {"cat <file>", "Display file contents"}, {"edit <file>", "Edit file"}, {"mkdir <name>", "Create directory"}, {"touch <name>", "Create empty file"}, {"rm <file> [-r]", "Remove file/directory"}, {"cp <src> <dest>", "Copy file"}, {"mv <src> <dest>", "Move file"}, {"echo <text> <file>", "Write text to file"}, - {"size <file>", "Get file size"} + {"size <file>", "Get file size"}, + {"filesize <file>", "Get file size (alias)"} };
203-214: Add 'mkfs' to Development helpmkfs exists as a command but isn’t listed here.
static const HelpEntry dev_cmds[] = { {"elfload <path>", "Load ELF executable"}, {"alloc <size>", "Allocate memory"}, {"panic <msg>", "Trigger panic"}, {"vmemfreelist", "Show VMem free list"}, {"heapvallvl <0/1/2>", "Set heap validation level"}, {"fstest", "Run filesystem tests"}, {"arptest", "Perform ARP test"}, {"setup", "Copy system files"}, - {"isocp <iso> <vfs>", "Copy from ISO to VFS"} + {"isocp <iso> <vfs>", "Copy from ISO to VFS"}, + {"mkfs", "Initialize VFS layout (create rootfs directories)"} };
222-235: Reduce lock churn and simplify alignment in ShowHelpCategoryUse a single formatted print per entry; avoids multiple PrintKernel calls and manual padding.
static void ShowHelpCategory(int cat_idx) { const HelpCategory* cat = &help_categories[cat_idx]; PrintKernelSuccess(cat->category); PrintKernel(" Commands:\n"); - for (int i = 0; i < cat->count; i++) { - PrintKernel(" "); - PrintKernel(cat->entries[i].name); - int pad = 20 - StringLength(cat->entries[i].name); - for (int j = 0; j < pad && j < 20; j++) PrintKernel(" "); - PrintKernel("- "); - PrintKernel(cat->entries[i].desc); - PrintKernel("\n"); - } + for (size_t i = 0; i < cat->count; i++) { + PrintKernelF(" %-20s- %s\n", + cat->entries[i].name, + cat->entries[i].desc); + } }
238-290: Make help category matching case-insensitive and support common aliasesAvoid looping with i checks; normalize once, then route. Keeps logic simple and user-friendly.
- char* category = GetArg(args, 1); - int num_categories = sizeof(help_categories)/sizeof(HelpCategory); + char* category = GetArg(args, 1); + const int num_categories = (int)(sizeof(help_categories)/sizeof(HelpCategory)); @@ - // Check if it's a number - int cat_num = atoi(category); - if (cat_num > 0 && cat_num <= num_categories) { - ShowHelpCategory(cat_num - 1); - KernelFree(category); - return; - } + // Check if it's a number + const int cat_num = atoi(category); + if (cat_num > 0 && cat_num <= num_categories) { + ShowHelpCategory(cat_num - 1); + KernelFree(category); + return; + } @@ - // Check category names - for (int i = 0; i < num_categories; i++) { - if (FastStrCmp(category, "system") == 0 && i == 0) { - ShowHelpCategory(i); - KernelFree(category); - return; - } - if (FastStrCmp(category, "files") == 0 && i == 1) { - ShowHelpCategory(i); - KernelFree(category); - return; - } - if (FastStrCmp(category, "hardware") == 0 && i == 2) { - ShowHelpCategory(i); - KernelFree(category); - return; - } - if (FastStrCmp(category, "dev") == 0 && i == 3) { - ShowHelpCategory(i); - KernelFree(category); - return; - } - } + // Normalize to lowercase + for (char* p = category; *p; ++p) { + if (*p >= 'A' && *p <= 'Z') *p = (char)(*p + ('a' - 'A')); + } + // Direct name/alias mapping + if (FastStrCmp(category, "system") == 0 || FastStrCmp(category, "sys") == 0) { + ShowHelpCategory(0); + } else if (FastStrCmp(category, "files") == 0 || FastStrCmp(category, "fs") == 0) { + ShowHelpCategory(1); + } else if (FastStrCmp(category, "hardware") == 0 || FastStrCmp(category, "hw") == 0) { + ShowHelpCategory(2); + } else if (FastStrCmp(category, "development") == 0 || FastStrCmp(category, "dev") == 0) { + ShowHelpCategory(3); + } else { + PrintKernel("Unknown category. Use 'help' to see available categories.\n"); + KernelFree(category); + return; + } - - PrintKernel("Unknown category. Use 'help' to see available categories.\n"); - KernelFree(category); + KernelFree(category);
966-1025: Typo in mkfs message: 'roofs' → 'rootfs'Minor user-facing text fix.
- PrintKernel("VFS: Creating roofs on /Devices/Storage/VFSystemDrive...\n"); + PrintKernel("VFS: Creating rootfs on /Devices/Storage/VFSystemDrive...\n");drivers/ISA/ISA.c (1)
141-164: Probe helper ignores io_size; either use it or drop param
IsaProbeDevice(..., io_size)reads only 4 bytes regardless ofio_size. Either iterate up tomin(io_size, 4)or remove the unused parameter to avoid confusion. Reads from “write-only” registers can have side effects on some HW; keeping the probe minimal is good, but please align signature with behavior.drivers/sound/SB16.c (2)
36-38: Ensure speaker is disabled on early returnsWrap the body with a small helper or
goto outpath so0xD3is always issued if any write fails mid‑beep.Also applies to: 50-51
6-33: Probe: small nitsThe reset/read sequence is fine. Consider shortening the poll or making the timeout configurable; also, some cards require a brief read‑buffer flush before checking for 0xAA, but this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
drivers/ISA/ISA.c(1 hunks)drivers/sound/SB16.c(1 hunks)fs/VFS.c(1 hunks)kernel/etc/Shell.c(1 hunks)meson.build(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- meson.build
- fs/VFS.c
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code graph analysis (3)
kernel/etc/Shell.c (4)
kernel/etc/Console.c (3)
PrintKernelSuccess(213-218)PrintKernel(181-199)PrintKernelInt(314-338)kernel/etc/StringOps.c (2)
StringLength(3-8)FastStrCmp(42-52)mm/KernelHeap.c (1)
KernelFree(377-422)include/stdlib.h (1)
atoi(18-44)
drivers/sound/SB16.c (1)
drivers/sound/SB16.h (1)
dsp_write(21-31)
drivers/ISA/ISA.c (1)
kernel/etc/Console.c (2)
PrintKernelSuccess(213-218)PrintKernel(181-199)
⏰ 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)
kernel/etc/Shell.c (4)
163-175: System help entries look goodCommand names and descriptions align with handlers.
191-202: Hardware help entries match handlersNo issues spotted.
215-221: Category registry is clear and extensibleCounts derived via sizeof keep it correct when entries change.
1152-1186: Potential leak depending on FormatS semanticsIf FormatS allocates, the two calls in the VfsIsFile branch aren’t freed. Consider building the path once and freeing it after use.
- if (VfsIsFile(FormatS("%s/%s", DataDir, cmd_name))) { - const char* full = FormatS("%s/%s", DataDir, cmd_name); + const char* full = FormatS("%s/%s", DataDir, cmd_name); + if (VfsIsFile(full)) { const ElfLoadOptions opts = { @@ - KernelFree(cmd_name); + KernelFree(cmd_name); + /* if FormatS allocates, also free 'full' here */ return; // avoid also running a built-in with the same name } + /* if FormatS allocates, free 'full' here as well */Please confirm whether FormatS returns a heap allocation or a static buffer; if heap, I can send a precise patch that frees it in all paths.
drivers/ISA/ISA.c (1)
166-171: Confirm SB16 DMA resource trackingMeson’s QEMU invocation (meson.build:268) supplies both dma=1 and dma16=5 for the Sound Blaster 16, but IsaRegisterDevice (drivers/ISA/ISA.c:166–171) only records a single
uint8_t dmachannel. If your SB16 implementation will use 16-bit DMA (channel 5) alongside 8-bit DMA, extend the device struct andIsaRegisterDeviceAPI to store both channels.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
drivers/ethernet/realtek/RTL8139.c (2)
105-107: TX uses virtual address for DMA; convert to physical.
tx_phys_addris set from the virtual buffer pointer. The NIC needs a physical address; RX path already does this correctly.Apply:
- uint32_t tx_phys_addr = (uint32_t)tx_buffer; + uint32_t tx_phys_addr = (uint32_t)VIRT_TO_PHYS(tx_buffer);
71-75: Avoid relying on identity mapping for MMIO/DMA.Comment still states “Assuming identity mapping”. Elsewhere in the codebase we’ve been moving away from that assumption. Consider mapping with VMemMapMMIO and always converting buffers via VMemGetPhysAddr/VIRT_TO_PHYS.
🧹 Nitpick comments (11)
drivers/PCI/PCI.h (1)
38-41: 16-bit config helpers are a useful addition.API naming is slightly asymmetric (
PciReadConfig16vsPciConfigReadDWord), but functionally fine.drivers/PCI/PCI.c (3)
55-64: LGTM: 16-bit write via RMW of aligned dword.Consider IRQ masking if you foresee concurrent config writes in future.
105-107: Reading BAR0 during scan is fine.Follow-up: detect I/O vs MMIO and 64-bit BAR pairs when you expand MMIO sizing/mapping.
202-278: BAR sizing helper needs I/O-BAR handling and tighter safety.Current logic assumes MMIO (clears lower 4 bits). For I/O BARs, mask should clear lower 2 bits; also consider disabling MEM/IO decode during probing per PCI spec.
Apply (illustrative):
- // Clear the lower 4 bits (they're not part of the address) - size_mask &= 0xFFFFFFF0; + // Clear attribute bits based on BAR type + if ((original_bar & 0x1) == 0x1) { + // I/O BAR + size_mask &= 0xFFFFFFFC; + } else { + // MMIO BAR + size_mask &= 0xFFFFFFF0; + }kernel/core/Kernel.c (1)
632-641: Moving memory-protection earlier is sensible; small nit.This runs after PIC/PIT init but before input/shell. Consider masking interrupts during CR* and MSR writes to avoid preemption mid-write on SMP later.
drivers/ethernet/intel/E1000.c (2)
160-184: TX submit path: add memory barrier and avoid unbounded spin.
- Add an acquire on descriptor status load or a simple CPU relax in the spin.
- Emit an
sfence/compiler barrier before updating TDT.Apply:
- while (!(g_e1000_device.tx_descs[g_e1000_device.tx_cur].status & 1)) { - // Descriptor not ready - } + while (!(g_e1000_device.tx_descs[g_e1000_device.tx_cur].status & 1)) { + cpu_relax(); + } @@ - g_e1000_device.tx_descs[g_e1000_device.tx_cur].status = 0; + g_e1000_device.tx_descs[g_e1000_device.tx_cur].status = 0; + __asm__ volatile ("" ::: "memory"); // ensure desc visible before tail
98-158: Optional: program RAL/RAH from registers when EEPROM absent.Fallback to RAL/RAH read if EERPOM MAC words are zero.
drivers/ethernet/intel/E1000.h (2)
47-50: Export a max TX length constant consistent with IFCSE1000 inserts FCS when IFCS is set; the host should not include it. The standard non-VLAN frame is 1514 bytes.
#define E1000_NUM_RX_DESC 32 #define E1000_NUM_TX_DESC 32 +// Max TX length (no FCS, IFCS=1; no VLAN) +#define E1000_MAX_TX_LEN 1514Follow-up in E1000.c: replace
length > 1518withlength > E1000_MAX_TX_LEN.
52-71: Descriptor layout looks correct; consider enforcing size at compile timeRX/TX descriptor fields match legacy 16-byte format. Add static-size guards if your toolchain supports them.
_Static_assert(sizeof(E1000RxDesc) == 16, "E1000RxDesc must be 16 bytes"); _Static_assert(sizeof(E1000TxDesc) == 16, "E1000TxDesc must be 16 bytes");kernel/etc/Shell.c (2)
224-237: Minor: guard negative padding explicitlyPad is already bounded by the inner loop, but a max(0, 20-len) improves clarity.
- int pad = 20 - StringLength(cat->entries[i].name); - for (int j = 0; j < pad && j < 20; j++) PrintKernel(" "); + int pad = 20 - StringLength(cat->entries[i].name); + if (pad < 0) pad = 0; + for (int j = 0; j < pad && j < 20; j++) PrintKernel(" ");
239-292: Accept 'development' as a help alias and update examplesCurrently only "dev" matches category 4. Also expand the examples to reflect available names.
- PrintKernel("\nExamples: 'help 1', 'help system', 'help files'\n"); + PrintKernel("\nExamples: 'help 1', 'help system', 'help files', 'help hardware', 'help development'\n");- if (FastStrCmp(category, "dev") == 0 && i == 3) { + if ((FastStrCmp(category, "dev") == 0 || FastStrCmp(category, "development") == 0) && i == 3) { ShowHelpCategory(i); KernelFree(category); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
drivers/PCI/PCI.c(3 hunks)drivers/PCI/PCI.h(2 hunks)drivers/ethernet/intel/E1000.c(1 hunks)drivers/ethernet/intel/E1000.h(1 hunks)drivers/ethernet/realtek/RTL8139.c(1 hunks)kernel/core/Kernel.c(5 hunks)kernel/etc/Shell.c(5 hunks)meson.build(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- meson.build
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
📚 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-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-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-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-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-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-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-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-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-12T02:13:46.266Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T02:13:46.266Z
Learning: User assembler-0 has existing ELF infrastructure in VoidFrame kernel with Elf.h header definitions and CreateProcess() function that properly sets up process context including stack allocation, ProcessExitStub return address, and register initialization. The issue was missing ELF loader implementation, not compilation problems.
Applied to files:
kernel/core/Kernel.c
🧬 Code graph analysis (5)
drivers/PCI/PCI.h (1)
drivers/PCI/PCI.c (3)
PciReadConfig16(27-30)PciConfigWriteDWord(37-53)PciWriteConfig16(55-64)
drivers/ethernet/intel/E1000.c (4)
mm/KernelHeap.c (1)
KernelMemoryAlloc(272-330)drivers/PCI/PCI.c (3)
PciFindDevice(160-175)PciReadConfig16(27-30)PciWriteConfig16(55-64)kernel/etc/Console.c (4)
PrintKernel(181-199)PrintKernelInt(314-338)PrintKernelHex(235-258)PrintKernelSuccess(213-218)mm/MemOps.c (1)
FastMemcpy(75-162)
drivers/ethernet/intel/E1000.h (1)
drivers/ethernet/intel/E1000.c (3)
E1000_Init(98-158)E1000_SendPacket(160-184)E1000_GetDevice(186-188)
kernel/etc/Shell.c (5)
drivers/ethernet/intel/E1000.c (2)
E1000_GetDevice(186-188)E1000_SendPacket(160-184)kernel/etc/Console.c (4)
PrintKernelError(220-225)PrintKernelSuccess(213-218)PrintKernel(181-199)PrintKernelInt(314-338)kernel/etc/StringOps.c (2)
StringLength(3-8)FastStrCmp(42-52)mm/KernelHeap.c (1)
KernelFree(377-422)include/stdlib.h (1)
atoi(18-44)
kernel/core/Kernel.c (4)
kernel/etc/Console.c (3)
PrintKernel(181-199)PrintKernelWarning(227-232)PrintKernelSuccess(213-218)mm/PMem.c (1)
GetDetailedMemoryStats(320-370)mm/StackGuard.c (1)
StackGuardInit(10-15)drivers/ethernet/intel/E1000.c (1)
E1000_Init(98-158)
🔇 Additional comments (10)
drivers/ethernet/realtek/RTL8139.c (1)
2-4: Include path updates look correct.Paths now reflect the extra directory level under drivers; no functional concerns.
drivers/PCI/PCI.h (1)
16-17: Adding BAR0 to PciDevice is good and unblocks MMIO drivers.drivers/PCI/PCI.c (1)
27-30: LGTM: 16-bit read from aligned dword.kernel/core/Kernel.c (4)
3-3: Keep RTL8139 include if still used elsewhere; otherwise drop.If all users moved to path-based includes, ensure duplicate includes aren’t lingering.
7-7: FAT1x include path update looks correct.
25-25: SVGAII include is guarded later; include itself is fine.
34-34: E1000 header include added correctly.drivers/ethernet/intel/E1000.c (1)
1-9: Missing includes for MMIO mapping helpers.If you adopt the mapping fixes, include VMem.h and possibly Cpu.h for
cpu_relax().Apply:
+#include "VMem.h"drivers/ethernet/intel/E1000.h (1)
10-46: Registers/bits look good for 82540EMCTRL/RCTL/TCTL offsets and common bit defs align with 82540EM usage.
kernel/etc/Shell.c (1)
154-176: Data-driven help catalog looks solidGood move to structured entries and categories; counts via sizeof are correct.
Also applies to: 178-191, 193-204, 205-216, 217-223
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
kernel/etc/Shell.c (2)
106-135: ARP test: wrong MAC for E1000 and unnecessary dual-NIC requirementBuild and send per-NIC packets using each NIC’s MAC; allow either NIC.
- const E1000Device* e1000 = E1000_GetDevice(); - if (!nic || !e1000) { - PrintKernelError("No NIC found & ready"); - return; - } + const E1000Device* e1000 = E1000_GetDevice(); + if (!nic && !e1000) { + PrintKernelError("No NIC found & ready\n"); + return; + } - FastMemset(packet.eth.dest_mac, 0xFF, 6); - FastMemcpy(packet.eth.src_mac, nic->mac_address, 6); - packet.eth.ethertype = HTONS(0x0806); - packet.arp.hardware_type = HTONS(1); // 1 = Ethernet - packet.arp.protocol_type = HTONS(0x0800); // 0x0800 = IPv4 - packet.arp.hardware_addr_len = 6; - packet.arp.protocol_addr_len = 4; - packet.arp.opcode = HTONS(1); - FastMemcpy(packet.arp.sender_mac, nic->mac_address, 6); - packet.arp.sender_ip[0] = 192; - packet.arp.sender_ip[1] = 168; - packet.arp.sender_ip[2] = 1; - packet.arp.sender_ip[3] = 100; - FastMemset(packet.arp.target_mac, 0x00, 6); - packet.arp.target_ip[0] = 192; - packet.arp.target_ip[1] = 168; - packet.arp.target_ip[2] = 1; - packet.arp.target_ip[3] = 1; - uint32_t packet_size = sizeof(EthernetHeader) + sizeof(ArpPacket); - Rtl8139_SendPacket(&packet, packet_size); - E1000_SendPacket(&packet, packet_size); + const uint32_t packet_size = sizeof(EthernetHeader) + sizeof(ArpPacket); + + if (nic) { + FullArpPacket p = {0}; + FastMemset(p.eth.dest_mac, 0xFF, 6); + FastMemcpy(p.eth.src_mac, nic->mac_address, 6); + p.eth.ethertype = HTONS(0x0806); + p.arp.hardware_type = HTONS(1); + p.arp.protocol_type = HTONS(0x0800); + p.arp.hardware_addr_len = 6; + p.arp.protocol_addr_len = 4; + p.arp.opcode = HTONS(1); + FastMemcpy(p.arp.sender_mac, nic->mac_address, 6); + p.arp.sender_ip[0] = 192; p.arp.sender_ip[1] = 168; p.arp.sender_ip[2] = 1; p.arp.sender_ip[3] = 100; + FastMemset(p.arp.target_mac, 0x00, 6); + p.arp.target_ip[0] = 192; p.arp.target_ip[1] = 168; p.arp.target_ip[2] = 1; p.arp.target_ip[3] = 1; + Rtl8139_SendPacket(&p, packet_size); + } + + if (e1000) { + FullArpPacket p = {0}; + FastMemset(p.eth.dest_mac, 0xFF, 6); + FastMemcpy(p.eth.src_mac, (void*)e1000->mac_address, 6); + p.eth.ethertype = HTONS(0x0806); + p.arp.hardware_type = HTONS(1); + p.arp.protocol_type = HTONS(0x0800); + p.arp.hardware_addr_len = 6; + p.arp.protocol_addr_len = 4; + p.arp.opcode = HTONS(1); + FastMemcpy(p.arp.sender_mac, (void*)e1000->mac_address, 6); + p.arp.sender_ip[0] = 192; p.arp.sender_ip[1] = 168; p.arp.sender_ip[2] = 1; p.arp.sender_ip[3] = 100; + FastMemset(p.arp.target_mac, 0x00, 6); + p.arp.target_ip[0] = 192; p.arp.target_ip[1] = 168; p.arp.target_ip[2] = 1; p.arp.target_ip[3] = 1; + E1000_SendPacket(&p, packet_size); + }
29-30: Fix E1000 header include path (current path won’t resolve)Match the RTL8139 include style or rely on added include dirs.
-#include "intel/E1000.h" +// Either relative (consistent with RTL8139)... +#include "../../drivers/ethernet/intel/E1000.h" // ...or, since include dirs now contain drivers/ethernet/intel, simply: // #include "E1000.h"
🧹 Nitpick comments (7)
README.md (1)
136-142: Fix nested list indentation (MD007) and correct SB16 bus labelIndent child bullets with two spaces; also SB16 is ISA (not PCI).
- - [x] E1000 (PCI) + - [x] E1000 (PCI) - - [x] SB16 (PCI) + - [x] SB16 (ISA) - - [x] Generic PC speaker + - [x] Generic PC speakermeson.build (2)
17-17: Prefer multi-candidate lookup for ext2 toolingImprove portability by allowing mke2fs fallback.
-mkfs_ext2 = find_program('mkfs.ext2') +mkfs_ext2 = find_program(['mkfs.ext2', 'mke2fs'])
255-277: Make disk format explicit in QEMU run argsOnce the image is raw, pass format explicitly instead of -hda.
- '-hda', 'VoidFrameDisk.img', + '-drive', 'file=VoidFrameDisk.img,format=raw,if=ide',drivers/sound/Generic.h (1)
4-4: Use angle brackets for standard headersMinor include style tweak; avoids accidental local header shadowing.
-#include "stdint.h" +#include <stdint.h>drivers/sound/Generic.c (1)
43-47: Guard against duration overflow when converting ms→µsLarge durations can overflow the 32-bit argument to delay().
- delay(duration_ms * 1000); // Convert ms to microseconds + uint64_t us = (uint64_t)duration_ms * 1000ULL; // ms → µs + if (us > 0xFFFFFFFFu) us = 0xFFFFFFFFu; // saturate + delay((uint32_t)us);kernel/etc/Shell.c (2)
194-205: Help text nit: spacing and unitsTighten wording; “Hz” capitalization and remove double space.
- {"pcbeep <x>", "PC speaker beep for <x> seconds (200hz)"}, + {"pcbeep <x>", "PC speaker beep for <x> seconds (200 Hz)"},
103-113: Zero-initialize ARP packet for safetyAvoid garbage in padding fields.
- FullArpPacket packet; + FullArpPacket packet = {0};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)drivers/sound/Generic.c(1 hunks)drivers/sound/Generic.h(1 hunks)kernel/core/Kernel.c(5 hunks)kernel/etc/Shell.c(7 hunks)meson.build(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kernel/core/Kernel.c
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code graph analysis (3)
drivers/sound/Generic.h (1)
drivers/sound/Generic.c (5)
PCSpkr_Init(8-12)PCSpkr_Beep(43-47)PCSpkr_PlayTone(14-31)PCSpkr_Stop(33-41)PCSpkr_PlayMelody(49-64)
drivers/sound/Generic.c (2)
kernel/etc/Console.c (2)
PrintKernel(181-199)PrintKernelSuccess(213-218)include/Io.h (2)
outb(6-8)inb(10-14)
kernel/etc/Shell.c (6)
drivers/ethernet/intel/E1000.c (2)
E1000_GetDevice(186-188)E1000_SendPacket(160-184)kernel/etc/Console.c (4)
PrintKernelError(220-225)PrintKernelSuccess(213-218)PrintKernel(181-199)PrintKernelInt(314-338)kernel/etc/StringOps.c (2)
StringLength(3-8)FastStrCmp(42-52)mm/KernelHeap.c (1)
KernelFree(377-422)include/stdlib.h (1)
atoi(18-44)drivers/sound/Generic.c (1)
PCSpkr_Beep(43-47)
🪛 markdownlint-cli2 (0.17.2)
README.md
138-138: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
140-140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
meson.build (1)
299-305: Don’t mkfs.ext2 directly on qcow2; either use raw+force or nbd.Formatting a qcow2 file corrupts the container; also missing -F/-t ext2.
- qemu_img.full_path() + ' create -f qcow2 VoidFrameDisk.img 16G' + ' && ' + - mkfs_ext2.full_path() + ' VoidFrameDisk.img' + qemu_img.full_path() + ' create -f raw VoidFrameDisk.img 16G' + ' && ' + + mkfs_ext2.full_path() + ' -F -t ext2 VoidFrameDisk.img'If you must keep qcow2, map via qemu-nbd and mkfs the /dev/nbd device.
kernel/core/Kernel.c (1)
728-735: Gate NIC “initialized” logs on success.Same concern as earlier reviews: don’t log success unconditionally; check return codes/device presence.
- Rtl8139_Init(); - PrintKernelSuccess("System: RTL8139 Driver initialized\n"); + if (Rtl8139_Init() == 0) { + PrintKernelSuccess("System: RTL8139 Driver initialized\n"); + } else { + PrintKernelWarning("System: RTL8139 not present or init failed\n"); + } - E1000_Init(); - PrintKernelSuccess("System: E1000 Driver initialized\n"); + if (E1000_Init() == 0) { + PrintKernelSuccess("System: E1000 Driver initialized\n"); + } else { + PrintKernelWarning("System: E1000 not present or init failed\n"); + }
🧹 Nitpick comments (13)
meson.build (3)
17-17: Add fallback to mke2fs for portability.mkfs.ext2 isn’t present on all distros; mke2fs is the common name.
-mkfs_ext2 = find_program('mkfs.ext2') +mkfs_ext2 = find_program(['mkfs.ext2', 'mke2fs'], required: true)
260-283: Specify drive formats explicitly to avoid auto-detect pitfalls.After switching to raw for VoidFrameDisk.img, pass format=raw (and keep Virtio as is).
- '-drive', 'file=VoidFrameDisk.img,if=ide', - '-drive', 'file=SataDisk.img,if=none,id=sata0', + '-drive', 'file=VoidFrameDisk.img,format=raw,if=ide', + '-drive', 'file=SataDisk.img,format=raw,if=none,id=sata0',
307-314: Consider ext2 on SataDisk to exercise AHCI+EXT2 path.Optional: keep Virtio FAT, make SataDisk ext2 for coverage.
- qemu_img.full_path() + ' create -f raw SataDisk.img 128M' + ' && ' + - mkfs_fat.full_path() + ' -F 16 SataDisk.img' + qemu_img.full_path() + ' create -f raw SataDisk.img 128M' + ' && ' + + mkfs_ext2.full_path() + ' -F -t ext2 SataDisk.img'README.md (3)
39-41: Clarify ext2 tool name across distros.Mention mke2fs to reduce setup friction.
-- mkfs.ext2 +- mkfs.ext2 (or mke2fs)
136-142: Fix list indentation and SB16 bus type.Resolves markdownlint MD007 and corrects SB16 (ISA, not PCI).
-- Network - - [x] RTL8139 (PCI) - - [x] E1000 (PCI) +- Network + - [x] RTL8139 (PCI) + - [x] E1000 (PCI) -- Sound - - [x] SB16 (PCI) - - [x] Generic PC speaker +- Sound + - [x] SB16 (ISA) + - [x] Generic PC speaker
163-165: Fix list indentation under Storage.Also satisfies MD007.
-- Storage - - [x] PATA (IDE) - - [x] VirtIO Block - - [x] AHCI +- Storage + - [x] PATA (IDE) + - [x] VirtIO Block + - [x] AHCIkernel/core/Kernel.c (3)
79-100: Early page-table code relies on identity-mapped low memory; verify assumption.AllocPageTable/BootstrapMapPage cast phys to pointers and write through. If the bootloader didn’t identity-map these frames, this will fault/corrupt. Confirm identity mapping coverage at this point or introduce a temporary mapping helper that uses current CR3 to map the allocated frames before touching them.
Also applies to: 101-176
463-486: Mapping a flat 4GB identity space is heavy; consider bounding by MMAP.Iterating 1M PTEs costs boot time. Prefer mapping only MMAP-present low regions (and devices) or a smaller window (e.g., 1GB) needed for bring-up.
- for (uint64_t paddr = 0; paddr < IDENTITY_MAP_SIZE; paddr += PAGE_SIZE) { + // Iterate Multiboot2 memory map and map only RAM/device ranges (or cap at 1GiB) + for (uint64_t paddr = 0; paddr < MIN(IDENTITY_MAP_SIZE, 1ULL<<30); paddr += PAGE_SIZE) {
256-266: CPUID feature checks: re-validate bit usage; don’t label CET_SS as STAC/CLAC.The “STAC/CLAC supported” message is tied to ECX bit 7 of leaf 7 (CET_SS on Intel), not SMAP/STAC/CLAC. Please correct the message or the check. Also confirm CR4.CET usage on your targets.
- if (ecx & (1 << 7)) { - PrintKernelSuccess("System: STAC/CLAC instructions are supported\n"); - } + if (ecx & (1 << 7)) { // CET_SS + PrintKernelSuccess("System: CET shadow stack instructions supported\n"); + }Also applies to: 308-321
drivers/storage/AHCI.h (1)
51-67: Bitfield layouts are ABI-sensitive; add size asserts to catch packing drift.Helps ensure headers stay spec-compliant across compilers/flags.
_Static_assert(sizeof(AHCICmdHeader) == 32, "AHCICmdHeader must be 32 bytes"); _Static_assert(sizeof(AHCIPrd) == 16, "AHCIPrd must be 16 bytes"); _Static_assert(sizeof(AHCICmdTable) >= 0x80, "AHCICmdTable too small");Also applies to: 68-76, 77-84
drivers/storage/AHCI.c (3)
258-269: Optional: size MMIO map from CAP.NP instead of fixed 0x1100You can map 0x100 + (NP+1)*0x80 after reading CAP.NP. Today you map 0x1100 up front; it’s fine, but dynamic sizing avoids under/over-mapping for non‑32‑port parts.
I can sketch a two-step map (map 4KB, read CAP, remap exact).
136-144: Clear PxSERR before starting portClearing SERR helps avoid stale error state carrying into first command.
// Clear interrupt status AHCI_WritePortReg(port, AHCI_PORT_IS, 0xFFFFFFFF); + // Clear error status + AHCI_WritePortReg(port, AHCI_PORT_SERR, 0xFFFFFFFF);
63-74: Optional: wait for FRE set before ST, and small barriersSome HBAs like a short delay or read-back after setting FRE before asserting ST.
// Set FRE bit uint32_t cmd = AHCI_ReadPortReg(port, AHCI_PORT_CMD); cmd |= AHCI_PORT_CMD_FRE; AHCI_WritePortReg(port, AHCI_PORT_CMD, cmd); + (void)AHCI_ReadPortReg(port, AHCI_PORT_CMD); // post write // Set ST bit cmd |= AHCI_PORT_CMD_ST; AHCI_WritePortReg(port, AHCI_PORT_CMD, cmd);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(3 hunks)drivers/storage/AHCI.c(1 hunks)drivers/storage/AHCI.h(1 hunks)kernel/core/Kernel.c(5 hunks)meson.build(6 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
📚 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-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-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-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-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-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-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-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-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-12T02:13:46.266Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T02:13:46.266Z
Learning: User assembler-0 has existing ELF infrastructure in VoidFrame kernel with Elf.h header definitions and CreateProcess() function that properly sets up process context including stack allocation, ProcessExitStub return address, and register initialization. The issue was missing ELF loader implementation, not compilation problems.
Applied to files:
kernel/core/Kernel.c
🧬 Code graph analysis (3)
drivers/storage/AHCI.h (1)
drivers/storage/AHCI.c (4)
AHCI_Init(218-346)AHCI_ReadSectors(348-354)AHCI_WriteSectors(356-362)AHCI_GetController(364-366)
drivers/storage/AHCI.c (4)
kernel/etc/Console.c (4)
PrintKernel(181-199)PrintKernelInt(314-338)PrintKernelHex(235-258)PrintKernelSuccess(213-218)mm/VMem.c (4)
VMemAlloc(243-309)VMemFree(311-375)VMemUnmap(516-555)VMemMapMMIO(587-672)mm/MemOps.c (1)
FastMemset(10-73)drivers/PCI/PCI.c (4)
PciFindByClass(188-200)PciReadConfig16(27-30)PciWriteConfig16(55-64)PciConfigReadDWord(14-25)
kernel/core/Kernel.c (6)
kernel/etc/Console.c (3)
PrintKernel(181-199)PrintKernelWarning(227-232)PrintKernelSuccess(213-218)mm/PMem.c (1)
GetDetailedMemoryStats(320-370)mm/StackGuard.c (1)
StackGuardInit(10-15)drivers/ethernet/intel/E1000.c (1)
E1000_Init(98-158)drivers/sound/Generic.c (1)
PCSpkr_Init(8-12)drivers/storage/AHCI.c (1)
AHCI_Init(218-346)
🪛 markdownlint-cli2 (0.17.2)
README.md
138-138: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
140-140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
164-164: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
165-165: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (2)
drivers/storage/AHCI.h (1)
7-11: Confirm AHCI PCI identifiers match your PCI layer.Typical AHCI HBAs expose class 0x01, subclass 0x06, prog-if 0x01. Ensure PciFindByClass uses prog-if filtering as intended; otherwise, search by class/subclass only.
drivers/storage/AHCI.c (1)
130-135: Confirm CTBA/CTBAU programming matches your AHCICmdHeader layoutSpec splits CTBA into CTBA/CTBAU dwords. If your struct uses two 32‑bit fields, you must set both; a single 64‑bit field is also fine if layout matches. Please confirm AHCI.h.
I can adjust this to:
- ctba = (uint32_t)(phys);
- ctbau = (uint32_t)(phys >> 32);
if your header has split fields.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores