Feature/#10 init simple memory map #11

archan0621 · 2025-12-27T16:59:04Z

closes #10

개발 내용

콘솔 스크롤 기능 - 화면이 가득 차면 자동 스크롤
PMM 구현 - 비트맵 기반 페이지 할당/해제, 비트맵 초기화 버그 수정, 0~1MB 제외
IDT 구현 - 예외/IRQ 핸들러 시스템
메모리 맵 덤프 - Multiboot2 메모리 맵 정보 출력
커널 통합 - IDT/PMM 초기화 및 테스트 코드
빌드 시스템 - IDT, MMAP, PMM 빌드 규칙 추가
링커 스크립트 - 커널 심볼 추가

Copilot

Pull request overview

This PR implements fundamental kernel infrastructure including a Physical Memory Manager (PMM), Interrupt Descriptor Table (IDT), memory map dumping functionality, and console auto-scrolling. The implementation adds bitmap-based page allocation with proper initialization that excludes the 0-1MB region, sets up exception and IRQ handlers, and enables the console to automatically scroll when the screen fills up.

Key Changes

PMM Implementation: Bitmap-based physical memory allocator with page allocation/deallocation functions, excluding 0-1MB from management
IDT Setup: Exception handlers (ISR 0-31) and IRQ handlers (32-47) with assembly stubs and C handler functions
Console Scrolling: Automatic screen scrolling for both VGA text mode and framebuffer mode when cursor exceeds screen height

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
src/mem/pmm.c	Implements bitmap-based physical memory manager with initialization, allocation, and deallocation functions
src/mem/mmap.c	Adds memory map dump functionality to display Multiboot2 memory map information
src/kernel.c	Integrates IDT and PMM initialization, adds test code for page allocation/deallocation
src/drivers/video/video.c	Implements screen scrolling for both VGA text and framebuffer modes
src/drivers/console/console.c	Adds automatic scrolling when cursor exceeds screen height
src/arch/x86/isr.asm	Assembly stubs for CPU exception handlers (ISR 0-31)
src/arch/x86/irq.asm	Assembly stubs for hardware interrupt handlers (IRQ 0-15)
src/arch/x86/idt_flush.asm	Assembly function to load IDT register
src/arch/x86/idt.c	IDT initialization and interrupt handler registration
src/arch/x86/gdt.c	Removes GDT verification output, fixes comment typo
linker.ld	Adds kernel_start and kernel_end symbols for PMM to determine kernel memory region
include/mem/pmm.h	Header defining PMM interface
include/mem/mmap.h	Header defining memory map dump function
include/drivers/video/video.h	Adds video_scroll_up function declaration
include/arch/x86/idt.h	Header defining IDT initialization interface
Makefile	Adds build rules for new IDT, ISR, IRQ, MMAP, and PMM modules

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-27T17:04:57Z

src/mem/pmm.c

+    uint8_t* p = start;
+    uint32_t entry_count = 0;
+
+    while (p < end && entry_count < 64) {


The entry count limit of 64 is arbitrary and undocumented. If there are more than 64 memory map entries, they will be silently ignored, potentially causing incorrect memory management. Consider either documenting why 64 is sufficient, or removing this limit entirely and relying on the bounds check against 'end' pointer.

Copilot · 2025-12-27T17:04:58Z

src/mem/pmm.c

+struct multiboot_tag {
+    uint32_t type;
+    uint32_t size;
+};
+
+struct multiboot_tag_mmap {
+    uint32_t type;
+    uint32_t size;
+    uint32_t entry_size;
+    uint32_t entry_version;
+};
+
+struct multiboot_mmap_entry {
+    uint64_t addr;
+    uint64_t len;
+    uint32_t type;
+    uint32_t zero;
+};
+


The multiboot2 structures (multiboot_tag, multiboot_tag_mmap, multiboot_mmap_entry) are duplicated in both pmm.c and mmap.c. This code duplication should be eliminated by defining these structures once in a shared header file (e.g., include/mem/mmap.h or a new multiboot2.h header) and including it in both files.

Suggested change

struct multiboot_tag {

uint32_t type;

uint32_t size;

};

struct multiboot_tag_mmap {

uint32_t type;

uint32_t size;

uint32_t entry_size;

uint32_t entry_version;

};

struct multiboot_mmap_entry {

uint64_t addr;

uint64_t len;

uint32_t type;

uint32_t zero;

};

/* multiboot_tag, multiboot_tag_mmap, and multiboot_mmap_entry

* are declared in mem/mmap.h to avoid duplication. */

Copilot · 2025-12-27T17:04:58Z

src/kernel.c

+
+	mmap_dump(magic, mbinfo);


The code uses tabs for indentation while other lines use spaces. This creates inconsistent indentation in the file. The entire file should use consistent indentation (spaces or tabs).

Suggested change

mmap_dump(magic, mbinfo);

mmap_dump(magic, mbinfo);

Copilot · 2025-12-27T17:04:58Z

src/mem/pmm.c

+        __asm__ __volatile__ (
+            "cld\n\t"
+            "rep stosb\n\t"
+            : "+c" (size), "+D" (bmp)
+            : "a" (0xFF)
+            : "memory"
+        );


The assembly uses "rep stosb" to fill the bitmap, which requires ECX to contain the count. However, 'size' is modified by the instruction (decremented to 0), and the "+c" constraint allows this. The 'bmp' pointer is also incremented to point past the end. While this works correctly, both variables are modified in potentially unexpected ways. Consider adding a comment explaining that these variables are consumed by the instruction.

Copilot · 2025-12-27T17:04:59Z

src/arch/x86/idt.c

+//   EDI, ESI, EBP, ESP, EBX, EDX, ECX, EAX (pusha)
+//   int_no, err_code (pushed manually)
+//   EIP, CS, EFLAGS (pushed by CPU)
+//   (useresp, ss only in user mode)
+struct interrupt_frame {
+    uint32_t gs, fs, es, ds;              // Segment registers (top of stack)
+    uint32_t edi, esi, ebp, esp, ebx, edx, ecx, eax;  // pusha order


The interrupt_frame struct member order for pusha registers appears to be incorrect. The pusha instruction pushes registers in the order: EAX, ECX, EDX, EBX, ESP, EBP, ESI, EDI. This means in memory (from low to high address), they appear as EAX, ECX, EDX, EBX, ESP, EBP, ESI, EDI. However, the struct defines them as "edi, esi, ebp, esp, ebx, edx, ecx, eax" (low address to high address). When ESP is passed to the C handler and cast to this struct, the first field should be at the lowest address. Since GS was pushed last, it's at the lowest address, so the struct starts correctly with gs. After the segment registers come the pusha registers, where EAX should be at the lowest address, not EDI. The correct order should be: eax, ecx, edx, ebx, esp, ebp, esi, edi.

Suggested change

// EDI, ESI, EBP, ESP, EBX, EDX, ECX, EAX (pusha)

// int_no, err_code (pushed manually)

// EIP, CS, EFLAGS (pushed by CPU)

// (useresp, ss only in user mode)

struct interrupt_frame {

uint32_t gs, fs, es, ds; // Segment registers (top of stack)

uint32_t edi, esi, ebp, esp, ebx, edx, ecx, eax; // pusha order

// EAX, ECX, EDX, EBX, ESP, EBP, ESI, EDI (pusha, from low to high address)

// int_no, err_code (pushed manually)

// EIP, CS, EFLAGS (pushed by CPU)

// (useresp, ss only in user mode)

struct interrupt_frame {

uint32_t gs, fs, es, ds; // Segment registers (top of stack)

uint32_t eax, ecx, edx, ebx, esp, ebp, esi, edi; // pusha order

Copilot · 2025-12-27T17:05:00Z

src/mem/pmm.c

+
+        __asm__ __volatile__ (
+            "cld\n\t"
+            "rep stosb\n\t"
+            : "+c" (size), "+D" (bmp)
+            : "a" (0xFF)


The inline assembly constraint uses "+D" for the pointer, which treats it as a pointer in the EDI register. However, 'bmp' is declared as uint8_t*, and after the assembly, bmp will point to the end of the bitmap. While this doesn't affect the operation since the result isn't used afterward, it's potentially confusing. Consider using "=D" and a dummy output variable, or make it clearer with a comment that bmp is modified.

Suggested change

__asm__ __volatile__ (

"cld\n\t"

"rep stosb\n\t"

: "+c" (size), "+D" (bmp)

: "a" (0xFF)

uint8_t* bmp_end;

__asm__ __volatile__ (

"cld\n\t"

"rep stosb\n\t"

: "+c" (size), "=D" (bmp_end)

: "a" (0xFF), "1" (bmp)

Copilot · 2025-12-27T17:05:01Z

src/mem/pmm.c

+    if (total_pages > 0x100000) {
+        total_pages = 0x100000;
+    }


The maximum page limit of 0x100000 (1048576 pages = 4GB of memory) is hardcoded. This arbitrary limit should be documented with a comment explaining why it exists, or ideally be defined as a named constant at the top of the file.

Copilot · 2025-12-27T17:05:01Z

src/mem/pmm.c

+    if (total_pages > 0) {
+        uint32_t t = total_pages;
+        char buf[12];
+        int idx = 0;
+        while (t > 0 && idx < 11) {
+            buf[idx++] = (char)('0' + (t % 10));
+            t /= 10;
+        }
+        while (idx--) console_putc(buf[idx]);
+    }


When v is 0, the function returns early and never enters the while loop. However, in pmm.c lines 294-303 and 305-314, the code assumes the loop will run at least once. If total_pages or free_pages is 0, nothing will be printed, which may not be the intended behavior. Consider handling the zero case explicitly by printing "0" instead of printing nothing.

Copilot · 2025-12-27T17:05:01Z

src/mem/pmm.c

+            buf[idx++] = (char)('0' + (f % 10));
+            f /= 10;
+        }
+        while (idx--) console_putc(buf[idx]);


When free_pages is 0, the while loop condition (f > 0) prevents any output. This means nothing will be printed between " total pages, " and " free pages\n", which creates confusing output like "X total pages, free pages". The zero case should be handled explicitly.

Suggested change

while (idx--) console_putc(buf[idx]);

while (idx--) console_putc(buf[idx]);

} else {

console_putc('0');

Copilot · 2025-12-27T17:05:01Z

src/mem/pmm.c

+
+#define MB2_MAGIC 0x36d76289


The MB2_MAGIC constant is defined in multiple files (pmm.c, mmap.c, and kernel.c). This should be defined once in a shared header to avoid duplication and potential inconsistencies.

Suggested change

#define MB2_MAGIC 0x36d76289

#ifndef MB2_MAGIC

#define MB2_MAGIC 0x36d76289

#endif

archan0621 added 7 commits December 28, 2025 00:50

feat: add console scroll functionality

6565ae3

feat: fix pmm bitmap initialization and exclude 0-1mb

ae5db65

feat: add idt implementation

1390628

feat: implement mmap dump

a5f5b1c

build: add kernel symbols to linker

ebc1fcd

feat: integrate pmm and idt in kernel main

b940770

build: add idt, mmap, pmm to build system

dafa889

archan0621 requested a review from Copilot December 27, 2025 16:59

archan0621 self-assigned this Dec 27, 2025

archan0621 added the enhancement New feature or request label Dec 27, 2025

Copilot started reviewing on behalf of archan0621 December 27, 2025 16:59 View session

Copilot AI reviewed Dec 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/#10 init simple memory map #11

Feature/#10 init simple memory map #11

Uh oh!

archan0621 commented Dec 27, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Feature/#10 init simple memory map #11

Are you sure you want to change the base?

Feature/#10 init simple memory map #11

Uh oh!

Conversation

archan0621 commented Dec 27, 2025

개발 내용

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants