General Protection Fault on IRQs - Immediately after sti

[minater247]

I've found 2 topics on this on this forum, but neither of their solutions have applied.

I've set up my GDT and IDT, and everything seems to be working properly except the IRQs. After enabling interrupts, I immediately get a General Protection Fault - error code 0x103. Going from this table https://wiki.osdev.org/Exceptions#Selector_Error_Code, it appears that this is interrupt 0x20 - which correlates with the Bochs error, "interrupt(): gate descriptor is not valid sys seg (vector=0x20)", leading me to believe this is the system timer IRQ. If I try and fire this interrupt address manually, it just freaks out and continually fires interrupt 0x20, although I am unable to see the error code for these as my VGA driver blanks out when scrolling too much.

One strange thing, though, is that code after this seems to work fine, and sometimes things after the init_descriptor_tables() function show up before the GPF. As long as no IRQs are fired, everything else (including normal interrupts) works just fine.

I'll try to include the relevant bits of code here, but if it would work better I have a GitHub repo I could make public for a bit. Any place it gets long and repetitive I've shortened it, but it's all just changing numbers by one per line or pushing one less byte for ISRs that take an error code.

Any help on this is much appreciated! I've been trying for a couple of days to track this one down and I just can't seem to find it.

(perhaps relevant?) gdt_flush definition [asmfuncs.asm]

Code: Select all

extern gp            ; Says that 'gp' is in another file
gdt_flush:
    lgdt [gp]        ; Load the GDT with our 'gp' which is a special pointer
    mov ax, 0x10      ; 0x10 is the offset in the GDT to our data segment
    mov ds, ax
    mov es, ax
    mov fs, ax
    mov gs, ax
    mov ss, ax
    jmp 0x08:flush2   ; 0x08 is the offset to our code segment: Far jump!
flush2:
    ret 

IDT and ISR loading [isrs.asm]

Code: Select all

section .text
align 4

global idt_load
extern idtp
idt_load:
    mov eax, [esp + 4]
    lidt [eax]
    ret

global isr0
[shortened for readability]
global isr31

isr0:
  cli
  push byte 0
  push byte 0
  jmp isr_common_stub

[shortened for readability]

isr31:
  cli
  push byte 0
  push byte 31
  jmp isr_common_stub

extern fault_handler

isr_common_stub:
    pusha
    push ds
    push es
    push fs
    push gs
    mov ax, 0x10
    mov ds, ax
    mov es, ax
    mov fs, ax
    mov gs, ax
    mov eax, esp
    push eax
    mov eax, fault_handler
    call eax
    pop eax
    pop gs
    pop fs
    pop es
    pop ds
    popa
    add esp, 8
    iret

; IRQs
global irq0
[shortened for readability]
global irq15

irq0:
  cli
  push byte 0
  push byte 32
  jmp irq_common_stub

[shortened for readability]

irq15:
  cli
  push byte 0
  push byte 47
  jmp irq_common_stub

extern irq_handler

irq_common_stub:
    pusha
    push ds
    push es
    push fs
    push gs
    mov ax, 0x10
    mov ds, ax
    mov es, ax
    mov fs, ax
    mov gs, ax
    mov eax, esp
    push eax
    mov eax, irq_handler
    call eax
    pop eax
    pop gs
    pop fs
    pop es
    pop ds
    popa
    add esp, 8
    iret

And finally, the C loading and error handling code: [tables.c]

Code: Select all

#include <asm/asmfuncs.h>
#include <lib/stdint.h>
#include <lib/vga.h>
#include <lib/string.h>


//#region GDT

struct gdt_entry {
    unsigned short limit_low;
    unsigned short base_low;
    unsigned char base_middle;
    unsigned char access;
    unsigned char granularity;
    unsigned char base_high;
} __attribute__((packed));

struct gdt_ptr {
    unsigned short limit;
    unsigned int base;
} __attribute__((packed));

struct gdt_entry gdt[3];
struct gdt_ptr gp;

extern void gdt_flush();

void gdt_set_gate(int num, unsigned long base, unsigned long limit, unsigned char access, unsigned char gran) {
    gdt[num].base_low = (base & 0xFFFF);
    gdt[num].base_middle = (base >> 16) & 0xFF;
    gdt[num].base_high = (base >> 24) & 0xFF;

    gdt[num].limit_low = (limit & 0xFFFF);
    gdt[num].granularity = ((limit >> 16) & 0x0F);

    gdt[num].granularity |= (gran & 0xF0);
    gdt[num].access = access;
}

void gdt_install() {
    gp.limit = (sizeof(struct gdt_entry) * 3) - 1;
    gp.base = (unsigned int)&gdt;

    gdt_set_gate(0, 0, 0, 0, 0);
    gdt_set_gate(1, 0, 0xFFFFFFFF, 0x9A, 0xCF);
    gdt_set_gate(2, 0, 0xFFFFFFFF, 0x92, 0xCF);

    gdt_flush();
}

//#endregion GDT

//#region IDT

struct idt_entry {
    unsigned short base_lo;
    unsigned short sel;
    unsigned char always0;
    unsigned char flags;
    unsigned short base_hi;
} __attribute__((packed));

struct idt_ptr {
    unsigned short limit;
    unsigned int base;
} __attribute__((packed));

struct regs {
    unsigned int gs, fs, es, ds;
    unsigned int edi, esi, ebp, esp, ebx, edx, ecx, eax;
    unsigned int int_no, err_code;
    unsigned int epi, cs, eflags, useresp, ss;
};

struct idt_entry idt[256];
struct idt_ptr idtp;

extern void idt_load(uint32 idt_ptr);
extern void isr0();
[shortened for readability]
extern void isr31();

void idt_set_gate(unsigned char num, unsigned long base, unsigned short sel, unsigned char flags) {
    idt[num].base_lo = (base & 0xFFFF);
    idt[num].base_hi = (base >> 16) & 0xFFFF;
    idt[num].sel = sel;
    idt[num].always0 = 0;
    idt[num].flags = flags;
}

void isrs_install() {
    idt_set_gate(0, (unsigned)isr0, 0x08, 0x8E);
    [shortened for readability]
    idt_set_gate(31, (unsigned)isr31, 0x08, 0x8E);
}

void idt_install() {
    idtp.limit = (sizeof (struct idt_entry) * 256) - 1;
    idtp.base = (unsigned int)&idt;

    memset(&idt, 0, sizeof(struct idt_entry) * 256);

    isrs_install();

    idt_load((uint64)&idtp);
}

extern void irq0();
[shortened for readability]
extern void irq15();

void *irq_routines[16] = {
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
};

void irq_install_handler(int irq, void (*handler)(struct regs *r)) {
    irq_routines[irq] = handler;
}

void irq_remap(void)
{
    outb(0x20, 0x11);
    outb(0xA0, 0x11);
    outb(0x21, 0x20);
    outb(0xA1, 0x28);
    outb(0x21, 0x04);
    outb(0xA1, 0x02);
    outb(0x21, 0x01);
    outb(0xA1, 0x01);
    outb(0x21, 0x0);
    outb(0xA1, 0x0);
}

void irq_install() {
    irq_remap();
    
    idt_set_gate(32, (unsigned)irq0, 0x08, 0x8E);
    [shortened for readability]
    idt_set_gate(47, (unsigned)irq15, 0x08, 0x8E);
}

void irq_handler(struct regs *r)
{
    /* This is a blank function pointer */
    void (*handler)(struct regs *r);

    /* Find out if we have a custom handler to run for this
    *  IRQ, and then finally, run it */
    handler = irq_routines[r->int_no - 32];
    if (handler)
    {
        handler(r);
    } else {
        print("Unhandled interrupt\n", 21);
    }

    /* If the IDT entry that was invoked was greater than 40
    *  (meaning IRQ8 - 15), then we need to send an EOI to
    *  the slave controller */
    if (r->int_no >= 40)
    {
        outb(0xA0, 0x20);
    }

    /* In either case, we need to send an EOI to the master
    *  interrupt controller too */
    outb(0x20, 0x20);
}

void fault_handler(struct regs *r) {

    if (r->int_no < 32) {
        char int_no_buf[3];
        int int_no_len = itoa(r->int_no, int_no_buf, 10);

        if (r->int_no == 13) {
            print("General protection fault:\n", 26);
            char err_code_buf[10];
            int err_code_len = itoa(r->err_code, err_code_buf, 10);
            print("Error code ", 12);
            print(err_code_buf, err_code_len);
            print(" (0x", 4);
            err_code_len = itoa(r->err_code, err_code_buf, 16);
            print(err_code_buf, err_code_len);
            print(")\n", 2);
        }else {
            print("Unhandled exception ", 21);
            print(int_no_buf, int_no_len);
            int_no_len = itoa(r->int_no, int_no_buf, 16);
            print(" (0x", 4);
            print(int_no_buf, int_no_len);
            print("}\n", 2);
            char err_code_buf[10];
            int err_code_len = itoa(r->err_code, err_code_buf, 10);
            print("Error code ", 12);
            print(err_code_buf, err_code_len);
            print(" (0x", 4);
            err_code_len = itoa(r->err_code, err_code_buf, 16);
            print(err_code_buf, err_code_len);
            print(")\n", 2);
        }
    } else if (r->int_no == 32) {
        setColor(4, 0);
        print("TRIPLE FAULT!", 13);
    }
}

//#endregion IDT

void init_descriptor_tables() {
    gdt_install();
    irq_install();
    idt_install();
    __asm__ __volatile__ ("sti");
}

[Octocontrabass]

Any place it gets long and repetitive I've shortened it, but it's all just changing numbers by one per line or pushing one less byte for ISRs that take an error code.

You should use macros to shorten it.

Code: Select all

extern gp            ; Says that 'gp' is in another file

Is there any particular reason why you don't pass a parameter like in idt_load()?

Code: Select all

isr0:
  cli

This is a very dangerous mistake. Placing CLI at the start of an ISR does not prevent an IRQ from arriving after the CPU has jumped to the ISR but before the CPU executes CLI. You should delete the CLI instruction. That way if you've mistakenly placed a trap gate in your IDT instead of an interrupt gate, you're more likely to notice so you can fix it.

Code: Select all

    mov eax, fault_handler
    call eax

You can directly call a label.

Code: Select all

irq_common_stub:

You don't need a separate stub for IRQs.

Code: Select all

#include <lib/stdint.h>

Your cross-compiler already provides its own stdint.h. You should use it instead of trying to write your own.

Code: Select all

struct gdt_entry {...} __attribute__((packed));

You don't need to pack this struct, and it can actually hurt performance by reducing the alignment requirements.

Code: Select all

struct idt_entry {...} __attribute__((packed));

You don't need to pack this struct either.

Code: Select all

    unsigned int edi, esi, ebp, esp, ebx, edx, ecx, eax;

Replace "esp" with "garbage".

Code: Select all

    unsigned int epi, cs, eflags, useresp, ss;

Replace "useresp" with "esp" and "epi" with "eip".

Code: Select all

void idt_install() {
...
    memset(&idt, 0, sizeof(struct idt_entry) * 256);

So idt_install() clears the IDT, overwriting any handlers that might already be there.

Code: Select all

    idt_load((uint64)&idtp);

You should see a compiler warning here.

Code: Select all

void irq_remap(void)
{
...
    outb(0x21, 0x0);
    outb(0xA1, 0x0);

It might be a bad idea to enable all IRQ sources before you're prepared to handle them.

Code: Select all

    /* If the IDT entry that was invoked was greater than 40
    *  (meaning IRQ8 - 15), then we need to send an EOI to
    *  the slave controller */
...
    /* In either case, we need to send an EOI to the master
    *  interrupt controller too */

What about spurious interrupts?

Code: Select all

        print("TRIPLE FAULT!", 13);

You can't print an error message for a triple fault - if the CPU triple faults, it won't execute any more of your code.

Code: Select all

    irq_install();
    idt_install();

So idt_install() clears the IDT, overwriting any handlers that might already be there.

[minater247]

I'm not getting that compiler warning (I believe I need some more -W flags), but most of the code problems here were originally for testing, and a good lot from tutorial code. First I had decent code with macros, then it didn't work and I got rid of the macros and changed some methods of parameter passing, and I eventually just tried copying tutorials directly to see if I could get anything working. I know it's pretty bad code right now, and yeah, all of these are valid things I need to work on. Just wanted to get it to do something other than immediately fault...

A couple of other ones not related to those reasons I'll note on - first the triple fault thing, that came from when I messed up the remapping initially and pushed the wrong interrupt number, it looked like a triple fault so I ran with it until I figured out that that made no sense. I meant to remove that already, but got distracted by the GPF. Spurious interrupts I was just saving until I got normal, EOI-expecting interrupts working properly and enabled.

But now it works! That was the problem, simply changing around those two functions has fixed the error. I hadn't intended them to be that way around, but somehow I must have put the cursor on the wrong line - I knew it'd be some kind of dumb mistake like that. No more fault! Now I'll go clean up the code, thank you so much!!