Development #134
Development #134
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a TSC timing module and replaces header dependencies; removes an inline delay from Cpu.h. Introduces full ACPI subsystem (init/shutdown/reboot) and wires it into kernel and shell. Reworks APIC timer calibration. Extends RTC with Unix-time APIs and records Ext2 deletion times. Adjusts AHCI timeouts and build sources. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant K as Kernel
participant TSC as TSC Driver
participant APIC as APIC Timer
participant CPU as rdtsc
rect rgb(235,245,255)
note right of K: TSC calibration flow
K->>TSC: TSCInit()
TSC->>APIC: configure calibration interval
TSC->>CPU: rdtsc (start)
APIC-->>TSC: wait interval elapses
TSC->>CPU: rdtsc (end)
TSC-->>K: compute/store tsc_freq_hz
end
rect rgb(245,235,255)
note right of K: Delay using TSC
K->>TSC: delay_us(n)
TSC->>CPU: rdtsc-loop until target cycles reached
TSC-->>K: return
end
sequenceDiagram
autonumber
participant K as Kernel
participant ACPI as ACPI Subsystem
participant FW as Firmware (BIOS/EBDA)
participant PWR as PM Controller
rect rgb(235,255,235)
note right of K: ACPI init & shutdown flow
K->>ACPI: ACPIInit()
ACPI->>FW: search RSDP (EBDA/BIOS)
ACPI-->>ACPI: map/validate RSDT/FADT checksums
ACPI-->>K: init ok / fail
K->>ACPI: ACPIShutdown()
ACPI->>PWR: enable ACPI mode if required
ACPI->>PWR: write SLP_TYP | SLP_EN
alt if fails
ACPI-->>PWR: try fallbacks (I/O, QEMU methods)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
drivers/RTC/Rtc.c (1)
96-101: Critical: Misuse of Rtc_WriteRegister when toggling NMI and updates.Rtc_WriteRegister writes both index and data ports; passing RTC_CMOS_ADDRESS as a “register” writes garbage. Use direct outb to the index port to set/clear the NMI mask, and write Status B via data port only when the index is set to Status B.
Apply:
- // Disable NMI and updates while setting time - Rtc_WriteRegister(RTC_CMOS_ADDRESS, RTC_STATUS_B | 0x80); // Disable NMI - Rtc_WriteRegister(RTC_STATUS_B, statusB | 0x80); // Disable updates + // Disable NMI (select reg B with NMI masked) and disable updates via Status B bit 7 + outb(RTC_CMOS_ADDRESS, RTC_STATUS_B | 0x80); // NMI masked, select Status B + outb(RTC_CMOS_DATA, statusB | 0x80); // Disable updates @@ - // Re-enable updates and NMI - Rtc_WriteRegister(RTC_STATUS_B, statusB); - Rtc_WriteRegister(RTC_CMOS_ADDRESS, RTC_STATUS_B); // Re-enable NMI + // Re-enable updates and NMI + outb(RTC_CMOS_ADDRESS, RTC_STATUS_B | 0x80); // Keep NMI masked while restoring Status B + outb(RTC_CMOS_DATA, statusB); // Restore Status B + outb(RTC_CMOS_ADDRESS, RTC_STATUS_B); // Unmask NMI (bit7=0), still selecting Status BAlso applies to: 128-131
fs/EXT/Ext2.c (1)
1026-1056: Avoid nested lock acquisition in Ext2Delete (fs/EXT/Ext2.c:1026–1056)
Holding the write‐lock while calling Ext2ReadInode/Ext2WriteInode (which themselves lockvolume.lock) will deadlock on non-reentrant rwlocks. Introduce “unlocked” variants that assume the caller already holds the lock, or refactor to acquire the lock only once around the entire read–modify–write sequence.drivers/PCI/PCI.c (1)
208-285: BAR size helper hardcodes BAR0 offset — can read/overwrite the wrong BAR.GetPCIMMIOSize always probes offset 0x10 (BAR0) even if the selected BAR was at a different index. This can produce a wrong size and briefly clobber the wrong BAR.
Apply this refactor and update call sites to pass the found BAR index:
-uint64_t GetPCIMMIOSize(const PciDevice* pci_dev, uint32_t bar_value) { +uint64_t GetPCIMMIOSize(const PciDevice* pci_dev, uint8_t bar_index, uint32_t bar_value) { @@ - uint8_t bar_offset = 0x10; // We'll assume BAR0 for now, but this could be parameterized + uint8_t bar_offset = 0x10 + (bar_index * 4); @@ - uint32_t actual_bar = PciConfigReadDWord(pci_dev->bus, pci_dev->device, pci_dev->function, bar_offset); + uint32_t actual_bar = PciConfigReadDWord(pci_dev->bus, pci_dev->device, pci_dev->function, bar_offset); @@ - if (is_64bit) { + if (is_64bit) { // Read the upper 32 bits for 64-bit BARs original_bar_high = PciConfigReadDWord(pci_dev->bus, pci_dev->device, pci_dev->function, bar_offset + 4); PrintKernel("GetPCIMMIOSize: Detected 64-bit BAR\n"); }Then in drivers/xHCI/xHCI.c, pass the detected index i:
- uint64_t mmio_size = GetPCIMMIOSize(&controller->pci_device, bar_for_size_calc); + uint64_t mmio_size = GetPCIMMIOSize(&controller->pci_device, (uint8_t)i, bar_for_size_calc);drivers/xHCI/xHCI.c (5)
546-548: Doorbell base is wrong (using port regs space). Must use DBOFF.Doorbells aren’t at operational+0x400; they’re at Capability base + DBOFF. Writing here won’t ring commands/endpoints.
Patch to use DBOFF (add once and reuse):
+#define XHCI_CAP_DBOFF 0x14 // Doorbell Offset @@ static void xHCIRingCommandDoorbell(XhciController* controller) { - // Ring doorbell for command ring (doorbell 0) - volatile uint32_t* doorbell_regs = (volatile uint32_t*)((uint8_t*)controller->operational_regs + 0x400 + (controller->max_ports * 0x10)); - xHCIWriteRegister(&doorbell_regs[0], 0); + uint32_t db_off = *((volatile uint32_t*)((uint8_t*)controller->mmio_base + XHCI_CAP_DBOFF)); + volatile uint32_t* doorbell_regs = + (volatile uint32_t*)((uint8_t*)controller->mmio_base + (db_off & 0xFFFFFFFC)); + xHCIWriteRegister(&doorbell_regs[0], 0); }Repeat the same doorbell_base fix for the writes at Lines 725-727 and 1027-1030.
971-981: Severe OOB: allocating 1 byte for a 4KB ring, then memset 4096.VMemAlloc(1) then FastMemset(..., 4096) corrupts memory; also ring must hold 256 TRBs.
- XhciTRB* ep_ring = (XhciTRB*)VMemAlloc(1); + XhciTRB* ep_ring = (XhciTRB*)VMemAlloc(TRANSFER_RING_SIZE * sizeof(XhciTRB)); if (!ep_ring) return -1; - - FastMemset(ep_ring, 0, 4096); + FastMemset(ep_ring, 0, TRANSFER_RING_SIZE * sizeof(XhciTRB)); @@ - ep_ring[255].parameter_lo = (uint32_t)(uintptr_t)ep_ring; - ep_ring[255].parameter_hi = (uint32_t)((uintptr_t)ep_ring >> 32); - ep_ring[255].control = (6 << 10) | (1 << 1) | 1; + ep_ring[TRANSFER_RING_SIZE - 1].parameter_lo = (uint32_t)(uintptr_t)ep_ring; + ep_ring[TRANSFER_RING_SIZE - 1].parameter_hi = (uint32_t)((uintptr_t)ep_ring >> 32); + ep_ring[TRANSFER_RING_SIZE - 1].control = (TRB_TYPE_LINK << 10) | (1 << 1) | 1;
990-991: Wrong TRB type for Configure Endpoint command.You’re emitting Address Device (11) instead of Configure Endpoint (12).
- cmd_trb->control = (11 << 10) | (controller->command_ring_cycle ? 1 : 0); + cmd_trb->control = (12 << 10) | (controller->command_ring_cycle ? 1 : 0);
636-639: Slot speed/port number setup likely incorrect.speed=4 is SuperSpeed in xHCI, not “Full speed”. root_hub_port_number is hardcoded 1. Use the actual port’s PORTSC speed and port index.
Suggest passing port into xHCIAddressDevice(...) and setting:
- slot.root_hub_port_number = port + 1
- slot.speed = (PORTSC_SPEED from that port)
725-727: Same doorbell-address bug at endpoint rings.Replace both spots to use DBOFF-derived base as in the earlier fix.
- volatile uint32_t* doorbell_regs = (volatile uint32_t*)((uint8_t*)controller->operational_regs + 0x400 + (controller->max_ports * 0x10)); - xHCIWriteRegister(&doorbell_regs[slot_id], 1); // EP0 + uint32_t db_off = *((volatile uint32_t*)((uint8_t*)controller->mmio_base + XHCI_CAP_DBOFF)); + volatile uint32_t* doorbell_regs = + (volatile uint32_t*)((uint8_t*)controller->mmio_base + (db_off & 0xFFFFFFFC)); + xHCIWriteRegister(&doorbell_regs[slot_id], 1); // EP0- volatile uint32_t* doorbell = (volatile uint32_t*)((uintptr_t)controller->mmio_base + - 0x1000 + (slot_id * 4)); - *doorbell = endpoint; + uint32_t db_off = *((volatile uint32_t*)((uint8_t*)controller->mmio_base + XHCI_CAP_DBOFF)); + volatile uint32_t* doorbell_regs = + (volatile uint32_t*)((uint8_t*)controller->mmio_base + (db_off & 0xFFFFFFFC)); + doorbell_regs[slot_id] = endpoint;Also applies to: 1027-1030
drivers/sound/SB16.c (2)
47-47: Use the appropriate delay function for microseconds.Similar to the previous issue,
delay(10)will wait for 10ms instead of 10µs.- delay(10); + delay_us(10);
11-11: Use the microsecond delay API
delay()is in milliseconds, sodelay(1000)blocks for 1 s instead of ~3 µs. Replace with:- delay(1000); // Adjust loop count if needed. + delay_us(3); // Wait for at least 3 microsecondsdrivers/sound/Generic.c (1)
45-45: Use the correct API for microsecond delays
In drivers/sound/Generic.c line 45, replacedelay(duration_ms * 1000); // Convert ms to microsecondswith
delay_us(duration_ms * 1000);
delay()takes milliseconds, so passing a microsecond count causes a 1000× longer delay.
🧹 Nitpick comments (15)
fs/EXT/Ext2.c (1)
1050-1053: Make truncation explicit; ext2 i_dtime is 32‑bit.RtcGetUnixTime() returns uint64_t. ext2’s i_dtime is typically 32‑bit seconds. Make the cast explicit and document 2106 wrap considerations.
- inode.i_dtime = RtcGetUnixTime(); + // ext2 on-disk i_dtime is 32-bit seconds since 1970; explicit cast to avoid implicit truncation. + uint64_t now = RtcGetUnixTime(); + inode.i_dtime = (uint32_t)now;drivers/RTC/Rtc.c (3)
1-1: Normalize include path.Rtc.c is in the same directory as Rtc.h. Using "RTC/Rtc.h" risks a bad relative resolution.
-#include "RTC/Rtc.h" +#include "Rtc.h"If you prefer tree-anchored includes, standardize across the project (e.g., #include "drivers/RTC/Rtc.h") and set the include paths accordingly.
140-163: Bounds hardening for month/day before indexing.If RTC returns an invalid month (0 or >12), the loop and lookup can go out-of-bounds or miscompute. Cheap guard improves robustness.
- for (uint8_t m = 1; m < dt.month; m++) { + uint8_t month = (dt.month >= 1 && dt.month <= 12) ? dt.month : 1; + for (uint8_t m = 1; m < month; m++) { days += days_in_month[m - 1]; - if (m == 2 && is_leap_year(dt.year)) days++; // February in leap year + if (m == 2 && is_leap_year(dt.year)) days++; // February in leap year } - days += dt.day - 1; + uint8_t day = (dt.day >= 1 && dt.day <= 31) ? dt.day : 1; + days += day - 1;
165-191: RtcSetUnixTime: minor resilience improvement.Clamp computed month/day to valid ranges before calling RtcSetTime to avoid setting invalid RTC values on edge inputs.
Example:
- dt.month = 1; + dt.month = 1; while (days >= days_in_month[dt.month - 1] + (dt.month == 2 && is_leap_year(dt.year) ? 1 : 0)) { days -= days_in_month[dt.month - 1] + (dt.month == 2 && is_leap_year(dt.year) ? 1 : 0); dt.month++; } - dt.day = days + 1; + dt.day = (uint8_t)(days + 1); + if (dt.month < 1 || dt.month > 12) dt.month = 1; + if (dt.day < 1 || dt.day > 31) dt.day = 1;drivers/PCI/PCI.c (1)
199-201: Avoid magic “temporary” delay in hot path.The unconditional delay(1000) in PciFindByClass adds latency and can still be a no-op if TSC isn’t calibrated. Prefer a readiness/poll condition or remove it.
- delay(1000); // Temporary timing fixREADME.md (1)
160-160: Fix list indentation to satisfy markdownlint MD007.Indentation of the “TSC” bullet doesn’t match siblings.
- - [x] TSC (usable delay(), finally!) + - [x] TSC (usable delay(), finally!)drivers/TSC.h (1)
4-4: Guard: document init-before-use requirement.Add a brief comment that delay_* require TSCInit() to have succeeded.
void TSCInit(void); -void delay_us(uint32_t microseconds); +// Requires successful TSCInit() +void delay_us(uint32_t microseconds);drivers/APIC.c (1)
211-212: Consider making the calibration timeout configurable.The hardcoded timeout value of 100000 might be insufficient on some systems or during high load conditions.
+#define APIC_CALIBRATION_TIMEOUT 100000 + - uint32_t timeout = 100000; + uint32_t timeout = APIC_CALIBRATION_TIMEOUT;Also applies to: 217-218
drivers/ACPI.c (1)
150-152: Consider using ACPI-defined shutdown valuesThe hardcoded shutdown values array may not work on all systems. Consider using ACPI-defined values from the FADT.
The shutdown values should ideally come from parsing the FADT's PM1a_CNT_BLK SLP_TYPa values for S5 (shutdown) state. You may want to properly parse the DSDT/SSDT tables to get the correct _S5 package values.
kernel/core/Kernel.c (1)
645-650: Consider making ACPI initialization critical for power managementACPI initialization failure is only logged as a warning, but this means shutdown/reboot functionality may not work properly. Consider whether this should be a more serious error or if there should be a fallback mechanism clearly documented.
// Initialize ACPI for power management if (ACPIInit()) { PrintKernelSuccess("System: ACPI initialized\n"); } else { PrintKernelWarning("System: ACPI initialization failed\n"); + PrintKernelWarning("System: Shutdown/reboot may not work correctly\n"); }kernel/etc/Shell.c (1)
186-187: Help entry added; consider clearer verbs.
Optional: use "acpi shutdown/reboot" instead of "sd/rb" to reduce ambiguity in help output.drivers/ACPI.h (2)
36-76: FADT is limited to ACPI 1.0; consider v2+ fields for robustness.
Adding GAS-based fields (e.g., reset_reg, x_pm1*_block, XSDT support) will improve shutdown/reboot reliability across platforms.typedef struct { ACPISDTHeader header; ... uint32_t flags; + // ACPI 2.0+ (optional, for robustness) + // struct ACPIGAS reset_reg; + // uint8_t reset_value; + // uint64_t x_firmware_ctrl; + // uint64_t x_dsdt; + // struct ACPIGAS x_pm1a_control_block; + // struct ACPIGAS x_pm1b_control_block; } __attribute__((packed)) ACPIFADT;
78-81: SLP_ defines OK; add SLP_TYP shift helper for clarity.*
Minor readability aid.#define ACPI_SLP_TYP_MASK 0x1C00 #define ACPI_SLP_EN 0x2000 +// #define ACPI_SLP_TYP_SHIFT 10drivers/TSC.c (2)
8-18: APIC_HZ vs s_apic_bus_freq mismatch.
You gate on APIC_HZ but use s_apic_bus_freq for ticks. If APIC_HZ!=0 while s_apic_bus_freq==0, calibration breaks. Gate on the value actually used.- if (APIC_HZ == 0) { + if (s_apic_bus_freq == 0) {
20-31: Serialize rdtsc for accuracy.
Add lfence (or use rdtscp) before/after rdtsc to bound measurement.- uint64_t start_tsc = rdtsc(); + __asm__ __volatile__("lfence" ::: "memory"); + uint64_t start_tsc = rdtsc(); ... - uint64_t end_tsc = rdtsc(); + uint64_t end_tsc = rdtsc(); + __asm__ __volatile__("lfence" ::: "memory");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
README.md(1 hunks)arch/x86_64/cpu/Cpu.h(0 hunks)drivers/ACPI.c(1 hunks)drivers/ACPI.h(1 hunks)drivers/APIC.c(2 hunks)drivers/PCI/PCI.c(1 hunks)drivers/RTC/Rtc.c(1 hunks)drivers/RTC/Rtc.h(1 hunks)drivers/TSC.c(1 hunks)drivers/TSC.h(1 hunks)drivers/ethernet/intel/E1000.c(1 hunks)drivers/sound/Generic.c(1 hunks)drivers/sound/SB16.c(1 hunks)drivers/storage/AHCI.c(5 hunks)drivers/xHCI/xHCI.c(1 hunks)fs/EXT/Ext2.c(2 hunks)kernel/core/Kernel.c(2 hunks)kernel/etc/Shell.c(4 hunks)meson.build(2 hunks)
💤 Files with no reviewable changes (1)
- arch/x86_64/cpu/Cpu.h
🧰 Additional context used
🧠 Learnings (2)
📚 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:
drivers/xHCI/xHCI.c
📚 Learning: 2025-09-16T12:30:38.427Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#131
File: drivers/Apic.c:179-199
Timestamp: 2025-09-16T12:30:38.427Z
Learning: In the VoidFrame kernel APIC implementation in drivers/Apic.c, the hardcoded 100MHz APIC bus frequency assumption causes extremely high CPU usage. The user found that using frequency_hz directly as initial_count (instead of the complex calculation) provides the same smoothness with much lower CPU usage, indicating their system has a much slower APIC bus than assumed.
Applied to files:
drivers/APIC.c
🧬 Code graph analysis (10)
drivers/TSC.h (1)
drivers/TSC.c (5)
TSCInit(8-33)delay_us(35-42)delay(44-46)delay_s(48-50)TSCGetFrequency(52-54)
drivers/ACPI.c (4)
mm/MemOps.c (1)
FastMemcmp(204-240)mm/VMem.c (4)
VMemAlloc(385-423)VMemUnmap(539-590)VMemFree(425-449)VMemMapMMIO(622-673)kernel/etc/Console.c (4)
PrintKernel(174-192)PrintKernelError(213-218)PrintKernelHex(228-251)PrintKernelInt(298-322)include/Io.h (2)
outw(16-18)outb(6-8)
drivers/RTC/Rtc.h (1)
drivers/RTC/Rtc.c (2)
RtcGetUnixTime(140-163)RtcSetUnixTime(165-192)
fs/EXT/Ext2.c (1)
drivers/RTC/Rtc.c (1)
RtcGetUnixTime(140-163)
drivers/TSC.c (1)
kernel/etc/Console.c (2)
PrintKernelWarning(220-225)PrintKernelF(253-260)
kernel/etc/Shell.c (4)
kernel/etc/Console.c (1)
PrintKernel(174-192)kernel/etc/StringOps.c (1)
FastStrCmp(43-53)drivers/ACPI.c (2)
ACPIShutdown(135-174)ACPIReboot(176-186)mm/KernelHeap.c (1)
KernelFree(480-525)
drivers/storage/AHCI.c (1)
drivers/TSC.c (1)
delay(44-46)
drivers/ACPI.h (1)
drivers/ACPI.c (3)
ACPIInit(67-133)ACPIShutdown(135-174)ACPIReboot(176-186)
kernel/core/Kernel.c (2)
drivers/TSC.c (1)
TSCInit(8-33)drivers/ACPI.c (1)
ACPIInit(67-133)
drivers/APIC.c (2)
include/Io.h (2)
outb(6-8)inb(10-14)kernel/etc/Console.c (1)
PrintKernelWarning(220-225)
🪛 markdownlint-cli2 (0.18.1)
README.md
160-160: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Gitleaks (8.28.0)
drivers/APIC.c
[high] 227-227: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (20)
drivers/RTC/Rtc.h (1)
23-26: Unix time API addition looks good.Signatures are clear and match the implementation intent.
Please confirm all call sites expect UTC (no TZ/DST adjustments) and that consumers with 32‑bit on-disk fields (e.g., ext2 i_dtime) handle the width difference explicitly, as adjusted in Ext2.c.
drivers/RTC/Rtc.c (1)
133-139: LGTM on helpers.days_in_month and is_leap_year are correct and file-scoped.
drivers/xHCI/xHCI.c (2)
5-5: Header switch to TSC is OK.No direct Cpu.h use here; include change is safe.
321-325: Follow-up to BAR size fix: pass BAR index into GetPCIMMIOSize.After locating the 64-bit BAR at index i, use that index when sizing.
- uint64_t mmio_size = GetPCIMMIOSize(&controller->pci_device, bar_for_size_calc); + uint64_t mmio_size = GetPCIMMIOSize(&controller->pci_device, (uint8_t)i, bar_for_size_calc);drivers/ethernet/intel/E1000.c (1)
5-5: TSC initialization occurs before E1000 reset delay TSCInit() is called in kernel/core/Kernel.c at line 642 during early boot, prior to E1000_Init() being invoked in drivers/ethernet/Network.c.drivers/PCI/PCI.c (1)
4-4: TSCInit ordering is correct. TSCInit() in kernel/core/Kernel.c runs before any PCI scan or delay() calls (including PciFindByClass’s delay), so delay() will be properly calibrated.drivers/sound/Generic.c (1)
3-3: Header dependency change looks good.The migration from
Cpu.htoTSC.haligns with the broader refactoring across the codebase to centralize timing utilities.drivers/sound/SB16.c (1)
3-3: Header dependency change looks good.Consistent with the project-wide migration to TSC-based timing utilities.
drivers/storage/AHCI.c (2)
3-3: Header dependency migration approved.The change from
Cpu.htoTSC.his consistent with the project-wide refactoring.
200-200: Verify the impact of reduced completion timeout.The completion timeout has been reduced from 5000 to 500 iterations. Combined with the shorter delays, this represents a significant reduction in total wait time.
This could cause issues with slower drives or during high I/O load. Consider monitoring this in production or providing a way to adjust timeouts based on device characteristics.
drivers/APIC.c (4)
48-49: Global variables for calibration state look good.The addition of
s_apic_bus_freqands_calibratedimplements a sensible caching mechanism for the expensive calibration operation.Based on learnings
191-234: Excellent improvement to APIC timer calibration!This refactoring addresses the performance issues identified in previous learnings by:
- Performing calibration only once and caching the result
- Adding timeout protection to prevent hangs
- Including a sensible fallback value
The one-time calibration pattern significantly reduces CPU usage compared to the previous implementation.
Based on learnings
231-231: Good use of fallback frequency.The 100MHz fallback aligns with common APIC bus frequencies and provides a reasonable default when calibration fails.
43-43: s_lapic_base external usage is intentional
The symbol is referenced viaexternin drivers/TSC.c (lines 24–26) for APIC timer calibration, so removingstaticwas required to allow that access. Access occurs during single-core early-boot, so no additional synchronization is needed.meson.build (1)
203-205: LGTM! Clean integration of new driversThe new TSC and ACPI driver source files are properly added to the build system.
kernel/core/Kernel.c (1)
641-643: LGTM! TSC initialization properly sequencedTSC initialization is correctly placed after APIC initialization, which it depends on for calibration.
kernel/etc/Shell.c (2)
5-5: Header include looks correct.
ACPI public API is used by this file; include is appropriate.
1212-1213: Command registration LGTM.
Command table entry matches handler and help entry.drivers/ACPI.h (1)
1-4: Header guard + includes are fine.
No issues.drivers/TSC.c (1)
32-33: The script has been queued to locatePrintKernelFdefinitions and usages.
| #include "../../mm/MemOps.h" | ||
| #include "../VFS.h" | ||
| #include "FileSystem.h" | ||
| #include "Rtc.h" |
There was a problem hiding this comment.
Fix header include path to avoid build break.
This file sits at fs/EXT/Ext2.c while the header is drivers/RTC/Rtc.h. Including "Rtc.h" without a path is unlikely to resolve unless an include dir is configured. Recommend using a stable relative path.
Apply:
-#include "Rtc.h"
+#include "../../drivers/RTC/Rtc.h"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include "Rtc.h" | |
| #include "../../drivers/RTC/Rtc.h" |
🤖 Prompt for AI Agents
In fs/EXT/Ext2.c around line 11 the include uses "Rtc.h" which can break builds;
replace it with the stable relative path to the header (e.g. #include
"../../drivers/RTC/Rtc.h") so the compiler can locate drivers/RTC/Rtc.h from
fs/EXT. Update the include line accordingly and verify compilation.
| command : [ | ||
| qemu_system_x86_64, | ||
| '-cpu', 'max', | ||
| '-cpu', 'host', |
There was a problem hiding this comment.
Security concern: Using '-cpu host' with KVM
Using -cpu host exposes all host CPU features to the guest, which may include features that the kernel doesn't properly handle or validate. This could lead to unexpected behavior or security issues.
Consider using a more conservative CPU model for testing:
- '-cpu', 'host',
+ '-cpu', 'qemu64,+sse,+sse2,+sse3,+ssse3,+sse4.1,+sse4.2',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| '-cpu', 'host', | |
| '-cpu', 'qemu64,+sse,+sse2,+sse3,+ssse3,+sse4.1,+sse4.2', |
🤖 Prompt for AI Agents
In meson.build around line 411, the QEMU invocation uses the '-cpu', 'host'
option which exposes all host CPU features to the guest; change this to a
conservative, well-defined CPU model for testing (for example 'qemu64' or
'kvm64' or a specific stable model like 'haswell') or make the CPU model
configurable via a build option/environment variable with a safe default; update
the invocation to use that safer model and add a brief comment explaining why
'host' is avoided for tests.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation