Conversation
…tead of the address
📝 WalkthroughWalkthroughAdds a new AHCI driver surface (FIS/HBA types, enums, errors), integrates AHCI detection and MMIO mapping into kernel PCI enumeration/startup, replaces the disk driver with an ATA module, adds QEMU runner scripts and editor settings, and updates paging, allocator, macros, and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Kernel as Kernel (main)
participant PCI as PCI Scanner
participant MMU as MMU / PageMapper
participant HBA as AHCI HBA (MMIO)
Note over Kernel,PCI: Startup PCI enumeration probes devices
Kernel->>PCI: scan_pci() — detect device/vendor and BAR(s)
PCI-->>Kernel: AHCI device + BAR physical address
Kernel->>MMU: align BAR to page boundary, request mapping with IO page flags
MMU-->>Kernel: return virtual address for HBAMemoryRegisters
Kernel->>HBA: read HBAMemoryRegisters (version, caps, ports_implemented)
HBA-->>Kernel: register values (interface speed, port status)
Kernel->>Kernel: probe per-port detection and print diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel/src/memory/allocators/page_allocator/extensions.rs (1)
83-97: Fix page table traversal for big/huge page sizesThe traversal with
PageTableLevel::iterator_without_last()always descends through PML4 → PDPT → PD, leavingtablepointing at the PT level. However, the final mapping level is computed frompage_size:
- Regular (2):
3 - 2 = 1→ PT (correct,tableis PT)- Big (1):
3 - 1 = 2→ PD (mismatch,tableis PT)- Huge (0):
3 - 0 = 3→ PDPT (mismatch,tableis PT)For Big and Huge pages, the code would index into PT entries using PD or PDPT indices, producing incorrect mappings. This should be fixed by stopping traversal at the correct level for each page size before mapping, rather than always descending to PT:
@@ fn map( - let mut table = PageTable::current_table_mut(); - for level in PageTableLevel::iterator_without_last() { - let index = self.index_of(*level); - let entry = &mut table.entries[index]; - let resolved_table = entry.force_resolve_table_mut(); - table = resolved_table; - } - unsafe { - table.entries[self - .index_of((3 - page_size as u8).try_into().unwrap())] - .map_unchecked(address, flags); - } + let mut table = PageTable::current_table_mut(); + let target_level: PageTableLevel = + (3 - page_size as u8).try_into().unwrap(); + + for level in PageTableLevel::iterator_without_last() { + let index = self.index_of(*level); + let entry = &mut table.entries[index]; + + if *level == target_level { + unsafe { + entry.map_unchecked(address, flags); + } + return; + } + + let resolved_table = entry.force_resolve_table_mut(); + table = resolved_table; + } + + debug_assert!(matches!(page_size, PageSize::Regular)); + unsafe { + table.entries[self.index_of(PageTableLevel::PT)] + .map_unchecked(address, flags); + }
🧹 Nitpick comments (7)
build/runner/runner.sh (1)
1-8: Consider adding basic error handling and validation.The runner script works but could be more robust with validation checks:
- Verify
qemu-system-x86_64is in PATH- Check that
build/image.binexists before launching QEMU- Exit with a clear message if preconditions aren't met
This improves the developer experience when dependencies are missing or the build hasn't completed yet.
Example enhancement:
#!/bin/sh + +if ! command -v qemu-system-x86_64 >/dev/null 2>&1; then + echo "Error: qemu-system-x86_64 not found in PATH" >&2 + exit 1 +fi + +if [ ! -f build/image.bin ]; then + echo "Error: build/image.bin not found. Build the kernel first." >&2 + exit 1 +fi qemu-system-x86_64 \ -M q35 \ -drive id=disk0,format=raw,file=build/image.bin,if=none \ -device ahci,id=ahci0 \ -device ide-hd,drive=disk0,bus=ahci0.0 \ -monitor stdio.vscode/settings.json (1)
98-100: Consider the performance trade-off of building in release mode.Adding
--releasetorust-analyzer.cargo.extraArgsforces the analyzer to build optimized code, which significantly increases compilation time for analysis and can degrade IDE responsiveness (slower autocomplete, slower error checking, etc.).This is usually only needed if you're debugging release-specific issues or optimizations. For typical development, the default debug build is faster and provides a better IDE experience.
build/runner/runner.bat (1)
1-9: Consider adding basic error handling and validation (consistent with runner.sh).Similar to the shell script, the batch file would benefit from validation:
- Check if
qemu-system-x86_64.exeis available- Verify
build\image.binexists- Provide clear error messages
Example enhancement:
@echo off +where qemu-system-x86_64.exe >nul 2>&1 +if errorlevel 1 ( + echo Error: qemu-system-x86_64.exe not found in PATH >&2 + exit /b 1 +) + +if not exist build\image.bin ( + echo Error: build\image.bin not found. Build the kernel first. >&2 + exit /b 1 +) qemu-system-x86_64 ^ -M q35 ^ -drive id=disk0,file=build/image.bin,if=none,format=raw ^ -device ahci,id=ahci0 ^ -device ide-hd,drive=disk0,bus=ahci0.0 ^ -monitor stdioshared/cpu_utils/src/structures/paging/entry_flags.rs (1)
72-83: Consider marking IO pages non-executable by default
regular_io_page_flagslooks correct for uncached IO mappings (present + writable + disable_cache + global). Given these are MMIO regions, it’s usually safe and desirable to also set NX by default to reduce the blast radius of bugs or corrupt control flow:pub const fn regular_io_page_flags() -> Self { - PageEntryFlags::default() - .present() - .writable() - .disable_cache() - .global() + PageEntryFlags::default() + .present() + .writable() + .disable_cache() + .global() + .not_executable() }If you have a specific case where executable IO mappings are needed, that can be handled with a more specialized helper instead.
learnix-macros/src/lib.rs (2)
122-151: ro_flag docs don’t match the generated API
ro_flagcurrently generates a single methodsupports_<flag_name>, but the docstring still refers to “three methods” and namesis_$flag_name. This is misleading for consumers of the macro.You can tighten the docs to reflect what’s actually emitted, e.g.:
-/// With this information it will automatically generate -/// -/// three methods -/// -/// -/// 1. `is_$flag_name`: return true if the flag is set or false if not +/// With this information it will generate a single method: +/// +/// - `supports_$flag_name`: returns true if the flag is set or false if not.Also consider explicitly mentioning the
supports_prefix in the summary comment so users don’t expect anis_-prefixed getter.
153-199: Clarify rwc_flag / rw1_flag semantics (naming vs. behaviour)Both
rwc_flagandrw1_flagexpand to methods that simply OR a1into the underlying bit:
rwc_flag→clear_<flag>(&mut self)that doesself.0 |= 1 << bit;rw1_flag→set_<flag>(&mut self)that doesself.0 |= 1 << bit;This is fine for RW1C-style bits, but the naming and docs are ambiguous:
clear_<flag>suggests the bit becomes 0, while the implementation writes a 1.- The doc comment on
rwc_flagjust says “Sets the corresponding flag”.To avoid misuse, it would help to make the RW1C behaviour explicit in the docs, e.g.:
- /// Sets the corresponding flag + /// Writes a `1` to the corresponding bit (typically RW1C “clear” semantics).And optionally, a short comment in the macro docs that
rwc_flagis intended for “read/write 1 to clear” fields whilerw1_flagis for “write 1 to set” fields would make call sites self-explanatory.shared/common/src/enums/pci.rs (1)
79-103: Consider adding a fallback arm for future-proofing.While
#[non_exhaustive]onVendorIDallows exhaustive matching within the same crate, adding a catch-all arm would make the code more resilient to future additions.VendorID::NonExistent => { write!(f, "NonExistent") } + _ => { + write!(f, "Unknown(0x{:04x})", unsafe { self.device.num }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.cargo/config.toml(1 hunks).vscode/settings.json(1 hunks)build/runner/runner.bat(1 hunks)build/runner/runner.sh(1 hunks)kernel/src/drivers/ata/ahci/mod.rs(1 hunks)kernel/src/drivers/ata/mod.rs(1 hunks)kernel/src/drivers/disk/mod.rs(0 hunks)kernel/src/drivers/mod.rs(1 hunks)kernel/src/drivers/pci.rs(6 hunks)kernel/src/main.rs(2 hunks)kernel/src/memory/allocators/page_allocator/extensions.rs(1 hunks)learnix-macros/src/lib.rs(1 hunks)shared/common/Cargo.toml(1 hunks)shared/common/src/enums/ahci.rs(1 hunks)shared/common/src/enums/mod.rs(2 hunks)shared/common/src/enums/paging.rs(1 hunks)shared/common/src/enums/pci.rs(6 hunks)shared/cpu_utils/src/structures/paging/entry_flags.rs(1 hunks)
💤 Files with no reviewable changes (1)
- kernel/src/drivers/disk/mod.rs
🧰 Additional context used
🧬 Code graph analysis (3)
kernel/src/memory/allocators/page_allocator/extensions.rs (1)
shared/common/src/enums/paging.rs (1)
iterator_without_last(44-54)
kernel/src/main.rs (4)
kernel/src/drivers/pci.rs (2)
common(381-383)scan_pci(386-429)shared/common/src/enums/keyboard.rs (1)
from_scancode(197-386)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)kernel/src/drivers/ata/ahci/mod.rs (3)
major_version(161-163)minor_version(166-168)interface_speed(35-37)
kernel/src/drivers/ata/ahci/mod.rs (2)
kernel/src/drivers/pci.rs (7)
core(307-307)core(310-310)core(313-313)core(316-316)core(319-319)core(322-322)common(381-383)learnix-macros/src/lib.rs (4)
flag(74-119)ro_flag(131-150)rw1_flag(179-198)rwc_flag(155-174)
🔇 Additional comments (12)
.cargo/config.toml (1)
8-15: LGTM! OS-specific runner configuration is correct.The platform-specific runner paths use appropriate separators and invocation methods for each OS. The relative paths will work correctly when
cargo runis invoked from the project root.shared/common/src/enums/mod.rs (1)
1-15: AHCI enums module wiring is consistent with existing patternAdding
pub mod ahci;pluspub use ahci::*;mirrors how the other enum modules are exposed here, so consumers can keep pulling everything fromcommon::enumswithout extra path changes. No issues from a module/export perspective.kernel/src/drivers/ata/mod.rs (1)
1-1: ATA AHCI module anchor looks good
pub mod ahci;cleanly sets up the ATA driver namespace for the AHCI implementation. Assumingkernel/src/drivers/ata/ahci/mod.rsexists and builds, there’s nothing to change here.shared/common/src/enums/paging.rs (1)
44-54: iterator_without_last matches existing iterator pattern and suits upper-level walksThe new
iterator_without_lasthelper is a clean mirror ofiterator(), restricted to[PML4, PDPT, PD]. This is exactly what’s needed for “walk all intermediate levels” logic like the page-table traversal inVirtualAddress::map. No issues spotted here.kernel/src/drivers/mod.rs (1)
1-1: Module rename to ata is complete—no callers still depend on drivers::diskThe switch from
drivers::disktodrivers::atais already fully migrated. A search of the codebase found no remaining references todrivers::disk, confirming all callers have been updated.The
use disk::DiskAddressPacket;reference inkernel/stages/first_stage/src/main.rsis a local module import unrelated to this change.shared/common/Cargo.toml (1)
6-9: Thenum_enumdependency and"complex-expressions"feature are correctly configuredThe
num_enum = "0.7.5"dependency withfeatures = ["complex-expressions"]is necessary, not optional. The codebase uses#[num_enum(alternatives = [3..=5, 7, 9..=14])]inshared/common/src/enums/ahci.rs, which relies on the"complex-expressions"feature to support range patterns in the alternatives attribute.Version
0.7.5is compatible with your Rust2024edition (num_enum itself publishes with edition2021andrust-version = "1.70.0"). The configuration aligns withno_stdconstraints viadefault-features = false.shared/common/src/enums/ahci.rs (1)
17-29: LGTM!The
InterfaceCommunicationControlenum correctly usesnum_enumalternatives to handle reserved values, ensuring safe conversion from raw hardware values.kernel/src/main.rs (1)
96-103: LGTM!The keyboard input loop correctly handles PS/2 scancode reading and termination on Enter key press.
shared/common/src/enums/pci.rs (2)
735-743: LGTM!The
is_ahci()method correctly identifies AHCI controllers by checking the class code (MassStorageController), subclass (SATA), and programming interface (AHCI1).
552-558: No issues found. The#[repr(C, packed)]attribute onPciDeviceTypedoes not cause alignment problems. All fields (prog_if,subclass,class) are 1-byte enums/unions, naturally aligned at byte boundaries. Direct field access is safe. The unsafe blocks inis_ahci()and theDebugimpl are correct usage for union field access, not related to struct packing.kernel/src/drivers/pci.rs (1)
280-300: LGTM!Making the
GeneralDeviceHeaderfields public enables direct access to BAR registers and other PCI configuration data needed for AHCI device initialization.kernel/src/drivers/ata/ahci/mod.rs (1)
638-652: LGTM!The address composition helpers correctly combine the high and low 32-bit parts while respecting the alignment requirements (1KB for command list, 256B for FIS base).
kernel/src/drivers/ata/ahci/mod.rs
Outdated
| pub fn interface_speed(&self) -> AHCIInterfaceSpeed { | ||
| unsafe { core::mem::transmute(((self.0 >> 20) & 0xf) as u8) } | ||
| } |
There was a problem hiding this comment.
Unsafe transmute may cause UB for invalid hardware values.
The interface_speed() method uses transmute to convert a 4-bit field to AHCIInterfaceSpeed. However, the enum only defines values 1, 2, 3. If hardware reports 0 (reserved) or values ≥4, this is UB.
Consider using TryFromPrimitive which is already derived on the enum:
pub fn interface_speed(&self) -> AHCIInterfaceSpeed {
- unsafe { core::mem::transmute(((self.0 >> 20) & 0xf) as u8) }
+ AHCIInterfaceSpeed::try_from(((self.0 >> 20) & 0xf) as u8)
+ .unwrap_or(AHCIInterfaceSpeed::Gen1) // or handle error appropriately
}Alternatively, return Option<AHCIInterfaceSpeed> or Result to properly handle invalid values.
📝 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.
| pub fn interface_speed(&self) -> AHCIInterfaceSpeed { | |
| unsafe { core::mem::transmute(((self.0 >> 20) & 0xf) as u8) } | |
| } | |
| pub fn interface_speed(&self) -> AHCIInterfaceSpeed { | |
| AHCIInterfaceSpeed::try_from(((self.0 >> 20) & 0xf) as u8) | |
| .unwrap_or(AHCIInterfaceSpeed::Gen1) // or handle error appropriately | |
| } |
🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/mod.rs around lines 35 to 37, the method
currently uses unsafe core::mem::transmute on a 4-bit hardware field which can
produce undefined behavior for values not represented by AHCIInterfaceSpeed;
replace the transmute with a safe conversion: extract the 4-bit value as u8,
then use the enum's TryFromPrimitive-derived conversion (e.g.
AHCIInterfaceSpeed::try_from(value)) and return a Result or Option (or map
unknown values to a sensible default) instead of transmuting; remove the unsafe
block and handle the Err/None case explicitly so invalid hardware values are
handled safely.
kernel/src/drivers/ata/ahci/mod.rs
Outdated
| // Programmed I/O multiple Data request block | ||
| ro_flag!(pmd, 15); | ||
|
|
||
| // Slumber state capable | ||
| ro_flag!(ssc, 15); | ||
|
|
||
| // Partial state capable | ||
| ro_flag!(psc, 14); |
There was a problem hiding this comment.
Duplicate bit position: both ssc and pmd use bit 15.
According to AHCI spec, bit 15 is PMD (PIO Multiple DRQ Block) and bit 14 is SSC (Slumber State Capable). The ssc flag should use bit 14, not 15.
// Programmed I/O multiple Data request block
ro_flag!(pmd, 15);
// Slumber state capable
- ro_flag!(ssc, 15);
+ ro_flag!(ssc, 14);
// Partial state capable
- ro_flag!(psc, 14);
+ ro_flag!(psc, 13);📝 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.
| // Programmed I/O multiple Data request block | |
| ro_flag!(pmd, 15); | |
| // Slumber state capable | |
| ro_flag!(ssc, 15); | |
| // Partial state capable | |
| ro_flag!(psc, 14); | |
| // Programmed I/O multiple Data request block | |
| ro_flag!(pmd, 15); | |
| // Slumber state capable | |
| ro_flag!(ssc, 14); | |
| // Partial state capable | |
| ro_flag!(psc, 13); |
🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/mod.rs around lines 48-55, change the ssc flag to
use bit 14 instead of 15 to match the AHCI spec (PMD = bit 15, SSC = bit 14);
update the ro_flag invocation for ssc to use 14 (and if this creates a duplicate
with psc, verify and correct psc to the proper bit per spec as well).
kernel/src/drivers/ata/ahci/mod.rs
Outdated
| /// ZERO is invalid | ||
| /// TODO understand how to check if i have both receive and transmit | ||
| pub fn buffet_size(&self) -> Option<NonZero<usize>> { | ||
| NonZero::new((self.0 & 0xffff) as usize) | ||
| } |
There was a problem hiding this comment.
Typo: "buffet_size" should be "buffer_size".
/// ZERO is invalid
/// TODO understand how to check if i have both receive and transmit
- pub fn buffet_size(&self) -> Option<NonZero<usize>> {
+ pub fn buffer_size(&self) -> Option<NonZero<usize>> {
NonZero::new((self.0 & 0xffff) as usize)
}📝 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.
| /// ZERO is invalid | |
| /// TODO understand how to check if i have both receive and transmit | |
| pub fn buffet_size(&self) -> Option<NonZero<usize>> { | |
| NonZero::new((self.0 & 0xffff) as usize) | |
| } | |
| /// ZERO is invalid | |
| /// TODO understand how to check if i have both receive and transmit | |
| pub fn buffer_size(&self) -> Option<NonZero<usize>> { | |
| NonZero::new((self.0 & 0xffff) as usize) | |
| } |
🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/mod.rs around lines 246 to 250 the method name is
misspelled as "buffet_size" but should be "buffer_size"; rename the function to
buffer_size (keeping the signature and return type Option<NonZero<usize>> and
the existing implementation), update any inline doc comments to reflect
"buffer_size", and search & update all call sites, trait impls, and tests to use
the new name so references compile without behavior changes.
kernel/src/drivers/ata/ahci/mod.rs
Outdated
| #[repr(C)] | ||
| /// Host Bus Adapter Memory Registers | ||
| pub struct HBAMemoryRegisters { | ||
| pub ghc: GenericHostControl, | ||
| pub _reserved: [u8; 0x60], | ||
| pub vsr: VendorSpecificRegisters, | ||
| pub ports: [PortControlRegisters; 32], | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n kernel/src/drivers/ata/ahci/mod.rs | head -700 | tail -100Repository: sagi21805/LearnixOS
Length of output: 3071
🏁 Script executed:
rg -n "struct GenericHostControl" kernel/src/drivers/ata/ahci/mod.rs -A 20Repository: sagi21805/LearnixOS
Length of output: 754
🏁 Script executed:
rg -n "struct VendorSpecificRegisters" kernel/src/drivers/ata/ahci/mod.rs -A 10Repository: sagi21805/LearnixOS
Length of output: 393
🏁 Script executed:
rg -n "struct PortControlRegisters" kernel/src/drivers/ata/ahci/mod.rs -A 30Repository: sagi21805/LearnixOS
Length of output: 979
🌐 Web query:
AHCI 1.3.1 specification HBA memory registers layout offset 0x00 0x2C 0x60
💡 Result:
Summary from the AHCI 1.3.1 spec:
-
0x00 — start of the Generic Host Control registers (0x00–0x2B). This block contains CAP (Host Capabilities at 0x00), GHC (Global Host Control 0x04), IS (Interrupt Status 0x08), PI (Ports Implemented 0x0C), VS (Version 0x10), CCC_CTL/CCC_PORTS, EM_LOC/EM_CTL, CAP2, etc. [1][2]
-
0x2C — start of a reserved region for AHCI global registers (0x2C–0x5F are reserved in AHCI). (PCI/ABAR layout shows 0x2C is the next region boundary after the generic host control block.) [2]
-
0x60 — start of the NVMHCI-under-AHCI global registers (0x60–0x9F) when CAP2.NVMP = 1; otherwise 0x60–0x9F are reserved. If present these include NVMHCI CAP, PI, VS, etc. [2]
Sources: Intel AHCI 1.3.1 specification listing HBA memory register map and NVMHCI notes. [1][2]
References
[1] Intel: "Serial ATA AHCI: Specification, Rev. 1.3.1" (AHCI spec page).
[2] AHCI 1.3.1 specification PDF (HBA memory/register map and NVMHCI section).
Fix HBA memory layout to match AHCI 1.3.1 specification.
The current layout is incorrect per AHCI 1.3.1 spec:
_reserved: [u8; 0x60]at offset 0x2C should be[u8; 0x34](52 bytes for 0x2C–0x5F)vsr: VendorSpecificRegistersshould be[u8; 0x40](64 bytes for 0x60–0x9F), not 0x74 (116 bytes)- Missing reserved region
[u8; 0x60](96 bytes for 0xA0–0xFF) before ports
Correct layout: GHC (0x00–0x2B) → Reserved (0x2C–0x5F) → Vendor/NVMHCI (0x60–0x9F) → Reserved (0xA0–0xFF) → Ports (0x100+).
🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/mod.rs around lines 654 to 661, the
HBAMemoryRegisters struct layout does not match AHCI 1.3.1: adjust the reserved
and vendor fields so offsets match the spec by changing the first reserved from
[u8; 0x60] to [u8; 0x34] (for offsets 0x2C–0x5F), replace vsr:
VendorSpecificRegisters with a raw vendor/nvmhci reserved region of [u8; 0x40]
(0x60–0x9F), and insert an additional reserved field [u8; 0x60] for offsets
0xA0–0xFF before the ports array so ports start at 0x100.
| pub fn is_64bit(&self) -> bool { | ||
| self.identify() == BaseAddressRegisterType::Memory | ||
| && unsafe { | ||
| self.memory.0 & BaseAddressRegisterSize::Bit64 as u32 != 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect bit check for 64-bit BAR detection.
The 64-bit detection logic is incorrect. According to PCI spec, bits 2:1 of a memory BAR indicate the type: 00b = 32-bit, 10b = 64-bit. The current check self.memory.0 & BaseAddressRegisterSize::Bit64 as u32 != 0 tests if bit 1 is set (since Bit64 = 2), but this would also match the reserved value 01b.
pub fn is_64bit(&self) -> bool {
self.identify() == BaseAddressRegisterType::Memory
- && unsafe {
- self.memory.0 & BaseAddressRegisterSize::Bit64 as u32 != 0
- }
+ && unsafe {
+ ((self.memory.0 >> 1) & 0x3) == BaseAddressRegisterSize::Bit64 as u32
+ }
}📝 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.
| pub fn is_64bit(&self) -> bool { | |
| self.identify() == BaseAddressRegisterType::Memory | |
| && unsafe { | |
| self.memory.0 & BaseAddressRegisterSize::Bit64 as u32 != 0 | |
| } | |
| } | |
| pub fn is_64bit(&self) -> bool { | |
| self.identify() == BaseAddressRegisterType::Memory | |
| && unsafe { | |
| ((self.memory.0 >> 1) & 0x3) == BaseAddressRegisterSize::Bit64 as u32 | |
| } | |
| } |
🤖 Prompt for AI Agents
In kernel/src/drivers/pci.rs around lines 255-260, the 64-bit BAR detection
currently tests a single bit and can misclassify reserved values; instead
extract the two-bit BAR type field (bits 2:1) and compare it to the 64-bit
pattern (binary 10). Concretely, shift the BAR value right by 1 and mask with
0b11, then check that equals 0b10 while still ensuring identify() ==
BaseAddressRegisterType::Memory.
| pub fn address(&self) -> usize { | ||
| if !self.is_64bit() { | ||
| (unsafe { self.io.0 } & 0xfffffff0) as usize | ||
| } else { | ||
| unimplemented!("Still didn't implemented 64bit addresses") | ||
| } | ||
| } |
There was a problem hiding this comment.
Address extraction uses wrong mask for IO BARs.
The method accesses self.io.0 but uses mask 0xfffffff0 which is correct for memory BARs, not IO BARs. IO BARs should use 0xfffffffc (bits 1:0 are reserved/type). Also, the condition !self.is_64bit() includes both 32-bit memory BARs and IO BARs, which need different masks.
pub fn address(&self) -> usize {
- if !self.is_64bit() {
- (unsafe { self.io.0 } & 0xfffffff0) as usize
+ if self.identify() == BaseAddressRegisterType::IO {
+ (unsafe { self.io.0 } & 0xfffffffc) as usize
+ } else if !self.is_64bit() {
+ (unsafe { self.memory.0 } & 0xfffffff0) as usize
} else {
unimplemented!("Still didn't implemented 64bit addresses")
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kernel/src/drivers/pci.rs around lines 262-268, the address() method
currently treats all non-64-bit BARs the same and uses the memory BAR mask
0xfffffff0 on self.io.0; change the logic to distinguish IO BARs from memory
BARs, applying 0xfffffffc for IO BARs (masking off bits 1:0) and 0xfffffff0 for
32-bit memory BARs, and keep the 64-bit branch as an appropriate implementation
or explicit unimplemented/panic; ensure you read the correct field (self.io.0)
for IO BARs and the memory BAR field for memory BARs and return the masked value
as usize.
| for function in 1..8 { | ||
| println!("{}", function); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug print statement.
This println! appears to be a debug artifact that will output function numbers during PCI enumeration, potentially spamming the console.
for function in 1..8 {
- println!("{}", function);
let common = PciConfigurationCycle::read_common_header(📝 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.
| for function in 1..8 { | |
| println!("{}", function); | |
| for function in 1..8 { | |
| let common = PciConfigurationCycle::read_common_header( |
🤖 Prompt for AI Agents
In kernel/src/drivers/pci.rs around lines 409-410, there's a leftover debug
println!("{}" , function) inside the loop iterating functions 1..8; remove that
println! to avoid spamming the console during PCI enumeration (if you want to
keep diagnostics instead, replace it with a low-verbosity logger call such as
debug!/trace! or conditionally compile it behind a cfg attribute).
kernel/src/main.rs
Outdated
| let a = unsafe { | ||
| PhysicalAddress::new_unchecked( | ||
| device.general_device.bar5.address(), | ||
| ) | ||
| }; | ||
|
|
||
| println!("unaligned: {:x?}", a); | ||
|
|
||
| let aligned = a.align_down(REGULAR_PAGE_ALIGNMENT); | ||
|
|
||
| println!("aligned: {:x?}", aligned); | ||
|
|
||
| aligned.map( | ||
| aligned.as_usize().into(), | ||
| PageEntryFlags::regular_io_page_flags(), | ||
| PageSize::Regular, | ||
| ); | ||
|
|
||
| let hba_ptr = | ||
| unsafe { &*aligned.as_mut_ptr::<GenericHostControl>() }; | ||
|
|
||
| println!( | ||
| "AHCI Version: {}.{}", | ||
| hba_ptr.vs.major_version(), | ||
| hba_ptr.vs.minor_version() | ||
| ); | ||
|
|
||
| println!("Ports Implemented: {:b}", hba_ptr.pi.0); | ||
|
|
||
| println!("Interface Speed: {}", hba_ptr.cap.interface_speed()) |
There was a problem hiding this comment.
Potential AHCI register access misalignment.
When the BAR5 address is not page-aligned, you align it down for mapping but then access the GenericHostControl at the aligned address instead of the original BAR5 address. This would read from the wrong memory location.
For example, if BAR5 is 0xFEBF1000 and page alignment is 0x1000, the aligned address would be 0xFEBF1000 (already aligned). But if BAR5 were 0xFEBF1400, aligning down gives 0xFEBF1000, yet you'd access at 0xFEBF1000 instead of 0xFEBF1400.
Consider preserving and using the original address offset:
let aligned = a.align_down(REGULAR_PAGE_ALIGNMENT);
println!("aligned: {:x?}", aligned);
aligned.map(
aligned.as_usize().into(),
PageEntryFlags::regular_io_page_flags(),
PageSize::Regular,
);
- let hba_ptr =
- unsafe { &*aligned.as_mut_ptr::<GenericHostControl>() };
+ let hba_ptr =
+ unsafe { &*a.as_mut_ptr::<GenericHostControl>() };🤖 Prompt for AI Agents
In kernel/src/main.rs around lines 111 to 140, the code aligns the BAR5 base
down to a page boundary for mapping but then dereferences the aligned address
directly, losing the original BAR5 offset and potentially reading the wrong
registers; preserve the original PhysicalAddress, compute the offset delta =
original.as_usize() - aligned.as_usize(), map the aligned page(s) as before, and
then obtain the pointer to GenericHostControl by advancing the mapped aligned
pointer by that delta (e.g., get aligned.as_mut_ptr::<u8>(), add delta, then
cast to GenericHostControl) so you access the AHCI registers at the original
BAR5 offset; ensure the pointer arithmetic respects the mapped range and keep
unsafe blocks minimal and justified.
| #[display("Gen1: 1.5Gb/s")] | ||
| Gen1 = 1, | ||
| #[display("Gen1: 3.0Gb/s")] | ||
| Gen2 = 2, | ||
| #[display("Gen1: 6.0Gb/s")] | ||
| Gen3 = 3, |
There was a problem hiding this comment.
Display strings have incorrect generation labels.
The display attributes for Gen2 and Gen3 incorrectly use "Gen1" as the prefix instead of their actual generation names.
#[display("Gen1: 1.5Gb/s")]
Gen1 = 1,
- #[display("Gen1: 3.0Gb/s")]
+ #[display("Gen2: 3.0Gb/s")]
Gen2 = 2,
- #[display("Gen1: 6.0Gb/s")]
+ #[display("Gen3: 6.0Gb/s")]
Gen3 = 3,📝 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.
| #[display("Gen1: 1.5Gb/s")] | |
| Gen1 = 1, | |
| #[display("Gen1: 3.0Gb/s")] | |
| Gen2 = 2, | |
| #[display("Gen1: 6.0Gb/s")] | |
| Gen3 = 3, | |
| #[display("Gen1: 1.5Gb/s")] | |
| Gen1 = 1, | |
| #[display("Gen2: 3.0Gb/s")] | |
| Gen2 = 2, | |
| #[display("Gen3: 6.0Gb/s")] | |
| Gen3 = 3, |
🤖 Prompt for AI Agents
In shared/common/src/enums/ahci.rs around lines 8 to 13, the #[display(...)]
attributes for Gen2 and Gen3 mistakenly use "Gen1" in their strings; update
those attributes so they read "Gen2: 3.0Gb/s" for Gen2 and "Gen3: 6.0Gb/s" for
Gen3 (leave Gen1 unchanged), ensuring the display labels match the enum variant
names and speeds.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
shared/common/src/enums/ahci.rs (2)
20-25: Display strings have incorrect generation labels.The display attributes for
Gen2(line 22) andGen3(line 24) incorrectly show "Gen1" instead of their actual generation names.Apply this diff:
#[display("Gen1: 1.5Gb/s")] Gen1 = 1, - #[display("Gen1: 3.0Gb/s")] + #[display("Gen2: 3.0Gb/s")] Gen2 = 2, - #[display("Gen1: 6.0Gb/s")] + #[display("Gen3: 6.0Gb/s")] Gen3 = 3,
76-81: Display strings have incorrect generation labels.The display attributes for
Gen2(line 78) andGen3(line 80) inInterfaceSpeedRestrictionincorrectly show "Gen1" instead of their actual generation names—the same issue as inInterfaceSpeedabove.Apply this diff:
#[display("Gen1: 1.5Gb/s")] Gen1 = 1, - #[display("Gen1: 3.0Gb/s")] + #[display("Gen2: 3.0Gb/s")] Gen2 = 2, - #[display("Gen1: 6.0Gb/s")] + #[display("Gen3: 6.0Gb/s")] Gen3 = 3,kernel/src/main.rs (1)
113-128: AHCI register access uses wrong address after alignment.When BAR5 is not page-aligned, the code aligns it down for memory mapping (line 119) but then creates the
HBAMemoryRegisterspointer at the aligned address (line 127-128) instead of the original BAR5 address. This causes the driver to access AHCI registers at the wrong memory location.For example, if BAR5 is
0xFEBF1400and page alignment is0x1000, the aligned address becomes0xFEBF1000. Accessing registers at the aligned address instead of0xFEBF1400reads from 0x400 bytes too early.Preserve the original BAR5 address for register access:
let a = unsafe { PhysicalAddress::new_unchecked( device.general_device.bar5.address(), ) }; let aligned = a.align_down(REGULAR_PAGE_ALIGNMENT); aligned.map( aligned.as_usize().into(), PageEntryFlags::regular_io_page_flags(), PageSize::Regular, ); let hba_ptr = - unsafe { &*aligned.as_mut_ptr::<HBAMemoryRegisters>() }; + unsafe { &*a.as_mut_ptr::<HBAMemoryRegisters>() };kernel/src/drivers/ata/ahci/mod.rs (5)
46-48: Unsafe transmute can cause undefined behavior for invalid hardware values.The
interface_speed()method usestransmuteto convert a 4-bit hardware field toInterfaceSpeed. If the hardware reports values outside the defined enum variants (0, 1, 2, 3), this causes undefined behavior.Use the safe
TryFromPrimitiveconversion that's already derived:pub fn interface_speed(&self) -> InterfaceSpeed { - unsafe { core::mem::transmute(((self.0 >> 20) & 0xf) as u8) } + InterfaceSpeed::try_from(((self.0 >> 20) & 0xf) as u8) + .unwrap_or(InterfaceSpeed::DevNotPresent) }Alternatively, return
Result<InterfaceSpeed, ConversionError<u8>>to properly handle invalid values.
59-66: Duplicate bit position: bothpmdandsscuse bit 15.According to the AHCI specification, bit 15 is PMD (PIO Multiple DRQ Block) and bit 14 is SSC (Slumber State Capable). Currently, both
pmdandsscuse bit 15, causing a collision.Correct the bit positions:
// Programmed I/O multiple Data request block ro_flag!(pmd, 15); // Slumber state capable - ro_flag!(ssc, 15); + ro_flag!(ssc, 14); // Partial state capable - ro_flag!(psc, 14); + ro_flag!(psc, 13);
257-261: Typo: "buffet_size" should be "buffer_size".The method name is misspelled as "buffet_size" but should be "buffer_size".
/// ZERO is invalid /// TODO understand how to check if i have both receive and transmit - pub fn buffet_size(&self) -> Option<NonZero<usize>> { + pub fn buffer_size(&self) -> Option<NonZero<usize>> { NonZero::new((self.0 & 0xffff) as usize) }
547-554: CCS field mask is too wide and validation is too restrictive.The Current Command Slot (CCS) field occupies bits 12:8 (5 bits, values 0-31). The code validates only
0x0..=0xf(line 550) but CCS can be 0-31. Additionally, clearing 8 bits with0xff << 8(line 551) affects bits 8-15 when only bits 8-12 should be modified.pub fn set_current_cmd(&mut self, ccs: u8) -> Option<()> { - (0x0u8..=0xfu8).contains(&ccs).then(|| { - self.0 &= !(0xff << 8); + (ccs <= 31).then(|| { + self.0 &= !(0x1f << 8); // Clear only bits 12:8 self.0 |= (ccs as u32) << 8; }) }
731-738: HBA memory layout doesn't match AHCI 1.3.1 specification.The current layout is incorrect per AHCI 1.3.1 spec:
_reserved: [u8; 0x60](96 bytes) at offset 0x2C should be[u8; 0x34](52 bytes for offsets 0x2C–0x5F)vsr: VendorSpecificRegisterswith[u8; 0x74](116 bytes) should be[u8; 0x40](64 bytes for offsets 0x60–0x9F)- Missing reserved region
[u8; 0x60](96 bytes for offsets 0xA0–0xFF) before portsCorrect layout per spec: GHC (0x00–0x2B) → Reserved (0x2C–0x5F) → Vendor/NVMHCI (0x60–0x9F) → Reserved (0xA0–0xFF) → Ports (0x100+).
Apply this diff:
#[repr(C)] /// Host Bus Adapter Memory Registers pub struct HBAMemoryRegisters { pub ghc: GenericHostControl, - pub _reserved: [u8; 0x60], - pub vsr: VendorSpecificRegisters, + _reserved1: [u8; 0x34], // 0x2C-0x5F + _vendor_nvmhci: [u8; 0x40], // 0x60-0x9F + _reserved2: [u8; 0x60], // 0xA0-0xFF pub ports: [PortControlRegisters; 32], }
🧹 Nitpick comments (1)
build/runner/runner.sh (1)
3-10: Optional: Add KVM acceleration for better performance.Consider adding
-enable-kvm(on Linux) or use a fallback to TCG if KVM is unavailable. This significantly improves performance:qemu-system-x86_64 \ -M q35 \ -m 1G \ -cpu host \ + -enable-kvm \ -drive id=disk0,format=raw,file=build/image.bin,if=none \Alternatively, add fallback logic:
+kvm_flag="" +if command -v kvm >/dev/null 2>&1 || grep -q "^flags.*vmx\|svm" /proc/cpuinfo 2>/dev/null; then + kvm_flag="-enable-kvm" +fi + qemu-system-x86_64 \ -M q35 \ -m 1G \ -cpu host \ + $kvm_flag \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build/runner/runner.bat(1 hunks)build/runner/runner.sh(1 hunks)kernel/Cargo.toml(1 hunks)kernel/src/drivers/ata/ahci/mod.rs(1 hunks)kernel/src/main.rs(2 hunks)shared/common/src/enums/ahci.rs(1 hunks)shared/cpu_utils/src/structures/paging/entry_flags.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- build/runner/runner.bat
- shared/cpu_utils/src/structures/paging/entry_flags.rs
🧰 Additional context used
🧬 Code graph analysis (3)
kernel/src/main.rs (6)
kernel/src/memory/memory_map.rs (1)
parse_map(178-245)kernel/src/drivers/pci.rs (3)
common(381-383)scan_pci(386-429)new_unchecked(26-37)shared/common/src/enums/keyboard.rs (1)
from_scancode(197-386)shared/common/src/bitmap.rs (1)
new_unchecked(31-39)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)kernel/src/drivers/ata/ahci/mod.rs (6)
major_version(172-174)minor_version(177-179)interface_speed(46-48)power(602-607)speed(609-612)detection(614-619)
shared/common/src/enums/ahci.rs (1)
kernel/src/drivers/ata/ahci/mod.rs (1)
error(585-587)
kernel/src/drivers/ata/ahci/mod.rs (1)
learnix-macros/src/lib.rs (4)
flag(74-119)ro_flag(131-150)rw1_flag(179-198)rwc_flag(155-174)
🔇 Additional comments (1)
kernel/Cargo.toml (1)
12-14: LGTM!The
num_enumdependency addition is appropriate for the AHCI driver implementation, enabling type-safe enum conversions viaTryFromPrimitiveandUnsafeFromPrimitivederives.
build/runner/runner.sh
Outdated
| qemu-system-x86_64 \ | ||
| -M q35 \ | ||
| -drive id=disk0,format=raw,file=build/image.bin,if=none \ | ||
| -drive id=disk1,format=raw,file=build/image.bin,if=none \ | ||
| -device ahci,id=ahci0 \ | ||
| -device ide-hd,drive=disk0,bus=ahci0.0 \ | ||
| -device ide-hd,drive=disk1,bus=ahci0.1 \ | ||
| -monitor stdio No newline at end of file |
There was a problem hiding this comment.
Add memory and CPU specifications to the QEMU command.
The QEMU setup is missing -m (memory) and -cpu flags, which are essential for the VM to boot and function properly. Without these, the VM will use minimal defaults that are likely insufficient for booting the kernel.
Apply this diff to add memory and CPU specifications:
qemu-system-x86_64 \
-M q35 \
+ -m 1G \
+ -cpu host \
-drive id=disk0,format=raw,file=build/image.bin,if=none \Adjust -m to match the OS memory requirements and -cpu host to match your CPU architecture (or use -cpu max for broader compatibility).
📝 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.
| qemu-system-x86_64 \ | |
| -M q35 \ | |
| -drive id=disk0,format=raw,file=build/image.bin,if=none \ | |
| -drive id=disk1,format=raw,file=build/image.bin,if=none \ | |
| -device ahci,id=ahci0 \ | |
| -device ide-hd,drive=disk0,bus=ahci0.0 \ | |
| -device ide-hd,drive=disk1,bus=ahci0.1 \ | |
| -monitor stdio | |
| qemu-system-x86_64 \ | |
| -M q35 \ | |
| -m 1G \ | |
| -cpu host \ | |
| -drive id=disk0,format=raw,file=build/image.bin,if=none \ | |
| -drive id=disk1,format=raw,file=build/image.bin,if=none \ | |
| -device ahci,id=ahci0 \ | |
| -device ide-hd,drive=disk0,bus=ahci0.0 \ | |
| -device ide-hd,drive=disk1,bus=ahci0.1 \ | |
| -monitor stdio |
🤖 Prompt for AI Agents
In build/runner/runner.sh around lines 3 to 10, the qemu command is missing
explicit memory and CPU flags which can prevent the VM from booting; add a
memory flag (e.g., -m 2048 or adjust as required by the OS) and a CPU flag
(e.g., -cpu host or -cpu max) to the qemu-system-x86_64 invocation so the VM has
sufficient RAM and a compatible CPU model; place the flags after
qemu-system-x86_64 and before the drive/device options and verify values match
your target environment.
build/runner/runner.sh
Outdated
| -drive id=disk0,format=raw,file=build/image.bin,if=none \ | ||
| -drive id=disk1,format=raw,file=build/image.bin,if=none \ |
There was a problem hiding this comment.
Consider robustness improvements for file paths and error handling.
The script uses a relative path (build/image.bin) that assumes execution from the repository root. If the script is run from a different working directory, it will fail silently or with an unhelpful error.
Consider adding:
- A check to verify the image file exists before launching QEMU.
- A check to verify
qemu-system-x86_64is available. - Optional: Use
$(cd "$(dirname "$0")" && pwd)to compute an absolute path.
Example improvement:
#!/bin/sh
+
+set -e # Exit on error
+
+# Verify image file exists
+if [ ! -f "build/image.bin" ]; then
+ echo "Error: build/image.bin not found. Run from repository root."
+ exit 1
+fi
+
+# Verify QEMU is installed
+if ! command -v qemu-system-x86_64 >/dev/null 2>&1; then
+ echo "Error: qemu-system-x86_64 not found. Please install QEMU."
+ exit 1
+fi
+
qemu-system-x86_64 \Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In build/runner/runner.sh around lines 5-6 the script uses a relative image path
(-drive ... file=build/image.bin) and lacks checks for qemu and the image file;
update the script to compute an absolute image path using the script directory
(e.g., resolve DIR via cd "$(dirname "$0")" && pwd and join with
build/image.bin), verify qemu-system-x86_64 is available (use command -v or
which and exit with a clear error if not found), check that the resolved image
file exists and is readable before launching QEMU (exit with a helpful message
and non-zero status if missing), and use the resolved absolute path in the
-drive arguments so the script works regardless of the current working
directory.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
kernel/src/main.rs (1)
112-157: CRITICAL: Register access uses aligned address instead of original BAR5 offset.Lines 129-130 create the HBA register pointer from the aligned address rather than the original BAR5 address. This is the same issue flagged in the previous review.
When BAR5 is not page-aligned, you must:
- Map the aligned page(s)
- Calculate the offset:
offset = original_address - aligned_address- Access registers at:
aligned_ptr + offsetExample: If BAR5 =
0xFEBF1400and page alignment is0x1000:
- Aligned =
0xFEBF1000- Offset =
0x400- Register access should be at
aligned_ptr + 0x400, notaligned_ptrApply this fix:
let aligned = a.align_down(REGULAR_PAGE_ALIGNMENT); - println!("A: {:x?}, Aligned: {:x?}", aligned, a); + println!("A: {:x?}, Aligned: {:x?}", a, aligned); aligned.map( aligned.as_usize().into(), PageEntryFlags::regular_io_page_flags(), PageSize::Regular, ); + // Calculate offset from aligned address to actual BAR5 + let offset = a.as_usize() - aligned.as_usize(); let hba_ptr = - unsafe { &*aligned.as_mut_ptr::<HBAMemoryRegisters>() }; + unsafe { &*aligned.as_mut_ptr::<u8>().add(offset).cast::<HBAMemoryRegisters>() };kernel/src/drivers/ata/ahci/mod.rs (4)
47-49: Unsafe transmute causes UB for invalid hardware values.The
interface_speed()method usestransmuteon a 4-bit hardware field. TheInterfaceSpeedenum only defines values 1, 2, 3. If hardware reports 0 (reserved/not present) or values ≥4, this causes undefined behavior.Use the derived
TryFromPrimitiveinstead:pub fn interface_speed(&self) -> InterfaceSpeed { - unsafe { core::mem::transmute(((self.0 >> 20) & 0xf) as u8) } + let val = ((self.0 >> 20) & 0xf) as u8; + InterfaceSpeed::try_from(val) + .unwrap_or(InterfaceSpeed::DevNotPresent) }
60-67: Duplicate bit positions: ssc and pmd both use bit 15.According to the AHCI specification:
- Bit 15: PMD (PIO Multiple DRQ Block)
- Bit 14: SSC (Slumber State Capable)
- Bit 13: PSC (Partial State Capable)
Apply this fix:
// Programmed I/O multiple Data request block ro_flag!(pmd, 15); // Slumber state capable - ro_flag!(ssc, 15); + ro_flag!(ssc, 14); // Partial state capable - ro_flag!(psc, 14); + ro_flag!(psc, 13);
258-262: Typo in method name: "buffet_size" should be "buffer_size".Apply this fix:
/// ZERO is invalid /// TODO understand how to check if i have both receive and transmit - pub fn buffet_size(&self) -> Option<NonZero<usize>> { + pub fn buffer_size(&self) -> Option<NonZero<usize>> { NonZero::new((self.0 & 0xffff) as usize) }
782-789: HBA memory layout does not match AHCI 1.3.1 specification.The current layout is incorrect per AHCI 1.3.1 spec. The correct layout should be:
- 0x00-0x2B: Generic Host Control (44 bytes)
- 0x2C-0x5F: Reserved (52 bytes)
- 0x60-0x9F: Vendor Specific / NVMHCI (64 bytes)
- 0xA0-0xFF: Reserved (96 bytes)
- 0x100+: Port Control Registers (128 bytes each × 32 ports)
Apply this fix:
#[repr(C)] /// Host Bus Adapter Memory Registers pub struct HBAMemoryRegisters { pub ghc: GenericHostControl, - pub _reserved: [u8; 0x60], - pub vsr: VendorSpecificRegisters, + _reserved0: [u8; 0x34], // 0x2C - 0x5F + _vendor_nvmhci: [u8; 0x40], // 0x60 - 0x9F + _reserved1: [u8; 0x60], // 0xA0 - 0xFF pub ports: [PortControlRegisters; 32], }Also remove or update the
VendorSpecificRegistersstruct definition at lines 365-368 since it's no longer used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.vscode/settings.json(1 hunks)kernel/Cargo.toml(1 hunks)kernel/src/drivers/ata/ahci/mod.rs(1 hunks)kernel/src/main.rs(2 hunks)shared/common/Cargo.toml(1 hunks)shared/common/src/error/ahci.rs(1 hunks)shared/common/src/error/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/settings.json
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/src/main.rs (4)
kernel/src/memory/memory_map.rs (1)
parse_map(178-245)kernel/src/drivers/pci.rs (2)
common(381-383)scan_pci(386-429)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)kernel/src/drivers/ata/ahci/mod.rs (6)
major_version(173-175)minor_version(178-180)interface_speed(47-49)power(604-609)speed(611-614)detection(616-621)
🔇 Additional comments (4)
kernel/Cargo.toml (1)
12-18: LGTM! Kernel dependency additions align with shared crate.These dependency additions mirror those in
shared/common/Cargo.toml, ensuring consistent enum and macro support across the kernel and shared crates for AHCI integration.shared/common/src/error/mod.rs (1)
1-7: LGTM! Standard module re-export pattern.The AHCI error module follows the established pattern for general and paging modules, cleanly exposing AHCI-specific error types to the public API.
kernel/src/main.rs (2)
29-60: LGTM! Import additions support AHCI integration.The new imports provide the necessary AHCI types, memory mapping utilities, and paging flags required for AHCI device detection and initialization.
142-155: LGTM! Port detection logic is correct.The port iteration properly checks for Active power management state before printing disk information, and correctly uses the AHCI status accessors.
kernel/src/drivers/ata/ahci/mod.rs
Outdated
| pub fn speed(&self) -> InterfaceSpeed { | ||
| let speed = ((self.0 >> 4) & 0xf) as u8; | ||
| unsafe { InterfaceSpeed::unchecked_transmute_from(speed) } | ||
| } |
There was a problem hiding this comment.
Unsafe transmute in speed() can cause UB.
Similar to the interface_speed() issue, using unchecked_transmute_from on hardware values can cause undefined behavior if the hardware reports invalid speed values.
Use safe conversion:
pub fn speed(&self) -> InterfaceSpeed {
let speed = ((self.0 >> 4) & 0xf) as u8;
- unsafe { InterfaceSpeed::unchecked_transmute_from(speed) }
+ InterfaceSpeed::try_from(speed)
+ .unwrap_or(InterfaceSpeed::DevNotPresent)
}🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/mod.rs around lines 611 to 614, the method reads
a 4-bit hardware field and unsafely transmutes it to InterfaceSpeed; replace the
unsafe unchecked_transmute_from with a safe conversion that validates the value
(e.g. use TryFrom/TryInto or a match over the expected numeric values) and map
out-of-range values to a safe default (such as InterfaceSpeed::Unknown/Error) so
that invalid hardware-reported values cannot cause UB; remove the unsafe call
and return the validated/mapped InterfaceSpeed instead.
kernel/src/drivers/ata/ahci/mod.rs
Outdated
| pub fn max_speed(&self) -> InterfaceSpeedRestriction { | ||
| let speed = ((self.0 >> 4) & 0xf) as u8; | ||
| unsafe { | ||
| InterfaceSpeedRestriction::unchecked_transmute_from(speed) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsafe transmute in max_speed() can cause UB.
Using unchecked_transmute_from on hardware register values is unsafe.
Use safe conversion:
pub fn max_speed(&self) -> InterfaceSpeedRestriction {
let speed = ((self.0 >> 4) & 0xf) as u8;
- unsafe {
- InterfaceSpeedRestriction::unchecked_transmute_from(speed)
- }
+ InterfaceSpeedRestriction::try_from(speed)
+ .unwrap_or(InterfaceSpeedRestriction::NoRestriction)
}📝 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.
| pub fn max_speed(&self) -> InterfaceSpeedRestriction { | |
| let speed = ((self.0 >> 4) & 0xf) as u8; | |
| unsafe { | |
| InterfaceSpeedRestriction::unchecked_transmute_from(speed) | |
| } | |
| } | |
| pub fn max_speed(&self) -> InterfaceSpeedRestriction { | |
| let speed = ((self.0 >> 4) & 0xf) as u8; | |
| InterfaceSpeedRestriction::try_from(speed) | |
| .unwrap_or(InterfaceSpeedRestriction::NoRestriction) | |
| } |
🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/mod.rs around lines 640 to 645, the function
max_speed() unsafely uses InterfaceSpeedRestriction::unchecked_transmute_from on
raw register bits which can lead to UB; replace the unchecked transmute with a
safe conversion that validates the u8 value — implement or use a TryFrom<u8> /
from_u8 helper on InterfaceSpeedRestriction (or a match mapping valid bit
patterns to enum variants) and return a Result/Option or a documented default
for out-of-range values; call that safe converter here with the extracted speed
instead of unchecked_transmute_from.
kernel/src/drivers/ata/ahci/mod.rs
Outdated
| pub fn set_status(&mut self, port: u8) { | ||
| self.cmd.set_st(); | ||
| (0x0u8..=0x1fu8).contains(&port).then(|| { | ||
| self.sact.0 &= !(0x1f << 8); | ||
| self.sact.0 |= (port as u32) << 8; | ||
| }); | ||
| } | ||
|
|
||
| pub fn send_command(&mut self, port: u8) { | ||
| self.cmd.set_st(); | ||
| (0x0u8..=0x1fu8).contains(&port).then(|| { | ||
| self.ci.0 &= !(0x1f << 8); | ||
| self.ci.0 |= (port as u32) << 8; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Incorrect bit manipulation in set_status() and send_command().
Both methods incorrectly shift the port number by 8 bits and clear bits 12:8. The SACT (SATA Active) and CI (Command Issue) registers are bitmasks where each bit position corresponds to a command slot (0-31), not shifted values.
Apply this fix:
pub fn set_status(&mut self, port: u8) {
- self.cmd.set_st();
- (0x0u8..=0x1fu8).contains(&port).then(|| {
- self.sact.0 &= !(0x1f << 8);
- self.sact.0 |= (port as u32) << 8;
- });
+ if port <= 31 {
+ self.sact.0 |= 1 << port;
+ }
}
pub fn send_command(&mut self, port: u8) {
- self.cmd.set_st();
- (0x0u8..=0x1fu8).contains(&port).then(|| {
- self.ci.0 &= !(0x1f << 8);
- self.ci.0 |= (port as u32) << 8;
- });
+ if port <= 31 {
+ self.ci.0 |= 1 << port;
+ }
}Note: The parameter should probably be renamed from port to slot since these registers operate on command slots, not port numbers.
🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/mod.rs around lines 765 to 779, set_status() and
send_command() incorrectly treat the argument as a port and shift by 8, clearing
and setting bits 12:8; instead treat the argument as a command slot (0..=31),
validate the slot range, clear the single bit for that slot with sact.0 &=
!(1u32 << slot) and then set it with sact.0 |= 1u32 << slot (and similarly for
ci.0 in send_command), and rename the parameter from `port` to `slot` to reflect
its actual meaning.
| num_enum = { version = "0.7.5", default-features = false, features = [ | ||
| "complex-expressions", | ||
| ] } | ||
| derive_more = { version = "2.0.1", default-features = false, features = [ | ||
| "full", | ||
| ] } | ||
| strum_macros = { version = "0.27", default-features = false } | ||
| strum = { version = "0.27", default-features = false } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest versions of num_enum crate rust 2024 2025
💡 Result:
- Latest in 2024: num_enum 0.7.3 — released July 29, 2024. [1]
- Latest in 2025: num_enum 0.7.5 — documentation/build entry dated Oct 19, 2025 (0.7.5 release). [2]
Sources: [1] lib.rs crate history for num_enum; [2] docs.rs / num_enum (0.7.5) build/docs entry.
🌐 Web query:
latest versions of strum crate rust 2024 2025
💡 Result:
Summary — latest releases:
- 2024 (latest release in 2024): strum 0.26.3 (released Jun 23, 2024). [1]
- 2025 (most recent stable releases): strum 0.27.2 and strum_macros 0.27.2 (current latest documented). [2][3]
Sources:
- lib.rs release list (shows 0.26.3 on 2024-06-23). [1]
- docs.rs strum (latest release metadata). [2]
- docs.rs strum_macros (latest release metadata). [3]
🌐 Web query:
latest versions of strum_macros crate rust 2024 2025
💡 Result:
- 2024 — latest published series: 0.26.x; most recent 2024 release listed: 0.26.3 (2024-06-23). [1]
- 2025 — latest series: 0.27.x; releases in 2025 include 0.27.1 (2025-02-17) and the crate docs show 0.27.2 as the current/latest release. [1][2]
Sources:
[1] lib.rs strum release list (shows 0.26.3 — 2024-06-23 and 0.27.1 — 2025-02-17).
[2] docs.rs strum_macros (latest = 0.27.2).
🏁 Script executed:
find . -name "Cargo.toml" -path "*/shared/common/*" | head -20Repository: sagi21805/LearnixOS
Length of output: 89
🏁 Script executed:
cat -n ./shared/common/Cargo.toml | sed -n '1,25p'Repository: sagi21805/LearnixOS
Length of output: 672
LGTM, but strum and strum_macros have newer patch versions available.
The added dependencies enable the AHCI enum conversions and macro-based utilities. All dependencies correctly disable default features for no_std compatibility. However:
- num_enum 0.7.5 is current (Oct 2025)
- strum and strum_macros are pinned to 0.27, but the latest stable patch is 0.27.2 (released Feb 2025)
Consider updating strum and strum_macros to 0.27.2 for the latest patches, though 0.27 remains semver-compatible.
🤖 Prompt for AI Agents
In shared/common/Cargo.toml around lines 7 to 14 the strum and strum_macros
dependencies are pinned to "0.27" while patch 0.27.2 is available; update the
version strings for both strum and strum_macros to "0.27.2" (keeping
default-features = false and existing feature lists intact), then run cargo
update -p strum -p strum_macros (or cargo update) to refresh the lockfile so the
project uses the patched releases.
| #[repr(u16)] | ||
| #[derive(Debug, Clone, Copy, TryFromPrimitive, Error, EnumIter)] | ||
| #[num_enum(error_type(name = ConversionError<u16>, constructor = ConversionError::CantConvertFrom))] | ||
| pub enum AhciError { | ||
| #[error("Data integrity error that occurred was recovered")] | ||
| RecoveredDataIntegrityError = 1 << 0, | ||
| #[error("Comm between device and host was lost and re-established")] | ||
| RecoveredCommunicationError = 1 << 1, | ||
| #[error("Data integrity error was occurred and NOT recovered")] | ||
| DataIntegrityError = 1 << 8, | ||
| #[error("A communication error that was not recovered occurred")] | ||
| PersistentCommORDataIntegrityError = 1 << 9, | ||
| #[error("A violation of the SATA protocol was detected")] | ||
| ProtocolError = 1 << 10, | ||
| #[error("The host bus adapter experienced an internal error")] | ||
| InternalError = 1 << 11, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for SERR and SataError references in the codebase
rg -n "SERR|SataError|sata.*[Ee]rror" --type rust -i -A 2 -B 2 | head -100Repository: sagi21805/LearnixOS
Length of output: 2531
🌐 Web query:
AHCI 1.3.1 SERR register error bits specification
💡 Result:
Below are the PxSERR (Port x Serial ATA Error / SCR1 SError) bit fields and their meanings from the AHCI Spec Rev. 1.3.1 (PxSERR at port offset 0x30). See the AHCI spec for full normative text. [1][2]
- Bits 31:27 — DIAG (Diagnostics) — diagnostic information for vendor/diagnostic use. [1].
- Bits 26:16 — Reserved. [1].
- Bit 15 — X (Exchanged) — indicates device-presence change (set on COMINIT, reflected to PxIS.PCS). [1].
- Bit 14 — F (Unknown FIS Type) — one or more received FIS had an unrecognized type (good CRC). [1].
- Bit 13 — T (Transport state transition error) — error in Transport-layer state transition. [1].
- Bit 12 — S (Link Sequence Error) — one or more Link-layer state machine errors. [1].
- Bit 11 — H (Handshake Error) — one or more R_ERR handshake responses (CRC/8b/10b decode etc.). [1].
- Bit 10 — C (CRC Error) — one or more Link-layer CRC errors. [1].
- Bit 9 — D (Disparity Error) — not used by AHCI. [1].
- Bit 8 — B (10B->8B Decode Error) — one or more 10b->8b decoding errors. [1].
- Bit 7 — W (Comm Wake) — Comm Wake detected by the PHY. [1].
- Bit 6 — I (Phy Internal Error) — PHY internal error detected. [1].
- Bit 5 — N (PhyRdy Change) — PhyRdy signal changed state (reflected in PxIS.PRCS). [1].
- Bits 4:2 — Reserved. [1].
- Bit 1 — E (Internal Error) — HBA internal error (e.g., DMA/overflow/sync FIFO, master/target abort). Typically accompanied by PxIS non‑fatal/fatal bits. [1].
- Bit 0 — P (Protocol Error) — Serial ATA protocol violation detected. [1].
Note: The spec also defines interpretations for additional ERR subfields such as Persistent Communication/Data Integrity (C), Transient Data Integrity (T), Recovered errors (M/I) etc., and the register is RWC (read/clear by writing 1s) — consult the normative AHCI Rev.1.3.1 document for implementation details and exact bit numbering/clear semantics. [1][2]
Sources: AHCI Spec Rev. 1.3.1 (PxSERR / SCR1 definitions). [1] Studylib copy of "Serial ATA AHCI 1.3.1 Specification" (section 3.3.12). [2] Intel AHCI spec page (download).
🏁 Script executed:
# Get the SataError struct and its methods
sed -n '673,750p' kernel/src/drivers/ata/ahci/mod.rsRepository: sagi21805/LearnixOS
Length of output: 2116
🏁 Script executed:
# Search for how AhciError is used in the codebase
rg "AhciError" --type rust -B 2 -A 2Repository: sagi21805/LearnixOS
Length of output: 1300
🏁 Script executed:
# Look for documentation or comments around AhciError definition
sed -n '1,60p' shared/common/src/error/ahci.rsRepository: sagi21805/LearnixOS
Length of output: 2204
🏁 Script executed:
# Search for any reference documentation about these error bits
rg -i "recovered|integrity|communication|protocol.*error|handshake" kernel/src/drivers/ata/ --type rust -B 3 -A 3Repository: sagi21805/LearnixOS
Length of output: 949
🏁 Script executed:
# Check if there's a specification document referenced in the repo
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.pdf" \) | xargs grep -l "AHCI\|SATA\|error.*bit" 2>/dev/null | head -5Repository: sagi21805/LearnixOS
Length of output: 45
Fix AhciError enum to match AHCI 1.3.1 PxSERR specification or document the custom interpretation.
The AhciError enum bit assignments do not correspond to the AHCI 1.3.1 specification for the PxSERR Error Register (bits 15:0). For example:
- Bit 0 in spec = Protocol Error (P), but code defines RecoveredDataIntegrityError
- Bit 10 in spec = CRC Error (C), but code defines ProtocolError
- Bit 11 in spec = Handshake Error (H), but code defines InternalError
Either align the enum to the standard specification or add a comment explaining the custom vendor/diagnostic interpretation if intentional.
Additionally, fix the grammar in the "DataIntegrityError" message: "was occurred" should be "occurred".
🤖 Prompt for AI Agents
In shared/common/src/error/ahci.rs around lines 36 to 52, the AhciError enum's
bit assignments and variant meanings do not match the AHCI 1.3.1 PxSERR (bits
15:0) specification and one variant message has bad grammar; either realign the
enum variant names and numeric values to the official PxSERR bit meanings (e.g.,
set Protocol Error to bit 0, CRC Error to bit 10, Handshake Error to bit 11,
etc.), update all variant names and #[error(...)] messages accordingly (fix "was
occurred" → "occurred"), or if the current mapping is intentional add a clear
top-of-file comment documenting the custom interpretation and its source, and
ensure the num_enum attributes still reflect the intended conversions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
kernel/src/memory/allocators/page_allocator/allocator.rs (1)
219-235: Silent failure whenalign_tofails in deallocate.If
layout.align_to(...)returnsErr, the deallocate silently does nothing—the block remains marked as allocated, leaking memory. Consider logging or panicking on alignment failure, as this indicates a programming error.🔎 Suggested fix
unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) { - if let Ok(layout) = - layout.align_to(REGULAR_PAGE_ALIGNMENT.as_usize()) - { + let layout = layout + .align_to(REGULAR_PAGE_ALIGNMENT.as_usize()) + .expect("deallocate: layout alignment failed"); let start_position = Self::resolve_address(PhysicalAddress::new_unchecked( ptr.as_ptr() as usize - PHYSICAL_MEMORY_OFFSET, )); let block = ContiguousBlockLayout::from_start_size( &start_position, layout.size() / REGULAR_PAGE_SIZE, ); self.map_mut() .unset_contiguous_block(&start_position, &block); - } }shared/common/src/enums/paging.rs (1)
32-44: Replace unsafe transmute with safe TryFromPrimitive conversion.Both
next()andprev()useunsafe { core::mem::transmute(n) }after arithmetic, which can cause undefined behavior if the computed value is outside the valid enum range (1-4). While boolean guards currently prevent invalid values, this is fragile and error-prone.🔎 Proposed fix using the derived TryFromPrimitive
pub fn next(&self) -> Option<Self> { let n = (*self as u8) - 1; - (n > 0).then(|| unsafe { core::mem::transmute(n) }) + Self::try_from(n).ok().filter(|_| n > 0) } pub fn prev(&self) -> Result<Self, TableError> { let n = (*self as u8) + 1; - (n <= 4) - .then(|| unsafe { core::mem::transmute(n) }) - .ok_or(TableError::Full) + Self::try_from(n).map_err(|_| TableError::Full) }
♻️ Duplicate comments (11)
kernel/src/drivers/ata/ahci/fis.rs (2)
20-20: Typo:_resvered0should be_reserved0.
133-134: Fix bit extraction instatus_high()to return bits 6-4.The mask
& !0x7(0xF8) after shifting discards bits 2-0 of the shifted value, but you need bits 2-0 (originally bits 6-4). Use& 0x7instead:pub fn status_high(&self) -> u8 { - (self.status >> 4) & !0x7 + (self.status >> 4) & 0x7 }kernel/src/main.rs (1)
116-127: AHCI register access misalignment persists.The code aligns BAR5 down to a page boundary for mapping but then accesses the AHCI registers at the aligned address instead of the original BAR5 address. If BAR5 is not page-aligned (e.g.,
0xFEBF1400), aligning down gives0xFEBF1000, and accessing registers at the aligned address reads from the wrong location.🔎 Proposed fix preserving the offset
The fix requires either:
- Passing the original BAR5 address to
HBAMemoryRegisters::newand letting it handle alignment internally, or- Computing and preserving the offset here and adjusting the pointer after mapping.
For option 1, modify the call:
let a = unsafe { PhysicalAddress::new_unchecked( device.general_device.bar5.address(), ) }; - let aligned = a.align_down(REGULAR_PAGE_ALIGNMENT); - let hba = HBAMemoryRegisters::new(aligned); + let hba = HBAMemoryRegisters::new(a); if let Err(e) = hba { println!("{}", e) }Then update
HBAMemoryRegisters::newinhba.rsto handle alignment internally (see comments on that file).kernel/src/drivers/ata/ahci/hba.rs (8)
65-67: Unsafe transmute can cause undefined behavior.Using
core::mem::transmuteon a 4-bit masked value (0-15) is unsafe whenInterfaceSpeedonly defines variants 0-3. If hardware returns an invalid value, this causes undefined behavior.🔎 Proposed fix using safe conversion
pub fn interface_speed(&self) -> InterfaceSpeed { - unsafe { core::mem::transmute(((self.0 >> 20) & 0xf) as u8) } + let speed = ((self.0 >> 20) & 0xf) as u8; + InterfaceSpeed::try_from(speed).unwrap_or(InterfaceSpeed::DevNotPresent) }
79-82: Bit collision: bothpmdandsscuse bit 15.According to the AHCI spec, PMD (PIO Multiple DRQ Block) is bit 15 and SSC (Slumber State Capable) is bit 14. The current code defines both on bit 15, causing a collision.
🔎 Proposed fix
// Programmed I/O multiple Data request block ro_flag!(pmd, 15); // Slumber state capable - ro_flag!(ssc, 15); + ro_flag!(ssc, 14);
87-90: Off-by-one:number_of_commandsneeds +1.Per AHCI spec, this field is "Number of Command Slots - 1". The method should add 1 after masking to return the actual count (1-32).
🔎 Proposed fix
// This value is between 1 and 32 pub fn number_of_commands(&self) -> u8 { - ((self.0 >> 8) & 0x1f) as u8 + ((self.0 >> 8) & 0x1f) as u8 + 1 }
101-104: Off-by-one:number_of_portsneeds +1.Per AHCI spec, this field is "Number of Ports - 1". Add 1 after masking to return the actual count.
🔎 Proposed fix
/// Returns the number of ports implemented pub fn number_of_ports(&self) -> u8 { - (self.0 & 0x1f) as u8 + (self.0 & 0x1f) as u8 + 1 }
277-281: Typo:buffet_sizeshould bebuffer_size.Method name has a typo. Rename to
buffer_sizefor clarity.🔎 Proposed fix
/// ZERO is invalid /// TODO understand how to check if i have both receive and transmit - pub fn buffet_size(&self) -> Option<NonZero<usize>> { + pub fn buffer_size(&self) -> Option<NonZero<usize>> { NonZero::new((self.0 & 0xffff) as usize) }
642-676: Unsafeunchecked_transmute_fromcan cause UB.Both
speed()(line 644) andmax_speed()(lines 673-674) useunchecked_transmute_fromon 4-bit masked values, assuming hardware always returns valid enum values. Invalid hardware values (4-15 for a 4-bit field) cause undefined behavior.🔎 Proposed fixes using safe conversions
pub fn speed(&self) -> InterfaceSpeed { let speed = ((self.0 >> 4) & 0xf) as u8; - unsafe { InterfaceSpeed::unchecked_transmute_from(speed) } + InterfaceSpeed::try_from(speed).unwrap_or(InterfaceSpeed::DevNotPresent) }pub fn max_speed(&self) -> InterfaceSpeedRestriction { let speed = ((self.0 >> 4) & 0xf) as u8; - unsafe { - InterfaceSpeedRestriction::unchecked_transmute_from(speed) - } + InterfaceSpeedRestriction::try_from(speed) + .unwrap_or(InterfaceSpeedRestriction::NoRestriction) }
929-934: Inverted assert conditions will panic on valid inputs.The assertions check
len < 2andlen > 16, which will panic for valid lengths between 2 and 16 DWORDs. The conditions are inverted.🔎 Proposed fix
/// Length of command FIS in dwords pub fn set_command_fis_len_dw(&mut self, len: u8) { - assert!(len < 2, "Len must be smaller then 2"); - assert!(len > 16, "Len must be greater then 16 "); + assert!(len >= 2, "Len must be at least 2"); + assert!(len <= 16, "Len must be at most 16"); self.0 |= len as u32; }
971-976:set_dbconly asserts but never sets the value.The function validates the input but doesn't actually modify
self.0with the byte count. The DBC field is 22 bits (bits 21:0), and bit 31 contains the interrupt flag.🔎 Proposed fix
/// Set the data byte count of the buffer on the prdt pub fn set_dbc(&mut self, dbc: u32) { const MB: u32 = 1 << 20; - assert!(dbc < 4 * MB, "DBC should be smller then 4Mib"); + assert!(dbc < 4 * MB, "DBC should be smaller than 4MiB"); + // DBC is bits 21:0, preserve interrupt flag at bit 31 + self.0 = (self.0 & (1 << 31)) | (dbc & 0x3FFFFF); }
🧹 Nitpick comments (2)
kernel/src/memory/allocators/page_allocator/mod.rs (1)
13-25: Macro relies on caller importingREGULAR_PAGE_SIZEandREGULAR_PAGE_ALIGNMENT.The macro uses
REGULAR_PAGE_SIZEandREGULAR_PAGE_ALIGNMENTwithout importing them, requiring callers to have these in scope. Consider adding the imports inside the macro for self-contained usage:🔎 Suggested fix
macro_rules! alloc_pages { ($page_number: expr) => {{ use core::alloc::{Allocator, Layout}; + use common::constants::{REGULAR_PAGE_SIZE, REGULAR_PAGE_ALIGNMENT}; use $crate::memory::allocators::page_allocator::ALLOCATOR; ALLOCATOR .assume_init_ref() .allocate(Layout::from_size_align_unchecked( REGULAR_PAGE_SIZE * $page_number, REGULAR_PAGE_ALIGNMENT.as_usize(), )) .unwrap() .addr() .get() }}; }kernel/src/drivers/ata/ahci/fis.rs (1)
138-148: Consider addingalign(4)to theFisunion for consistent alignment.The individual FIS structs like
RegisterH2DandDmaSetupusealign(4), but the union itself only hasrepr(C). For hardware MMIO consistency, the union should also enforce 4-byte alignment.🔎 Suggested fix
-#[repr(C)] +#[repr(C, align(4))] pub union Fis {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
kernel/src/drivers/ata/ahci/fis.rs(1 hunks)kernel/src/drivers/ata/ahci/hba.rs(1 hunks)kernel/src/main.rs(2 hunks)kernel/src/memory/allocators/page_allocator/allocator.rs(1 hunks)kernel/src/memory/allocators/page_allocator/extensions.rs(2 hunks)kernel/src/memory/allocators/page_allocator/mod.rs(1 hunks)shared/common/src/enums/ata.rs(1 hunks)shared/common/src/enums/mod.rs(2 hunks)shared/common/src/enums/paging.rs(3 hunks)
🔇 Additional comments (10)
kernel/src/memory/allocators/page_allocator/allocator.rs (1)
197-215: LGTM on the allocate refactor.The flattened if-let structure with early return is cleaner than nested matches and correctly maintains the same allocation logic.
shared/common/src/enums/mod.rs (1)
1-2: LGTM on the new module additions.The
ahciandatamodules follow the existing pattern of declaring submodules and re-exporting their contents via glob imports.Also applies to: 16-17
shared/common/src/enums/ata.rs (1)
5-12: LGTM on the AtaCommand enum.Clean implementation using
num_enum::TryFromPrimitivefor safe primitive conversion with proper error handling viaConversionError. Therepr(u8)ensures ABI compatibility with hardware command codes.kernel/src/memory/allocators/page_allocator/extensions.rs (1)
87-99: The array bounds are safe—no verification needed.The code correctly accesses
PageTableLevel::VARIANTS, which contains exactly 4 elements[PML4, PDPT, PD, PT]in declaration order. SincePageSizehas only 3 variants with maximum value 2 (Regular = 2), the highest index accessed isVARIANTS[2 + 1] = VARIANTS[3], which is valid. TheVARIANTSordering matches the expected x86-64 page table hierarchy (highest to lowest level).kernel/src/memory/allocators/page_allocator/mod.rs (1)
16-22: Review comment is incorrect—the code follows idiomatic Rust patterns and compiles without errors.The macro correctly defers the
unsaferequirement to callers. Both invocations in the codebase (main.rslines 80, 87 andhba.rsline 1020) are already wrapped inunsafeblocks as required by the compiler. Adding anunsafeblock inside the macro would be redundant and non-idiomatic.shared/common/src/enums/paging.rs (2)
13-24: LGTM! Derive-based enum conversions.The use of
TryFromPrimitive,EnumIter, andVariantArraywith a custom error type is idiomatic and eliminates manual conversion boilerplate. This approach is safer and more maintainable.
45-136: LGTM! PageSize enum and utility methods.The derive-based approach matches
PageTableLevel, and the utility methods (from_layout,from_alignment,size_in_regular_pages) provide clean, safe conversions without unsafe code.kernel/src/main.rs (1)
29-63: LGTM! Imports organized appropriately.The imports are well-structured and support the AHCI integration, with appropriate types from drivers, common enums, and paging structures.
kernel/src/drivers/ata/ahci/hba.rs (2)
1-42: LGTM! Well-documented AHCI implementation.The file header clearly references the Intel AHCI specification, and the imports are appropriately organized. The use of proc-macros for flag generation is a good approach for register access.
1063-1081: LGTM! Port detection logic is correct.The port enumeration loop correctly checks implemented ports, validates power state, and prints device information. The logic follows the AHCI specification for port discovery.
| #[repr(C, align(4))] | ||
| #[derive(Clone, Copy, Debug)] | ||
| pub struct RegisterH2D { | ||
| fis_type: FisType, | ||
| pm_flags: u8, | ||
| command: AtaCommand, | ||
| features: u8, | ||
| lba1: u8, | ||
| lba2: u8, | ||
| lba3: u8, | ||
| device: u8, | ||
| lba4: u8, | ||
| lba5: u8, | ||
| lba6: u8, | ||
| features_ext: u8, | ||
| sector_count: u8, | ||
| sector_count_ext: u8, | ||
| _resvered0: u8, | ||
| control: u8, | ||
| _reserved1: [u8; 4], | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read the fis.rs file
find . -name "fis.rs" -path "*/ahci/*" | head -1 | xargs wc -l
# Extract the RegisterH2D struct
find . -name "fis.rs" -path "*/ahci/*" | head -1 | xargs cat
# Search for size assertions related to FIS structures
find . -name "*.rs" -path "*/ahci/*" -exec grep -l "size_of\|assert.*size" {} \;Repository: sagi21805/LearnixOS
Length of output: 2908
🏁 Script executed:
#!/bin/bash
# Find FisType and AtaCommand definitions
find . -path "*/common/*enums*" -name "*.rs" | head -5
# Search for FisType definition
find . -name "*.rs" | xargs grep -l "enum FisType" 2>/dev/null
# Search for AtaCommand definition
find . -name "*.rs" | xargs grep -l "enum AtaCommand" 2>/dev/null
# Search for any size-related assertions in test modules or with static_assertions
find . -name "*.rs" -path "*/ahci/*" | xargs grep -E "size_of|assert.*size|static_assertions" 2>/dev/nullRepository: sagi21805/LearnixOS
Length of output: 448
🏁 Script executed:
#!/bin/bash
# Read FisType enum definition
echo "=== FisType enum ==="
cat ./shared/common/src/enums/ahci.rs
echo ""
echo "=== AtaCommand enum ==="
cat ./shared/common/src/enums/ata.rsRepository: sagi21805/LearnixOS
Length of output: 4214
Add compile-time size assertion for RegisterH2D struct and fix field name typo.
The AHCI H2D Register FIS format must be exactly 20 bytes. The struct size is correct at 20 bytes, but add a static assertion to enforce this constraint and prevent accidental layout changes. Also, the field name _resvered0 contains a typo and should be _reserved0.
#[repr(C, align(4))]
#[derive(Clone, Copy, Debug)]
pub struct RegisterH2D {
fis_type: FisType,
pm_flags: u8,
command: AtaCommand,
features: u8,
lba1: u8,
lba2: u8,
lba3: u8,
device: u8,
lba4: u8,
lba5: u8,
lba6: u8,
features_ext: u8,
sector_count: u8,
sector_count_ext: u8,
_reserved0: u8,
control: u8,
_reserved1: [u8; 4],
}
const _: () = assert!(core::mem::size_of::<RegisterH2D>() == 20);🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/fis.rs around lines 3 to 23, fix the typo in the
struct field name from `_resvered0` to `_reserved0` and add a compile-time size
assertion to lock the RegisterH2D layout to 20 bytes; update the struct field
name and then add a const assertion using core::mem::size_of::<RegisterH2D>() ==
20 (e.g. const _: () = assert!(core::mem::size_of::<RegisterH2D>() == 20);) to
fail compilation if the struct size changes.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
kernel/src/main.rs (1)
118-133: BAR5 alignment loses register offset.Line 125 aligns the BAR5 address down, but line 126 passes the aligned address to
HBAMemoryRegisters::new(). If BAR5 is not page-aligned (e.g.,0xFEBF1400), this causes register access at the wrong offset within the mapped page.The correct approach is to pass the original BAR5 address to
HBAMemoryRegisters::new()and let it handle alignment internally by mapping the aligned page and computing the offset to the actual registers.🔎 Proposed fix
if device.common().device_type.is_ahci() { let a = unsafe { PhysicalAddress::new_unchecked( device.general_device.bar5.address(), ) }; - let aligned = a.align_down(REGULAR_PAGE_ALIGNMENT); - let hba = HBAMemoryRegisters::new(aligned).unwrap(); + let hba = HBAMemoryRegisters::new(a).unwrap(); let active = hba.probe(); let controller = hba.map_device(0); println!( "{:x?}", controller.port_commands as *const _ as usize ) }Note: This requires updating
HBAMemoryRegisters::new()to handle unaligned addresses internally (see related comment on hba.rs).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kernel/src/drivers/ata/ahci/hba.rs(1 hunks)kernel/src/main.rs(2 hunks)kernel/src/memory/allocators/page_allocator/allocator.rs(1 hunks)shared/common/src/error/ahci.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/common/src/error/ahci.rs
🧰 Additional context used
🧬 Code graph analysis (2)
kernel/src/main.rs (3)
kernel/src/drivers/pci.rs (2)
common(381-383)scan_pci(386-429)kernel/src/drivers/ata/ahci/hba.rs (1)
new(1038-1057)kernel/src/memory/allocators/page_allocator/allocator.rs (1)
new(47-57)
kernel/src/drivers/ata/ahci/hba.rs (3)
learnix-macros/src/lib.rs (4)
flag(74-119)ro_flag(131-150)rw1_flag(179-198)rwc_flag(155-174)kernel/src/memory/allocators/page_allocator/allocator.rs (1)
Self(206-208)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)
🔇 Additional comments (4)
kernel/src/memory/allocators/page_allocator/allocator.rs (2)
197-214: Clean refactoring with improved readability.The streamlined if-let chain replacing nested matches is more concise and maintains the same logic flow. Early return on success and unified error handling work well here.
217-217: TODO comment for future page invalidation.The comment indicates future work to invalidate pages during deallocation. Consider tracking this as a separate issue if not already captured.
kernel/src/drivers/ata/ahci/hba.rs (1)
1060-1090: Device probing logic looks sound.The port iteration and device detection logic is well-structured with appropriate power state checks and error handling. Note that this depends on fixing the off-by-one issue in
number_of_ports()(already flagged in past reviews).kernel/src/main.rs (1)
104-111: Clean keyboard input handling.The loop correctly reads raw scancodes and checks for the Enter key to proceed. The if-let pattern with early break is appropriate.
| pub fn set_pm_notif(&mut self, pm_port: u8) { | ||
| (0x0..0xf) | ||
| .contains(&pm_port) | ||
| .then(|| self.0 |= pm_port as u32); | ||
| } | ||
|
|
||
| /// Get port multiplier notification | ||
| pub fn get_pm_notif(&self, pm_port: u8) -> bool { | ||
| if (0x0..0xf).contains(&pm_port) { | ||
| (self.0 & !0xffff) & (1 << pm_port) != 0 | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
Logic error in get_pm_notif bit masking.
Line 742 uses (self.0 & !0xffff) which clears the lower 16 bits, but PM notification bits are in the lower 16 bits (0-15). The mask should preserve, not clear, those bits.
🔎 Proposed fix
pub fn get_pm_notif(&self, pm_port: u8) -> bool {
if (0x0..0xf).contains(&pm_port) {
- (self.0 & !0xffff) & (1 << pm_port) != 0
+ (self.0 & 0xffff) & (1 << pm_port) != 0
} else {
false
}
}📝 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.
| pub fn set_pm_notif(&mut self, pm_port: u8) { | |
| (0x0..0xf) | |
| .contains(&pm_port) | |
| .then(|| self.0 |= pm_port as u32); | |
| } | |
| /// Get port multiplier notification | |
| pub fn get_pm_notif(&self, pm_port: u8) -> bool { | |
| if (0x0..0xf).contains(&pm_port) { | |
| (self.0 & !0xffff) & (1 << pm_port) != 0 | |
| } else { | |
| false | |
| } | |
| } | |
| pub fn set_pm_notif(&mut self, pm_port: u8) { | |
| (0x0..0xf) | |
| .contains(&pm_port) | |
| .then(|| self.0 |= pm_port as u32); | |
| } | |
| /// Get port multiplier notification | |
| pub fn get_pm_notif(&self, pm_port: u8) -> bool { | |
| if (0x0..0xf).contains(&pm_port) { | |
| (self.0 & 0xffff) & (1 << pm_port) != 0 | |
| } else { | |
| false | |
| } | |
| } |
kernel/src/drivers/ata/ahci/hba.rs
Outdated
| pub fn map_device( | ||
| &'static mut self, | ||
| port_number: usize, | ||
| ) -> AhciDeviceController { | ||
| AhciDeviceController { | ||
| port: &mut self.ports[port_number], | ||
| port_commands: PortCommands::empty(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing validation in map_device.
The method indexes self.ports[port_number] without validating that:
port_numberis within bounds (0-29, given the 30-port array)- The port is actually implemented (
self.ghc.pi.is_port_implemented()) - A device is present and active on that port
This could panic or return a controller for a non-existent device.
🔎 Proposed fix
pub fn map_device(
&'static mut self,
port_number: usize,
) -> AhciDeviceController {
+ assert!(port_number < 30, "Port number must be less than 30");
+ assert!(
+ self.ghc.pi.is_port_implemented(port_number as u8),
+ "Port {} is not implemented", port_number
+ );
+ // Optionally check device presence/power state
AhciDeviceController {
port: &mut self.ports[port_number],
port_commands: PortCommands::empty(),
}
}Committable suggestion skipped: line range outside the PR's diff.
kernel/src/main.rs
Outdated
| let active = hba.probe(); | ||
| let controller = hba.map_device(0); |
There was a problem hiding this comment.
Hardcoded port 0 ignores probe results.
Line 127 calls probe() which returns the count of active devices, but line 128 hardcodes map_device(0) regardless of where devices were actually detected. If a device exists only on port 1 or higher, this will map an empty or non-existent port.
Consider using the probe results to map the first detected active port:
let active = hba.probe();
if active > 0 {
// Find first implemented port with active device
let port_idx = (0..30)
.find(|&i| hba.ghc.pi.is_port_implemented(i as u8)
&& matches!(hba.ports[i].ssts.power(), Ok(InterfacePowerManagement::Active)))
.expect("Active device reported but none found");
let controller = hba.map_device(port_idx);
// ...
}🤖 Prompt for AI Agents
In kernel/src/main.rs around lines 127 to 128, the code calls hba.probe() then
unconditionally maps port 0 which ignores probe results; instead, use the probe
count to decide whether to map any device and, if active > 0, scan implemented
ports to find the first port whose ssts.power() indicates Active (and that is
implemented via hba.ghc.pi.is_port_implemented), then call hba.map_device with
that found index; if no active port is found despite active > 0, return or error
appropriately, and if active == 0 skip mapping.
change into write volitile
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
kernel/src/drivers/ata/ahci/fis.rs (2)
3-25: Previously flagged: Typo and missing size assertion.The typo
_resvered0→_reserved0and missing compile-time size assertion (struct must be exactly 20 bytes per AHCI spec) were already flagged in previous reviews.
128-136: Previously flagged: Incorrect bit mask instatus_high().The bit extraction error in
status_high()using& !0x7instead of& 0x7was already identified in previous reviews.kernel/src/drivers/ata/ahci/hba.rs (2)
69-71: Previously flagged: Multiple critical and major issues remain unresolved.The following issues were identified in earlier reviews and are still present:
- Line 70: Unsafe transmute for enum conversion (major)
- Line 86: Bit collision between
sscandpmdflags (critical)- Lines 92-94: Off-by-one in
number_of_commandsmask (major)- Lines 106-108: Off-by-one in
number_of_portsmask (major)- Lines 677-680: Unsafe
unchecked_transmute_from(major)- Lines 980-985: Inverted assert conditions (critical)
- Lines 1028-1033:
set_dbcdoesn't set the value (critical)- Lines 765-778: Logic error in
get_pm_notifbit masking (critical)- Lines 917-931: Incorrect bit manipulation for SACT/CI registers (critical)
Also applies to: 85-86, 91-94, 104-108, 677-680, 980-985, 1028-1033, 765-778, 917-931
281-285: Previously flagged: Additional issues requiring attention.The following issues from earlier reviews remain unaddressed:
- Line 283: Typo
buffet_sizeshould bebuffer_size(minor)- Lines 1064-1081: Missing type parameter in
size_ofcalls (major refactor)- Lines 1096-1108:
HBAMemoryRegisters::newalignment handling (critical)- Lines 1150-1159: Missing validation in
map_device(major)Also applies to: 1064-1081, 1096-1108, 1150-1159
kernel/src/main.rs (1)
125-129: Previously flagged: BAR5 alignment and hardcoded port issues.Two issues were identified in earlier reviews:
- Line 126: Passing
alignedinstead ofatoHBAMemoryRegisters::new()may access wrong memory if BAR5 is not page-aligned- Line 128: Hardcoding
map_device(0)ignores the probe results and may map an inactive port
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kernel/src/drivers/ata/ahci/fis.rs(1 hunks)kernel/src/drivers/ata/ahci/hba.rs(1 hunks)kernel/src/main.rs(2 hunks)learnix-macros/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/src/main.rs (4)
kernel/src/drivers/pci.rs (2)
common(381-383)scan_pci(386-429)kernel/src/drivers/ata/ahci/hba.rs (2)
new(1096-1115)new(1167-1187)kernel/src/memory/allocators/page_allocator/allocator.rs (1)
new(47-57)shared/cpu_utils/src/structures/interrupt_descriptor_table.rs (1)
new(201-220)
🔇 Additional comments (1)
learnix-macros/src/lib.rs (1)
177-199: LGTM!The
rw1_flagmacro correctly generates aset_$flag_namemethod that sets the specified bit using the bitwise OR operation.
| // ANCHOR: ro_flag | ||
| /// This macro will obtain `flag_name` and the corresponding | ||
| /// `bit_number` and create read-only flag functionality | ||
| /// | ||
| /// With this information it will automatically generate | ||
| /// three methods | ||
| /// | ||
| /// 1. `is_$flag_name`: return true if the flag is set or false if not | ||
| #[proc_macro] |
There was a problem hiding this comment.
Fix documentation comment.
The comment states "three methods" but the macro only generates one method (is_$flag_name).
🔎 Proposed fix
/// This macro will obtain `flag_name` and the corresponding
/// `bit_number` and create read-only flag functionality
///
/// With this information it will automatically generate
-/// three methods
+/// one method
///
/// 1. `is_$flag_name`: return true if the flag is set or false if not🤖 Prompt for AI Agents
In learnix-macros/src/lib.rs around lines 122 to 130, the doc comment
incorrectly says the macro generates "three methods" but it only generates one
(`is_$flag_name`); update the documentation to accurately reflect that a single
read-only method `is_$flag_name` is generated (remove the "three methods"
wording and the numbered list or change it to a single item describing the
`is_$flag_name` method) so the comment matches the implementation.
| // ANCHOR: rwc_flag | ||
| #[proc_macro] | ||
| pub fn rwc_flag(input: TokenStream) -> TokenStream { | ||
| let FlagInput { name, bit, .. } = | ||
| syn::parse_macro_input!(input as FlagInput); | ||
|
|
||
| // build identifiers | ||
| let name_str = name.to_string(); | ||
| let clear_ident = format_ident!("clear_{}", name_str); | ||
|
|
||
| let expanded = quote! { | ||
| #[inline] | ||
| #[allow(dead_code)] | ||
| #[allow(unused_attributes)] | ||
| /// Sets the corresponding flag | ||
| pub const fn #clear_ident(&mut self) { | ||
| self.0 |= 1 << #bit; | ||
| } | ||
| }; | ||
|
|
||
| expanded.into() | ||
| } | ||
| // ANCHOR_END: rwc_flag |
There was a problem hiding this comment.
Critical: Clear operation sets the bit instead of clearing it.
The rwc_flag macro generates a clear_$flag_name method, but line 169 uses |= which sets the bit rather than clearing it. To clear a bit, use the bitwise AND with complement: &= !(1 << #bit).
Additionally, the comment on line 167 incorrectly states "Sets the corresponding flag" when it should say "Clears the corresponding flag".
🔎 Proposed fix
let expanded = quote! {
#[inline]
#[allow(dead_code)]
#[allow(unused_attributes)]
- /// Sets the corresponding flag
+ /// Clears the corresponding flag
pub const fn #clear_ident(&mut self) {
- self.0 |= 1 << #bit;
+ self.0 &= !(1 << #bit);
}
};📝 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.
| // ANCHOR: rwc_flag | |
| #[proc_macro] | |
| pub fn rwc_flag(input: TokenStream) -> TokenStream { | |
| let FlagInput { name, bit, .. } = | |
| syn::parse_macro_input!(input as FlagInput); | |
| // build identifiers | |
| let name_str = name.to_string(); | |
| let clear_ident = format_ident!("clear_{}", name_str); | |
| let expanded = quote! { | |
| #[inline] | |
| #[allow(dead_code)] | |
| #[allow(unused_attributes)] | |
| /// Sets the corresponding flag | |
| pub const fn #clear_ident(&mut self) { | |
| self.0 |= 1 << #bit; | |
| } | |
| }; | |
| expanded.into() | |
| } | |
| // ANCHOR_END: rwc_flag | |
| // ANCHOR: rwc_flag | |
| #[proc_macro] | |
| pub fn rwc_flag(input: TokenStream) -> TokenStream { | |
| let FlagInput { name, bit, .. } = | |
| syn::parse_macro_input!(input as FlagInput); | |
| // build identifiers | |
| let name_str = name.to_string(); | |
| let clear_ident = format_ident!("clear_{}", name_str); | |
| let expanded = quote! { | |
| #[inline] | |
| #[allow(dead_code)] | |
| #[allow(unused_attributes)] | |
| /// Clears the corresponding flag | |
| pub const fn #clear_ident(&mut self) { | |
| self.0 &= !(1 << #bit); | |
| } | |
| }; | |
| expanded.into() | |
| } | |
| // ANCHOR_END: rwc_flag |
🤖 Prompt for AI Agents
In learnix-macros/src/lib.rs around lines 153 to 175, the generated clear_<name>
method is wrong: it uses |= (which sets the bit) and has the doc comment "Sets
the corresponding flag"; change the implementation to perform a bitwise AND with
the complement to clear the bit (use &= !(1 << #bit)) and update the doc comment
to "Clears the corresponding flag"; keep inline/allow attributes and const fn
signature unchanged.
them out in some cases.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
learnix-macros/src/lib.rs (1)
89-128: Inconsistent volatile operation usage within the macro.The
set_identandis_identmethods now use volatile operations (appropriate for MMIO), but theunset_identmethod (line 114-116) and the named method (line 106-108) still use direct memory access without volatile operations. For MMIO registers (like AHCI hardware registers), all read-modify-write operations must use volatile semantics to prevent compiler optimizations.🔎 Proposed fix for consistent volatile operations
#[inline] #[allow(dead_code)] #[allow(unused_attributes)] - /// Sets the corresponding flag while returning self - pub const fn #name(self) -> Self { - Self(self.0 | (1 << #bit)) - } + /// Sets the corresponding flag while returning the modified value + pub fn #name(&mut self) -> usize { + unsafe { + let val = core::ptr::read_volatile( + self as *const _ as *mut usize + ); + let new_val = val | (1 << #bit); + core::ptr::write_volatile( + self as *const _ as *mut usize, + new_val + ); + new_val + } + } #[inline] #[allow(dead_code)] #[allow(unused_attributes)] /// Unset the corresponding flag - pub const fn #unset_ident(&mut self) { - self.0 &= !(1 << #bit); + pub fn #unset_ident(&mut self) { + unsafe { + let val = core::ptr::read_volatile( + self as *const _ as *mut usize + ); + core::ptr::write_volatile( + self as *const _ as *mut usize, + val & !(1 << #bit) + ) + } }
♻️ Duplicate comments (15)
learnix-macros/src/lib.rs (2)
135-143: Fix documentation comment.The documentation states "three methods" but the macro only generates one method (
is_$flag_name).🔎 Proposed fix
/// This macro will obtain `flag_name` and the corresponding /// `bit_number` and create read-only flag functionality /// /// With this information it will automatically generate -/// three methods +/// one method /// /// 1. `is_$flag_name`: return true if the flag is set or false if not
170-192: Critical: Clear operation sets the bit instead of clearing it.The
rwc_flagmacro generates aclear_$flag_namemethod, but line 186 uses|=which sets the bit rather than clearing it. To clear a bit, use the bitwise AND with complement:&= !(1 << #bit).Additionally, the comment on line 184 incorrectly states "Sets the corresponding flag" when it should say "Clears the corresponding flag".
🔎 Proposed fix
#[inline] #[allow(dead_code)] #[allow(unused_attributes)] - /// Sets the corresponding flag + /// Clears the corresponding flag pub const fn #clear_ident(&mut self) { - self.0 |= 1 << #bit; + self.0 &= !(1 << #bit); }kernel/src/drivers/ata/ahci/hba.rs (13)
1064-1081: Missing type parameter insize_ofcalls.Lines 1069 and 1071 use
size_of::<PortCommands>()without specifying theENTRIESconst generic. Should explicitly usesize_of::<PortCommands<ENTRIES>>().
69-71: Unsafe transmute can cause undefined behavior.Using
core::mem::transmutewith a 4-bit masked value that can range 0-15, butInterfaceSpeedonly defines variants 0-3, can lead to undefined behavior for invalid hardware values.
85-86: Bit collision:sscuses bit 15, same aspmdon line 83.Both
pmd(Programmed I/O Multiple DRQ Block) andssc(Slumber State Capable) are defined on bit 15. Per AHCI spec, PMD is bit 15 and SSC is bit 14.
91-94: Off-by-one: missing +1 for command slot count.Per AHCI spec, this field represents "Number of Command Slots - 1", so you must add 1 to get the actual count (1-32).
104-108: Off-by-one: missing +1 for port count.Per AHCI spec, this field is "Number of Ports - 1", so add 1 to return the actual count.
281-285: Typo:buffet_sizeshould bebuffer_size.
677-680: Unsafeunchecked_transmute_fromcan cause undefined behavior.Both
speed()andmax_speed()useunchecked_transmute_fromwhich assumes hardware always returns valid enum values. If the 4-bit field contains an undefined value (4-15), this causes undefined behavior.Also applies to: 706-711
772-778: Logic error: wrong mask inget_pm_notif.The mask
(self.0 & !0xffff)clears the lower 16 bits, but PM notification bits are in the lower 16 bits. Should use(self.0 & 0xffff)instead.
917-931: Incorrect bit manipulation for SACT and CI registers.Both methods shift the port number left by 8 and mask bits 8-12, but per AHCI spec, these registers use each bit to represent a command slot (0-31), not a shifted value. Should set bit directly:
self.sact.0 |= 1 << port.
980-985: Inverted assert conditions will panic for valid inputs.The assertions check
len < 2andlen > 16, but comments suggest valid lengths are 2-16 DWORDs. These conditions are inverted - they panic for valid values and pass for invalid ones.
1028-1033:set_dbconly asserts but never sets the value.The function validates input but doesn't modify
self.0with the byte count value.
1150-1159: Missing validation inmap_device.The method doesn't validate:
port_numberis within bounds (0-29)- The port is actually implemented
- A device is present and active on that port
This could panic or return a controller for a non-existent device.
1189-1189: Empty method implementation.The
identity_packetmethod has no implementation and performs no operation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
kernel/src/drivers/ata/ahci/hba.rs(1 hunks)learnix-macros/src/lib.rs(2 hunks)shared/cpu_utils/src/structures/paging/page_table_entry.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/src/drivers/ata/ahci/hba.rs (4)
learnix-macros/src/lib.rs (4)
flag(74-132)ro_flag(144-167)rw1_flag(196-215)rwc_flag(172-191)shared/cpu_utils/src/structures/paging/page_table_entry.rs (1)
empty(24-26)kernel/src/memory/allocators/page_allocator/allocator.rs (1)
Self(206-208)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)
🔇 Additional comments (2)
shared/cpu_utils/src/structures/paging/page_table_entry.rs (1)
66-109: The review comment's claim about const removal is incorrect. The methodsmap_unchecked(line 66) andmap(line 100) do not haveconstqualifiers in the current code and show no evidence of recent removal. These methods are currentlypub unsafe fn(non-const). The review may be confusing these with other const-qualified methods in the same file, such asset_flagsandset_flags_unchecked, which are properly markedpub const fnandpub const unsafe fnrespectively.Likely an incorrect or invalid review comment.
kernel/src/drivers/ata/ahci/hba.rs (1)
891-894: The review comment is incorrect. The methods already enforce alignment requirements through bit masking.The
cmd_list()method masks with& !((1 << 10) - 1), which enforces 1K-byte alignment—exactly the requirement that Command List must be located at 1K aligned memory address. Similarly,received_fis()masks with& !((1 << 8) - 1)to enforce 256-byte alignment, matching the requirement that Received FIS be 256 bytes aligned. The alignment validation is implemented correctly and already present in the code. The addresses are read from driver-initialized hardware registers, ensuring they come from known, properly-aligned memory allocations.Likely an incorrect or invalid review comment.
| pub fn set_current_cmd(&mut self, ccs: u8) { | ||
| if !self.is_st() { | ||
| return; | ||
| } | ||
| (0x0u8..=0x1fu8).contains(&ccs).then(|| { | ||
| self.0 &= !(0x1f << 8); | ||
| self.0 |= (ccs as u32) << 8; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Logic error: .then() result is discarded, bit manipulation may not execute.
The .then(|| { ... }) returns an Option that is immediately discarded. This means the bit manipulation inside the closure may or may not execute as intended depending on how the compiler optimizes this. If the goal is to conditionally execute the bit manipulation, use an if statement instead.
🔎 Proposed fix
pub fn set_current_cmd(&mut self, ccs: u8) {
if !self.is_st() {
return;
}
- (0x0u8..=0x1fu8).contains(&ccs).then(|| {
+ if (0x0u8..=0x1fu8).contains(&ccs) {
self.0 &= !(0x1f << 8);
self.0 |= (ccs as u32) << 8;
- });
+ }
}🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/hba.rs around lines 559 to 567, the closure
passed to .then() is ignored so the bit-manipulation may never run; replace the
.then(...) call with an explicit if condition to perform the mask and set
operations. Concretely: keep the existing is_st() guard, then use if
(0x0u8..=0x1fu8).contains(&ccs) { self.0 &= !(0x1fu32 << 8); self.0 |= (ccs as
u32) << 8; } ensuring the mask is cast to u32 and ccs is cast to u32 when
shifted.
| pub fn set_pm_notif(&mut self, pm_port: u8) { | ||
| (0x0..0xf) | ||
| .contains(&pm_port) | ||
| .then(|| self.0 |= pm_port as u32); |
There was a problem hiding this comment.
Logic error: setting wrong bits in PM notification.
The method sets self.0 |= pm_port as u32 which would set bits 0-15 directly based on the pm_port value (e.g., pm_port=5 sets bits 0 and 2). Instead, it should set bit pm_port using 1 << pm_port.
🔎 Proposed fix
pub fn set_pm_notif(&mut self, pm_port: u8) {
(0x0..0xf)
.contains(&pm_port)
- .then(|| self.0 |= pm_port as u32);
+ .then(|| self.0 |= 1 << pm_port);
}🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/hba.rs around lines 765 to 768, the code ORs the
raw pm_port value into self.0 which sets the wrong bits (e.g., pm_port=5 sets
bits corresponding to value 5 instead of bit 5); change the guard to validate
pm_port is in 0..16 (or 0..=0xF) and set the bit using a left shift: compute
(1u32 << pm_port as u32) and OR that into self.0 so the method sets the single
bit at position pm_port.
| // ANCHOR: rwc_flag | ||
| #[proc_macro] | ||
| pub fn rwc_flag(input: TokenStream) -> TokenStream { | ||
| let FlagInput { name, bit, .. } = | ||
| syn::parse_macro_input!(input as FlagInput); | ||
|
|
||
| // build identifiers | ||
| let name_str = name.to_string(); | ||
| let clear_ident = format_ident!("clear_{}", name_str); | ||
|
|
||
| let expanded = quote! { | ||
| #[inline] | ||
| #[allow(dead_code)] | ||
| #[allow(unused_attributes)] | ||
| /// Sets the corresponding flag | ||
| pub const fn #clear_ident(&mut self) { | ||
| self.0 |= 1 << #bit; | ||
| } | ||
| }; | ||
|
|
||
| expanded.into() | ||
| } | ||
| // ANCHOR_END: rwc_flag | ||
|
|
||
| // ANCHOR: rw1_flag | ||
| #[proc_macro] | ||
| pub fn rw1_flag(input: TokenStream) -> TokenStream { | ||
| let FlagInput { name, bit, .. } = | ||
| syn::parse_macro_input!(input as FlagInput); | ||
|
|
||
| // build identifiers | ||
| let name_str = name.to_string(); | ||
| let set_ident = format_ident!("set_{}", name_str); | ||
|
|
||
| let expanded = quote! { | ||
| #[inline] | ||
| #[allow(dead_code)] | ||
| #[allow(unused_attributes)] | ||
| /// Sets the corresponding flag | ||
| pub const fn #set_ident(&mut self) { | ||
| self.0 |= 1 << #bit; | ||
| } | ||
| }; | ||
|
|
||
| expanded.into() | ||
| } | ||
| // ANCHOR_END: rw1_flag |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add volatile operations for MMIO register semantics.
The rwc_flag and rw1_flag macros generate code for hardware register manipulation (Read-Write-Clear and Write-1-to-set semantics are common in MMIO registers like AHCI). However, unlike the main flag macro which now uses volatile operations, these macros use direct memory access. For MMIO correctness, these should also use volatile writes to prevent compiler optimizations.
🔎 Proposed fix for volatile operations
For rwc_flag:
#[inline]
#[allow(dead_code)]
#[allow(unused_attributes)]
/// Clears the corresponding flag
- pub const fn #clear_ident(&mut self) {
- self.0 &= !(1 << #bit);
+ pub fn #clear_ident(&mut self) {
+ unsafe {
+ let val = core::ptr::read_volatile(
+ self as *const _ as *mut usize
+ );
+ core::ptr::write_volatile(
+ self as *const _ as *mut usize,
+ val & !(1 << #bit)
+ )
+ }
}For rw1_flag:
#[inline]
#[allow(dead_code)]
#[allow(unused_attributes)]
/// Sets the corresponding flag
- pub const fn #set_ident(&mut self) {
- self.0 |= 1 << #bit;
+ pub fn #set_ident(&mut self) {
+ unsafe {
+ core::ptr::write_volatile(
+ self as *const _ as *mut usize,
+ 1 << #bit
+ )
+ }
}Note: For write-1-to-set semantics, you typically write only the bit to set (not read-modify-write), as writing 1 sets and writing 0 has no effect.
Committable suggestion skipped: line range outside the PR's diff.
| // ANCHOR: rw1_flag | ||
| #[proc_macro] | ||
| pub fn rw1_flag(input: TokenStream) -> TokenStream { | ||
| let FlagInput { name, bit, .. } = | ||
| syn::parse_macro_input!(input as FlagInput); | ||
|
|
||
| // build identifiers | ||
| let name_str = name.to_string(); | ||
| let set_ident = format_ident!("set_{}", name_str); | ||
|
|
||
| let expanded = quote! { | ||
| #[inline] | ||
| #[allow(dead_code)] | ||
| #[allow(unused_attributes)] | ||
| /// Sets the corresponding flag | ||
| pub const fn #set_ident(&mut self) { | ||
| self.0 |= 1 << #bit; | ||
| } | ||
| }; | ||
|
|
||
| expanded.into() | ||
| } | ||
| // ANCHOR_END: rw1_flag |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file and examine context around rw1_flag
find . -name "lib.rs" -path "*/learnix-macros/*" | head -5Repository: sagi21805/LearnixOS
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Search for rw1_flag usage and related macros
rg -n "rw[01]_flag|rw1_flag|FlagInput" --type rust -B 2 -A 10 | head -100Repository: sagi21805/LearnixOS
Length of output: 5618
🏁 Script executed:
#!/bin/bash
# Check what the FlagInput struct looks like and understand the macro better
rg -n "struct FlagInput|enum FlagInput" --type rust -A 20 | head -80Repository: sagi21805/LearnixOS
Length of output: 833
🏁 Script executed:
#!/bin/bash
# Find usage examples of rw1_flag in AHCI code
rg -n "rw1_flag\!" --type rust -B 2 -A 2 | head -60Repository: sagi21805/LearnixOS
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Check the hwba.rs file to see context and how flags are used
grep -n "rw1_flag\|rwc_flag\|flag\!" kernel/src/drivers/ata/ahci/hba.rs | head -40Repository: sagi21805/LearnixOS
Length of output: 1178
🏁 Script executed:
#!/bin/bash
# Get more context around the flag macro definitions and compare with rwc_flag
sed -n '171,192p' learnix-macros/src/lib.rs
sed -n '194,216p' learnix-macros/src/lib.rsRepository: sagi21805/LearnixOS
Length of output: 1226
🏁 Script executed:
#!/bin/bash
# Search for actual uses of rw1_flag! macro invocations
rg "rw1_flag!" --type rust -B 1 -A 1Repository: sagi21805/LearnixOS
Length of output: 516
🏁 Script executed:
#!/bin/bash
# Check if rwc_flag has the same implementation issue
# and understand the semantics: both use |= which is read-modify-write
cat learnix-macros/src/lib.rs | sed -n '73,130p'Repository: sagi21805/LearnixOS
Length of output: 1787
rw1_flag macro uses incorrect patterns for W1S hardware register semantics.
W1S (Write-1-to-Set) registers require writing 1 to the target position to set the bit, but the current self.0 |= 1 << #bit; implements read-modify-write. W1S design eliminates the need to read current contents before writing. Additionally, the macro uses const fn and cannot perform volatile operations required for memory-mapped hardware register access. Compare with the flag macro which correctly uses read_volatile and write_volatile. For AHCI hardware registers (reset, tm, dec flags), implement direct volatile write semantics that match W1S hardware specifications.
🤖 Prompt for AI Agents
In learnix-macros/src/lib.rs around lines 194 to 216, the rw1_flag macro
currently generates a const fn that does a read-modify-write (self.0 |= 1 <<
#bit) which is incorrect for W1S registers and cannot perform the required
volatile memory access. Change the generated function to a non-const method that
performs a direct volatile write of the single-bit mask (1 << #bit) to the
register (no read), using core::ptr::write_volatile to the register address (or
the inner pointer) so the macro emits the same W1S semantics as the existing
flag macro (write_volatile with mask only) and remove the const qualifier and
any RMW operations.
better job at initializing it first.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (24)
kernel/src/drivers/ata/ahci/fis.rs (4)
20-20: Typo:_resvered0should be_reserved0.This typo was already flagged in a previous review.
173-175: Fix bit extraction instatus_high()to match specification.The mask logic error for extracting bits 6-4 was already flagged in a previous review.
3-23: Add compile-time size assertion and fix field name typo.Both the missing size assertion and the field name typo were already flagged in a previous review.
197-199: Add accessor methods or make field public.The inaccessible
datafield issue was already flagged in a previous review.kernel/src/main.rs (1)
134-134: Hardcoded port 0 ignores probe results.Line 134 calls
map_device(0)regardless of which ports were detected during probe. This issue was already flagged in a previous review.kernel/src/drivers/pci.rs (3)
264-269: Incorrect bit check for 64-bit BAR detection.The 64-bit BAR detection logic error was already flagged in a previous review.
271-277: Address extraction uses wrong mask for IO BARs.The mask mismatch between memory and IO BARs was already flagged in a previous review.
419-419: Remove debug print statement.The debug println in the multifunction loop was already flagged in a previous review.
kernel/src/drivers/ata/ahci/hba.rs (16)
70-72: Unsafe transmute for enum conversion is risky.This unsafe transmute issue was already flagged in a previous review.
86-87: Bit collision: bothsscandpmduse bit 15.This bit collision issue was already flagged in a previous review.
92-95: Off-by-one innumber_of_commandsmask.The missing +1 adjustment was already flagged in a previous review.
105-109: Same off-by-one issue innumber_of_ports.The missing +1 adjustment was already flagged in a previous review.
282-286: Typo:buffet_sizeshould bebuffer_size.This typo was already flagged in a previous review.
564-572: Logic error:.then()result is discarded.This issue with discarded closure result was already flagged in a previous review.
589-622: Typo in panic messages: "Timout" should be "Timeout".These typos were already flagged in a previous review.
674-677: Unsafeunchecked_transmute_fromcan cause UB.This unsafe conversion issue in
speed()was already flagged in a previous review.
703-708: Unsafeunchecked_transmute_fromcan cause UB.This unsafe conversion issue in
max_speed()was already flagged in a previous review.
762-766: Logic error: setting wrong bits in PM notification.This bit manipulation error was already flagged in a previous review.
769-775: Logic error inget_pm_notifbit masking.This mask inversion error was already flagged in a previous review.
914-920: Incorrect bit manipulation for SACT register.This bit manipulation error in
set_statuswas already flagged in a previous review.
982-987: Inverted assert conditions will always panic for valid inputs.These inverted assertions were already flagged in a previous review.
1031-1035:set_dbconly asserts but never sets the value.This missing implementation was already flagged in a previous review.
1075-1094: Missing type parameter insize_ofcalls.The missing const generic in size_of was already flagged in a previous review.
1163-1172: Missing validation inmap_device.The lack of bounds/implementation checks was already flagged in a previous review.
🧹 Nitpick comments (2)
kernel/src/main.rs (1)
139-139: Missing error handling foridentity_packet.The call to
identity_packetreturnsOption<()>but the result is ignored. If the command fails (e.g., no available command slots), execution continues silently without indication.Consider handling the error case:
🔎 Proposed fix
- controller.identity_packet(&mut b); + if controller.identity_packet(&mut b).is_none() { + println!("Failed to read device identity"); + continue; + }kernel/src/drivers/ata/ahci/hba.rs (1)
1228-1233: Consider adding a method for setting CI bits.The direct volatile manipulation of
port.ci.0works but bypasses any potential validation. Consider adding a helper method likeci.set_slot(slot)to encapsulate the volatile operations and improve maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kernel/src/drivers/ata/ahci/fis.rs(1 hunks)kernel/src/drivers/ata/ahci/hba.rs(1 hunks)kernel/src/drivers/pci.rs(8 hunks)kernel/src/main.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
kernel/src/main.rs (5)
kernel/stages/second_stage/src/main.rs (1)
panic(47-49)kernel/src/drivers/pci.rs (2)
common(390-392)scan_pci(395-438)kernel/src/drivers/ata/ahci/fis.rs (1)
new(26-64)kernel/src/drivers/ata/ahci/hba.rs (2)
new(1109-1128)new(1180-1200)kernel/src/memory/allocators/page_allocator/allocator.rs (1)
new(47-57)
kernel/src/drivers/pci.rs (1)
shared/common/src/enums/pci.rs (4)
fmt(80-103)fmt(347-349)fmt(547-549)fmt(561-732)
kernel/src/drivers/ata/ahci/hba.rs (6)
learnix-macros/src/lib.rs (2)
flag(74-132)ro_flag(144-167)kernel/src/drivers/ata/ahci/fis.rs (1)
new(26-64)kernel/src/main.rs (1)
panic(158-164)shared/cpu_utils/src/structures/paging/page_table_entry.rs (1)
empty(24-26)kernel/src/memory/allocators/page_allocator/allocator.rs (1)
Self(206-208)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)
🔇 Additional comments (1)
kernel/src/drivers/ata/ahci/hba.rs (1)
1202-1253:identity_packetimplementation differs from past comment.The past review flagged this as empty, but the current implementation (lines 1202-1253) contains a full ATA IDENTIFY command sequence. This comment is now outdated.
| table.table[0].dbc.set_i(); | ||
| table.table[0].dbc.set_dbc(511); // 256 words - 1 | ||
| table.table[0].set_buffer(buf); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "hba.rs" -type fRepository: sagi21805/LearnixOS
Length of output: 99
🏁 Script executed:
# Check file size first
wc -l kernel/src/drivers/ata/ahci/hba.rs
# View the set_dbc implementation around lines 1031-1035
sed -n '1025,1040p' kernel/src/drivers/ata/ahci/hba.rs
# View the call site around lines 1214-1216
sed -n '1210,1220p' kernel/src/drivers/ata/ahci/hba.rsRepository: sagi21805/LearnixOS
Length of output: 860
Critical: set_dbc call at line 1215 has no effect and must be fixed.
The set_dbc method only validates input without modifying the DBC field. This causes the data byte count to remain at its default value, preventing the identity command from transferring the correct amount of data. The method must be updated to actually set bits 0-21 of the PRDT entry with the provided byte count.
of virtual (sketchy and need change)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
learnix-macros/src/lib.rs (1)
63-72: Fix documentation: states "three methods" but generates four.The documentation comment says "three methods" but then lists four numbered items and the macro actually generates four methods:
set_$flag_name,$flag_name(builder-style),unset_$flag_name, andis_$flag_name.🔎 Proposed fix
/// This macro will obtain `flag_name` and the corresponding /// `bit_number` /// /// With this information it will automatically generate -/// three methods +/// four methods /// /// 1. `set_$flag_name`: set the bit without returning self /// 2. `$flag_name`: set the bit and will return self /// 3. `unset_$flag_name`: unset the bit without returning self /// 4. `is_$flag_name`: return true if the flag is set or false if not
♻️ Duplicate comments (13)
learnix-macros/src/lib.rs (3)
144-152: Fix documentation comment.The comment states "three methods" but the macro only generates one method (
is_$flag_name).🔎 Proposed fix
/// This macro will obtain `flag_name` and the corresponding /// `bit_number` and create read-only flag functionality /// /// With this information it will automatically generate -/// three methods +/// one method /// /// 1. `is_$flag_name`: return true if the flag is set or false if not
179-201: Critical: Clear operation sets the bit instead of clearing it.The
rwc_flagmacro generates aclear_$flag_namemethod, but line 195 uses|=which sets the bit rather than clearing it. To clear a bit, use the bitwise AND with complement:&= !(1 << #bit).Additionally, the comment on line 193 incorrectly states "Sets the corresponding flag" when it should say "Clears the corresponding flag".
Furthermore, this macro should use volatile operations for MMIO register correctness, similar to the
flagmacro.🔎 Proposed fix
let expanded = quote! { #[inline] #[allow(dead_code)] #[allow(unused_attributes)] - /// Sets the corresponding flag - pub const fn #clear_ident(&mut self) { - self.0 |= 1 << #bit; + /// Clears the corresponding flag + pub fn #clear_ident(&mut self) { + unsafe { + let val = core::ptr::read_volatile( + self as *const _ as *mut usize + ); + core::ptr::write_volatile( + self as *const _ as *mut usize, + val & !(1 << #bit) + ) + } } };
203-225:rw1_flagmacro uses incorrect patterns for W1S hardware register semantics.W1S (Write-1-to-Set) registers require writing 1 to the target position to set the bit, but the current
self.0 |= 1 << #bit;implements read-modify-write. W1S design eliminates the need to read current contents before writing. Additionally, the macro usesconst fnand cannot perform volatile operations required for memory-mapped hardware register access. Compare with theflagmacro which correctly usesread_volatileandwrite_volatile. For AHCI hardware registers, implement direct volatile write semantics that match W1S hardware specifications.🔎 Proposed fix
let expanded = quote! { #[inline] #[allow(dead_code)] #[allow(unused_attributes)] /// Sets the corresponding flag - pub const fn #set_ident(&mut self) { - self.0 |= 1 << #bit; + pub fn #set_ident(&mut self) { + unsafe { + core::ptr::write_volatile( + self as *const _ as *mut usize, + 1 << #bit + ) + } } };Note: For write-1-to-set semantics, you typically write only the bit to set (not read-modify-write), as writing 1 sets and writing 0 has no effect.
kernel/src/drivers/pci.rs (3)
433-433: Duplicate: Debug print statement.This issue was already flagged in a previous review. The
println!will output function numbers during PCI enumeration, potentially spamming the console. Consider removing it or replacing with a conditional debug log.
267-272: Duplicate: Incorrect 64-bit BAR detection.This issue was already flagged in a previous review. The current check tests a single bit and can misclassify reserved values. The 2-bit BAR type field (bits 2:1) should be extracted and compared to the 64-bit pattern (0b10).
274-280: Duplicate: Wrong mask for address extraction.This issue was already flagged in a previous review. The method applies the memory BAR mask
0xfffffff0toself.io.0, but IO BARs require mask0xfffffffc(bits 1:0 are reserved). The method should distinguish between IO BARs and 32-bit memory BARs.kernel/src/drivers/ata/ahci/hba.rs (7)
88-89: Duplicate: Bit collision on bit 15.This issue was already flagged in a previous review. Both
pmdandsscare defined on bit 15, but per AHCI spec, SSC should be on bit 14.
984-989: Duplicate: Inverted assert conditions.This issue was already flagged in a previous review. The assertions will panic for valid lengths between 2 and 16, which is the opposite of the intended behavior.
1033-1038: Duplicate:set_dbconly asserts without setting the value.This issue was already flagged in a previous review and marked as addressed in commit 4c69fdf, but the implementation still only validates the input without modifying
self.0. The DBC field (bits 21:0) is never written.
764-768: Duplicate: Wrong bit manipulation inset_pm_notif.This issue was already flagged in a previous review. The method sets
self.0 |= pm_port as u32which sets multiple bits based on the pm_port value, instead of setting bitpm_portusing1 << pm_port.
771-777: Duplicate: Inverted mask inget_pm_notif.This issue was already flagged in a previous review. The mask
!0xffffclears the lower 16 bits where PM notification bits reside, instead of preserving them with0xffff.
916-922: Duplicate: Incorrect bit manipulation for SACT register.This issue was already flagged in a previous review and marked as addressed in commit 4c69fdf, but the implementation still uses the incorrect shift/mask logic. The SACT register uses individual bits (0-31) for command slots, not a shifted/masked value. The parameter should also be named
slotinstead ofport.
566-574: Duplicate: Discarded.then()result may not execute bit manipulation.This issue was already flagged in a previous review. The
.then()closure returns anOptionthat is immediately discarded, which may prevent the bit manipulation from executing as intended. Use an explicitifstatement instead.
🧹 Nitpick comments (1)
kernel/src/memory/allocators/page_allocator/extensions.rs (1)
1-1: Remove unused imports.The imports
crate::panic(line 1) andstrum::IntoEnumIterator(line 17) are not used in this file. Thepanic!invocation at line 112 uses the built-in Rust macro, not the imported crate module. TheIntoEnumIteratortrait is unused; the code usesPageTableLevel::VARIANTSfrom theVariantArraytrait instead. Remove both imports to keep the code clean.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
kernel/src/drivers/ata/ahci/hba.rskernel/src/drivers/keyboard/mod.rskernel/src/drivers/pci.rskernel/src/drivers/pic8259.rskernel/src/drivers/timer.rskernel/src/memory/allocators/page_allocator/extensions.rslearnix-macros/src/lib.rsshared/cpu_utils/src/structures/paging/init.rs
🧰 Additional context used
🧬 Code graph analysis (3)
shared/cpu_utils/src/structures/paging/init.rs (1)
shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
huge_io_page_flags(85-92)
kernel/src/memory/allocators/page_allocator/extensions.rs (4)
shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
table_flags(63-65)shared/cpu_utils/src/structures/paging/page_table.rs (2)
address(78-82)current_table_mut(66-72)shared/cpu_utils/src/structures/paging/page_table_entry.rs (1)
set_flags(52-55)shared/common/src/enums/paging.rs (1)
from_alignment(97-104)
kernel/src/drivers/ata/ahci/hba.rs (3)
shared/cpu_utils/src/structures/paging/page_table_entry.rs (1)
empty(24-26)kernel/src/memory/allocators/page_allocator/allocator.rs (1)
Self(206-208)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)
🔇 Additional comments (7)
learnix-macros/src/lib.rs (1)
89-137: LGTM! Volatile operations correctly implemented.The
set_ident,unset_ident, andis_identmethods now correctly use volatile operations (read_volatileandwrite_volatile) which prevents compiler optimizations that could break MMIO register semantics.kernel/src/drivers/keyboard/mod.rs (1)
18-49: LGTM with verification note.The simplified EOI signaling is consistent with the updated PIC pattern. As with the timer handler, ensure that
PIC::init()is called before this interrupt handler can execute (verified via the script in timer.rs review).kernel/src/drivers/pic8259.rs (2)
9-9: Cleaner static initialization pattern.Removing the
MaybeUninitwrapper simplifies the code sinceCascadedPIC::default()is const and fully initializes all fields at compile time. This is a valid refactor that maintains correctness while improving readability.
81-119: Correctly refactored initialization method.The signature and implementation changes properly align with the removal of
MaybeUninit. Since the staticPICis now directly initialized viadefault(), theinitmethod can operate directly on&'static mut Selfwithout requiringassume_init_mut(). All hardware configuration I/O operations are correctly updated.kernel/src/drivers/timer.rs (1)
10-12: The code is correct.CascadedPIC::init(&mut PIC)is properly called at line 90 in kernel startup beforeinterrupts::enable()at line 99, establishing the required initialization sequence. The removal ofMaybeUninit::assume_init_mut()does not introduce a safety issue in this context.Likely an incorrect or invalid review comment.
kernel/src/memory/allocators/page_allocator/extensions.rs (2)
164-165: Consistent with map, but inherits implicit panic behavior.These
.unwrap()calls follow the same pattern as in themapfunction (line 102), but they will panic implicitly ifforce_resolve_table_mutreturnsNone(whenNotATableis encountered).During physical memory mapping at kernel initialization, such a panic would indicate serious page table corruption or misconfiguration. Consider the same remediation suggested for
force_resolve_table_mut(lines 46-70) to make the panic explicit with a clear diagnostic message.Also applies to: 170-171
96-109: The array bounds are safe, but reconsider the.unwrap()safety concern.The indexing
PageTableLevel::VARIANTS[page_size as usize + 1]is safe: withPageSize::Huge=0,Big=1,Regular=2, the maximum index accessed is2 + 1 = 3, which is valid for a 4-element VARIANTS array[PML4, PDPT, PD, PT]. The iteration range[0..=page_size as usize]correctly maps to the appropriate page table levels.However, the
.unwrap()on line 102 is a genuine concern:force_resolve_table_mut()returnsNonewhen encounteringEntryError::NotATable. If any intermediate page table entry is not actually a table (e.g., a huge page entry), the unwrap will panic. Consider handling this case explicitly rather than panicking, or ensure the caller guarantees intermediate entries are always valid tables.
| fn force_resolve_table_mut(&mut self) -> Option<&mut PageTable> { | ||
| match self.mapped_table_mut() { | ||
| Ok(table) => Some(table), | ||
| Err(EntryError::NotATable) => None, | ||
| Err(EntryError::NoMapping) => { | ||
| let resolved_table = | ||
| unsafe { ALLOCATOR.assume_init_ref().alloc_table() }; | ||
| unsafe { | ||
| self.map_unchecked( | ||
| PhysicalAddress::new_unchecked( | ||
| resolved_table.address().as_usize(), | ||
| ), | ||
| PageEntryFlags::table_flags(), | ||
| ); | ||
| } | ||
| unsafe { | ||
| Some( | ||
| &mut *self | ||
| .mapped_unchecked() | ||
| .as_mut_ptr::<PageTable>(), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Address misleading documentation and implicit panic on NotATable.
The function documentation (lines 40-45) states "valid table is guaranteed to be returned," but the implementation now returns None for the NotATable case (line 49). This occurs when an entry is already mapped to something other than a table (e.g., a huge page at an intermediate level). All callers use .unwrap(), causing an implicit panic on None.
Consider:
- Update the documentation to reflect that
Nonecan be returned and explain when this occurs. - Make the panic explicit with a descriptive error message if
NotATableis truly unrecoverable, or - Propagate the error to callers so they can handle it appropriately.
The implicit panic on unwrap obscures the root cause when debugging page table conflicts.
🔎 Suggested fix: Make panic explicit with clear error message
fn force_resolve_table_mut(&mut self) -> Option<&mut PageTable> {
match self.mapped_table_mut() {
Ok(table) => Some(table),
- Err(EntryError::NotATable) => None,
+ Err(EntryError::NotATable) => {
+ panic!(
+ "Page table entry conflict: entry is mapped to a page, not a table. \
+ This likely indicates a huge page is already mapped at this level."
+ )
+ }
Err(EntryError::NoMapping) => {And update the documentation:
- /// This function will return a table mapped in this
- /// entry if there is one.
- ///
- /// Else, it will override what is inside the entry and
- /// map a new table to it so valid table is guaranteed
- /// to be returned.
+ /// This function will return a table mapped in this entry if there is one.
+ ///
+ /// If the entry is not mapped, it will allocate and map a new table.
+ ///
+ /// # Panics
+ /// Panics if the entry is already mapped to a page (not a table), indicating
+ /// a conflict with an existing huge page mapping.
fn force_resolve_table_mut(&mut self) -> Option<&mut PageTable> {📝 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.
| fn force_resolve_table_mut(&mut self) -> Option<&mut PageTable> { | |
| match self.mapped_table_mut() { | |
| Ok(table) => Some(table), | |
| Err(EntryError::NotATable) => None, | |
| Err(EntryError::NoMapping) => { | |
| let resolved_table = | |
| unsafe { ALLOCATOR.assume_init_ref().alloc_table() }; | |
| unsafe { | |
| self.map_unchecked( | |
| PhysicalAddress::new_unchecked( | |
| resolved_table.address().as_usize(), | |
| ), | |
| PageEntryFlags::table_flags(), | |
| ); | |
| } | |
| unsafe { | |
| Some( | |
| &mut *self | |
| .mapped_unchecked() | |
| .as_mut_ptr::<PageTable>(), | |
| ) | |
| } | |
| } | |
| } | |
| } | |
| fn force_resolve_table_mut(&mut self) -> Option<&mut PageTable> { | |
| match self.mapped_table_mut() { | |
| Ok(table) => Some(table), | |
| Err(EntryError::NotATable) => { | |
| panic!( | |
| "Page table entry conflict: entry is mapped to a page, not a table. \ | |
| This likely indicates a huge page is already mapped at this level." | |
| ) | |
| } | |
| Err(EntryError::NoMapping) => { | |
| let resolved_table = | |
| unsafe { ALLOCATOR.assume_init_ref().alloc_table() }; | |
| unsafe { | |
| self.map_unchecked( | |
| PhysicalAddress::new_unchecked( | |
| resolved_table.address().as_usize(), | |
| ), | |
| PageEntryFlags::table_flags(), | |
| ); | |
| } | |
| unsafe { | |
| Some( | |
| &mut *self | |
| .mapped_unchecked() | |
| .as_mut_ptr::<PageTable>(), | |
| ) | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
kernel/src/memory/allocators/page_allocator/extensions.rs around lines 46-70:
the current doc claims a valid table is guaranteed but the code returns None for
EntryError::NotATable and callers unwrap(), causing implicit panics; change the
function signature to return Result<&mut PageTable, EntryError> (update the doc
comment to remove the incorrect guarantee and document when NotATable is
returned), in the NotATable branch return Err(EntryError::NotATable), keep the
NoMapping branch allocating the table and return Ok(&mut PageTable) after
mapping, and then update all callers to handle the Result (propagate the error
or panic explicitly with a descriptive message) instead of calling unwrap().
| fn set_flags(&self, flags: PageEntryFlags) -> Result<(), EntryError> { | ||
| let page_size = PageSize::from_alignment(self.alignment()) | ||
| .expect("self address is not aligned to a page size"); | ||
|
|
||
| let mut table = PageTable::current_table_mut(); | ||
|
|
||
| for level in PageTableLevel::VARIANTS[0..page_size as usize].iter() | ||
| { | ||
| let index = self.index_of(*level); | ||
| let entry = &mut table.entries[index]; | ||
| table = entry.mapped_table_mut()?; | ||
| } | ||
| table.entries[self | ||
| .index_of(PageTableLevel::VARIANTS[page_size as usize + 1])] | ||
| .set_flags(flags); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Critical logic error in set_flags traversal.
The iteration range in set_flags differs from map and appears incorrect:
- Line 125 (
set_flags):PageTableLevel::VARIANTS[0..page_size as usize](exclusive end) - Line 97 (
map):PageTableLevel::VARIANTS[0..=page_size as usize](inclusive end)
For a regular page (page_size as usize == 0), set_flags iterates over an empty range [0..0), meaning it doesn't traverse any table levels before accessing the final entry. This would operate on the wrong page table level, leading to incorrect flag modifications or panics.
🔎 Proposed fix: Use inclusive range like map does
- for level in PageTableLevel::VARIANTS[0..page_size as usize].iter()
+ for level in PageTableLevel::VARIANTS[0..=page_size as usize].iter()
{
let index = self.index_of(*level);
let entry = &mut table.entries[index];
table = entry.mapped_table_mut()?;
}📝 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.
| fn set_flags(&self, flags: PageEntryFlags) -> Result<(), EntryError> { | |
| let page_size = PageSize::from_alignment(self.alignment()) | |
| .expect("self address is not aligned to a page size"); | |
| let mut table = PageTable::current_table_mut(); | |
| for level in PageTableLevel::VARIANTS[0..page_size as usize].iter() | |
| { | |
| let index = self.index_of(*level); | |
| let entry = &mut table.entries[index]; | |
| table = entry.mapped_table_mut()?; | |
| } | |
| table.entries[self | |
| .index_of(PageTableLevel::VARIANTS[page_size as usize + 1])] | |
| .set_flags(flags); | |
| Ok(()) | |
| } | |
| fn set_flags(&self, flags: PageEntryFlags) -> Result<(), EntryError> { | |
| let page_size = PageSize::from_alignment(self.alignment()) | |
| .expect("self address is not aligned to a page size"); | |
| let mut table = PageTable::current_table_mut(); | |
| for level in PageTableLevel::VARIANTS[0..=page_size as usize].iter() | |
| { | |
| let index = self.index_of(*level); | |
| let entry = &mut table.entries[index]; | |
| table = entry.mapped_table_mut()?; | |
| } | |
| table.entries[self | |
| .index_of(PageTableLevel::VARIANTS[page_size as usize + 1])] | |
| .set_flags(flags); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In kernel/src/memory/allocators/page_allocator/extensions.rs around lines 119 to
135, the traversal in set_flags uses an exclusive range
PageTableLevel::VARIANTS[0..page_size as usize] which is wrong for regular pages
(it yields an empty iterator); change the iteration to the inclusive range
PageTableLevel::VARIANTS[0..=page_size as usize] to match map()’s traversal so
the code walks the same table levels before accessing the final entry; keep the
final index calculation (page_size as usize + 1) unchanged.
| identity_page_table_l2.entries[0].map_unchecked( | ||
| PhysicalAddress::new_unchecked(0), | ||
| PageEntryFlags::huge_page_flags(), | ||
| /// TODO PATCH REMOVE LATER | ||
| PageEntryFlags::huge_io_page_flags(), | ||
| ); |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
# Find and examine the paging init file
find . -name "init.rs" -path "*/paging/*" | head -5
# Look for PageEntryFlags definitions
fd -t f -e rs | xargs rg -l "PageEntryFlags" | head -5
# Look for huge_page_flags and huge_io_page_flags definitions
rg -B5 -A10 "fn huge_(io_)?page_flags" --type rustRepository: sagi21805/LearnixOS
Repository: sagi21805/LearnixOS
Exit code: 0
stdout:
./shared/cpu_utils/src/structures/paging/init.rs
shared/cpu_utils/src/structures/paging/page_table_entry.rs
shared/cpu_utils/src/structures/paging/init.rs
shared/cpu_utils/src/structures/paging/entry_flags.rs
kernel/src/main.rs
kernel/src/memory/allocators/page_allocator/extensions.rs
shared/cpu_utils/src/structures/paging/entry_flags.rs- pub const fn table_flags() -> Self {
shared/cpu_utils/src/structures/paging/entry_flags.rs- PageEntryFlags::default().present().writable().table()
shared/cpu_utils/src/structures/paging/entry_flags.rs- }
shared/cpu_utils/src/structures/paging/entry_flags.rs-
shared/cpu_utils/src/structures/paging/entry_flags.rs- /// Default flags for entry that contains huge page.
shared/cpu_utils/src/structures/paging/entry_flags.rs: pub const fn huge_page_flags() -> Self {
shared/cpu_utils/src/structures/paging/entry_flags.rs- PageEntryFlags::default().present().writable().huge_page()
shared/cpu_utils/src/structures/paging/entry_flags.rs- }
shared/cpu_utils/src/structures/paging/entry_flags.rs-
shared/cpu_utils/src/structures/paging/entry_flags.rs- /// Default flags for entry that contains regular page.
shared/cpu_utils/src/structures/paging/entry_flags.rs- pub const fn regular_page_flags() -> Self {
shared/cpu_utils/src/structures/paging/entry_flags.rs- PageEntryFlags::default().present().writable()
shared/cpu_utils/src/structures/paging/entry_flags.rs- }
shared/cpu_utils/src/structures/paging/entry_flags.rs-
shared/cpu_utils/src/structures/paging/entry_flags.rs- pub const fn regular_io_page_flags() -> Self {
shared/cpu_utils/src/structures/paging/entry_flags.rs- PageEntryFlags::default()
--
shared/cpu_utils/src/structures/paging/entry_flags.rs- .writable()
shared/cpu_utils/src/structures/paging/entry_flags.rs- .disable_cache()
shared/cpu_utils/src/structures/paging/entry_flags.rs- .global()
shared/cpu_utils/src/structures/paging/entry_flags.rs- }
shared/cpu_utils/src/structures/paging/entry_flags.rs-
shared/cpu_utils/src/structures/paging/entry_flags.rs: pub const fn huge_io_page_flags() -> Self {
shared/cpu_utils/src/structures/paging/entry_flags.rs- PageEntryFlags::default()
shared/cpu_utils/src/structures/paging/entry_flags.rs- .present()
shared/cpu_utils/src/structures/paging/entry_flags.rs- .writable()
shared/cpu_utils/src/structures/paging/entry_flags.rs- .huge_page()
shared/cpu_utils/src/structures/paging/entry_flags.rs- .disable_cache()
shared/cpu_utils/src/structures/paging/entry_flags.rs- .global()
shared/cpu_utils/src/structures/paging/entry_flags.rs- }
shared/cpu_utils/src/structures/paging/entry_flags.rs-}
shared/cpu_utils/src/structures/paging/entry_flags.rs-// ANCHOR_END: impl_page_entry_flags
Script executed:
#!/bin/bash
# Get context around the identity mapping code in init.rs
sed -n '50,90p' shared/cpu_utils/src/structures/paging/init.rs
# Look for AHCI references in the codebase
rg -i "ahci" --type rust | head -20
# Check git history or comments about why this change was made
rg -B3 -A3 "huge_io_page_flags" shared/cpu_utils/src/structures/paging/init.rsRepository: sagi21805/LearnixOS
Repository: sagi21805/LearnixOS
Exit code: 0
stdout:
);
identity_page_table_l3.entries[0].map_unchecked(
PhysicalAddress::new_unchecked(IDENTITY_PAGE_TABLE_L2_OFFSET),
PageEntryFlags::table_flags(),
);
identity_page_table_l2.entries[0].map_unchecked(
PhysicalAddress::new_unchecked(0),
/// TODO PATCH REMOVE LATER
PageEntryFlags::huge_io_page_flags(),
);
}
// ANCHOR_END: setup_page_tables
// ANCHOR: setup_top_page_tables
unsafe {
// Setup kernel identity paging Mapping at the top half
// of the address space
// The kernel is mapped from 0xffff800000000000 to
// 0xffff8000001fffff to the physical addresses of
// 0x000000-0x1fffff
// This mapping will allow the kernel to access physical addresses
// without any dependency on the current mapping
identity_page_table_l4.entries[256].map_unchecked(
PhysicalAddress::new_unchecked(
TOP_IDENTITY_PAGE_TABLE_L3_OFFSET,
),
PageEntryFlags::table_flags(),
);
top_identity_page_table_l3.entries[0].map_unchecked(
PhysicalAddress::new_unchecked(
TOP_IDENTITY_PAGE_TABLE_L2_OFFSET,
),
PageEntryFlags::table_flags(),
);
top_identity_page_table_l2.entries[0].map_unchecked(
PhysicalAddress::new_unchecked(0),
PageEntryFlags::huge_io_page_flags(),
);
}
// ANCHOR_END: setup_top_page_tables
// ANCHOR: set_cr3
unsafe {
shared/common/src/enums/pci.rs: SataControllerAHCI = 0x2822,
shared/common/src/enums/pci.rs: AHCI1 = 0x01,
shared/common/src/enums/pci.rs: pub fn is_ahci(&self) -> bool {
shared/common/src/enums/pci.rs: && self.prog_if.sata == SATAControllerPI::AHCI1
shared/common/src/error/mod.rs:pub mod ahci;
shared/common/src/error/mod.rs:pub use ahci::*;
shared/common/src/error/ahci.rs: #[error("Disparity Error (Not use by AHCI)")]
shared/common/src/error/ahci.rs:pub enum AhciError {
shared/common/src/enums/mod.rs:pub mod ahci;
shared/common/src/enums/mod.rs:pub use ahci::*;
shared/common/src/enums/ahci.rs:// ANCHOR: AHCIInterfaceSpeed
shared/common/src/enums/ahci.rs:// ANCHOR_END: AHCIInterfaceSpeed
kernel/src/main.rs: ata::ahci::{
kernel/src/main.rs: AhciDeviceController, GenericHostControl, HBAMemoryRegisters,
kernel/src/main.rs: if device.header.common().device_type.is_ahci() {
kernel/src/drivers/pci.rs: drivers::ata::ahci::AHCIBaseAddress,
kernel/src/drivers/pci.rs: pub abar: AHCIBaseAddress,
kernel/src/drivers/ata/mod.rs:pub mod ahci;
kernel/src/drivers/ata/ahci/hba.rs:/// AHCI implementation for the learnix operating system
kernel/src/drivers/ata/ahci/hba.rs:/// Implemented directly from https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/serial-ata-ahci-spec-rev1-3-1.pdf
identity_page_table_l2.entries[0].map_unchecked(
PhysicalAddress::new_unchecked(0),
/// TODO PATCH REMOVE LATER
PageEntryFlags::huge_io_page_flags(),
);
}
// ANCHOR_END: setup_page_tables
--
);
top_identity_page_table_l2.entries[0].map_unchecked(
PhysicalAddress::new_unchecked(0),
PageEntryFlags::huge_io_page_flags(),
);
}
// ANCHOR_END: setup_top_page_tables
Reconsider the broad cache-disable application and clarify the TODO.
The use of huge_io_page_flags() adds cache-disable and is partially justified: the low 2MB region (0x0-0x200000) contains VGA video memory at 0xA0000-0xBFFFF, which requires uncached access for correct MMIO semantics. However, this region also contains bootloader code and normal RAM, making a blanket cache-disable for the entire 2MB overly broad.
Specific concerns:
-
TODO requires explanation: Line 57 indicates this is a temporary patch to be removed later. Please clarify what conditions must be met before removal and whether shipping temporary code is acceptable.
-
Cache-disable scope: Instead of disabling cache for the entire 2MB identity mapping, consider whether cache-disable should be applied only to specific MMIO regions (e.g., VGA memory at 0xA0000-0xBFFFF) while keeping normal boot RAM cached.
-
Minor: Incorrect comment style: Line 57 uses
///(doc comment) for a TODO; use//(regular comment) instead.
🤖 Prompt for AI Agents
In shared/cpu_utils/src/structures/paging/init.rs around lines 55-59, the
current mapping uses huge_io_page_flags() (with cache-disable) for the entire
2MiB identity-mapped L2 entry and leaves a doc-TODO; change this so only the
actual MMIO subrange (e.g., 0xA0000..=0xBFFFF) is mapped with
cache-disabled/MMIO flags while the rest of the 2MiB uses normal cached page
flags: split the L2 mapping logic to map the L2 entry with standard
PageEntryFlags for normal RAM and create a separate mapping/entry (or sub-page
mappings as supported by your paging APIs) that applies huge_io_page_flags()
only for the VGA/MMIO addresses; replace the doc comment '/// TODO' with a
regular '// TODO' and expand the TODO to state the exact condition for removal
(e.g., when MMIO regions are mapped separately by platform init or when a
per-region page-table mapper is implemented) and whether shipping it temporarily
is acceptable, ensuring the log/comment documents the required follow-up and
acceptance criteria.
| top_identity_page_table_l2.entries[0].map_unchecked( | ||
| PhysicalAddress::new_unchecked(0), | ||
| PageEntryFlags::huge_page_flags(), | ||
| PageEntryFlags::huge_io_page_flags(), | ||
| ); |
There was a problem hiding this comment.
Question the cache-disable for top-half identity mapping.
Similar to the identity mapping at line 58, this change disables caching for the 2MB physical region mapped to the top half of the address space (0xffff800000000000-0xffff8000001fffff → 0x0-0x1fffff).
The same performance concerns apply: disabling cache for normal RAM within this region will degrade performance. If only specific MMIO regions require cache-disabled access, consider mapping those separately rather than disabling cache for the entire 2MB range.
🤖 Prompt for AI Agents
In shared/cpu_utils/src/structures/paging/init.rs around lines 83 to 86, the
top-half 2MB identity mapping is being created with cache-disabled IO flags
which will unnecessarily degrade performance for normal RAM; change this mapping
to use normal huge-page (cached) flags for RAM and only apply
PageEntryFlags::huge_io_page_flags() to the specific physical MMIO subranges
that actually require uncached access (create separate entries or a separate
page table for those addresses), and add a short comment documenting why each
mapping uses cached vs IO flags and validate the exact address ranges used for
MMIO before applying IO flags.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
kernel/src/main.rs (2)
142-143: BAR5 offset within page is lost after alignment.The original BAR5 address is aligned down to a page boundary, but then
HBAMemoryRegisters::newaccesses registers at the aligned address rather than at the original BAR5 offset within the mapped page. If BAR5 is not already page-aligned, AHCI registers will be accessed at the wrong location.You need to preserve the intra-page offset and add it back after mapping:
let offset = a.as_usize() - aligned.as_usize(); // After mapping, access at: mapped_base + offset
144-145: Probe result discarded; port 0 hardcoded regardless of device location.The
probe()call returns information about active ports, but the result is discarded. Thenmap_device(0)unconditionally selects port 0. If the SATA device is connected to a different port, this will fail.Use the probe results to find the first active port:
let active_ports = hba.probe(); if active_ports > 0 { // Find first implemented port with an active device let port_idx = (0..30) .find(|&i| /* check if port i is implemented and has active device */) .expect("No active port found"); let controller = hba.map_device::<13>(port_idx); // ... }
🧹 Nitpick comments (2)
kernel/src/main.rs (2)
143-143: Consider adding error context to the unwrap.Using
.unwrap()in kernel initialization code will panic without context ifHBAMemoryRegisters::newfails. Consider using.expect("descriptive message")to provide useful debugging information, or handle the error gracefully:let hba = HBAMemoryRegisters::new(aligned) .expect("Failed to initialize HBA memory registers");
116-118: Remove commented-out debug code.These commented-out print statements appear to be debug remnants. Consider removing them to keep the code clean, or convert them to a conditional debug logging mechanism if they may be needed again.
Also applies to: 164-164
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.zed/settings.jsonkernel/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/src/main.rs (3)
kernel/src/memory/memory_map.rs (1)
parse_map(178-245)kernel/src/drivers/ata/ahci/hba.rs (3)
header(1228-1228)new(1113-1138)new(1190-1215)shared/cpu_utils/src/structures/interrupt_descriptor_table.rs (1)
new(201-220)
🪛 Biome (2.1.2)
.zed/settings.json
[error] 9-9: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 13-13: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 21-21: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 25-25: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 29-29: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 35-35: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 43-43: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 51-51: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 54-54: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 55-55: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 59-59: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 60-60: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 61-61: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 62-62: Expected a property but instead found '}'.
Expected a property here.
(parse)
🔇 Additional comments (5)
.zed/settings.json (1)
6-43: Verify whether personal editor preferences should be committed.This file contains personal UI preferences (vim_mode, font sizes, themes, minimap settings) that may not be relevant to other contributors. Consider whether these settings are intended as team-wide standards or were accidentally committed.
The rust-analyzer LSP configuration (lines 47-60) might be valuable for the team, but the UI preferences are typically user-specific.
kernel/src/main.rs (4)
23-68: Import changes look appropriate for AHCI integration.The new imports for AHCI types (
AhciDeviceController,GenericHostControl,HBAMemoryRegisters,IdentityPacketData), memory allocator extensions, and paging structures align with the driver integration work.
106-113: Busy-wait polling loop for keyboard input.This busy-wait pattern works but consumes CPU cycles continuously. For early kernel development this is acceptable, but consider transitioning to interrupt-driven input handling when the keyboard interrupt handler is more mature.
156-162: Volatile read pattern for DMA buffer is reasonable but consider memory barriers.The
read_volatileprevents the compiler from optimizing away reads from a DMA-written buffer. However, for DMA operations, you may also need explicit memory barriers/fences to ensure the CPU sees the DMA-written data, not just volatile semantics. Verify thatidentity_packetincludes appropriate barriers after the DMA completes.
167-171: Main loop keyboard handling looks correct.The final loop reads characters from the keyboard and prints them in green. The unsafe block is necessary for accessing the static
KEYBOARD, andassume_init_mutis safe here since initialization occurred earlier in_start().
| "minimap": { | ||
| "display_in": "active_editor", | ||
| "show": "always", | ||
| }, | ||
| "inlay_hints": { | ||
| "show_background": true, | ||
| "enabled": true, | ||
| }, | ||
| "relative_line_numbers": "enabled", | ||
| "show_edit_predictions": false, | ||
| "terminal": {}, | ||
| "lsp_document_colors": "inlay", | ||
| "indent_guides": { | ||
| "coloring": "fixed", | ||
| "background_coloring": "disabled", | ||
| }, | ||
| "colorize_brackets": true, | ||
| "features": { | ||
| "edit_prediction_provider": "zed", | ||
| }, | ||
| "telemetry": { | ||
| "diagnostics": true, | ||
| "metrics": true, | ||
| }, | ||
| "vim_mode": true, | ||
| "icon_theme": { | ||
| "mode": "dark", | ||
| "light": "Zed (Default)", | ||
| "dark": "Material Icon Theme", | ||
| }, | ||
| "base_keymap": "VSCode", | ||
| "ui_font_size": 16, | ||
| "buffer_font_size": 15, | ||
| "theme": { | ||
| "mode": "dark", | ||
| "light": "One Dark", | ||
| "dark": "One Dark Pro", | ||
| }, | ||
| "lsp": { | ||
| "rust-analyzer": { | ||
| "initialization_options": { | ||
| "cargo": { | ||
| "target": "x86_64-unknown-linux-none" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| "inlayHints": { | ||
| "maxLength": null, | ||
| "lifetimeElisionHints": { | ||
| "useParameterNames": true, | ||
| }, | ||
| "closureReturnTypeHints": { | ||
| "enable": "always", | ||
| }, | ||
| }, | ||
| "check": { | ||
| "command": "clippy", | ||
| "extraArgs": ["--release"], | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Remove trailing commas to ensure strict JSON compliance.
The static analysis tool flagged trailing commas before closing braces on lines 9, 13, 21, 25, 29, 35, 43, 51, 54, and 59. While Zed may support JSONC (which allows trailing commas), removing them ensures compatibility with strict JSON parsers.
🔎 Proposed fix
"minimap": {
"display_in": "active_editor",
- "show": "always",
+ "show": "always"
},
"inlay_hints": {
"show_background": true,
- "enabled": true,
+ "enabled": true
},
"indent_guides": {
"coloring": "fixed",
- "background_coloring": "disabled",
+ "background_coloring": "disabled"
},
"features": {
- "edit_prediction_provider": "zed",
+ "edit_prediction_provider": "zed"
},
"telemetry": {
"diagnostics": true,
- "metrics": true,
+ "metrics": true
},
"icon_theme": {
"mode": "dark",
"light": "Zed (Default)",
- "dark": "Material Icon Theme",
+ "dark": "Material Icon Theme"
},
"theme": {
"mode": "dark",
"light": "One Dark",
- "dark": "One Dark Pro",
+ "dark": "One Dark Pro"
},
"inlayHints": {
"maxLength": null,
"lifetimeElisionHints": {
- "useParameterNames": true,
+ "useParameterNames": true
},
"closureReturnTypeHints": {
- "enable": "always",
+ "enable": "always"
},
},
"check": {
"command": "clippy",
- "extraArgs": ["--release"],
+ "extraArgs": ["--release"]
},📝 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.
| "minimap": { | |
| "display_in": "active_editor", | |
| "show": "always", | |
| }, | |
| "inlay_hints": { | |
| "show_background": true, | |
| "enabled": true, | |
| }, | |
| "relative_line_numbers": "enabled", | |
| "show_edit_predictions": false, | |
| "terminal": {}, | |
| "lsp_document_colors": "inlay", | |
| "indent_guides": { | |
| "coloring": "fixed", | |
| "background_coloring": "disabled", | |
| }, | |
| "colorize_brackets": true, | |
| "features": { | |
| "edit_prediction_provider": "zed", | |
| }, | |
| "telemetry": { | |
| "diagnostics": true, | |
| "metrics": true, | |
| }, | |
| "vim_mode": true, | |
| "icon_theme": { | |
| "mode": "dark", | |
| "light": "Zed (Default)", | |
| "dark": "Material Icon Theme", | |
| }, | |
| "base_keymap": "VSCode", | |
| "ui_font_size": 16, | |
| "buffer_font_size": 15, | |
| "theme": { | |
| "mode": "dark", | |
| "light": "One Dark", | |
| "dark": "One Dark Pro", | |
| }, | |
| "lsp": { | |
| "rust-analyzer": { | |
| "initialization_options": { | |
| "cargo": { | |
| "target": "x86_64-unknown-linux-none" | |
| } | |
| } | |
| } | |
| } | |
| "inlayHints": { | |
| "maxLength": null, | |
| "lifetimeElisionHints": { | |
| "useParameterNames": true, | |
| }, | |
| "closureReturnTypeHints": { | |
| "enable": "always", | |
| }, | |
| }, | |
| "check": { | |
| "command": "clippy", | |
| "extraArgs": ["--release"], | |
| }, | |
| }, | |
| }, | |
| }, | |
| "minimap": { | |
| "display_in": "active_editor", | |
| "show": "always" | |
| }, | |
| "inlay_hints": { | |
| "show_background": true, | |
| "enabled": true | |
| }, | |
| "relative_line_numbers": "enabled", | |
| "show_edit_predictions": false, | |
| "terminal": {}, | |
| "lsp_document_colors": "inlay", | |
| "indent_guides": { | |
| "coloring": "fixed", | |
| "background_coloring": "disabled" | |
| }, | |
| "colorize_brackets": true, | |
| "features": { | |
| "edit_prediction_provider": "zed" | |
| }, | |
| "telemetry": { | |
| "diagnostics": true, | |
| "metrics": true | |
| }, | |
| "vim_mode": true, | |
| "icon_theme": { | |
| "mode": "dark", | |
| "light": "Zed (Default)", | |
| "dark": "Material Icon Theme" | |
| }, | |
| "base_keymap": "VSCode", | |
| "ui_font_size": 16, | |
| "buffer_font_size": 15, | |
| "theme": { | |
| "mode": "dark", | |
| "light": "One Dark", | |
| "dark": "One Dark Pro" | |
| }, | |
| "lsp": { | |
| "rust-analyzer": { | |
| "initialization_options": { | |
| "inlayHints": { | |
| "maxLength": null, | |
| "lifetimeElisionHints": { | |
| "useParameterNames": true | |
| }, | |
| "closureReturnTypeHints": { | |
| "enable": "always" | |
| } | |
| }, | |
| "check": { | |
| "command": "clippy", | |
| "extraArgs": ["--release"] | |
| } | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 9-9: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 13-13: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 21-21: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 25-25: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 29-29: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 35-35: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 43-43: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 51-51: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 54-54: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 55-55: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 59-59: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 60-60: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 61-61: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 62-62: Expected a property but instead found '}'.
Expected a property here.
(parse)
🤖 Prompt for AI Agents
In .zed/settings.json around lines 6 to 62, several object and property entries
contain trailing commas (lines 9, 13, 21, 25, 29, 35, 43, 51, 54, 59) which
violate strict JSON; remove each trailing comma before the corresponding closing
brace or bracket so every object/array member list ends without a comma, then
validate the file with a strict JSON linter or parser to ensure there are no
remaining trailing commas or syntax errors.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel/src/drivers/interrupt_handlers.rs (1)
64-70: Empty panic message provides no diagnostic value.Line 69 adds
panic!("")with an empty string. This change introduces a panic without explanation, making debugging harder. Consider either removing the panic or providing a meaningful message.
♻️ Duplicate comments (19)
build/runner/runner.sh (2)
3-9: Critical: AHCI driver cannot be tested with IDE device attachment.The script attaches disks as legacy IDE devices (
ide-hd), but this PR introduces an AHCI/DMA driver. The AHCI code path will never execute with this configuration.Switch to an AHCI controller with SATA devices:
🔎 Recommended fix
qemu-system-x86_64 \ -M q35 \ + -device ahci,id=ahci0 \ -drive id=disk0,format=raw,file=build/image.bin,if=none \ -drive id=disk1,format=raw,file=build/bin/first_stage,if=none \ - -device ide-hd,drive=disk0,bus=ide.0 \ - -device ide-hd,drive=disk1,bus=ide.1 \ + -device sata-hd,drive=disk0,bus=ahci0.0 \ + -device sata-hd,drive=disk1,bus=ahci0.1 \ -monitor stdio
1-9: Add memory/CPU specifications and robustness checks.The script lacks:
- Memory (
-m) and CPU (-cpu) flags — QEMU defaults may be insufficient- Validation that
qemu-system-x86_64andbuild/image.binexist🔎 Suggested improvements
#!/bin/sh + +set -e + +if ! command -v qemu-system-x86_64 >/dev/null 2>&1; then + echo "Error: qemu-system-x86_64 not found" + exit 1 +fi + +if [ ! -f "build/image.bin" ]; then + echo "Error: build/image.bin not found" + exit 1 +fi qemu-system-x86_64 \ -M q35 \ + -m 2G \ + -cpu max \ -drive id=disk0,format=raw,file=build/image.bin,if=none \kernel/src/main.rs (2)
142-143: Verify BAR5 alignment handling.Lines 142-143 align the BAR5 address down before passing to
HBAMemoryRegisters::new(). If BAR5 is not page-aligned (e.g., 0xFEBF1400), this alignment loses the offset within the page, potentially causing AHCI register access at the wrong address.While a past review noted this was addressed in commit 9646fc4, ensure that
HBAMemoryRegisters::new()in hba.rs correctly handles the offset within the mapped page when accessing registers.
145-145: Hardcoded port 0 ignores probe results.Line 145 calls
map_device::<13>(0)unconditionally, regardless of which ports have active devices. If the active device is on port 1 or higher, this maps an incorrect port.kernel/src/drivers/ata/ahci/hba.rs (15)
72-74: Unsafe transmute can cause UB for invalid hardware values.The
interface_speed()method usestransmuteon a 4-bit hardware field. If hardware reports values outside the enum range (0-3), this causes undefined behavior.Use a safe conversion with
try_fromand a fallback:pub fn interface_speed(&self) -> InterfaceSpeed { let speed = ((self.0 >> 20) & 0xf) as u8; InterfaceSpeed::try_from(speed).unwrap_or(InterfaceSpeed::DevNotPresent) }
86-92: Bit collision: bothpmdandsscuse bit 15.Lines 86 and 89 define different flags on the same bit. Per AHCI spec, PMD is bit 15 and SSC (Slumber State Capable) is bit 14.
// Programmed I/O multiple Data request block ro_flag!(pmd, 15); // Slumber state capable -ro_flag!(ssc, 15); +ro_flag!(ssc, 14); // Partial state capable -ro_flag!(psc, 14); +ro_flag!(psc, 13);
94-97: Off-by-one: AHCI field is "Number of Command Slots - 1".Per AHCI spec, this field encodes (actual_count - 1). The method should add 1 to return the actual count (1-32).
pub fn number_of_commands(&self) -> u8 { - ((self.0 >> 8) & 0x1f) as u8 + ((self.0 >> 8) & 0x1f) as u8 + 1 }
108-111: Off-by-one: AHCI field is "Number of Ports - 1".Per AHCI spec, this field encodes (actual_count - 1). The method should add 1.
pub fn number_of_ports(&self) -> u8 { - (self.0 & 0x1f) as u8 + (self.0 & 0x1f) as u8 + 1 }
284-288: Typo: "buffet_size" should be "buffer_size".Line 286 contains a typo in the method name.
-pub fn buffet_size(&self) -> Option<NonZero<usize>> { +pub fn buffer_size(&self) -> Option<NonZero<usize>> {
566-574: Discarded.then()result may not execute bit manipulation.Line 570 uses
.then(|| { ... })but discards the returnedOption, which means the compiler may not execute the closure. Use an explicitifstatement instead.pub fn set_current_cmd(&mut self, ccs: u8) { if !self.is_st() { return; } - (0x0u8..=0x1fu8).contains(&ccs).then(|| { + if (0x0u8..=0x1fu8).contains(&ccs) { self.0 &= !(0x1f << 8); self.0 |= (ccs as u32) << 8; - }); + } }
591-624: Typo in panic messages: "Timout" should be "Timeout".Lines 603 and 616 contain typos in the panic messages.
-panic!("Timout ended on port stop"); +panic!("Timeout ended on port stop");
676-679: Unsafe transmute inspeed()can cause UB.Using
unchecked_transmute_fromon hardware-reported values is unsafe. If hardware reports invalid speed values, this causes undefined behavior.pub fn speed(&self) -> InterfaceSpeed { let speed = ((self.0 >> 4) & 0xf) as u8; - unsafe { InterfaceSpeed::unchecked_transmute_from(speed) } + InterfaceSpeed::try_from(speed) + .unwrap_or(InterfaceSpeed::DevNotPresent) }
705-710: Unsafe transmute inmax_speed()can cause UB.Using
unchecked_transmute_fromon hardware values is unsafe.pub fn max_speed(&self) -> InterfaceSpeedRestriction { let speed = ((self.0 >> 4) & 0xf) as u8; - unsafe { - InterfaceSpeedRestriction::unchecked_transmute_from(speed) - } + InterfaceSpeedRestriction::try_from(speed) + .unwrap_or(InterfaceSpeedRestriction::NoRestriction) }
764-768: Logic error: setting wrong bits in PM notification.Line 767 ORs the raw
pm_portvalue, which sets incorrect bits. Should use bit shift.pub fn set_pm_notif(&mut self, pm_port: u8) { (0x0..0xf) .contains(&pm_port) - .then(|| self.0 |= pm_port as u32); + .then(|| self.0 |= 1 << pm_port); }
771-777: Logic error: wrong bit mask inget_pm_notif.Line 773 uses
(self.0 & !0xffff)which clears the lower 16 bits, but PM notification bits are in the lower 16 bits.pub fn get_pm_notif(&self, pm_port: u8) -> bool { if (0x0..0xf).contains(&pm_port) { - (self.0 & !0xffff) & (1 << pm_port) != 0 + (self.0 & 0xffff) & (1 << pm_port) != 0 } else { false } }
916-922: Incorrect bit manipulation inset_status.Lines 918-921 shift the port number by 8 and manipulate bits 8-12, but SACT registers use each bit to represent a command slot (0-31). The method should set bit
portdirectly.pub fn set_status(&mut self, port: u8) { self.cmd.set_st(); (0x0u8..=0x1fu8).contains(&port).then(|| { - self.sact.0 &= !(0x1f << 8); - self.sact.0 |= (port as u32) << 8; + self.sact.0 |= 1 << port; }); }Also rename
porttoslotsince SACT tracks command slots.
984-989: Inverted assert conditions will panic for valid inputs.Lines 986-987 have inverted conditions. Valid lengths are 2-16 DWORDs, but the asserts reject them.
pub fn set_command_fis_len_dw(&mut self, len: u8) { - assert!(len < 2, "Len must be smaller then 2"); - assert!(len > 16, "Len must be greater then 16 "); + assert!(len >= 2, "Len must be at least 2"); + assert!(len <= 16, "Len must be at most 16"); self.0 |= len as u32; }
1033-1037:set_dbcvalidates but doesn't actually set the value.The function asserts the range but never modifies
self.0with the byte count. DBC is bits 21:0.pub fn set_dbc(&mut self, dbc: u32) { const MB: u32 = 1 << 20; - assert!(dbc < 4 * MB, "DBC should be smller then 4Mib"); + assert!(dbc < 4 * MB, "DBC should be smaller than 4MiB"); + // Preserve interrupt flag at bit 31, set DBC in bits 21:0 + self.0 = (self.0 & (1 << 31)) | (dbc & 0x3FFFFF); }
1173-1181: Missing validation inmap_device.The method doesn't validate that:
port_numberis within bounds (0-29 for the 30-port array)- The port is actually implemented via
self.ghc.pi.is_port_implemented()- A device is present and active on that port
This could panic on out-of-bounds access or return a controller for a non-existent device.
pub fn map_device<const ENTRIES: usize>( &'static mut self, port_number: usize, ) -> AhciDeviceController<ENTRIES> { assert!(port_number < 30, "Port number must be less than 30"); assert!( self.ghc.pi.is_port_implemented(port_number as u8), "Port {} is not implemented", port_number ); // ... rest of method }
🧹 Nitpick comments (1)
kernel/src/drivers/ata/ahci/mod.rs (1)
1-6: Consider explicit re-exports as the API matures.Wildcard re-exports (
pub use fis::*; pub use hba::*;) flatten the module hierarchy but can make it harder to track the public API surface and may cause name collisions if both modules export identically named items.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
build/runner/runner.shkernel/src/drivers/ata/ahci/hba.rskernel/src/drivers/ata/ahci/mod.rskernel/src/drivers/interrupt_handlers.rskernel/src/main.rsshared/common/src/enums/interrupts.rs
🧰 Additional context used
🧬 Code graph analysis (3)
kernel/src/drivers/ata/ahci/mod.rs (1)
kernel/src/main.rs (2)
hba(145-145)panic(176-182)
kernel/src/drivers/interrupt_handlers.rs (1)
kernel/src/drivers/ata/ahci/mod.rs (1)
ahci_interrupt(8-12)
kernel/src/drivers/ata/ahci/hba.rs (4)
kernel/src/main.rs (2)
panic(176-182)hba(145-145)shared/cpu_utils/src/structures/paging/page_table_entry.rs (1)
empty(24-26)kernel/src/memory/allocators/page_allocator/allocator.rs (1)
Self(206-208)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)
🔇 Additional comments (6)
kernel/src/drivers/ata/ahci/mod.rs (1)
8-12: LGTM: Stub implementation is appropriate for early development.The panic placeholder clearly indicates unimplemented interrupt handling, which is expected during initial AHCI driver scaffolding.
shared/common/src/enums/interrupts.rs (1)
30-30: LGTM: AHCI interrupt variant added correctly.The new
Ahci = 0x2avariant is properly placed in the user-defined interrupt range (after 0x1f reserved by Intel).kernel/src/drivers/interrupt_handlers.rs (1)
401-408: LGTM: AHCI interrupt registration is correct.The AHCI interrupt handler is properly registered with Ring0 privilege and Trap type, consistent with other device interrupt handlers (Timer, Keyboard).
kernel/src/main.rs (2)
23-68: LGTM: Import additions support AHCI integration.The new imports and type additions properly wire AHCI components into the kernel's main initialization flow.
146-165: LGTM: Identity packet flow for testing.The identity packet test flow (buffer allocation, command execution, result reading) appears reasonable for early driver development and testing.
kernel/src/drivers/ata/ahci/hba.rs (1)
1217-1268: LGTM: Identity packet implementation is functional.The
identity_packetmethod now implements the ATA IDENTIFY command sequence with proper FIS construction, buffer setup, command slot management, and timeout handling.
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 (1)
kernel/src/memory/allocators/page_allocator/extensions.rs (1)
149-189: Unwraps will cause implicit panics on page table conflicts.Lines 163 and 169 call
.unwrap()onforce_resolve_table_mut(), which returnsNonewhen entries are mapped to pages rather than tables. This will cause implicit panics without descriptive error messages.Once the
force_resolve_table_mutmethod is fixed to panic explicitly with clear messages (as flagged in the earlier comment), these call sites will automatically benefit from better error reporting.
♻️ Duplicate comments (22)
kernel/src/memory/allocators/page_allocator/extensions.rs (2)
38-68: Critical: Documentation still misleading; implicit panic on NotATable remains unaddressed.The previous review correctly identified that the documentation (lines 38-43) claims "valid table is guaranteed to be returned," but the function now returns
Option<&mut PageTable>and returnsNonefor theNotATablecase (line 47). All callers (lines 100, 163, 169) still use.unwrap(), causing implicit panics when an entry is mapped to a page rather than a table (e.g., huge page conflict).This is a critical correctness issue that must be resolved before merge.
🔎 Recommended fix: Make panic explicit with clear error message
fn force_resolve_table_mut(&mut self) -> Option<&mut PageTable> { match self.mapped_table_mut() { Ok(table) => Some(table), - Err(EntryError::NotATable) => None, + Err(EntryError::NotATable) => { + panic!( + "Page table entry conflict: entry is mapped to a page, not a table. \ + This likely indicates a huge page is already mapped at this level." + ) + } Err(EntryError::NoMapping) => {And update the documentation:
- /// This function will return a table mapped in this - /// entry if there is one. - /// - /// Else, it will override what is inside the entry and - /// map a new table to it so valid table is guaranteed - /// to be returned. + /// Returns a mutable reference to the page table mapped by this entry. + /// + /// If the entry is not mapped, allocates and maps a new table. + /// + /// # Panics + /// Panics if the entry is already mapped to a page (not a table), + /// indicating a conflict with an existing huge page mapping. fn force_resolve_table_mut(&mut self) -> Option<&mut PageTable> {
117-133: Critical logic error: Incorrect iteration range causes wrong page table level access.The previous review correctly identified that
set_flagsuses an exclusive range[0..page_size as usize](line 123) whilemapuses an inclusive range[0..=page_size as usize](line 95).For regular pages where
page_size as usize == 0:
set_flagsiterates over[0..0)→ empty range, no traversalmapiterates over[0..=0]→ traverses level 0This means
set_flagswill access the final entry at levelpage_size as usize + 1(line 130) without traversing to the correct page table level first. The function will operate on the wrong level, leading to incorrect flag modifications or panics.This is a critical correctness bug that must be fixed.
🔎 Proposed fix: Use inclusive range like map
- for level in PageTableLevel::VARIANTS[0..page_size as usize].iter() + for level in PageTableLevel::VARIANTS[0..=page_size as usize].iter() { let index = self.index_of(*level); let entry = &mut table.entries[index]; table = entry.mapped_table_mut()?; }shared/common/src/error/ahci.rs (1)
36-52: AhciError specification issues already flagged.The bit position misalignment and grammar error on line 44 ("was occurred" should be "occurred") in
AhciErrorwere already identified in past review comments.kernel/src/drivers/ata/ahci/hba.rs (14)
1112-1138: Consider handling BAR5 alignment internally.The function rejects unaligned addresses, but BAR5 may not be page-aligned. If the caller aligns the address down before calling, the offset to actual AHCI registers is lost. Consider handling alignment internally:
🔎 Proposed approach
pub fn new(a: PhysicalAddress) -> Result<&'static mut Self, HbaError> { - if !a.is_aligned(REGULAR_PAGE_ALIGNMENT) { - return Err(HbaError::AddressNotAligned); - } + let aligned = a.align_down(REGULAR_PAGE_ALIGNMENT); + let offset = a.as_usize() - aligned.as_usize(); - a.map( - a.translate(), + aligned.map( + aligned.translate(), PageEntryFlags::regular_io_page_flags(), PageSize::Regular, ); - let hba: &'static mut HBAMemoryRegisters = - unsafe { &mut *a.translate().as_mut_ptr() }; + let hba: &'static mut HBAMemoryRegisters = unsafe { + let base_ptr = aligned.translate().as_mut_ptr::<u8>(); + &mut *(base_ptr.add(offset) as *mut HBAMemoryRegisters) + };
72-74: Unsafe transmute for enum conversion is risky.Using
core::mem::transmuteto convert a masked u8 toInterfaceSpeedcan cause undefined behavior if the value doesn't correspond to a valid enum variant. The 4-bit mask allows values 0-15, butInterfaceSpeedlikely only defines a few valid variants.🔎 Proposed fix
pub fn interface_speed(&self) -> InterfaceSpeed { - unsafe { core::mem::transmute(((self.0 >> 20) & 0xf) as u8) } + let speed = ((self.0 >> 20) & 0xf) as u8; + InterfaceSpeed::try_from(speed).unwrap_or(InterfaceSpeed::DevNotPresent) }
86-92: Bit collision: bothpmdandsscuse bit 15.According to AHCI spec, PMD (PIO Multiple DRQ Block) is bit 15 and SSC (Slumber State Capable) is bit 14. Currently both are defined on bit 15, and PSC is on bit 14.
ro_flag!(pmd, 15); // Slumber state capable - ro_flag!(ssc, 15); + ro_flag!(ssc, 14); // Partial state capable - ro_flag!(psc, 14); + ro_flag!(psc, 13);
94-111: Off-by-one innumber_of_commandsandnumber_of_ports.Per AHCI spec, these fields encode "Number of X - 1", so you must add 1 to get the actual count.
pub fn number_of_commands(&self) -> u8 { - ((self.0 >> 8) & 0x1f) as u8 + ((self.0 >> 8) & 0x1f) as u8 + 1 }pub fn number_of_ports(&self) -> u8 { - (self.0 & 0x1f) as u8 + (self.0 & 0x1f) as u8 + 1 }
284-288: Typo:buffet_sizeshould bebuffer_size.- pub fn buffet_size(&self) -> Option<NonZero<usize>> { + pub fn buffer_size(&self) -> Option<NonZero<usize>> { NonZero::new((self.0 & 0xffff) as usize) }
566-574: Logic error:.then()result is discarded, bit manipulation may not execute.The
.then(|| { ... })returns anOptionthat is discarded. Use anifstatement instead to ensure the bit manipulation executes.🔎 Proposed fix
pub fn set_current_cmd(&mut self, ccs: u8) { if !self.is_st() { return; } - (0x0u8..=0x1fu8).contains(&ccs).then(|| { + if (0x0u8..=0x1fu8).contains(&ccs) { self.0 &= !(0x1f << 8); self.0 |= (ccs as u32) << 8; - }); + } }
676-679: Unsafeunchecked_transmute_fromcan cause UB on invalid hardware values.The 4-bit field can contain undefined values (4-15), causing undefined behavior.
pub fn speed(&self) -> InterfaceSpeed { let speed = ((self.0 >> 4) & 0xf) as u8; - unsafe { InterfaceSpeed::unchecked_transmute_from(speed) } + InterfaceSpeed::try_from(speed).unwrap_or(InterfaceSpeed::DevNotPresent) }
705-710: Unsafeunchecked_transmute_fromcan cause UB on invalid hardware values.Same issue as
speed()- the 4-bit field may contain invalid enum values.pub fn max_speed(&self) -> InterfaceSpeedRestriction { let speed = ((self.0 >> 4) & 0xf) as u8; - unsafe { - InterfaceSpeedRestriction::unchecked_transmute_from(speed) - } + InterfaceSpeedRestriction::try_from(speed) + .unwrap_or(InterfaceSpeedRestriction::NoRestriction) }
764-777: Logic errors in PM notification methods.
set_pm_notifsetspm_port as u32directly instead of1 << pm_portget_pm_notifuses& !0xffffwhich clears the lower 16 bits instead of preserving them🔎 Proposed fix
pub fn set_pm_notif(&mut self, pm_port: u8) { (0x0..0xf) .contains(&pm_port) - .then(|| self.0 |= pm_port as u32); + .then(|| self.0 |= 1 << pm_port); } pub fn get_pm_notif(&self, pm_port: u8) -> bool { if (0x0..0xf).contains(&pm_port) { - (self.0 & !0xffff) & (1 << pm_port) != 0 + (self.0 & 0xffff) & (1 << pm_port) != 0 } else { false } }
916-922: Incorrect bit manipulation for SACT register.Per AHCI spec, SACT uses each bit to represent a command slot (0-31). The parameter should be
slotnotport, and should set bitslotdirectly.🔎 Proposed fix
- pub fn set_status(&mut self, port: u8) { + pub fn set_status(&mut self, slot: u8) { self.cmd.set_st(); - (0x0u8..=0x1fu8).contains(&port).then(|| { - self.sact.0 &= !(0x1f << 8); - self.sact.0 |= (port as u32) << 8; + if (0x0u8..=0x1fu8).contains(&slot) { + self.sact.0 |= 1 << slot; - }); + } }
984-990: Inverted assert conditions will always panic for valid inputs.The assertions check
len < 2andlen > 16, which will panic for all valid lengths (2-16).pub fn set_command_fis_len_dw(&mut self, len: u8) { - assert!(len < 2, "Len must be smaller then 2"); - assert!(len > 16, "Len must be greater then 16 "); + assert!(len >= 2, "Len must be at least 2"); + assert!(len <= 16, "Len must be at most 16"); self.0 |= len as u32; }
1033-1038:set_dbconly asserts but never sets the value.The function validates input but doesn't modify
self.0. The DBC field (bits 21:0) is never written.pub fn set_dbc(&mut self, dbc: u32) { const MB: u32 = 1 << 20; - assert!(dbc < 4 * MB, "DBC should be smaller then 4Mib"); + assert!(dbc < 4 * MB, "DBC should be smaller than 4MiB"); + // DBC is bits 21:0, preserve interrupt flag at bit 31 + self.0 = (self.0 & (1 << 31)) | (dbc & 0x3FFFFF); }
1173-1181: Missing validation inmap_device.The method indexes
self.ports[port_number]without validating:
port_numberis within bounds (0-29)- The port is actually implemented via
self.ghc.pi.is_port_implemented()pub fn map_device<const ENTRIES: usize>( &'static mut self, port_number: usize, ) -> AhciDeviceController<ENTRIES> { + assert!(port_number < 30, "Port number must be less than 30"); + assert!( + self.ghc.pi.is_port_implemented(port_number as u8), + "Port {} is not implemented", port_number + ); AhciDeviceController::<ENTRIES>::new(
1228-1231:set_dbc(511)call has no effect due to bug inset_dbc.As noted earlier,
PrdtDescriptionInfo::set_dbconly validates but never writes the DBC value. This means the data byte count remains at its default (likely 0), causing the identity command to transfer no data.kernel/src/drivers/ata/ahci/fis.rs (2)
3-23: Add compile-time size assertion for AHCI FIS layout correctness.The AHCI H2D Register FIS must be exactly 20 bytes per specification. Add a static assertion to prevent accidental layout changes:
const _: () = assert!(core::mem::size_of::<RegisterH2D>() == 20);
168-176: Fix bit extraction instatus_high()to match AHCI specification.The current implementation incorrectly uses
& !0x7(which keeps only bit 3 of the shifted value) instead of& 0x7(which would correctly extract bits 6-4). Per AHCI spec, the Set Device Bits FIS status field contains status bits that need proper extraction:pub fn status_high(&self) -> u8 { - (self.status >> 4) & !0x7 + (self.status >> 4) & 0x7 }kernel/src/drivers/pci.rs (3)
267-272: Incorrect bit check for 64-bit BAR detection.According to PCI spec, bits 2:1 of a memory BAR indicate the type:
00b= 32-bit,10b= 64-bit. The current check tests if bit 1 is set, which would also match the reserved value01b.pub fn is_64bit(&self) -> bool { self.identify() == BaseAddressRegisterType::Memory && unsafe { - self.memory.0 & BaseAddressRegisterSize::Bit64 as u32 != 0 + ((self.memory.0 >> 1) & 0x3) == BaseAddressRegisterSize::Bit64 as u32 } }
274-280: Address extraction uses wrong mask for IO BARs.The method accesses
self.io.0but uses mask0xfffffff0(for memory BARs). IO BARs should use0xfffffffc(bits 1:0 are type flags). Also, for 32-bit memory BARs, should accessself.memory.0.pub fn address(&self) -> usize { - if !self.is_64bit() { - (unsafe { self.io.0 } & 0xfffffff0) as usize + if self.identify() == BaseAddressRegisterType::IO { + (unsafe { self.io.0 } & 0xfffffffc) as usize + } else if !self.is_64bit() { + (unsafe { self.memory.0 } & 0xfffffff0) as usize } else { unimplemented!("Still didn't implemented 64bit addresses") } }
432-433: Remove debug print statement.This
println!("{}", function)appears to be a debug artifact that will spam the console during PCI enumeration.for function in 1..8 { - println!("{}", function); let common = PciConfigurationCycle::read_common_header(
🧹 Nitpick comments (5)
kernel/src/drivers/ata/ahci/fis.rs (1)
67-110: Consider adding compile-time size assertions for D2H FIS types.Per AHCI specification, these structures have fixed sizes. Adding static assertions would catch layout regressions:
const _: () = assert!(core::mem::size_of::<RegisterD2H>() == 20); const _: () = assert!(core::mem::size_of::<DmaActivateD2H>() == 4); const _: () = assert!(core::mem::size_of::<DmaSetup>() == 28);kernel/src/drivers/pci.rs (4)
76-78: Remove commented-out code.These commented lines appear to be leftover from a previous implementation. Either implement the functionality or remove the dead code.
- // uninit.bus = bus; - // uninit.device = device; - // uninit.function = function; uninit
222-224: Remove commented-out code.Same as above - these appear to be remnants of previous implementation.
bist: BISTRegister(0), - // bus: 0xff, - // device: 0xff, - // function: 0xff, }
318-335: Consider usingcore::mem::zeroed()for BAR initialization.The repeated
transmute::<i32, BaseAddressRegister>(0)pattern is verbose and usesi32instead ofu32. Usingzeroed()would be cleaner:- bar0: unsafe { - core::mem::transmute::<i32, BaseAddressRegister>(0) - }, + bar0: unsafe { core::mem::zeroed() },
405-407: Empty method implementation should usetodo!().The
enable_interruptsmethod has an empty body. Mark it as unimplemented:pub fn enable_interrupts(&self, irq: CascadedPicInterruptLine) { + todo!("Implement PCI interrupt enable for IRQ {:?}", irq) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/settings.jsonkernel/src/drivers/ata/ahci/fis.rskernel/src/drivers/ata/ahci/hba.rskernel/src/drivers/pci.rskernel/src/memory/allocators/page_allocator/extensions.rsshared/common/src/error/ahci.rs
🧰 Additional context used
🧬 Code graph analysis (3)
shared/common/src/error/ahci.rs (1)
kernel/src/drivers/ata/ahci/hba.rs (2)
error(640-642)error(747-750)
kernel/src/drivers/pci.rs (1)
shared/common/src/enums/pci.rs (4)
fmt(80-103)fmt(347-349)fmt(547-549)fmt(561-732)
kernel/src/drivers/ata/ahci/hba.rs (2)
kernel/src/memory/allocators/page_allocator/allocator.rs (1)
Self(206-208)shared/cpu_utils/src/structures/paging/entry_flags.rs (1)
regular_io_page_flags(77-83)
🔇 Additional comments (8)
.vscode/settings.json (1)
172-172: Verify this workspace setting change is intentional.Enabling chaining hints affects all developers working on this project. This is a subjective preference—some find it helpful while others consider it visual clutter. Please confirm this change is agreed upon by the team.
shared/common/src/error/ahci.rs (1)
54-58: LGTM!The
HbaErrorenum is correctly defined with an appropriate error message.kernel/src/drivers/ata/ahci/fis.rs (4)
25-65: LGTM!The
RegisterH2D::new()constructor correctly extracts and assigns all FIS fields including the 48-bit LBA split across 6 bytes and the extended features/sector_count fields.
112-147: LGTM!The BIST Activate and PIO Setup D2H FIS structures are correctly defined with appropriate field layouts matching the AHCI specification.
178-194: LGTM!The
Fisunion correctly provides access to all FIS variants with a 64-byte raw backing store. TheDefaultimplementation properly zero-initializes the structure.
196-199: LGTM!The
IdentityPacketDatastruct is correctly aligned to 512 bytes for sector-aligned DMA transfers, and thedatafield is now public as previously suggested.kernel/src/drivers/ata/ahci/hba.rs (1)
1-44: LGTM!The imports are appropriate for the AHCI HBA implementation, pulling in necessary types from the common crate and related modules.
kernel/src/drivers/pci.rs (1)
82-113: LGTM!The
read_pci_devicefunction correctly reads the device-specific portion of the PCI header and constructs aPciDevicewith location information.
| "ctba", | ||
| "ctbau", | ||
| "dbau", | ||
| "desciprtor", |
There was a problem hiding this comment.
Fix typos in spell checker dictionary.
The spell checker dictionary contains misspellings:
- Line 42: "desciprtor" should be "descriptor"
- Line 121: "purporse" should be "purpose"
Adding misspellings to the dictionary defeats the purpose of spell checking.
🔎 Proposed fix
- "desciprtor",
+ "descriptor",- "purporse",
+ "purpose",Also applies to: 121-121
🤖 Prompt for AI Agents
.vscode/settings.json around lines 42 and 121: the spell-check dictionary
contains typos; replace "desciprtor" on line 42 with "descriptor" and replace
"purporse" on line 121 with "purpose" so the dictionary only contains correctly
spelled words.
| "rust-analyzer.cargo.extraArgs": [ | ||
| "--release" | ||
| ], |
There was a problem hiding this comment.
Reconsider forcing release mode for rust-analyzer.
Building in release mode makes rust-analyzer significantly slower, degrading IDE responsiveness for all developers. Release builds are typically unnecessary for code analysis and will impact developer productivity.
Consider removing this setting unless there's a specific requirement for release-mode analysis.
🔎 Proposed fix
- "rust-analyzer.cargo.extraArgs": [
- "--release"
- ],📝 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.
| "rust-analyzer.cargo.extraArgs": [ | |
| "--release" | |
| ], |
🤖 Prompt for AI Agents
In .vscode/settings.json around lines 174 to 176, the
rust-analyzer.cargo.extraArgs currently forces "--release", which slows IDE
analysis; remove the "--release" entry (or replace it with an empty array) so
rust-analyzer uses default dev builds, or if a special case is needed, document
and gate the release flag behind a project-specific setting or comment
indicating why it's required.
| impl<const ENTRIES: usize> PortCommands<ENTRIES> { | ||
| pub fn empty() -> &'static mut PortCommands<ENTRIES> { | ||
| // TODO CREATE EXTERNAL UTIL FUNCTION FOR THIS AND USE ALSO ON PAGE | ||
| // TABLE CREATION | ||
| let zeroed = unsafe { | ||
| core::slice::from_raw_parts_mut( | ||
| alloc_pages!( | ||
| size_of::<PortCommands::<ENTRIES>>() | ||
| / REGULAR_PAGE_SIZE | ||
| ) as *mut usize, | ||
| size_of::<PortCommands<ENTRIES>>() / size_of::<usize>(), | ||
| ) | ||
| }; | ||
| zeroed.fill(0); | ||
|
|
||
| // TODO MAKE LESS SKEYTCHY | ||
| let port_cmd_ptr = (zeroed.as_mut_ptr() as usize | ||
| - PHYSICAL_MEMORY_OFFSET) | ||
| as *mut PortCommands<ENTRIES>; | ||
|
|
||
| unsafe { &mut *port_cmd_ptr } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check PHYSICAL_MEMORY_OFFSET definition and usage patterns
rg -n "PHYSICAL_MEMORY_OFFSET" --type rust -B 2 -A 2 | head -60
# Also search for how virtual-to-physical conversion is done elsewhere
rg -n "as.*usize.*PHYSICAL_MEMORY_OFFSET" --type rust -B 1 -A 1Repository: sagi21805/LearnixOS
Length of output: 4565
🏁 Script executed:
#!/bin/bash
# Find PortCommands struct definition to understand its size
rg -n "struct PortCommands" --type rust -A 20Repository: sagi21805/LearnixOS
Length of output: 1659
🏁 Script executed:
#!/bin/bash
# Check REGULAR_PAGE_SIZE definition
rg -n "REGULAR_PAGE_SIZE" --type rust -B 1 -A 1 | head -30Repository: sagi21805/LearnixOS
Length of output: 1989
🏁 Script executed:
#!/bin/bash
# Look for similar allocation patterns in the codebase to understand conventions
rg -n "alloc_pages!" --type rust -B 3 -A 5 | head -80Repository: sagi21805/LearnixOS
Length of output: 2516
🏁 Script executed:
#!/bin/bash
# Find ReceivedFis, CommandList, CommandTable size definitions
rg -n "struct ReceivedFis|struct CommandList|struct CommandTable" --type rust -A 30 | head -120Repository: sagi21805/LearnixOS
Length of output: 6533
🏁 Script executed:
#!/bin/bash
# Check alloc_pages! macro definition to understand what it returns
rg -n "macro_rules! alloc_pages" --type rust -A 10Repository: sagi21805/LearnixOS
Length of output: 1071
Address conversion produces a physical pointer that cannot be safely dereferenced as a virtual address.
The code allocates virtual memory via alloc_pages!, subtracts PHYSICAL_MEMORY_OFFSET to get a physical address, then casts and dereferences it as a virtual pointer. In protected/long mode, physical addresses cannot be directly dereferenced—they must be accessed through virtual mappings. This violates memory safety.
Additionally, integer division for page count may allocate fewer pages than needed if size_of::<PortCommands<ENTRIES>>() is not a multiple of REGULAR_PAGE_SIZE.
🤖 Prompt for AI Agents
In kernel/src/drivers/ata/ahci/hba.rs around lines 1076-1098, the current
implementation converts the virtual address returned by alloc_pages! into a
physical address by subtracting PHYSICAL_MEMORY_OFFSET and then dereferences
it—this is unsafe in protected/long mode because physical addresses cannot be
directly dereferenced as virtual pointers; also the page count calculation uses
integer division and can under-allocate. Fix by (1) computing the required page
count with ceiling division: pages = (size_of::<PortCommands<ENTRIES>>() +
REGULAR_PAGE_SIZE - 1) / REGULAR_PAGE_SIZE, (2) call alloc_pages! with that page
count and keep/use the returned virtual pointer directly (do NOT subtract
PHYSICAL_MEMORY_OFFSET or convert to a physical address), (3) create a mutable
slice from the returned virtual pointer, fill it with zeros, and then cast that
same virtual pointer to &mut PortCommands<ENTRIES> to return; ensure the
allocation’s lifetime satisfies 'static (e.g., leak or use a kernel allocator
that provides 'static memory) so the returned reference is valid.
| if self.port.is.0 != 0 { | ||
| panic!("ERROR ON IDENTITY READ, {}", self.port.is.0); | ||
| } |
There was a problem hiding this comment.
Error check may trigger false positives.
Checking self.port.is.0 != 0 will trigger on any interrupt status bit, including successful completion indicators (DHRS, PSS, DSS). Consider checking only error bits:
- if self.port.is.0 != 0 {
- panic!("ERROR ON IDENTITY READ, {}", self.port.is.0);
+ // Check for fatal error conditions
+ if self.port.is.is_tfes() || self.port.is.is_hbfs() ||
+ self.port.is.is_hbds() || self.port.is.is_ifs() {
+ panic!("ERROR ON IDENTITY READ, status: {:#x}", self.port.is.0);
}Committable suggestion skipped: line range outside the PR's diff.
| for level in | ||
| PageTableLevel::VARIANTS[0..=page_size as usize].iter() | ||
| { | ||
| let index = self.index_of(*level); | ||
| let entry = &mut table.entries[index]; | ||
| let resolved_table = entry.force_resolve_table_mut(); | ||
| let resolved_table = | ||
| entry.force_resolve_table_mut().unwrap(); | ||
| table = resolved_table; | ||
| } | ||
| unsafe { | ||
| table.entries[self | ||
| .index_of((3 - page_size as u8).try_into().unwrap())] | ||
| .map_unchecked(address, flags); | ||
| table.entries[self.index_of( | ||
| PageTableLevel::VARIANTS[page_size as usize + 1], | ||
| )] | ||
| .map(address, flags); | ||
| } |
There was a problem hiding this comment.
Unwrap will cause implicit panic on page table conflicts.
Line 100 calls .unwrap() on force_resolve_table_mut(), which returns None when an entry is mapped to a page rather than a table. This will cause an implicit panic without a descriptive error message, making debugging difficult.
This is a direct consequence of the unresolved issue in force_resolve_table_mut. Once that method is fixed to panic explicitly with a clear message, this call site will benefit from better error reporting.
🤖 Prompt for AI Agents
In kernel/src/memory/allocators/page_allocator/extensions.rs around lines 94 to
108, the code currently calls .unwrap() on force_resolve_table_mut(), which will
cause an implicit panic if the entry is mapped to a page; replace this unsafe
unwrap with explicit handling: call force_resolve_table_mut() and match its
Option, returning or propagating a Result error (or panic!) with a clear message
that includes the table index and current PageTableLevel if None is returned;
ensure the chosen error path provides descriptive context for debugging instead
of an implicit unwrap panic.
| #[repr(u16)] | ||
| #[derive(Debug, Clone, Copy, TryFromPrimitive, Error, EnumIter)] | ||
| #[num_enum(error_type(name = ConversionError<u16>, constructor = ConversionError::CantConvertFrom))] | ||
| pub enum DiagnosticError { | ||
| #[error("Physical ready signal changed state")] | ||
| PhyRdyChange = 1 << 0, | ||
| #[error("Internal error in the physical layer")] | ||
| PhyInternal = 1 << 1, | ||
| #[error("Communication wake signal detected")] | ||
| CommWake = 1 << 2, | ||
| #[error("10B to 8B decoding errors occurred")] | ||
| DecodingError = 1 << 3, | ||
| #[error("Disparity Error (Not use by AHCI)")] | ||
| DisparityError = 1 << 4, | ||
| #[error("One or more CRC errors occurred on link layer")] | ||
| CrcError = 1 << 5, | ||
| #[error("Handshake error, one or more R_ERR responses were received")] | ||
| HandshakeError = 1 << 6, | ||
| #[error("One or more link state machine errors were encountered")] | ||
| LinkSequenceError = 1 << 7, | ||
| #[error("Error on transport layer transition change")] | ||
| TransportStateError = 1 << 8, | ||
| #[error("One or more FISs were received with unknown type")] | ||
| UnknownFisType = 1 << 9, | ||
| #[error("A change in device presence has been detected")] | ||
| Exchanged = 1 << 10, | ||
| } |
There was a problem hiding this comment.
Fix DiagnosticError bit positions to match AHCI 1.3.1 PxSERR specification.
The DiagnosticError enum bit assignments are offset from the AHCI 1.3.1 PxSERR register specification. According to the spec (see past comment web search results), these diagnostic/error bits occupy positions 5-15 in PxSERR, but the code currently assigns them to bits 0-10. For example:
PhyRdyChangeis bit 0 in code but should be bit 5 (N) per specCrcErroris bit 5 in code but should be bit 10 (C) per specExchangedis bit 10 in code but should be bit 15 (X) per spec
This misalignment will cause incorrect error detection when reading hardware PxSERR values. Either correct the bit positions to match the specification, or document the intentional custom mapping.
Additionally, the typo on line 20 ("Not use by AHCI") was already flagged in a past comment.
🔎 Proposed fix to align with AHCI 1.3.1 spec
pub enum DiagnosticError {
#[error("Physical ready signal changed state")]
- PhyRdyChange = 1 << 0,
+ PhyRdyChange = 1 << 5,
#[error("Internal error in the physical layer")]
- PhyInternal = 1 << 1,
+ PhyInternal = 1 << 6,
#[error("Communication wake signal detected")]
- CommWake = 1 << 2,
+ CommWake = 1 << 7,
#[error("10B to 8B decoding errors occurred")]
- DecodingError = 1 << 3,
+ DecodingError = 1 << 8,
- #[error("Disparity Error (Not use by AHCI)")]
- DisparityError = 1 << 4,
+ #[error("Disparity Error (Not used by AHCI)")]
+ DisparityError = 1 << 9,
#[error("One or more CRC errors occurred on link layer")]
- CrcError = 1 << 5,
+ CrcError = 1 << 10,
#[error("Handshake error, one or more R_ERR responses were received")]
- HandshakeError = 1 << 6,
+ HandshakeError = 1 << 11,
#[error("One or more link state machine errors were encountered")]
- LinkSequenceError = 1 << 7,
+ LinkSequenceError = 1 << 12,
#[error("Error on transport layer transition change")]
- TransportStateError = 1 << 8,
+ TransportStateError = 1 << 13,
#[error("One or more FISs were received with unknown type")]
- UnknownFisType = 1 << 9,
+ UnknownFisType = 1 << 14,
#[error("A change in device presence has been detected")]
- Exchanged = 1 << 10,
+ Exchanged = 1 << 15,
}📝 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.
| #[repr(u16)] | |
| #[derive(Debug, Clone, Copy, TryFromPrimitive, Error, EnumIter)] | |
| #[num_enum(error_type(name = ConversionError<u16>, constructor = ConversionError::CantConvertFrom))] | |
| pub enum DiagnosticError { | |
| #[error("Physical ready signal changed state")] | |
| PhyRdyChange = 1 << 0, | |
| #[error("Internal error in the physical layer")] | |
| PhyInternal = 1 << 1, | |
| #[error("Communication wake signal detected")] | |
| CommWake = 1 << 2, | |
| #[error("10B to 8B decoding errors occurred")] | |
| DecodingError = 1 << 3, | |
| #[error("Disparity Error (Not use by AHCI)")] | |
| DisparityError = 1 << 4, | |
| #[error("One or more CRC errors occurred on link layer")] | |
| CrcError = 1 << 5, | |
| #[error("Handshake error, one or more R_ERR responses were received")] | |
| HandshakeError = 1 << 6, | |
| #[error("One or more link state machine errors were encountered")] | |
| LinkSequenceError = 1 << 7, | |
| #[error("Error on transport layer transition change")] | |
| TransportStateError = 1 << 8, | |
| #[error("One or more FISs were received with unknown type")] | |
| UnknownFisType = 1 << 9, | |
| #[error("A change in device presence has been detected")] | |
| Exchanged = 1 << 10, | |
| } | |
| #[repr(u16)] | |
| #[derive(Debug, Clone, Copy, TryFromPrimitive, Error, EnumIter)] | |
| #[num_enum(error_type(name = ConversionError<u16>, constructor = ConversionError::CantConvertFrom))] | |
| pub enum DiagnosticError { | |
| #[error("Physical ready signal changed state")] | |
| PhyRdyChange = 1 << 5, | |
| #[error("Internal error in the physical layer")] | |
| PhyInternal = 1 << 6, | |
| #[error("Communication wake signal detected")] | |
| CommWake = 1 << 7, | |
| #[error("10B to 8B decoding errors occurred")] | |
| DecodingError = 1 << 8, | |
| #[error("Disparity Error (Not used by AHCI)")] | |
| DisparityError = 1 << 9, | |
| #[error("One or more CRC errors occurred on link layer")] | |
| CrcError = 1 << 10, | |
| #[error("Handshake error, one or more R_ERR responses were received")] | |
| HandshakeError = 1 << 11, | |
| #[error("One or more link state machine errors were encountered")] | |
| LinkSequenceError = 1 << 12, | |
| #[error("Error on transport layer transition change")] | |
| TransportStateError = 1 << 13, | |
| #[error("One or more FISs were received with unknown type")] | |
| UnknownFisType = 1 << 14, | |
| #[error("A change in device presence has been detected")] | |
| Exchanged = 1 << 15, | |
| } |
🤖 Prompt for AI Agents
In shared/common/src/error/ahci.rs around lines 8 to 34, the DiagnosticError
enum assigns bit values starting at 1<<0..1<<10 but AHCI PxSERR bits are at
positions 5..15; update each enum variant to use the correct bit positions
(shift by N where PhyRdyChange should be 1<<5, PhyInternal 1<<6, CommWake 1<<7,
DecodingError 1<<8, DisparityError 1<<9, CrcError 1<<10, HandshakeError 1<<11,
LinkSequenceError 1<<12, TransportStateError 1<<13, UnknownFisType 1<<14,
Exchanged 1<<15) so the enum maps exactly to the AHCI 1.3.1 PxSERR
specification, and fix the typo in the DisparityError message ("Not used by
AHCI").
1 participant
Summary by CodeRabbit
New Features
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.