Skip to content
This repository was archived by the owner on Jan 4, 2026. It is now read-only.

Conversation

@assembler-0
Copy link
Owner

@assembler-0 assembler-0 commented Oct 30, 2025

NVMe

Summary by CodeRabbit

  • New Features

    • Added NVMe driver shutdown support and integration with system reset procedures
    • Added millisecond-precision time tracking capability
  • Bug Fixes

    • Improved NVMe stability with synchronized command submission and enhanced error handling
    • Refined system shutdown sequence with better resource cleanup
  • Chores

    • Changed storage filesystem format from FAT to EXT2
    • Optimized internal compilation dependencies

@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 2025

Walkthrough

This pull request introduces several system-level enhancements: the build system switches SataDisk.img from FAT to EXT2 format, time-based deadline functionality is added to TSC driver via GetTimeInMs(), NVMe driver undergoes significant refactoring including spinlock-protected command submission, PRP list management, and shutdown cleanup sequences, ACPI gains a dedicated reset procedure that integrates NVMe shutdown, and the panic handler now invokes ACPIResetProcedure during emergency shutdown. Additionally, kernel heap header dependency on Console.h is removed.

Poem

🐰 Whiskers twitching with delight,
EXT2 filesystems shining bright,
Spinlocks spinning, timeouts true,
NVMe shutdown fresh and new,
When panic calls, ACPI resets right,
This kernel hops to greater height! 🥕

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title "Development" is excessively vague and generic, failing to convey any meaningful information about the changeset. While the PR does contain significant development work—particularly centered around NVMe driver refactoring with spinlock implementation, PRP list handling, shutdown procedures, and integration with ACPI reset functionality—the title provides no indication of these specifics. A teammate reviewing git history would be unable to understand the primary focus of this PR from the title alone, as "Development" could describe almost any pull request and matches the pattern of uninformative titles explicitly identified in the evaluation criteria. The title should be replaced with a descriptive summary of the main changes. Based on the changeset, a more appropriate title would be something like "Add NVMe driver refactoring with spinlock and PRP list support" or "NVMe driver improvements: spinlock synchronization and shutdown flow" to clearly communicate the primary focus to readers scanning the project history.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
drivers/storage/NVMe.c (1)

348-355: Redundant conditional: Both branches are identical.

Static analysis correctly identifies that lines 349 and 353 have identical branches. The condition (bar0_val & 0x06) == 0x04 checks for 64-bit BAR, but both branches perform similar operations just with different address calculations.

Simplify the logic:

-    if ((bar0_val & 0x1) == 0) {
-        if ((bar0_val & 0x06) == 0x04) {
-            uint32_t bar1_val = PciConfigReadDWord(pci_dev.bus, pci_dev.device, pci_dev.function, PCI_BAR0_REG + 4);
-            mmio_phys = ((uint64_t)bar1_val << 32) | (uint64_t)(bar0_val & ~0xFULL);
-        } else {
-            mmio_phys = (uint64_t)(bar0_val & ~0xFULL);
-        }
+    if ((bar0_val & 0x1) == 0) {
+        mmio_phys = (uint64_t)(bar0_val & ~0xFULL);
+        if ((bar0_val & 0x06) == 0x04) {
+            // 64-bit BAR: read high 32 bits from BAR1
+            uint32_t bar1_val = PciConfigReadDWord(pci_dev.bus, pci_dev.device, pci_dev.function, PCI_BAR0_REG + 4);
+            mmio_phys |= ((uint64_t)bar1_val << 32);
+        }
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f3c5b0 and 6dd08c5.

📒 Files selected for processing (9)
  • CMakeLists.txt (1 hunks)
  • drivers/ACPI.c (2 hunks)
  • drivers/ACPI.h (1 hunks)
  • drivers/TSC.c (1 hunks)
  • drivers/TSC.h (1 hunks)
  • drivers/storage/NVMe.c (8 hunks)
  • drivers/storage/NVMe.h (4 hunks)
  • kernel/core/Panic.c (3 hunks)
  • mm/KernelHeap.h (0 hunks)
💤 Files with no reviewable changes (1)
  • mm/KernelHeap.h
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#101
File: kernel/etc/Shell.c:829-837
Timestamp: 2025-08-31T07:06:37.003Z
Learning: assembler-0 is working on optimizing ramfs performance in VoidFrame kernel and wants to implement proper filesystem characteristics like faster lookups, better memory management, and advanced features like copy-on-write and caching.
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.
Learnt from: assembler-0
PR: assembler-0/autocc#33
File: include/utils.hpp:138-190
Timestamp: 2025-08-04T13:24:48.984Z
Learning: The user assembler-0 tends to create wrapper functions during refactoring and sometimes accidentally leaves duplicate implementations. They have a casual, self-aware attitude about coding mistakes and are receptive to feedback about code duplication issues.
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.
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#80
File: README.md:7-8
Timestamp: 2025-08-13T08:43:54.750Z
Learning: User assembler-0 is working on VoidFrame kernel with a tight deadline (2-3 days before summer ends) and specifically requested runtime safety checks similar to __stack_chk_fail but for deadlocks and other kernel issues. The kernel already has extensive safety mechanisms including stack canaries, guard pages, heap integrity checks, and process security validation.
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.
📚 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.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-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/Panic.c
  • drivers/storage/NVMe.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: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 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-07-16T01:04:23.790Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#47
File: kernel/core/Kernel.c:434-434
Timestamp: 2025-07-16T01:04:23.790Z
Learning: VMemAlloc() in VoidFrame kernel fails with error code -2 (VMEM_ERROR_INVALID_ADDR) due to virtual address space layout conflict where the heap start address (_bss_end + KERNEL_VIRTUAL_OFFSET + 1MB margin) exceeds VIRT_ADDR_SPACE_END (0xFFFFFFFF80000000ULL), causing VMemMap validation to fail.

Applied to files:

  • drivers/storage/NVMe.c
📚 Learning: 2025-08-16T05:28:34.925Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#90
File: kernel/memory/MemOps.c:103-124
Timestamp: 2025-08-16T05:28:34.925Z
Learning: In VoidFrame kernel, the virtual address space management uses a simple linear allocator (kernel_space.next_vaddr) without any reclamation mechanism. VMemFree and VMemUnmapMMIO clear page tables but don't free virtual address space for reuse, causing eventual virtual address space exhaustion. The solution requires implementing a free list with coalescing in the VirtAddrSpace structure and updating allocation/deallocation functions to reuse freed ranges.

Applied to files:

  • drivers/storage/NVMe.c
🧬 Code graph analysis (7)
drivers/TSC.h (1)
drivers/TSC.c (1)
  • GetTimeInMs (36-39)
kernel/core/Panic.c (2)
kernel/etc/Console.c (2)
  • PrintKernel (198-211)
  • PrintKernelSuccess (225-230)
drivers/ACPI.c (1)
  • ACPIResetProcedure (149-161)
drivers/ACPI.h (1)
drivers/ACPI.c (2)
  • AcpiFindTable (80-102)
  • ACPIResetProcedure (149-161)
drivers/ACPI.c (2)
drivers/storage/NVMe.c (1)
  • NVMe_Shutdown (285-310)
kernel/etc/Console.c (1)
  • PrintKernelSuccess (225-230)
drivers/TSC.c (1)
kernel/atomic/rust/src/spinlock.rs (1)
  • rdtsc (13-15)
drivers/storage/NVMe.h (1)
drivers/storage/NVMe.c (1)
  • NVMe_Shutdown (285-310)
drivers/storage/NVMe.c (5)
drivers/TSC.c (2)
  • GetTimeInMs (36-39)
  • delay_us (41-48)
kernel/atomic/rust/src/ffi.rs (4)
  • rust_spinlock_lock_irqsave (211-222)
  • rust_spinlock_unlock_irqrestore (225-232)
  • rust_spinlock_free (40-51)
  • rust_spinlock_new (27-37)
mm/MemOps.c (2)
  • FastMemcpy (51-70)
  • FastMemset (34-49)
fs/DriveNaming.c (1)
  • GenerateDriveNameInto (11-32)
fs/BlockDevice.c (1)
  • BlockDeviceRegister (18-54)
🪛 Clang (14.0.6)
kernel/core/Panic.c

[note] 82-82: +2, including nesting penalty of 1, nesting level increased to 2

(clang)


[note] 86-86: +1, nesting level increased to 2

(clang)


[warning] 86-86: statement should be inside braces

(readability-braces-around-statements)

drivers/TSC.c

[warning] 37-37: statement should be inside braces

(readability-braces-around-statements)

drivers/storage/NVMe.c

[warning] 81-81: 100 is a magic number; consider replacing it with a named constant

(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)


[warning] 121-121: 50 is a magic number; consider replacing it with a named constant

(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)


[warning] 150-150: 16 is a magic number; consider replacing it with a named constant

(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)


[warning] 161-161: 16 is a magic number; consider replacing it with a named constant

(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)


[warning] 162-162: 16 is a magic number; consider replacing it with a named constant

(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)


[warning] 196-196: 2 adjacent parameters of 'NVMe_SetupPrpList' of similar type ('int') are easily swapped by mistake

(bugprone-easily-swappable-parameters)


[note] 196-196: the first parameter in the range is 'buffer_phys'

(clang)


[note] 196-196: the last parameter in the range is 'total_bytes'

(clang)


[warning] 196-196: parameter 'buffer_phys' is unused

(misc-unused-parameters)


[warning] 196-196: parameter 'total_bytes' is unused

(misc-unused-parameters)


[warning] 205-205: do not use 'else' after 'return'

(readability-else-after-return)


[warning] 223-223: 2 adjacent parameters of 'NVMe_ReadSectors' of similar type ('int') are easily swapped by mistake

(bugprone-easily-swappable-parameters)


[note] 223-223: the first parameter in the range is 'lba'

(clang)


[note] 223-223: the last parameter in the range is 'count'

(clang)


[warning] 223-223: parameter 'lba' is unused

(misc-unused-parameters)


[warning] 223-223: parameter 'count' is unused

(misc-unused-parameters)


[warning] 223-223: parameter 'buffer' is unused

(misc-unused-parameters)


[warning] 224-224: statement should be inside braces

(readability-braces-around-statements)


[warning] 236-236: variable 'num_prp_entries' is not initialized

(cppcoreguidelines-init-variables)


[warning] 256-256: 2 adjacent parameters of 'NVMe_WriteSectors' of similar type ('int') are easily swapped by mistake

(bugprone-easily-swappable-parameters)


[note] 256-256: the first parameter in the range is 'lba'

(clang)


[note] 256-256: the last parameter in the range is 'count'

(clang)


[warning] 256-256: parameter 'lba' is unused

(misc-unused-parameters)


[warning] 256-256: parameter 'count' is unused

(misc-unused-parameters)


[warning] 256-256: parameter 'buffer' is unused

(misc-unused-parameters)


[warning] 257-257: statement should be inside braces

(readability-braces-around-statements)


[warning] 269-269: variable 'num_prp_entries' is not initialized

(cppcoreguidelines-init-variables)


[warning] 279-279: variable name 'st' is too short, expected at least 3 characters

(readability-identifier-length)


[warning] 280-280: statement should be inside braces

(readability-braces-around-statements)


[warning] 286-286: statement should be inside braces

(readability-braces-around-statements)


[warning] 293-293: statement should be inside braces

(readability-braces-around-statements)


[warning] 294-294: statement should be inside braces

(readability-braces-around-statements)


[warning] 295-295: statement should be inside braces

(readability-braces-around-statements)


[warning] 296-296: statement should be inside braces

(readability-braces-around-statements)


[warning] 349-349: if with identical then and else branches

(bugprone-branch-clone)


[warning] 445-445: 16 is a magic number; consider replacing it with a named constant

(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)

🔇 Additional comments (27)
CMakeLists.txt (1)

219-220: Critical concern — Missing EXT2 and SATA driver implementations.

The change to format SataDisk.img with EXT2 cannot be verified as safe:

  • Ext2Mount function called in kernel/etc/Shell.c:1222 but implementation not found in kernel/
  • SATA driver — no implementation files exist in kernel/
  • EXT2 filesystem driver — no implementation files exist in kernel/

The codebase appears to rely on QEMU's SATA emulation, but the kernel has no code to read or write EXT2 filesystems on the SATA disk. Verify:

  1. Are filesystem drivers and SATA driver implementations in separate modules or pending completion?
  2. Will this NVMe PR include the missing Ext2Mount and EXT2 driver implementations?
  3. Has the SATA driver been tested to confirm it can access EXT2 volumes?
drivers/TSC.h (1)

6-6: LGTM - Clean API addition for time-based deadlines.

The new GetTimeInMs function provides a clean abstraction for time-based deadline tracking, which is utilized by the NVMe driver for timeout handling.

drivers/ACPI.h (1)

86-87: LGTM - New reset procedure API.

The new ACPIResetProcedure function provides a centralized shutdown sequence that integrates filesystem unmounting, process termination, and NVMe driver cleanup.

drivers/storage/NVMe.h (4)

6-6: LGTM - Adds spinlock support for thread-safe NVMe operations.

Including the spinlock header enables synchronization around command submission paths to prevent race conditions.


79-81: LGTM - Extended NVMe controller structure supports PRP lists and locking.

The additions properly support:

  • Thread-safe command submission via the spinlock
  • Efficient multi-page transfers via the PRP list infrastructure

111-111: LGTM - Shutdown API for proper resource cleanup.

The new shutdown function enables proper cleanup of NVMe resources during system shutdown or driver failures.


46-46: Verification complete: PRP_LIST_ENTRIES limit is correctly enforced.

The code properly validates PRP list capacity: NVMe_SetupPrpList calculates required entries and returns -1 if num_prp_entries exceeds PRP_LIST_ENTRIES, and both NVMe_ReadSectors and NVMe_WriteSectors check for this error and return -1. The 512-entry limit correctly enforces approximately 2MB maximum transfer size.

However, confirmation is needed: verify that the 2MB maximum I/O size aligns with your system's expected maximum transfer sizes. The functions accept uint16_t count (up to ~32MB worth of sectors), but silently reject requests exceeding the 2MB limit. Consider whether this design is acceptable or if explicit count validation/clamping should be added at the API level.

drivers/ACPI.c (2)

5-5: LGTM - Enables NVMe shutdown integration.

Including NVMe.h allows ACPIResetProcedure to invoke NVMe_Shutdown during the reset sequence.


149-161: LGTM - Well-structured reset procedure with proper sequencing.

The ACPIResetProcedure correctly sequences shutdown operations: unmounting filesystems, stopping processes, and shutting down the NVMe driver. The logging provides clear visibility into the shutdown progress.

drivers/TSC.c (1)

36-39: Check for potential arithmetic overflow in time calculation.

The implementation calculates time as (rdtsc() * 1000) / tsc_freq_hz. For a 3GHz TSC running for extended periods, rdtsc() * 1000 could overflow a 64-bit integer after approximately 213 days of uptime. While this may be acceptable for the use case, consider documenting this limitation or restructuring the calculation to delay the multiplication.

Alternative calculation to avoid overflow:

 uint64_t GetTimeInMs(void) {
     if (!tsc_calibrated) return 0;
-    return (rdtsc() * 1000) / tsc_freq_hz;
+    // Avoid overflow by dividing first, then scaling remainder
+    uint64_t tsc = rdtsc();
+    uint64_t ms = (tsc / tsc_freq_hz) * 1000;
+    ms += ((tsc % tsc_freq_hz) * 1000) / tsc_freq_hz;
+    return ms;
 }
kernel/core/Panic.c (3)

4-4: LGTM - Enables ACPI reset procedure integration.

Including ACPI.h allows the panic handler to invoke the reset procedure during emergency shutdown.


82-86: LGTM - Consistent message formatting.

The addition of the newline ensures proper message formatting in the panic output.


88-90: LGTM - Integrates graceful shutdown during panic.

Invoking ACPIResetProcedure during panic enables graceful cleanup including filesystem unmounting and NVMe shutdown before system halt.

Based on learnings

drivers/storage/NVMe.c (14)

34-45: LGTM - Time-based timeout replaces fixed-count loops.

The refactored NVMe_WaitReady now uses GetTimeInMs for time-based deadlines with a 5-second timeout, providing more predictable timeout behavior across different CPU speeds.


87-125: Same concerns as admin command submission - verify CQ protection.

The I/O command submission follows the same pattern as admin command submission. The same verification applies: ensure io_cq_head updates are properly protected if multiple threads can submit I/O commands concurrently.


148-170: LGTM - I/O queue creation updated with proper command formatting.

The command structure initialization is clear and correctly formatted.


249-254: LGTM - Flush command implementation.

The flush command is correctly structured.


256-283: LGTM - Write operation with flush ensures data persistence.

The write operation correctly follows up with a flush command to ensure data persistence. The same PRP optimization suggestion applies here as well.


285-310: LGTM - Comprehensive shutdown with proper resource cleanup.

NVMe_Shutdown performs thorough cleanup:

  • Disables the controller
  • Frees all queue memory
  • Frees the PRP list
  • Unmaps and frees MMIO region
  • Releases the spinlock
  • Zeros the controller structure

The sequence is correct and handles all allocated resources.


322-341: LGTM - Initialization allocates lock and PRP list.

The initialization properly allocates the spinlock and PRP list early, calling NVMe_Shutdown on failure.


401-405: LGTM - Proper error handling with shutdown on failure.

The initialization correctly invokes NVMe_Shutdown when admin queue allocation fails, ensuring cleanup of previously allocated resources.


424-428: LGTM - Consistent error handling pattern.

Controller enable timeout properly triggers shutdown.


430-434: LGTM - Consistent error handling pattern.

I/O queue creation failure properly triggers shutdown.


436-442: LGTM - Validates namespace size before proceeding.

The code now properly checks if namespace size is zero and fails initialization, preventing registration of an invalid device.


448-467: LGTM - Proper block device registration with error handling.

The block device registration uses the new wrapper functions and properly handles registration failure by calling NVMe_Shutdown.


172-194: Critical: Missing error handling and potential memory leak.

The identify_data buffer is allocated with KernelMemoryAlloc but not freed on all error paths. Line 188 frees it on command failure, but if nsze is 0 (line 191-193), the function returns 0 without freeing the buffer.

Apply this fix:

     uint64_t nsze = *(uint64_t*)identify_data;
     KernelFree(identify_data);
-    return nsze;
+    
+    if (nsze == 0) {
+        PrintKernel("NVMe: Namespace size is zero\n");
+    }
+    return nsze;
 }

Actually, the current code at lines 191-193 shows that identify_data is freed at line 192 before returning nsze at line 193, so there's no leak. Let me reconsider...

Looking more carefully at lines 191-193:

191    uint64_t nsze = *(uint64_t*)identify_data;
192    KernelFree(identify_data);
193    return nsze;

This is correct - the buffer is freed before returning. There's no issue here.

⛔ Skipped due to learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#57
File: kernel/memory/VMem.h:64-66
Timestamp: 2025-08-09T09:29:00.630Z
Learning: In the VoidFrame kernel, removing locks from KernelMemoryAlloc/KernelMemoryFree causes heap corruption where shell command strings (like "clear" = 0x61656C63) overwrite heap block magic values due to race conditions between allocation and free operations.
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.

47-85: Original review comment concerns are not substantiated by the codebase.

NVMe_SubmitAdminCommand is called only from three synchronous initialization sites (lines 153, 164, 185), with no concurrent callers or interrupt handlers detected. The completion queue fields (admin_cq_head and admin_cq_phase) are accessed only within the polling loop, making concurrent access impossible under the current call pattern. The phase bit logic (status & 1) == ctrl->admin_cq_phase correctly implements NVMe phase bit semantics to disambiguate completions after queue wrap. The lock scope—held during submission and doorbell write, released before polling—is a sound design for sequential command submission.

@assembler-0 assembler-0 merged commit 611a3f6 into main Oct 30, 2025
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant