Development #122
Development #122
Conversation
WalkthroughAdds an x86_64 register-dump subsystem (RegistersDumpT, DumpRegisters, PrintRegisters) and integrates it into fault handlers and a new shell regdump command; renames PROC_PRIV_USER → PROC_PRIV_NORM across loaders and scheduler, adds PROC_FLAG_NONE, introduces a POST test and build option for automatic POST, and updates CI/workflow flags. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Shell
participant CPU as Cpu.c (Dump/Print)
Note over Shell,CPU: regdump command calls register-dump subsystem
User->>Shell: regdump
Shell->>CPU: DumpRegisters(&dump)
CPU-->>Shell: RegistersDumpT
Shell->>CPU: PrintRegisters(&dump)
CPU-->>User: Register snapshot printed
sequenceDiagram
autonumber
participant HW as CPU/Hardware
participant IDT as Interrupts.c
participant CPU as Cpu.c (Dump/Print)
participant Panic as PanicFromInterrupt
Note over IDT,CPU: On faults 6/8/13/14, dump and print registers before panic
HW->>IDT: Fault (#GP/#PF/...)
IDT->>CPU: DumpRegisters(&dump)
CPU-->>IDT: RegistersDumpT
IDT->>CPU: PrintRegisters(&dump)
CPU-->>IDT: Printed
IDT->>Panic: PanicFromInterrupt(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/ARCHITECTURE.md (1)
384-399: Fix typos and wording in Debugging section.e.g., “kenrel” → “kernel”, “ive” → “I’ve”, “dont” → “don’t”, “I dont even know to hook it up” → “I don’t even know how to hook it up”.
kernel/execf/elf/ELFloader.c (1)
269-271: Enforce requested privilege when creating processes (critical)MLFQCreateProcess unconditionally forwards to CreateSecureProcess(..., PROC_PRIV_NORM, PROC_FLAG_NONE), so options->privilege_level and security_flags passed from ELFloader/PEloader are ignored. Fix required.
Locations: kernel/execf/elf/ELFloader.c:270; kernel/execf/pe/PEloader.c:217; include/Scheduler.h:19; kernel/sched/MLFQ.c:1173–1174.
Action: expose/use an API that accepts privilege/flags (or call CreateSecureProcess directly) and propagate options->privilege_level and options->security_flags into process creation. Example (unchanged suggestion):
- uint32_t pid = MLFQCreateProcess(filename, (void (*)(void))adjusted_entry); + uint32_t pid = + MLFQCreateProcessEx( + options->process_name ? options->process_name : filename, + (void (*)(void))adjusted_entry, + options->privilege_level, + PROC_FLAG_NONE, + options->security_flags);kernel/execf/pe/PEloader.c (1)
217-217: Apply privilege/security options when creating PE processesPE loader calls MLFQCreateProcess at kernel/execf/pe/PEloader.c:217 which ignores options->privilege_level and options->security_flags. MLFQCreateProcess is a thin wrapper that always does CreateSecureProcess(name, entry_point, PROC_PRIV_NORM, PROC_FLAG_NONE) (kernel/sched/MLFQ.c:1173–1174); CreateSecureProcess is static in kernel/sched/MLFQ.c (definition ~1020) and is not accessible from PEloader.
Action (recommended): add an exported API that accepts privilege/flags (e.g., declare in kernel/sched/MLFQ.h and implement in kernel/sched/MLFQ.c)
- Prototype: uint32_t MLFQCreateProcessEx(const char *name, void (*entry_point)(void), uint8_t privilege, uint32_t initial_flags);
- Impl: return CreateSecureProcess(name, entry_point, privilege, initial_flags);
- Callsite change (PEloader.c:217): call MLFQCreateProcessEx(options->process_name ? options->process_name : filename, (void (*)(void))entry_point, options->privilege_level, options->security_flags);
Alternative: export CreateSecureProcess (remove static + header prototype) and call it directly from PEloader — only if you want to avoid an extra wrapper; keep existing internal privilege checks.
kernel/execf/pe/PEloader.c:217 — must be updated so PE processes honor provided privilege/security flags.
arch/x86_64/interrupts/Interrupts.c (1)
63-80: Double-fault: IST is NOT configured — high triple-fault risk; set an IST stack for vector 8 or remove heavy diagnosticsFindings (brief): arch/x86_64/idt/Idt.c currently forces g_Idt[num].Reserved = 0 in IdtSetGate (Idt.c ~268) and IdtSetGate(8, ...) is called at Idt.c ~295; arch/x86_64/gdt/Gdt.h defines tss.ist1..ist7 but Gdt.c never initializes them (Gdt.c ~56–66 loads the TSS). arch/x86_64/interrupts/Interrupts.c performs heavy register/MSR/DR formatted printing in the double-fault path (the case block shown), which can triple-fault when IST/stack is absent.
Actions (critical):
- Preferred (safe): Allocate a dedicated double-fault stack, set tss.ist1 = stack_top during boot (arch/x86_64/gdt/Gdt.c), and modify IdtSetGate (arch/x86_64/idt/Idt.c) to accept/set the IST byte (g_Idt[num].Reserved = ist & 0x7) and call it for vector 8 with that IST index — then keep deep diagnostics.
- Short-term mitigation: Change the double-fault path in arch/x86_64/interrupts/Interrupts.c to avoid MSR/DR reads and heavy formatted printing for regs->interrupt_number == 8; perform minimal logging and immediate panic until IST+stack are installed.
Files to edit: arch/x86_64/idt/Idt.c (IdtSetGate + IdtInstall for vector 8), arch/x86_64/gdt/Gdt.c (initialize tss.istN and allocate stack), arch/x86_64/interrupts/Interrupts.c (double-fault handling).
arch/x86_64/cpu/Cpu.c (1)
49-55: Fix XCR0 programming: set bit 0 (x87) and gate AVX bit by CPUID to avoid #GP(0).Writing XCR0 with SSE/AVX bits set while x87 (bit 0) is clear will #GP. Also, XCR0[2] (AVX) must only be set if CPUID.01H:ECX.AVX=1. Current code unconditionally sets bit 2 and omits bit 0.
Apply:
- uint64_t xcr0 = (1 << 1) | (1 << 2); // Enable SSE and AVX state saving - __asm__ volatile("xsetbv" :: "c"(0), "a"((uint32_t)xcr0), "d"((uint32_t)(xcr0 >> 32))); + // XCR0[0]=x87 must be set whenever enabling SSE/AVX; gate AVX by CPUID + bool avx_supported = ((ecx >> 28) & 1) != 0; // CPUID.01H:ECX.AVX + uint64_t xcr0 = (1ull << 0) | (1ull << 1) | (avx_supported ? (1ull << 2) : 0); + __asm__ volatile("xsetbv" :: "c"(0), "a"((uint32_t)xcr0), "d"((uint32_t)(xcr0 >> 32)));Optionally, validate requested bits against CPUID.(EAX=0DH,ECX=0):EAX mask before xsetbv.
🧹 Nitpick comments (11)
docs/ARCHITECTURE.md (1)
206-206: Document all supported privilege levels and clarifyis_user_modevsprivilege_level.The rename to
PROC_PRIV_NORMis reflected, but the doc omitsPROC_PRIV_RESTRICTED(present in MLFQ.h). Also consider a one-liner clarifying howis_user_moderelates toprivilege_levelto avoid ambiguity.kernel/execf/elf/ELFloader.h (1)
67-71: LGTM; align comment with full enum set.Comment mentions only
PROC_PRIV_NORM/PROC_PRIV_SYSTEM; consider addingPROC_PRIV_RESTRICTEDfor completeness since it exists in scheduler headers.arch/x86_64/cpu/Cpu.h (2)
5-5: Prefer angle brackets for standard headers.Use
<stdbool.h>to avoid accidentally including a project-local header named “stdbool.h”.-#include "stdbool.h" +#include <stdbool.h>
61-63: Add nonnull to APIs to catch misuse.Annotate parameters to prevent null misuse at call sites.
-void DumpRegisters(RegistersDumpT* dump); +void DumpRegisters(RegistersDumpT* dump) __attribute__((nonnull)); -void PrintRegisters(const RegistersDumpT* dump); +void PrintRegisters(const RegistersDumpT* dump) __attribute__((nonnull));kernel/sched/MLFQ.c (2)
29-34: Centralize flags in a header.
PROC_FLAG_NONE(and related flags) are effectively public; move them to a shared header (e.g., MLFQ.h) to avoid duplication and ensure consistency.
1815-1816: UsePROC_FLAG_NONEinstead of literal0for consistency.- uint32_t vfc_pid = CreateSecureProcess("VFCompositor", VFCompositor, PROC_PRIV_NORM, 0); + uint32_t vfc_pid = CreateSecureProcess("VFCompositor", VFCompositor, PROC_PRIV_NORM, PROC_FLAG_NONE);kernel/etc/Shell.c (2)
1130-1137: RegDumpHandler is fine; consider minor UX tweak.Prefix output with a header once (PrintRegisters already prints one). Optional.
649-651: Message says “ELF Executable loaded” even for PE.Use a generic message to match ExecLoader’s auto-detect.
- PrintKernelSuccess("ELF Executable loaded (PID: "); + PrintKernelSuccess("Executable loaded (PID: ");arch/x86_64/cpu/Cpu.c (3)
133-147: Use irq_flags_t and avoid standalone cli(); prefer atomic save/disable helper.Use the same type as Io.h and a single helper that both saves flags and disables interrupts to avoid reorder bugs.
Apply:
-static void DumpCR(RegistersDumpT* dump) { - unsigned long flags = save_irq_flags(); - cli(); // Critical section +static void DumpCR(RegistersDumpT* dump) { + irq_flags_t flags = save_irq_flags(); + cli(); // disable IF; if you have save_and_cli(), use that instead @@ - restore_irq_flags(flags); + restore_irq_flags(flags); }Also confirm cli() is declared in Io.h or a proper header; otherwise this won’t build.
159-169: Guard RDMSR reads by CPUID.MSR support or add a safe_rdmsr.On x86-64 it’s effectively guaranteed, but a quick MSR capability check (CPUID.01H:EDX.MSR) or a safe_rdmsr wrapper makes this robust and easier to reuse elsewhere. Also consider printing zeros on #GP instead of crashing.
I can add a safe_rdmsr(u32 msr, u64* out) that handles #GP via a fault handler hook if you have one.
171-176: Note on structure initialization.DumpGP writes 16-bit segment selectors; if callers forget to zero the dump, upper bits may contain stale data. Either memset in DumpRegisters or enforce zero-init in API docs.
Apply:
void DumpRegisters(RegistersDumpT* dump) { + // Ensure clean upper bits for 16-bit selectors + __builtin_memset(dump, 0, sizeof(*dump)); DumpGP(dump); DumpCR(dump); DumpDR(dump); DumpMSR(dump); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
arch/x86_64/cpu/Cpu.c(2 hunks)arch/x86_64/cpu/Cpu.h(3 hunks)arch/x86_64/interrupts/Interrupts.c(1 hunks)docs/ARCHITECTURE.md(1 hunks)kernel/core/Kernel.c(1 hunks)kernel/etc/Shell.c(6 hunks)kernel/execf/ExecLoader.c(2 hunks)kernel/execf/elf/ELFloader.c(1 hunks)kernel/execf/elf/ELFloader.h(1 hunks)kernel/execf/pe/PEloader.c(1 hunks)kernel/sched/MLFQ.c(3 hunks)kernel/sched/MLFQ.h(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
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#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.
📚 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:
arch/x86_64/cpu/Cpu.carch/x86_64/interrupts/Interrupts.ckernel/etc/Shell.carch/x86_64/cpu/Cpu.h
📚 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:
arch/x86_64/cpu/Cpu.carch/x86_64/interrupts/Interrupts.ckernel/etc/Shell.carch/x86_64/cpu/Cpu.h
📚 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: VoidFrame's interrupt handlers in arch/x86_64/interrupts/Interrupts.asm only save basic GPRs and segment registers, missing critical registers for deep fault tracing: control registers (CR0/CR2/CR3/CR4), SIMD state (XMM/YMM/ZMM), debug registers (DR0-DR7), and extended FPU state. The CPU initialization code enables SSE/AVX/XSAVE features but interrupt handlers don't preserve this extended state. Enhanced fault tracing requires selective extended state saving for critical exceptions (ISR 6, 8, 13, 14) while keeping lightweight handling for regular interrupts.
Applied to files:
arch/x86_64/cpu/Cpu.carch/x86_64/interrupts/Interrupts.ckernel/etc/Shell.carch/x86_64/cpu/Cpu.h
📚 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:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-08-14T02:11:27.395Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#82
File: kernel/process/Process.h:0-0
Timestamp: 2025-08-14T02:11:27.395Z
Learning: In VoidFrame kernel, FastMemcpy was using SSE2 movdqu instructions without preserving XMM registers, causing crashes when called from interrupt context (like the scheduler's FastSchedule function). The fix is to use only general-purpose registers with 64-bit unrolled copies to maintain performance while being interrupt-safe.
Applied to files:
arch/x86_64/interrupts/Interrupts.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:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-08-14T02:11:58.963Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#82
File: kernel/process/Process.h:0-0
Timestamp: 2025-08-14T02:11:58.963Z
Learning: In VoidFrame kernel's FastMemcpy implementation (kernel/memory/MemOps.c), memory corruption occurs under high load due to interrupts occurring between SSE2 movdqu load and store instructions, corrupting XMM register state. The fix requires disabling interrupts around SSE operations and adding memory barriers (sfence) to ensure stores complete atomically.
Applied to files:
arch/x86_64/interrupts/Interrupts.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:
arch/x86_64/interrupts/Interrupts.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-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/execf/elf/ELFloader.c
📚 Learning: 2025-09-02T02:37:06.993Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.h:153-154
Timestamp: 2025-09-02T02:37:06.993Z
Learning: In VoidFrame MLFQ.c, the ProcessRuntimePath cleanup should happen for both normal exit (ProcessExitStub → TerminateProcess with TERM_NORMAL) and kill (MLFQKillProcess → TerminateProcess with TERM_KILLED) since both go through the same TerminateProcess function which has VfsDelete cleanup code when VF_CONFIG_PROCINFO_AUTO_CLEANUP is enabled. If normal exit leaves directories but kill doesn't, this suggests VfsDelete might be failing silently for normal exits, or there's a race condition in the cleanup logic.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-02T00:55:58.224Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.224Z
Learning: The VoidFrame MLFQ scheduler has a critical bug where ProcessRuntimePath uses FormatS() which returns a static buffer, causing all processes to share the same path string. This results in process directory cleanup operations affecting the wrong directories - killing one process can delete another process's /Runtime/Processes directory because they all point to the same FormatS static buffer.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-02T00:55:58.224Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.224Z
Learning: The VoidFrame MLFQ ProcessRuntimePath bug occurs because: 1) ProcessRuntimePath is defined as char* (not char array), 2) All processes get assigned the same FormatS static buffer pointer, 3) Directory cleanup via VfsDelete happens during TerminateProcess() not final cleanup, 4) When terminating a process, VfsDelete uses whatever path is currently in the shared static buffer, causing wrong directories to be deleted. Fix requires either changing ProcessRuntimePath to char array or using dynamic allocation.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-02T00:55:58.224Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.224Z
Learning: In VoidFrame's MLFQ.c, line 1076 assigns ProcessRuntimePath using FormatS() which returns a static buffer, causing all processes to share the same path pointer. During cleanup, VfsDelete() can delete the wrong process directory because all ProcessRuntimePath fields point to the same static buffer containing the path of the most recently created process. This causes directory cleanup operations to affect unintended processes.
Applied to files:
kernel/sched/MLFQ.c
🧬 Code graph analysis (5)
arch/x86_64/cpu/Cpu.c (2)
include/Io.h (2)
save_irq_flags(38-42)restore_irq_flags(44-46)kernel/etc/Console.c (1)
PrintKernelF(260-267)
arch/x86_64/interrupts/Interrupts.c (1)
arch/x86_64/cpu/Cpu.c (2)
DumpRegisters(171-176)PrintRegisters(178-201)
kernel/core/Kernel.c (1)
kernel/etc/Console.c (1)
PrintKernelF(260-267)
kernel/etc/Shell.c (2)
arch/x86_64/cpu/Cpu.c (2)
DumpRegisters(171-176)PrintRegisters(178-201)kernel/etc/Console.c (2)
PrintKernel(181-199)PrintKernelSuccess(213-218)
arch/x86_64/cpu/Cpu.h (1)
arch/x86_64/cpu/Cpu.c (2)
DumpRegisters(171-176)PrintRegisters(178-201)
🔇 Additional comments (10)
kernel/sched/MLFQ.h (1)
89-92: Confirm no remaining references to PROC_PRIV_USERRename PROC_PRIV_USER → PROC_PRIV_NORM looks good; automated search produced no matches—re-run across the repo (e.g. rg -nP '\bPROC_PRIV_USER\b' -S) and update any hits.
kernel/sched/MLFQ.c (1)
1174-1175: LGTM: default to normal privilege with explicit no-flags.Clearer than a bare zero; consistent with the privilege rename.
kernel/execf/ExecLoader.c (2)
53-57: Defaulting to PROC_PRIV_NORM looks good.Consistent with privilege normalization across the PR.
63-67: PE path default privilege normalization is consistent.No issues.
kernel/execf/elf/ELFloader.c (1)
112-116: LGTM on the new default privilege.Matches the repo-wide rename to PROC_PRIV_NORM.
kernel/execf/pe/PEloader.c (1)
102-106: Default PROC_PRIV_NORM is fine.Aligned with the rest of the PR.
kernel/etc/Shell.c (3)
215-215: Help entry for regdump: good addition.Matches the new command.
641-645: execve defaults to PROC_PRIV_NORM: consistent.Works with your ExecLoader defaults.
1196-1200: Data-dir exec path defaulting to PROC_PRIV_NORM: consistent.Looks good.
arch/x86_64/cpu/Cpu.c (1)
3-3: Io.h include: confirm availability of cli()/sti() and rdmsr().This file relies on cli() and rdmsr(); ensure they’re declared in Io.h or add the appropriate header to avoid implicit-decl build issues under -Werror.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
arch/x86_64/interrupts/Interrupts.c (1)
74-82: Resolved: dumping the faulting context (RIP/CS/RFLAGS/RSP/SS) — LGTM.Overriding DumpRegisters with regs fields fixes the “current vs faulting” RIP mismatch during exceptions.
kernel/etc/Shell.c (1)
362-377: PanicHandler still passes NULL regs to PrintDetailedFaultInfo and PanicFromInterrupt (crash risk)regs is NULL; both paths may dereference it. Guard and call the non-interrupt Panic variant when regs is NULL.
static void PanicHandler(const char * args) { char* str = GetArg(args, 1); - Registers * regs = {0}; + Registers *regs = NULL; @@ FaultContext ctx = {0}; ctx.fault_reason = str; - PrintDetailedFaultInfo(&ctx, regs); + if (regs) { PrintDetailedFaultInfo(&ctx, regs); } RegistersDumpT dump = {0}; DumpRegisters(&dump); PrintRegisters(&dump); - PanicFromInterrupt(ctx.fault_reason, regs); + if (regs) { PanicFromInterrupt(ctx.fault_reason, regs); } + else { Panic(ctx.fault_reason); } }
🧹 Nitpick comments (9)
.github/workflows/main.yaml (3)
39-43: Make POST outcome explicit and tighten the failure grep.Assert the success line and use ERE for the failure patterns.
- timeout 30s ninja -C build runmin || true + timeout 30s ninja -C build runmin || true @@ - ! grep -i "panic\|fault\|crash\|oops\|error\|failed" build/serial.log + ! grep -Ei "(panic|fault|crash|oops|error|failed)" build/serial.log + grep -q "POST test passed" build/serial.log
40-40: Fix trailing whitespace flagged by yamllint.Line 40 is a blank line containing spaces.
- +
61-68: Consider uploading logs to help triage CI flakes.Adding bootstrap.log and serial.log as artifacts will speed up failure analysis.
- name: Upload logs uses: actions/upload-artifact@v4 with: name: voidframe-logs-${{ github.sha }} path: | build/bootstrap.log build/serial.log retention-days: 7kernel/etc/POST.h (1)
1-6: Add C++ linkage guards for mixed C/C++ builds.Prevents name mangling if included from C++.
#ifndef VOIDFRAME_POST_H #define VOIDFRAME_POST_H +#ifdef __cplusplus +extern "C" { +#endif void POSTHandler(const char * args); +#ifdef __cplusplus +} +#endif #endif // VOIDFRAME_POST_Hkernel/etc/POST.c (3)
41-51: Avoid PANIC on OOM during fuzz; fail the POST gracefully.Crashing the kernel on POST failure makes CI brittle. Return false and report instead.
- } else { - size_t sz = (rnd() % 8192) + 1; // 1–8K - ptrs[idx] = KernelMemoryAlloc(sz); - if (!ptrs[idx]) PANIC("OOM during fuzz"); - } + } else { + size_t sz = (rnd() % 8192) + 1; // 1–8K + ptrs[idx] = KernelMemoryAlloc(sz); + if (!ptrs[idx]) { + PrintKernelWarning("[POST] OOM during fuzz\n"); + return false; + } + }
72-76: Only print “POST test passed” if all tests passed.Today it prints success even after warnings.
- if (!SerialTest()) PrintKernelWarning("Serial test failed\n"); - if (!MemoryTest()) PrintKernelWarning("Memory test failed\n"); - PrintKernelSuccess("POST test passed\n"); + bool ok_serial = SerialTest(); + bool ok_mem = MemoryTest(); + if (ok_serial && ok_mem) { + PrintKernelSuccess("POST test passed\n"); + } else { + PrintKernelWarning("POST test failed\n"); + }
23-30: Minor: bound initial alloc sizes to allocator classes for faster path hits.Optional micro-optimization: use sizes that align with allocator size classes to improve caching.
kernel/etc/Shell.c (2)
1132-1139: RegDumpHandler implementation — OKCommand is simple and uses the new dump/print path correctly. Consider guarding availability behind a build flag if you ship a non-dev kernel.
33-33: Add explicit CPU header for register dump APIskernel/etc/Shell.c uses RegistersDumpT/DumpRegisters/PrintRegisters but does not directly include arch/x86_64/cpu/Cpu.h; add the include.
#include "Panic.h" +#include "arch/x86_64/cpu/Cpu.h" #include "Pic.h"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/main.yaml(1 hunks)arch/x86_64/interrupts/Interrupts.c(1 hunks)kernel/core/Kernel.c(1 hunks)kernel/etc/POST.c(1 hunks)kernel/etc/POST.h(1 hunks)kernel/etc/Shell.c(7 hunks)meson.build(4 hunks)meson_options.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kernel/core/Kernel.c
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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#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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.123Z
Learning: VoidFrame's interrupt handlers in arch/x86_64/interrupts/Interrupts.asm only save basic GPRs and segment registers, missing critical registers for deep fault tracing: control registers (CR0/CR2/CR3/CR4), SIMD state (XMM/YMM/ZMM), debug registers (DR0-DR7), and extended FPU state. The CPU initialization code enables SSE/AVX/XSAVE features but interrupt handlers don't preserve this extended state. Enhanced fault tracing requires selective extended state saving for critical exceptions (ISR 6, 8, 13, 14) while keeping lightweight handling for regular interrupts.
📚 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:
arch/x86_64/interrupts/Interrupts.ckernel/etc/Shell.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: VoidFrame's interrupt handlers in arch/x86_64/interrupts/Interrupts.asm only save basic GPRs and segment registers, missing critical registers for deep fault tracing: control registers (CR0/CR2/CR3/CR4), SIMD state (XMM/YMM/ZMM), debug registers (DR0-DR7), and extended FPU state. The CPU initialization code enables SSE/AVX/XSAVE features but interrupt handlers don't preserve this extended state. Enhanced fault tracing requires selective extended state saving for critical exceptions (ISR 6, 8, 13, 14) while keeping lightweight handling for regular interrupts.
Applied to files:
arch/x86_64/interrupts/Interrupts.ckernel/etc/Shell.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:
arch/x86_64/interrupts/Interrupts.ckernel/etc/Shell.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:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-08-14T02:11:27.395Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#82
File: kernel/process/Process.h:0-0
Timestamp: 2025-08-14T02:11:27.395Z
Learning: In VoidFrame kernel, FastMemcpy was using SSE2 movdqu instructions without preserving XMM registers, causing crashes when called from interrupt context (like the scheduler's FastSchedule function). The fix is to use only general-purpose registers with 64-bit unrolled copies to maintain performance while being interrupt-safe.
Applied to files:
arch/x86_64/interrupts/Interrupts.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:
arch/x86_64/interrupts/Interrupts.ckernel/etc/Shell.c
📚 Learning: 2025-08-14T02:11:58.963Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#82
File: kernel/process/Process.h:0-0
Timestamp: 2025-08-14T02:11:58.963Z
Learning: In VoidFrame kernel's FastMemcpy implementation (kernel/memory/MemOps.c), memory corruption occurs under high load due to interrupts occurring between SSE2 movdqu load and store instructions, corrupting XMM register state. The fix requires disabling interrupts around SSE operations and adding memory barriers (sfence) to ensure stores complete atomically.
Applied to files:
arch/x86_64/interrupts/Interrupts.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:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-09-01T08:17:56.769Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#105
File: meson.build:252-257
Timestamp: 2025-09-01T08:17:56.769Z
Learning: In VoidFrame's Cerberus security system, CerberusRegisterProcess() causes kernel panics because CerberusInstallStackCanary() performs direct memory writes (*canary_ptr = STACK_CANARY_VALUE) during process creation. The stack memory may not be fully mapped/accessible at this point, causing page faults in critical kernel context. The fix is to either defer canary installation, add memory mapping validation, or skip canary installation during process registration.
Applied to files:
kernel/etc/Shell.c
🧬 Code graph analysis (4)
kernel/etc/POST.h (1)
kernel/etc/POST.c (1)
POSTHandler(72-77)
arch/x86_64/interrupts/Interrupts.c (1)
arch/x86_64/cpu/Cpu.c (2)
DumpRegisters(171-176)PrintRegisters(178-201)
kernel/etc/POST.c (5)
drivers/Serial.c (1)
SerialWrite(129-138)mm/KernelHeap.c (2)
KernelMemoryAlloc(272-330)KernelFree(377-422)mm/PMem.c (2)
AllocPage(178-234)FreePage(276-314)mm/VMem.c (2)
VMemMap(135-195)VMemUnmap(516-555)kernel/etc/Console.c (2)
PrintKernelWarning(227-232)PrintKernelSuccess(213-218)
kernel/etc/Shell.c (3)
arch/x86_64/cpu/Cpu.c (2)
DumpRegisters(171-176)PrintRegisters(178-201)kernel/etc/Console.c (2)
PrintKernel(181-199)PrintKernelSuccess(213-218)kernel/etc/POST.c (1)
POSTHandler(72-77)
🪛 YAMLlint (1.37.1)
.github/workflows/main.yaml
[error] 40-40: trailing spaces
(trailing-spaces)
⏰ 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 (6)
meson_options.txt (1)
1-2: Meson option looks good.automatic_post is clearly documented and off by default.
meson.build (2)
22-23: Option plumbed correctly.automatic_post is read once and reused to set VF_CONFIG_AUTOMATIC_POST.
298-299: Confirmed: vmware-svga → VMware SVGA‑II (PCI 0x15AD:0x0405).drivers/vmware/SVGAII.h defines vendor 0x15AD and device 0x0405, which matches QEMU's vmware-svga (SVGA II) emulation. (fossies.org)
qemu-system-x86_64 was not available in the sandbox — run
qemu-system-x86_64 --versionandqemu-system-x86_64 -device helpon your CI/runners to confirm the installed QEMU supports vmware-svga.kernel/etc/Shell.c (3)
216-222: Help: regdump/post entries wired — OKEntries look consistent with handlers and category placement.
1188-1190: Command table: regdump/post wired — OKHandlers are correctly exposed; ordering matches help.
642-647: Privilege rename verified — no PROC_PRIV_USER; PROC_PRIV_NORM = 1Search found zero PROC_PRIV_USER occurrences; PROC_PRIV_NORM is defined in kernel/sched/MLFQ.h as
#define PROC_PRIV_NORM 1and callers (ExecLoader, ELF/PE loaders, MLFQ, Shell) use PROC_PRIV_NORM.
| RegistersDumpT dump = {0}; | ||
| DumpRegisters(&dump); | ||
| // Override with fault context where applicable | ||
| dump.rip = regs->rip; | ||
| dump.cs = regs->cs; | ||
| dump.rflags = regs->rflags; | ||
| dump.rsp = regs->rsp; | ||
| dump.ss = regs->ss; | ||
| PrintRegisters(&dump); | ||
| // Still panic, but now with much more info |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-fault safety and panic CLI ordering — please verify.
Printing on DF can triple-fault unless ISR #8 uses an IST stack; also ensure PanicFromInterrupt disables interrupts as its first instruction.
🏁 Script executed:
#!/bin/bash
# 1) Verify DF (vector 8) uses IST in IDT setup / ASM stubs
rg -n -C3 -i 'double\s*fault|isr8|vector\s*8|ist' arch/x86_64
# 2) Confirm PanicFromInterrupt (and other panic entry points) execute cli first
rg -n -C3 'PanicFromInterrupt\s*\(|KernelPanicHandler\s*\(|Panic\w*\(' kernel | sed -n '1,200p'
rg -n 'asm\s+volatile\("cli"\)' kernelLength of output: 27567
Ensure double-fault uses an IST stack; panic CLI ordering is correct
- arch/x86_64/idt/Idt.c:295 currently calls
IdtSetGate(8, (uint64_t)isr8, kernelCodeSegment, flags);
without an IST index—assign a dedicated IST (and configure TSS.istN) for vector 8 to avoid triple-faults. - No change needed for PanicFromInterrupt or other panic entry points—all invoke
cli()as their first instruction.
| for (uintptr_t addr = 0x400000; addr < 0x800000; addr += 0x1000) { | ||
| void* frame = AllocPage(); | ||
| VMemMap(addr, (uint64_t)frame, PAGE_PRESENT | PAGE_WRITABLE); | ||
| VMemUnmap(addr, PAGE_SIZE); | ||
| FreePage(frame); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check map/unmap return codes and handle page alloc failure.
Ignoring errors risks leaking mapped pages or freeing still-mapped frames.
- for (uintptr_t addr = 0x400000; addr < 0x800000; addr += 0x1000) {
- void* frame = AllocPage();
- VMemMap(addr, (uint64_t)frame, PAGE_PRESENT | PAGE_WRITABLE);
- VMemUnmap(addr, PAGE_SIZE);
- FreePage(frame);
- }
+ for (uintptr_t addr = 0x400000; addr < 0x800000; addr += 0x1000) {
+ void* frame = AllocPage();
+ if (!frame) return false;
+ int rc = VMemMap(addr, (uint64_t)frame, PAGE_PRESENT | PAGE_WRITABLE);
+ if (rc != VMEM_SUCCESS) { FreePage(frame); return false; }
+ rc = VMemUnmap(addr, PAGE_SIZE);
+ if (rc != VMEM_SUCCESS) { FreePage(frame); return false; }
+ FreePage(frame);
+ }Committable suggestion skipped: line range outside the PR's diff.
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Free any remaining fuzz allocations to avoid a boot-time leak.
POST runs once but leaking kernel heap here is avoidable.
- return true;
+ // Cleanup any remaining allocations from fuzzing
+ for (int i = 0; i < N; i++) {
+ if (ptrs[i]) { KernelFree(ptrs[i]); ptrs[i] = NULL; }
+ }
+ return true;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kernel/etc/POST.c around lines 69-70, the function returns true without
freeing any remaining fuzz allocations, causing a boot-time kernel-heap leak;
locate the container(s) used to track fuzz allocations (e.g., fuzz buffer
list/array/struct and any count fields), iterate over each stored allocation and
call the appropriate kernel free function for each entry, clear or reset the
container/count after freeing, and ensure any ownership flags are updated so no
double-free occurs before returning true.
1 participant
Summary by CodeRabbit
New Features
Improvements
Changes in Defaults
Documentation
Build/CI