From 90ccd7c4db4fa8ca723ae0f8c082c82dc29074c3 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Sun, 28 Dec 2025 23:22:08 +0000
Subject: [PATCH] Address code review feedback

- Define EFI_MEMORY_DESCRIPTOR struct for better code clarity
- Use PAGE_SIZE constant instead of magic number 4096
- Add check in freePage() to prevent usedPages underflow

Co-authored-by: johndoe6345789 <224850594+johndoe6345789@users.noreply.github.com>
---
 kernel/src/memory.cpp | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/kernel/src/memory.cpp b/kernel/src/memory.cpp
index 1cdf1d1..3b46010 100644
--- a/kernel/src/memory.cpp
+++ b/kernel/src/memory.cpp
@@ -12,6 +12,15 @@
 // Physical memory bitmap constants
 #define BITMAP_SIZE 2097152  // Supports up to 64GB with 4KB pages (64GB / 4KB = 16M pages, 16M bits = 2MB bitmap)
 
+// EFI Memory Descriptor structure (from UEFI spec)
+struct EFI_MEMORY_DESCRIPTOR {
+    uint32_t Type;
+    uint64_t PhysicalStart;
+    uint64_t VirtualStart;
+    uint64_t NumberOfPages;
+    uint64_t Attribute;
+};
+
 /* PhysicalMemoryManager class implementation */
 
 /**
@@ -68,14 +77,7 @@ void PhysicalMemoryManager::init(BootInfo* bootInfo) {
         
         // First pass: find highest usable address and mark free pages
         for (uint64_t i = 0; i < num_descriptors; i++) {
-            // Cast to EFI_MEMORY_DESCRIPTOR structure
-            struct {
-                uint32_t Type;
-                uint64_t PhysicalStart;
-                uint64_t VirtualStart;
-                uint64_t NumberOfPages;
-                uint64_t Attribute;
-            }* desc = (decltype(desc))(map + i * descriptor_size);
+            EFI_MEMORY_DESCRIPTOR* desc = (EFI_MEMORY_DESCRIPTOR*)(map + i * descriptor_size);
             
             // Check if this is a usable memory type
             bool is_usable = (desc->Type == EfiConventionalMemory ||
@@ -86,7 +88,7 @@ void PhysicalMemoryManager::init(BootInfo* bootInfo) {
             
             if (is_usable) {
                 uint64_t region_start = desc->PhysicalStart;
-                uint64_t region_size = desc->NumberOfPages * 4096;  // EFI pages are 4KB
+                uint64_t region_size = desc->NumberOfPages * PAGE_SIZE;  // EFI pages are 4KB
                 uint64_t region_end = region_start + region_size;
                 
                 // Only consider memory at or above MEMORY_BASE
@@ -211,6 +213,11 @@ void PhysicalMemoryManager::freePage(void* page) {
         return;
     }
     
+    // Check if page is already free to prevent underflow
+    if (!(pageBitmap[byte] & (1 << bit))) {
+        return;  // Already free, nothing to do
+    }
+    
     // Mark as free
     pageBitmap[byte] &= ~(1 << bit);
     usedPages--;