Development #168
Development #168
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request centralizes APIC state into a PerCpuData structure and exposes a GetPerCpuData() accessor plus new APIC/PIC and timer APIs. TSC calibration and APIC/timer code now use per-CPU fields. The ISR_ERRCODE macro was removed. Build system changes silence rule messages, add SILENT_BUILD-controlled assembler warning flags and a Corrosion_CARGO_BUILD_FLAGS public variable, and integrate a corrosion subdirectory with Rust crate flags. A parsing alignment and RIP-relative directive were added in pxs.asm. No exported public type signatures were removed. Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/APIC/APIC.c (1)
194-247: Excellent calibration approach that addresses previous issues.The dynamic APIC bus frequency calibration using PIT correctly measures the actual hardware frequency instead of assuming a hardcoded value. This directly addresses the high CPU usage issue from PR#131 where a hardcoded 100MHz assumption didn't match the actual hardware. The formula
apic_bus_freq / frequency_hz(line 244) properly computes the initial count for the desired timer frequency.Based on learnings from PR#131.
Optional: Consider named constants for calibration values.
Lines 236 and 239 use magic numbers that could be more readable as named constants:
+#define APIC_CALIBRATION_SCALE_FACTOR 100 // Scale 10ms measurement to 1 second +#define APIC_FALLBACK_BUS_FREQ_HZ 100000000 // 100MHz fallback + void ApicTimerSetFrequency(uint32_t frequency_hz) {Then update the usage:
- cpu_data->apic_bus_freq = ticks_per_10ms * 100; + cpu_data->apic_bus_freq = ticks_per_10ms * APIC_CALIBRATION_SCALE_FACTOR;- cpu_data->apic_bus_freq = 100000000; // Fallback: 100MHz + cpu_data->apic_bus_freq = APIC_FALLBACK_BUS_FREQ_HZ;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)arch/x86_64/interrupts/Interrupts.asm(0 hunks)cmake/flags.cmake(1 hunks)drivers/APIC/APIC.c(5 hunks)drivers/APIC/APIC.h(1 hunks)drivers/TSC.c(2 hunks)
💤 Files with no reviewable changes (1)
- arch/x86_64/interrupts/Interrupts.asm
🧰 Additional context used
🧠 Learnings (1)
📚 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/APIC.cdrivers/TSC.c
🧬 Code graph analysis (3)
drivers/APIC/APIC.c (2)
kernel/etc/Console.c (2)
PrintKernelF(272-279)PrintKernelWarning(239-244)mm/VMem.c (1)
VMemMapMMIO(702-753)
drivers/TSC.c (1)
drivers/APIC/APIC.c (1)
GetPerCpuData(53-55)
drivers/APIC/APIC.h (1)
drivers/APIC/APIC.c (1)
GetPerCpuData(53-55)
🪛 Clang (14.0.6)
drivers/APIC/APIC.c
[warning] 44-44: variable 'g_bsp_per_cpu_data' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 195-195: statement should be inside braces
(readability-braces-around-statements)
[warning] 236-236: 100 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 239-239: 100000000 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
drivers/TSC.c
[warning] 6-6: variable 'tsc_freq_hz' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 7-7: variable 'tsc_calibrated' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 27-27: statement should be inside braces
(readability-braces-around-statements)
🪛 Gitleaks (8.28.0)
drivers/APIC/APIC.c
[high] 236-236: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (7)
cmake/flags.cmake (1)
8-8: Verify that suppressing all NASM warnings is intentional.The
-w-allflag suppresses ALL warnings from the NASM assembler, which could hide legitimate issues such as deprecated syntax, incorrect instruction usage, or potential bugs in assembly code. While this aligns with the silent build objective, ensure this trade-off is acceptable for your development workflow.drivers/APIC/APIC.h (1)
7-46: LGTM! Well-designed per-CPU APIC API.The PerCpuData structure and accessor function provide a clean foundation for SMP-aware APIC management. The public API comprehensively covers APIC/IOAPIC control and clearly documents the replacement of legacy PIC functions.
drivers/APIC/APIC.c (4)
43-55: Good incremental approach to per-CPU data.The BSP-only per-CPU data instance with a clean accessor provides a solid foundation for future SMP expansion. The initialization values are safe, and the comment clearly documents the current limitation and future direction.
90-96: LGTM! Clean refactoring to per-CPU data.The LAPIC accessor functions correctly use the per-CPU data structure instead of global state, maintaining the same logic while improving SMP readiness.
184-192: LGTM! Timer installation correctly uses per-CPU data.The function properly stores the frequency in per-CPU data and delegates to
ApicTimerSetFrequency()for calibration and setup.
260-290: LGTM! LAPIC setup correctly integrated with per-CPU data.The LAPIC MMIO mapping and initialization properly stores the virtual base address in the per-CPU data structure. The mapping sequence (allocate, unmap, remap with MMIO flags) and error handling are appropriate.
drivers/TSC.c (1)
12-27: I need to verify thatApicTimerSetFrequencyactually initializesapic_bus_freq. Let me search for its full implementation:Based on the verification results, I can now provide a definitive assessment. The initialization order shown in the kernel code is correct and resolves the concern raised in the original review comment.Initialization order is correct—no action required.
The script output confirms that
ApicTimerInstall(250)is called at line 602 of kernel/core/Kernel.c beforeTSCInit()is called at line 606. SinceApicTimerInstallimmediately invokesApicTimerSetFrequencysynchronously, the APIC calibration will have completed before TSC calibration begins. This ensures thatcpu_data->apic_bus_freqis properly initialized before being accessed by the TSC calibration code.The concern about potential infinite loops or zero-value division in TSC.c (lines 23 and 26-27) is mitigated by the proper boot sequence.
cmake/flags.cmake
Outdated
| if(SILENT_BUILD) | ||
| string(APPEND C_FLAGS " -w") | ||
| string(APPEND CMAKE_ASM_NASM_FLAGS " -w-all") | ||
| set(Corrosion_CARGO_BUILD_FLAGS "--quiet") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the correct Corrosion variable name and scope.
The variable name Corrosion_CARGO_BUILD_FLAGS should be verified against the Corrosion documentation to ensure it follows the correct naming convention. Corrosion typically uses uppercase variable names (e.g., CORROSION_*). Additionally, if this is intended to be a "public" build flag as indicated in the summary, confirm whether it should be set as a cache variable or with PARENT_SCOPE.
🌐 Web query:
What is the correct variable name to set Cargo build flags in Corrosion CMake?
💡 Result:
Use Corrosion's built‑in options rather than a single global variable:
- For import-time flags: pass FLAGS to corrosion_import_crate(... FLAGS ...). [1]
- Per-target (recommended): call corrosion_set_cargo_flags(<target_name> ...). [2]
(Some projects expose their own cache variable like SLINT_LIBRARY_CARGO_FLAGS, but that is project-specific — not a Corrosion standard.) [1][2]
Sources:
[1] Corrosion usage / corrosion_import_crate.
[2] Corrosion usage / per-target options (corrosion_set_cargo_flags).
Remove non-standard variable; use Corrosion's documented API instead.
The variable Corrosion_CARGO_BUILD_FLAGS is not a standard Corrosion variable. Instead, use either the FLAGS parameter in corrosion_import_crate() for import-time flags or call corrosion_set_cargo_flags() for per-target flags. The current variable will not be recognized by Corrosion and has no effect.
🤖 Prompt for AI Agents
In cmake/flags.cmake around line 9, remove the non-standard
set(Corrosion_CARGO_BUILD_FLAGS "--quiet") line because Corrosion does not honor
that variable; instead, pass cargo flags via the documented API: add FLAGS
"--quiet" to the relevant corrosion_import_crate(...) call for import-time flags
or call corrosion_set_cargo_flags(<target> "--quiet") after creating the
Corrosion target for per-target flags. Ensure you delete the stray variable,
update the import or target creation sites to use FLAGS or
corrosion_set_cargo_flags() respectively, and re-run CMake to verify the flag is
applied.
There was a problem hiding this comment.
so how do i make cargo actually shutup as this doesnt work?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all corrosion_import_crate calls
rg -n "corrosion_import_crate" -C 3
# Find all Corrosion-related function calls
rg -n "corrosion_" -C 2
# List all CMake files in the repository
fd -e cmake -e txt CMakeLists
Length of output: 67979
@assembler-0, to silence Cargo builds with Corrosion, you need to use the FLAGS parameter in your corrosion_import_crate() calls. The variable you're setting isn't a standard Corrosion variable.
Here's the fix:
In cmake/flags.cmake, remove the line:
set(Corrosion_CARGO_BUILD_FLAGS "--quiet")In CMakeLists.txt, modify both corrosion_import_crate() calls to conditionally add the FLAGS parameter:
if(SILENT_BUILD)
set(CARGO_QUIET_FLAG FLAGS "--quiet")
else()
set(CARGO_QUIET_FLAG "")
endif()
corrosion_import_crate(
MANIFEST_PATH ${RUST_HEAP_MANIFEST_PATH}
NO_STD
${CARGO_QUIET_FLAG}
)
corrosion_import_crate(
MANIFEST_PATH ${RUST_ATOMIC_MANIFEST_PATH}
NO_STD
${CARGO_QUIET_FLAG}
)This approach:
- Creates a variable containing
FLAGS "--quiet"whenSILENT_BUILDis enabled - Expands it into the
corrosion_import_crate()calls, which will add the--quietflag to all Cargo invocations for those crates
The FLAGS parameter is documented in Corrosion and passes arguments directly to cargo build.
There was a problem hiding this comment.
well that didnt help, there are still warnings-wait warnings, i think its -Awarnings right? so how to do that then and same for nasm, its not very silent
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How to set RUSTFLAGS in Corrosion CMake to suppress warnings
💡 Result:
Use Corrosion's helper functions — either add rustc flags for the target or set the RUSTFLAGS env var for the target.
Examples (replace rust-target-name with your imported crate target):
-
Add flags that propagate to crate and its dependencies:
corrosion_add_target_rustflags(rust-target-name "-A" "warnings") -
Add flags that apply only to the crate (not its cargo-built dependencies):
corrosion_add_target_local_rustflags(rust-target-name "-A" "warnings") -
Set RUSTFLAGS explicitly as an environment variable for the target:
corrosion_set_env_vars(rust-target-name RUSTFLAGS="-A warnings")
Notes:
- Use -A to allow/suppress a lint, -W to warn, -D to deny. Example to suppress unused warnings: "-A" "dead_code".
- corrosion_add_target_local_rustflags is useful to avoid rebuilding dependencies when switching flags.
Sources: Corrosion usage docs describing corrosion_add_target_rustflags / corrosion_add_target_local_rustflags / corrosion_set_env_vars. [1][2]
References
[1] Corrosion usage — per-target options (corrosion_add_target_rustflags, corrosion_add_target_local_rustflags, corrosion_set_env_vars).
[2] Corrosion advanced/usage pages.
🌐 Web query:
NASM assembler flags to suppress all warnings and messages
💡 Result:
Use NASM's -w and -s options.
- Disable all warnings: -w-all (or -Wno-all).
- Suppress non‑critical output (only critical errors remain): -s.
Example:
nasm -f elf -w-all -s source.asm -o out.o
Sources: NASM manual (warning classes / -w options) [1]; nasm man summary showing -s (silent) [2].
🏁 Script executed:
#!/bin/bash
# Find the Cargo.toml files for the Rust crates and extract the package names
echo "=== Rust crate names ==="
fd -e toml Cargo.toml --exec grep -H "^name = " {}
Length of output: 135
@assembler-0, yes, -A warnings is the correct Rust flag! Here's how to properly silence both Cargo and NASM:
For Rust warnings, add these lines in CMakeLists.txt right after your corrosion_import_crate() calls:
if(SILENT_BUILD)
corrosion_add_target_rustflags(voidframe-mm "-A" "warnings")
corrosion_add_target_rustflags(voidframe-spinlock "-A" "warnings")
endif()This passes -A warnings to rustc, which suppresses all compiler warnings.
For NASM, modify the line in cmake/flags.cmake to add the -s (silent) flag:
if(SILENT_BUILD)
string(APPEND C_FLAGS " -w")
string(APPEND CMAKE_ASM_NASM_FLAGS " -w-all -s")
# Remove the Corrosion_CARGO_BUILD_FLAGS line - it doesn't work
endif()The -s flag tells NASM to suppress all non-critical output, so combined with -w-all (disable all warnings), NASM will be completely silent.
Summary of changes:
- Remove the
set(Corrosion_CARGO_BUILD_FLAGS "--quiet")line fromcmake/flags.cmake - Add
-sto the NASM flags incmake/flags.cmake - Add the
corrosion_add_target_rustflags()calls inCMakeLists.txtafter importing the crates
This should give you a truly silent build! 🤫
silent build and apic SMP awareness
Summary by CodeRabbit
Chores
Refactor
Bug Fixes