fix: prevent NVMe completion-queue desync from breaking reads

kernel/src/Api/BuildNo.hpp

@@ -12,4 +12,4 @@
 
 #pragma once
 
-#define MONTAUK_BUILD_NUMBER 21
+#define MONTAUK_BUILD_NUMBER 22

kernel/src/Drivers/Storage/Nvme.cpp

@@ -15,6 +15,7 @@
 #include <Libraries/Memory.hpp>
 #include <Hal/Apic/Interrupts.hpp>
 #include <Hal/Apic/IoApic.hpp>
+#include <Timekeeping/ApicTimer.hpp>
 
 using namespace Kt;
 
@@ -114,92 +115,126 @@ namespace Drivers::Storage::Nvme {
     }
 
     // -------------------------------------------------------------------------
-    // Admin command submission
+    // Completion waiting (shared by the admin and I/O queues)
+    //
+    // The driver keeps a single command in flight per queue at a time (the VFS
+    // lock serializes all filesystem I/O), so head/phase tracking assumes
+    // strict submit-then-wait ordering. The hazard is a *slow* completion: a
+    // laptop NVMe SSD emerging from a low-power state (APST) can take several
+    // milliseconds, and a busy system -- e.g. continuous reads while a video
+    // plays -- makes such a latency spike likely. If we ever gave up on a
+    // command without consuming its eventual completion, head/phase would skew
+    // by one against the controller permanently, and every subsequent command
+    // would read the wrong CQ slot -- silently breaking *all* future reads
+    // until reboot (open/readdir/exec all fail).
+    //
+    // To stay robust we (1) wait against a generous wall-clock deadline rather
+    // than a raw spin count, and (2) match completions by Command ID: every
+    // completion carrying the expected phase is consumed (head/phase advanced,
+    // doorbell rung), and a stale completion left over from an earlier
+    // timed-out command is discarded harmlessly here, which keeps the queue in
+    // lockstep with the controller and lets it self-heal.
     // -------------------------------------------------------------------------
 
-    static void SubmitAdminCommand(SqEntry& cmd) {
+    static bool WaitCompletion(CqEntry* cq, uint16_t& head, uint8_t& phase,
-        cmd.CommandId = g_adminCmdId++;
+                               uint16_t depth, uint16_t queueId,
-        g_adminSq[g_adminSqTail] = cmd;
+                               uint16_t cmdId, CqEntry& out) {
-        g_adminSqTail = (g_adminSqTail + 1) % ADMIN_QUEUE_DEPTH;
+        // Wall-clock budget covers SSD power-state latency spikes. The spin cap
-        WriteSqTailDoorbell(0, g_adminSqTail);
+        // is only a backstop for the early-boot window before the timer ticks
-    }
+        // (or any path that polls with interrupts disabled), where elapsed time
+        // would otherwise never advance.
+        constexpr uint64_t kTimeoutMs = 5000;
+        constexpr uint64_t kMaxSpins  = 5'000'000'000ull;
+        uint64_t start = Timekeeping::GetMilliseconds();
+        uint64_t spins = 0;
 
-    static bool WaitAdminCompletion(CqEntry& out) {
+        for (;;) {
-        for (int i = 0; i < 5000000; i++) {
+            volatile CqEntry* cqe = &cq[head];
-            CqEntry* cqe = &g_adminCq[g_adminCqHead];
             uint16_t status = cqe->Status;
 
-            // Check phase bit matches expected
+            if ((status & CQE_PHASE_BIT) == phase) {
-            if ((status & CQE_PHASE_BIT) == g_adminCqPhase) {
+                uint16_t consumedId = cqe->CommandId;
-                out = *cqe;
+                uint32_t result = cqe->Result;
+                uint16_t sqHead = cqe->SqHead;
+                uint16_t sqId = cqe->SqId;
 
-                // Advance CQ head
+                // Advance head/phase and tell the controller we consumed it.
-                g_adminCqHead++;
+                head++;
-                if (g_adminCqHead >= ADMIN_QUEUE_DEPTH) {
+                if (head >= depth) { head = 0; phase ^= 1; }
-                    g_adminCqHead = 0;
+                WriteCqHeadDoorbell(queueId, head);
-                    g_adminCqPhase ^= 1;  // Toggle expected phase
-                }
-                WriteCqHeadDoorbell(0, g_adminCqHead);
 
-                // Check status code (bits 15:1, 0 = success)
+                if (consumedId == cmdId) {
+                    out.Result = result;
+                    out.SqHead = sqHead;
+                    out.SqId = sqId;
+                    out.CommandId = consumedId;
+                    out.Status = status;
                     if (status & CQE_STATUS_MASK) {
-                    KernelLogStream(ERROR, "NVMe") << "Admin command failed, status="
+                        KernelLogStream(ERROR, "NVMe") << "Command " << base::dec
+                            << (uint64_t)cmdId << " failed, status="
                             << base::hex << (uint64_t)(status >> 1);
                         return false;
                     }
                     return true;
                 }
-            asm volatile("" ::: "memory");
+
+                // Stale completion from a previously timed-out command. Discard
+                // it, grant the command we are actually waiting for a fresh
+                // budget, and keep draining.
+                KernelLogStream(WARNING, "NVMe") << "Discarded stale completion id="
+                    << base::dec << (uint64_t)consumedId << " (awaiting "
+                    << (uint64_t)cmdId << ")";
+                start = Timekeeping::GetMilliseconds();
+                spins = 0;
+                continue;
             }
 
-        KernelLogStream(ERROR, "NVMe") << "Admin command timeout";
+            spins++;
+            if ((spins & 0x3FFF) == 0 &&
+                Timekeeping::GetMilliseconds() - start >= kTimeoutMs) break;
+            if (spins >= kMaxSpins) break;
+            asm volatile("pause" ::: "memory");
+        }
+
+        KernelLogStream(ERROR, "NVMe") << "Command " << base::dec << (uint64_t)cmdId
+            << " timeout (queue " << (uint64_t)queueId << ")";
         return false;
     }
 
+    // -------------------------------------------------------------------------
+    // Admin command submission
+    // -------------------------------------------------------------------------
+
+    static uint16_t SubmitAdminCommand(SqEntry& cmd) {
+        cmd.CommandId = g_adminCmdId++;
+        g_adminSq[g_adminSqTail] = cmd;
+        g_adminSqTail = (g_adminSqTail + 1) % ADMIN_QUEUE_DEPTH;
+        WriteSqTailDoorbell(0, g_adminSqTail);
+        return cmd.CommandId;
+    }
+
     // Submit an admin command and wait for completion
     static bool AdminCommand(SqEntry& cmd, CqEntry& cqe) {
-        SubmitAdminCommand(cmd);
+        uint16_t id = SubmitAdminCommand(cmd);
-        return WaitAdminCompletion(cqe);
+        return WaitCompletion(g_adminCq, g_adminCqHead, g_adminCqPhase,
+                              (uint16_t)ADMIN_QUEUE_DEPTH, 0, id, cqe);
     }
 
     // -------------------------------------------------------------------------
     // I/O command submission
     // -------------------------------------------------------------------------
 
-    static void SubmitIoCommand(SqEntry& cmd) {
+    static uint16_t SubmitIoCommand(SqEntry& cmd) {
         cmd.CommandId = g_ioCmdId++;
         g_ioSq[g_ioSqTail] = cmd;
         g_ioSqTail = (g_ioSqTail + 1) % g_ioSqDepth;
         WriteSqTailDoorbell(1, g_ioSqTail);
+        return cmd.CommandId;
     }
 
-    static bool WaitIoCompletion(CqEntry& out) {
+    static bool WaitIoCompletion(uint16_t cmdId, CqEntry& out) {
-        for (int i = 0; i < 5000000; i++) {
+        return WaitCompletion(g_ioCq, g_ioCqHead, g_ioCqPhase,
-            CqEntry* cqe = &g_ioCq[g_ioCqHead];
+                              g_ioCqDepth, 1, cmdId, out);
-            uint16_t status = cqe->Status;
-
-            if ((status & CQE_PHASE_BIT) == g_ioCqPhase) {
-                out = *cqe;
-
-                g_ioCqHead++;
-                if (g_ioCqHead >= g_ioCqDepth) {
-                    g_ioCqHead = 0;
-                    g_ioCqPhase ^= 1;
-                }
-                WriteCqHeadDoorbell(1, g_ioCqHead);
-
-                if (status & CQE_STATUS_MASK) {
-                    KernelLogStream(ERROR, "NVMe") << "I/O command failed, status="
-                        << base::hex << (uint64_t)(status >> 1);
-                    return false;
-                }
-                return true;
-            }
-            asm volatile("" ::: "memory");
-        }
-
-        KernelLogStream(ERROR, "NVMe") << "I/O command timeout";
-        return false;
     }
 
     // -------------------------------------------------------------------------
@@ -756,10 +791,10 @@ namespace Drivers::Storage::Nvme {
         // CDW12: bits 15:0 = Number of Logical Blocks (0-based)
         cmd.Cdw12 = count - 1;
 
-        SubmitIoCommand(cmd);
+        uint16_t cmdId = SubmitIoCommand(cmd);
 
         CqEntry cqe;
-        bool ok = WaitIoCompletion(cqe);
+        bool ok = WaitIoCompletion(cmdId, cqe);
         if (ok) {
             memcpy(buffer, dmaVirt, totalBytes);
         }
@@ -783,10 +818,10 @@ namespace Drivers::Storage::Nvme {
         cmd.Opcode = IO_CMD_FLUSH;
         cmd.Nsid = g_namespaces[ns].Nsid;
 
-        SubmitIoCommand(cmd);
+        uint16_t cmdId = SubmitIoCommand(cmd);
 
         CqEntry cqe;
-        return WaitIoCompletion(cqe);
+        return WaitIoCompletion(cmdId, cqe);
     }
 
     bool WriteSectors(int ns, uint64_t lba, uint32_t count, const void* buffer) {
@@ -833,10 +868,10 @@ namespace Drivers::Storage::Nvme {
         cmd.Cdw11 = (uint32_t)(lba >> 32);
         cmd.Cdw12 = count - 1;
 
-        SubmitIoCommand(cmd);
+        uint16_t cmdId = SubmitIoCommand(cmd);
 
         CqEntry cqe;
-        bool ok = WaitIoCompletion(cqe);
+        bool ok = WaitIoCompletion(cmdId, cqe);
 
         if (pagesNeeded > 2 && cmd.Prp2 != 0 && cmd.Prp2 != dmaPhys + 0x1000) {
             Memory::g_pfa->Free((void*)Memory::HHDM(cmd.Prp2));