Kernel-Update-3 - #5
Conversation
WalkthroughThe PR updates kernel initialization to pass a new parameter to pmm_init, introduces a buddy-based PMM with new APIs, adds paging structure reservation helpers, expands paging entry flags and constants, and hardens paging enablement with verification steps. Header declarations are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Kernel
participant PMM
participant Paging
rect rgba(230,240,255,0.5)
note over Kernel: Boot init
Kernel->>PMM: pmm_init(memory_size, bitmap_addr)
PMM->>Paging: paging_get_reserved_start()/end()
Paging-->>PMM: reserved range
PMM->>PMM: Initialize buddy free-lists, reserve paging structures
end
rect rgba(230,255,230,0.5)
Kernel->>Paging: paging_install()
Paging->>Paging: Set up PDE/PTEs (+pwt/pcd/us flags)
Paging->>CPU: Set CR0.PG, forced fetch, re-read CR0
alt PG verified
Paging-->>Kernel: Paging enabled
else PG not set
Paging-->>Kernel: Error and abort install
end
end
sequenceDiagram
autonumber
participant Client as Kernel/Subsys
participant PMM
rect rgba(255,245,230,0.5)
Client->>PMM: pmm_alloc_page()
PMM->>PMM: Find/split blocks (buddy allocator)
PMM-->>Client: page ptr
Client->>PMM: pmm_free_page(ptr)
PMM->>PMM: Coalesce with buddy if free
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 4
🧹 Nitpick comments (6)
pmm.h (1)
8-8: bitmap_addr is currently unused; either use it or drop it.
pmm.cstoresbitmap_addrbut never uses it. This is confusing and risks drift. Either wire it into the allocator or remove it for now.paging.h (2)
4-4: Remove unnecessary dependency on idt.h from paging.h.Header layering: paging.h doesn't need IDT. Drop to avoid tight coupling.
Apply:
-#include "idt.h" //This is for interrupt handling
12-22: Bitfields for x86 paging entries are fragile.C bitfield layout is compiler/ABI-dependent. Consider using uint32_t with masks/shifts for PDE/PTE to avoid surprises.
pmm.c (3)
35-39: Remove duplicate “0x” in address log.
vga_print_hexalready prefixes with 0x.Apply:
- vga_print_string("Reserving paging structures: 0x"); + vga_print_string("Reserving paging structures: "); vga_print_hex(paging_start); - vga_print_string(" - 0x"); + vga_print_string(" - "); vga_print_hex(paging_end);
12-23: memory_bitmap is unused.Either wire the bitmap into the allocator or mark/remove it to avoid confusion.
Options:
- Use: actually implement bitmap tracking, or
- Keep but annotate:
static uint8_t* memory_bitmap /* unused for now */;, or- Remove param and field until needed.
96-124: No validation in free path.
pmm_free_blockstrusts theaddrandorder. Consider adding basic sanity checks in debug builds to catch double-frees or out-of-range frees early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
kernel.c(1 hunks)paging.c(4 hunks)paging.h(1 hunks)pmm.c(1 hunks)pmm.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
kernel.c (1)
pmm.c (1)
pmm_init(20-64)
paging.c (1)
vga.c (1)
vga_print_string(70-74)
paging.h (1)
paging.c (3)
paging_install(9-84)paging_get_reserved_start(87-89)paging_get_reserved_end(92-94)
pmm.c (2)
paging.c (2)
paging_get_reserved_start(87-89)paging_get_reserved_end(92-94)vga.c (2)
vga_print_string(70-74)vga_print_hex(76-87)
pmm.h (1)
pmm.c (5)
pmm_init(20-64)pmm_alloc_blocks(66-94)pmm_free_blocks(96-130)pmm_alloc_page(133-135)pmm_free_page(137-139)
🔇 Additional comments (3)
paging.c (1)
66-74: Paging enable sequence with CR0 write + pipeline flush looks solid.The
mov %cr0followed by a near jump to force a refetch is correct for reliably enabling paging.pmm.c (1)
141-159: Free memory accounting is OK for now.Simple traversal by order is fine; consider 64-bit accumulation if you plan >4GB later.
pmm.h (1)
8-15: Old PMM APIs fully removed. Searches yielded no references topmm_alloc,pmm_free, or single-argpmm_init.
|
|
||
| vga_print_string("Initializing PMM... "); | ||
| pmm_init(128 * 1024 * 1024); | ||
| pmm_init(128 * 1024 * 1024, (void*)0x200000); |
There was a problem hiding this comment.
Derive bitmap base from paging structures instead of hardcoding 0x200000.
Avoid overlap risks and linker-order assumptions by using the end of the reserved paging area.
Apply:
- pmm_init(128 * 1024 * 1024, (void*)0x200000);
+ pmm_init(128 * 1024 * 1024, (void*)paging_get_reserved_end());📝 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.
| pmm_init(128 * 1024 * 1024, (void*)0x200000); | |
| pmm_init(128 * 1024 * 1024, (void*)paging_get_reserved_end()); |
| // Return the start address of paging structures for PMM coordination | ||
| uint32_t paging_get_reserved_start(void) { | ||
| return (uint32_t)&page_directory; | ||
| } | ||
|
|
||
| // Return the end address of paging structures for PMM coordination | ||
| uint32_t paging_get_reserved_end(void) { | ||
| return (uint32_t)&fpage_table + sizeof(fpage_table); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Harden reserved range getters against linker/order differences.
If the linker ever places fpage_table before page_directory, current helpers return a wrong range. Compute min/max.
Apply:
-uint32_t paging_get_reserved_start(void) {
- return (uint32_t)&page_directory;
-}
+uint32_t paging_get_reserved_start(void) {
+ uint32_t a = (uint32_t)&page_directory;
+ uint32_t b = (uint32_t)&fpage_table;
+ return (a < b) ? a : b;
+}
@@
-uint32_t paging_get_reserved_end(void) {
- return (uint32_t)&fpage_table + sizeof(fpage_table);
-}
+uint32_t paging_get_reserved_end(void) {
+ uint32_t a = (uint32_t)&page_directory + sizeof(page_directory);
+ uint32_t b = (uint32_t)&fpage_table + sizeof(fpage_table);
+ return (a > b) ? a : b;
+}🤖 Prompt for AI Agents
In paging.c around lines 86 to 94, the reserved range getters assume
page_directory is before fpage_table and can return an incorrect range if the
linker places fpage_table first; update both paging_get_reserved_start and
paging_get_reserved_end to compute the range robustly by taking the minimum of
the two base addresses as the start and the maximum of (base + size) of each
structure as the end; use an integer pointer type (e.g., uintptr_t or uint32_t)
for address arithmetic and comparisons and cast back to uint32_t for the return
values to avoid incorrect results due to linker/order differences.
| extern pt_entry_t page_directory[1024]; | ||
| extern pt_entry_t fpage_table[1024]; |
There was a problem hiding this comment.
Fix compile error: pt_entry_t used before being defined.
extern pt_entry_t ... appears before the typedef. Move the typedef above the externs.
Apply:
-extern pt_entry_t page_directory[1024];
-extern pt_entry_t fpage_table[1024];
-//Below is for defining page table entry
-typedef struct pt_entry{
+// Page table entry
+typedef struct pt_entry{
@@
-} pt_entry_t;
+} pt_entry_t;
+extern pt_entry_t page_directory[1024];
+extern pt_entry_t fpage_table[1024];🤖 Prompt for AI Agents
In paging.h around lines 9-10, the extern declarations for pt_entry_t are placed
before the typedef that defines pt_entry_t, causing a compile error; move the
typedef (or its typedef declaration) above these extern declarations so
pt_entry_t is defined before usage, then keep the extern lines unchanged below
the typedef.
| // Add all available memory into the allocator, skipping reserved regions | ||
| uint32_t start_page = 0x100000 / PAGE_SIZE; | ||
| for (uint32_t page = start_page; page < total_pages; ) { | ||
| // Skip paging structures | ||
| if (page >= paging_start_page && page < paging_end_page) { | ||
| page = paging_end_page; | ||
| continue; | ||
| } | ||
| // Find the largest order block we can create at this address. | ||
| uint32_t order = MAX_ORDER; | ||
| while (order > 0) { | ||
| if ((page % (1 << order) == 0) && (page + (1 << order) <= total_pages)) { | ||
| break; | ||
| } | ||
| order--; | ||
| } | ||
|
|
||
| // Add the block to the appropriate free list. | ||
| buddy_node_t* block = (buddy_node_t*)(page * PAGE_SIZE); | ||
| block->next = free_lists[order]; | ||
| free_lists[order] = block; | ||
| page += (1 << order); | ||
| } |
There was a problem hiding this comment.
Blocks can straddle the reserved paging region.
When seeding free lists, the chosen order isn’t constrained to stay entirely before paging_start_page. This can add a free block that overlaps reserved structures.
Apply:
- uint32_t order = MAX_ORDER;
- while (order > 0) {
- if ((page % (1 << order) == 0) && (page + (1 << order) <= total_pages)) {
- break;
- }
- order--;
- }
+ uint32_t order = MAX_ORDER;
+ while (order > 0) {
+ uint32_t block_pages = (1U << order);
+ uint32_t block_end_page = page + block_pages;
+ if ((page % block_pages) == 0 &&
+ block_end_page <= total_pages &&
+ !(page < paging_start_page && block_end_page > paging_start_page)) {
+ break;
+ }
+ order--;
+ }📝 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.
| // Add all available memory into the allocator, skipping reserved regions | |
| uint32_t start_page = 0x100000 / PAGE_SIZE; | |
| for (uint32_t page = start_page; page < total_pages; ) { | |
| // Skip paging structures | |
| if (page >= paging_start_page && page < paging_end_page) { | |
| page = paging_end_page; | |
| continue; | |
| } | |
| // Find the largest order block we can create at this address. | |
| uint32_t order = MAX_ORDER; | |
| while (order > 0) { | |
| if ((page % (1 << order) == 0) && (page + (1 << order) <= total_pages)) { | |
| break; | |
| } | |
| order--; | |
| } | |
| // Add the block to the appropriate free list. | |
| buddy_node_t* block = (buddy_node_t*)(page * PAGE_SIZE); | |
| block->next = free_lists[order]; | |
| free_lists[order] = block; | |
| page += (1 << order); | |
| } | |
| // Add all available memory into the allocator, skipping reserved regions | |
| uint32_t start_page = 0x100000 / PAGE_SIZE; | |
| for (uint32_t page = start_page; page < total_pages; ) { | |
| // Skip paging structures | |
| if (page >= paging_start_page && page < paging_end_page) { | |
| page = paging_end_page; | |
| continue; | |
| } | |
| // Find the largest order block we can create at this address. | |
| uint32_t order = MAX_ORDER; | |
| while (order > 0) { | |
| uint32_t block_pages = (1U << order); | |
| uint32_t block_end_page = page + block_pages; | |
| if ((page % block_pages) == 0 && | |
| block_end_page <= total_pages && | |
| !(page < paging_start_page && block_end_page > paging_start_page)) { | |
| break; | |
| } | |
| order--; | |
| } | |
| // Add the block to the appropriate free list. | |
| buddy_node_t* block = (buddy_node_t*)(page * PAGE_SIZE); | |
| block->next = free_lists[order]; | |
| free_lists[order] = block; | |
| page += (1 << order); | |
| } |
🤖 Prompt for AI Agents
In pmm.c around lines 41 to 63, the seeding loop can add a free block whose
range overlaps the reserved paging region; ensure any chosen order produces a
block fully outside the reserved range by constraining the order so (page + (1
<< order) <= paging_start_page) when the block starts before the reserved region
or (page >= paging_end_page) when after it. Modify the order-selection logic to
reduce order if the candidate block would cross into [paging_start_page,
paging_end_page), and if no valid order remains advance the page to the
reservation boundary (set page = paging_end_page) to skip the reserved region;
keep alignment checks and bounds as before and only insert blocks that do not
overlap reserved pages.
1 participant
Improved the paging.c and paging.h. Also, the variables have been synchronized.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor