Lesson 23, section 6

cfenollosa · cfenollosa · commit b10969117987 · 2015-08-28T10:52:05.000+02:00
diff --git a/23-fixes/Makefile b/23-fixes/Makefile
@@ -7,7 +7,7 @@ OBJ = ${C_SOURCES:.c=.o cpu/interrupt.o}
 CC = /usr/local/i386elfgcc/bin/i386-elf-gcc
 GDB = /usr/local/i386elfgcc/bin/i386-elf-gdb
 # -g: Use debugging symbols in gcc
-CFLAGS = -g -ffreestanding -Wall -Wextra -fno-exceptions
+CFLAGS = -g -ffreestanding -Wall -Wextra -fno-exceptions -m32
 
 # First rule is run by default
 os-image.bin: boot/bootsect.bin kernel.bin
@@ -33,7 +33,7 @@ debug: os-image.bin kernel.elf
 # Generic rules for wildcards
 # To make an object, always compile from its .c
 %.o: %.c ${HEADERS}
-	${CC} ${CFLAGS} -ffreestanding -c $< -o $@
+	${CC} ${CFLAGS} -c $< -o $@
 
 %.o: %.asm
 	nasm $< -f elf -o $@
diff --git a/23-fixes/README.md b/23-fixes/README.md
@@ -1,4 +1,4 @@
-*Concepts you may want to Google beforehand: XX*
+*Concepts you may want to Google beforehand: freestanding, uint32_t, size_t,*
 
 **Goal: Fix miscellaneous issues with our code**
 
@@ -16,7 +16,8 @@ We add  `-ffreestanding` when compiling `.o` files, which includes `kernel_entry
 Before, we disabled libgcc (not libc) through the use of `-nostdlib` and we didn't re-enable
 it for linking. Since this is tricky, we'll delete `-nostdlib`
 
-`-nostdinc` was also pased to gcc, but we will need it for step 3.
+`-nostdinc` was also pased to gcc, but we will need it for step 3, so let's delete it.
+
 
 2. kernel.c `main()` function
 -----------------------------
@@ -69,11 +70,23 @@ bit telling whether interrupts are on or off.
 In other words the interrupt handler automatically restores interrupts whether or not 
 interrupts were enabled before this interrupt
 
-
 On `cpu/isr.h`, `struct registers_t` has a couple issues. 
 First, the alleged `esp` is renamed to `useless`.
 The value is useless because it has to do with the current stack context, not what was interrupted.
 Then, we rename `useresp` to `esp`
 
-Finally, we add `cld` just before `call isr_handler` on `cpu/interrupt.asm`
+We add `cld` just before `call isr_handler` on `cpu/interrupt.asm` as suggested
+by the osdev wiki.
+
+There is a final, important issue with `cpu/interrupt.asm`. The common stubs create an instance
+of `struct registers` on the stack and then call the C handler. But that breaks the ABI, since
+the stack belongs to the called function and they may change them as they please. It is needed
+to pass the struct as a pointer.
+
+To achieve this, edit `cpu/isr.h` and `cpu/isr.c` and change `registers_t r` into `registers_t *t`,
+then, instead of accessing the fields of the struct via `.`, access the fields of the pointer via `->`.
+Finally, in `cpu/interrupt.asm`, and add a `push esp` before calling both `isr_handler` and
+`irq_handler` -- remember to also `pop eax` to clear the pointer afterwards.
 
+Both current callbacks, the timer and the keyboard, also need to be changed to use a pointer to
+`registers_t`.
diff --git a/23-fixes/cpu/interrupt.asm b/23-fixes/cpu/interrupt.asm
@@ -13,13 +13,14 @@ isr_common_stub:
 	mov es, ax
 	mov fs, ax
 	mov gs, ax
-	
+	push esp ; registers_t *r
     ; 2. Call C handler
     cld ; C code following the sysV ABI requires DF to be clear on function entry
 	call isr_handler
 	
     ; 3. Restore state
 	pop eax 
+    pop eax
 	mov ds, ax
 	mov es, ax
 	mov fs, ax
@@ -39,9 +40,11 @@ irq_common_stub:
     mov es, ax
     mov fs, ax
     mov gs, ax
+    push esp
     cld
     call irq_handler ; Different than the ISR code
     pop ebx  ; Different than the ISR code
+    pop ebx
     mov ds, bx
     mov es, bx
     mov fs, bx
@@ -110,218 +113,186 @@ global irq15
 
 ; 0: Divide By Zero Exception
 isr0:
-    cli
     push byte 0
     push byte 0
     jmp isr_common_stub
 
 ; 1: Debug Exception
 isr1:
-    cli
     push byte 0
     push byte 1
     jmp isr_common_stub
 
 ; 2: Non Maskable Interrupt Exception
 isr2:
-    cli
     push byte 0
     push byte 2
     jmp isr_common_stub
 
 ; 3: Int 3 Exception
 isr3:
-    cli
     push byte 0
     push byte 3
     jmp isr_common_stub
 
 ; 4: INTO Exception
 isr4:
-    cli
     push byte 0
     push byte 4
     jmp isr_common_stub
 
 ; 5: Out of Bounds Exception
 isr5:
-    cli
     push byte 0
     push byte 5
     jmp isr_common_stub
 
 ; 6: Invalid Opcode Exception
 isr6:
-    cli
     push byte 0
     push byte 6
     jmp isr_common_stub
 
 ; 7: Coprocessor Not Available Exception
 isr7:
-    cli
     push byte 0
     push byte 7
     jmp isr_common_stub
 
 ; 8: Double Fault Exception (With Error Code!)
 isr8:
-    cli
     push byte 8
     jmp isr_common_stub
 
 ; 9: Coprocessor Segment Overrun Exception
 isr9:
-    cli
     push byte 0
     push byte 9
     jmp isr_common_stub
 
 ; 10: Bad TSS Exception (With Error Code!)
 isr10:
-    cli
     push byte 10
     jmp isr_common_stub
 
 ; 11: Segment Not Present Exception (With Error Code!)
 isr11:
-    cli
     push byte 11
     jmp isr_common_stub
 
 ; 12: Stack Fault Exception (With Error Code!)
 isr12:
-    cli
     push byte 12
     jmp isr_common_stub
 
 ; 13: General Protection Fault Exception (With Error Code!)
 isr13:
-    cli
     push byte 13
     jmp isr_common_stub
 
 ; 14: Page Fault Exception (With Error Code!)
 isr14:
-    cli
     push byte 14
     jmp isr_common_stub
 
 ; 15: Reserved Exception
 isr15:
-    cli
     push byte 0
     push byte 15
     jmp isr_common_stub
 
 ; 16: Floating Point Exception
 isr16:
-    cli
     push byte 0
     push byte 16
     jmp isr_common_stub
 
 ; 17: Alignment Check Exception
 isr17:
-    cli
     push byte 0
     push byte 17
     jmp isr_common_stub
 
 ; 18: Machine Check Exception
 isr18:
-    cli
     push byte 0
     push byte 18
     jmp isr_common_stub
 
 ; 19: Reserved
 isr19:
-    cli
     push byte 0
     push byte 19
     jmp isr_common_stub
 
 ; 20: Reserved
 isr20:
-    cli
     push byte 0
     push byte 20
     jmp isr_common_stub
 
 ; 21: Reserved
 isr21:
-    cli
     push byte 0
     push byte 21
     jmp isr_common_stub
 
 ; 22: Reserved
 isr22:
-    cli
     push byte 0
     push byte 22
     jmp isr_common_stub
 
 ; 23: Reserved
 isr23:
-    cli
     push byte 0
     push byte 23
     jmp isr_common_stub
 
 ; 24: Reserved
 isr24:
-    cli
     push byte 0
     push byte 24
     jmp isr_common_stub
 
 ; 25: Reserved
 isr25:
-    cli
     push byte 0
     push byte 25
     jmp isr_common_stub
 
 ; 26: Reserved
 isr26:
-    cli
     push byte 0
     push byte 26
     jmp isr_common_stub
 
 ; 27: Reserved
 isr27:
-    cli
     push byte 0
     push byte 27
     jmp isr_common_stub
 
 ; 28: Reserved
 isr28:
-    cli
     push byte 0
     push byte 28
     jmp isr_common_stub
 
 ; 29: Reserved
 isr29:
-    cli
     push byte 0
     push byte 29
     jmp isr_common_stub
 
 ; 30: Reserved
 isr30:
-    cli
     push byte 0
     push byte 30
     jmp isr_common_stub
 
 ; 31: Reserved
 isr31:
-    cli
     push byte 0
     push byte 31
     jmp isr_common_stub
diff --git a/23-fixes/cpu/isr.c b/23-fixes/cpu/isr.c
@@ -116,29 +116,29 @@ char *exception_messages[] = {
     "Reserved"
 };
 
-void isr_handler(registers_t r) {
+void isr_handler(registers_t *r) {
     kprint("received interrupt: ");
     char s[3];
-    int_to_ascii(r.int_no, s);
+    int_to_ascii(r->int_no, s);
     kprint(s);
     kprint("\n");
-    kprint(exception_messages[r.int_no]);
+    kprint(exception_messages[r->int_no]);
     kprint("\n");
 }
 
 void register_interrupt_handler(uint8_t n, isr_t handler) {
     interrupt_handlers[n] = handler;
 }
 
-void irq_handler(registers_t r) {
+void irq_handler(registers_t *r) {
     /* After every interrupt we need to send an EOI to the PICs
      * or they will not send another interrupt again */
-    if (r.int_no >= 40) port_byte_out(0xA0, 0x20); /* slave */
+    if (r->int_no >= 40) port_byte_out(0xA0, 0x20); /* slave */
     port_byte_out(0x20, 0x20); /* master */
 
     /* Handle the interrupt in a more modular way */
-    if (interrupt_handlers[r.int_no] != 0) {
-        isr_t handler = interrupt_handlers[r.int_no];
+    if (interrupt_handlers[r->int_no] != 0) {
+        isr_t handler = interrupt_handlers[r->int_no];
         handler(r);
     }
 }
diff --git a/23-fixes/cpu/isr.h b/23-fixes/cpu/isr.h
@@ -71,7 +71,13 @@ extern void irq15();
 #define IRQ14 46
 #define IRQ15 47
 
-/* Struct which aggregates many registers */
+/* Struct which aggregates many registers.
+ * It matches exactly the pushes on interrupt.asm. From the bottom:
+ * - Pushed by the processor automatically
+ * - `push byte`s on the isr-specific code: error code, then int number
+ * - All the registers by pusha
+ * - `push eax` whose lower 16-bits contain DS
+ */
 typedef struct {
    uint32_t ds; /* Data segment selector */
    uint32_t edi, esi, ebp, useless, ebx, edx, ecx, eax; /* Pushed by pusha. */
@@ -80,10 +86,10 @@ typedef struct {
 } registers_t;
 
 void isr_install();
-void isr_handler(registers_t r);
+void isr_handler(registers_t *r);
 void irq_install();
 
-typedef void (*isr_t)(registers_t);
+typedef void (*isr_t)(registers_t*);
 void register_interrupt_handler(uint8_t n, isr_t handler);
 
 #endif
diff --git a/23-fixes/cpu/timer.c b/23-fixes/cpu/timer.c
@@ -5,7 +5,7 @@
 
 uint32_t tick = 0;
 
-static void timer_callback(registers_t regs) {
+static void timer_callback(registers_t *regs) {
     tick++;
     UNUSED(regs);
 }
diff --git a/23-fixes/drivers/keyboard.c b/23-fixes/drivers/keyboard.c
@@ -25,7 +25,7 @@ const char sc_ascii[] = { '?', '?', '1', '2', '3', '4', '5', '6',
         'H', 'J', 'K', 'L', ';', '\'', '`', '?', '\\', 'Z', 'X', 'C', 'V', 
         'B', 'N', 'M', ',', '.', '/', '?', '?', '?', ' '};
 
-static void keyboard_callback(registers_t regs) {
+static void keyboard_callback(registers_t *regs) {
     /* The PIC leaves us the scancode in port 0x60 */
     uint8_t scancode = port_byte_in(0x60);
     
diff --git a/23-fixes/kernel/kernel.c b/23-fixes/kernel/kernel.c
@@ -9,6 +9,9 @@ void kernel_main() {
     isr_install();
     irq_install();
 
+    asm("int $2");
+    asm("int $3");
+
     kprint("Type something, it will go through the kernel\n"
         "Type END to halt the CPU or PAGE to request a kmalloc()\n> ");
 }

Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@`
`5`	`5`
`6`	`6`	`uint32_t tick = 0;`
`7`	`7`
`8`		`-static void timer_callback(registers_t regs) {`
	`8`	`+static void timer_callback(registers_t *regs) {`
`9`	`9`	`tick++;`
`10`	`10`	`UNUSED(regs);`
`11`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,9 @@ void kernel_main() {`
`9`	`9`	`isr_install();`
`10`	`10`	`irq_install();`
`11`	`11`
	`12`	`+ asm("int $2");`
	`13`	`+ asm("int $3");`
	`14`	`+`
`12`	`15`	`kprint("Type something, it will go through the kernel\n"`
`13`	`16`	`"Type END to halt the CPU or PAGE to request a kmalloc()\n> ");`
`14`	`17`	`}`