Implementing PIT

[Thpertic]

Hi all,
I finished the interrupt handler following this (http://www.jamesmolloy.co.uk/tutorial_h ... 20PIT.html) and other tutorials, now I want to implement to PIT.

I have an array of IRQs handlers and in the clock_init() function I register it and set the command port.

Code: Select all

void clock_init(uint32_t frequency) {
    // Set the timer as the first IRQ
    irq_installHandler(IRQ0, &tickHandler);

    // The value sent to the PIT is the value to divide its input clock
    // (1193180 Hz) by, to get the required frequency. 
    // Important to note is that the divisor must be small enough to fit into 16-bits.
    uint16_t divisor = (uint16_t) (1193180 / frequency);

    // The divisor must be sent 8 bits at a time
    uint8_t low = (uint8_t) (divisor & 0xFF);
    uint8_t high = (uint8_t) ((divisor >> 8) & 0xFF);

    // Send the command byte
    outportb(0x43, 0x36);

    // Send the frequency divisor
    outportb(0x40, low);
    outportb(0x40, high);
}

In the handler I only increase the tick of the system.

Code: Select all

void tickHandler(regs_t *r) {
    tick++;
    printf("Tick: %d\n", tick);
}

The problem is that when I use printf("Tick: %d\n", tick); the screen print Tick: Exception: Page fault (err code 2) and obviously halts as I set that.
I tried to use only printf("Tick\n"); and it works printing an infinite list of Ticks, though. Also if I don't print anything, the page fault doesn't occur.

Now, can someone please help me figure out if is that a problem and how can I solve it? Perhaps an error in the printf?

Code: Select all

int printf(const char *format, ...) {
    char *s;

    va_list list;
    va_start(list, format);

    while(*format != '\0') {
        if (*format == '%') {
            // Handle the format specifier
            format++;
            switch (*format) {
                // Character 
                case 'c':
                    putc((char)va_arg(list, int));
                    break;

                // String
                case 's':
                    s = (char *)va_arg(list, int);
                    puts(s);
                    break;

                // Hexadecimal
                case 'x':
                    s = utoa((uint64_t)va_arg(list, int), s, 16);
                    puts(s);
                    break;

                // Decimal
                case 'd':
                    s = itoa((int)va_arg(list, int), s, 10);
                    puts(s);
                    break;

                // Unsigned
                case 'u':
                    s = utoa((uint64_t)va_arg(list, int), s, 10);
                    puts(s);
                    break;

                // Binary
                case 'b':
                    s = utoa((uint64_t)va_arg(list, int), s, 2);
                    puts(s);
                    break;
                    
                default:
                    return -1;
            }
            format++;
        } else
            putc(*format++);            
    }
    va_end(list);

    return 1;
}

[MichaelPetch]

Do you have your project online or available somewhere that we can look at it in its entirety?

[Octocontrabass]

I finished the interrupt handler following this (http://www.jamesmolloy.co.uk/tutorial_h ... 20PIT.html) and other tutorials,

That tutorial is known to have many bugs; some of those bugs are documented here. Most other tutorials also have bugs.

Code: Select all

s = itoa((int)va_arg(list, int), s, 10);

When you convert an integer to a string, where are you storing that string?

[Thpertic]

Do you have your project online or available somewhere that we can look at it in its entirety?

I just uploaded the project to GitHub: https://github.com/thpertic/LostOS.

I controlled the documentation and that tutorial seems to have a problem with interrupt handlers: "Problem: Interrupt handlers corrupt interrupted state".
The author states that

The most practical method is to pass the structure as a pointer instead [...]

I can understand it, but how should I pass the structure as a pointer? Pushing everything on the stack and pass the ESP register doesn't seem like a great idea...

Code: Select all

s = itoa((int)va_arg(list, int), s, 10);

When you convert an integer to a string, where are you storing that string?

I use the s parameter. You can control in the GitHub project anyways.

[nullplan]

I can understand it, but how should I pass the structure as a pointer? Pushing everything on the stack and pass the ESP register doesn't seem like a great idea...

Why not? Though, a more structured approach like GCC uses might be pertinent here. As in

Code: Select all

sub esp, 80 ; allocate stack space for the registers and the parameter slot
mov dword [esp+76], esi ;save the registers
mov dword [esp+72], edi
...
mov dword [esp+12], eax
lea eax, [esp+12]
mov dword [esp], eax ; put the parameter on stack
call func
; now restore the registers.
...
add esp, 80

I use the s parameter.

The question was meant to guide you to the solution here: What exactly is s before that line? Oh, it's uninitialized. So you mustn't read it. Yet, you do, when you pass it as a parameter to itoa(). Sooo... itoa() tries to print your number somewhere, likely nowhere mapped.

As a hint: a 32-bit number takes up at most 10 decimal digits. Add one for the sign and one for the null termination, and you need to reserve 12 bytes for the number. Or you use the eyeball rule: 3 bytes of output for each byte of input. It is almost never a problem to allocate a little too much memory.

[Thpertic]

What exactly is s before that line? Oh, it's uninitialized. So you mustn't read it. Yet, you do, when you pass it as a parameter to itoa(). Sooo... itoa() tries to print your number somewhere, likely nowhere mapped.

As a hint: a 32-bit number takes up at most 10 decimal digits. Add one for the sign and one for the null termination, and you need to reserve 12 bytes for the number. Or you use the eyeball rule: 3 bytes of output for each byte of input. It is almost never a problem to allocate a little too much memory.

I solved the printf just initializing the s parameter, I will take the eyeball rule as a life motto though.

Code: Select all

char *s = "";

The GCC-like approach doesn't work for me, unfortunately.

Code: Select all

sub esp, 52                 ; Allocate stack space for the registers and the parameter slot
    
    ; Push the registers and the segments on the stack
    mov dword [esp + 48], eax   
    mov dword [esp + 44], ecx
    mov dword [esp + 40], edx
    mov dword [esp + 36], ebx
    mov dword [esp + 32], esp
    mov dword [esp + 28], ebp
    mov dword [esp + 24], esi
    mov dword [esp + 20], edi

    mov [esp + 16], ds
    mov [esp + 12], es
    mov [esp + 8], fs
    mov [esp + 4], gs

    lea eax, [esp + 4]
    mov dword [esp], eax        ; Put the parameter on the stack

    call irq_faultHandler

    ; Restore the stack
    mov gs, [esp + 4]
    mov fs, [esp + 8]
    mov es, [esp + 12]
    mov ds, [esp + 16]

    mov dword edi, [esp + 20]
    mov dword esi, [esp + 24]
    mov dword ebp, [esp + 28]
    mov dword esp, [esp + 32]
    mov dword ebx, [esp + 36]
    mov dword edx, [esp + 40]
    mov dword ecx, [esp + 44]
    mov dword eax, [esp + 48]   

    add esp, 52                 
    iret

When the tick is called, at first it is ok, but immediately after it an exception comes up: General protection fault.
I feel like is something with the pushing of the segments.

[nullplan]

Code: Select all

char *s = "";

Now you're making itoa() write into a string literal, which is undefined behavior. This only works because you currently have no write protection for your kernel, seeing as no paging is enabled yet. However, you are overwriting other data in the .rodata section, so other strings (and FP literals, but you likely don't have those). What I meant was

Code: Select all

char buf[sizeof (int) * 3 + 2]; /* 1 for the sign, 1 for the null terminator */
char *s = buf;

Now, itoa() will write on the stack an all will be fine.

As for the other problem: After saving the segments, do you load them with your kernel segments? I keep forgetting that, since I work in 64-bit mode where segmentation is mostly irrelevant. But yes, in 32-bit mode, you can only count on CS and SS being loaded by the CPU, and you have to load the other segments yourself.

[Thpertic]

Code: Select all

char buf[sizeof (int) * 3 + 2]; /* 1 for the sign, 1 for the null terminator */
char *s = buf;

Thanks. I kinda understood that before too, I just saw it working and without thinking I gave it for good.

I tried pushing everything to the stack, set the kernel's segments and pushing esp. It works without exceptions.
Thank you for the help.

Code: Select all

; Push the registers and the segments on the stack
    pusha

    push ds
    push es
    push fs
    push gs
    
    ; Load the segments with the kernel's segments 
    mov ax, 0x10
    mov ds, ax
    mov es, ax
    mov fs, ax
    mov gs, ax
    
    push esp
    
    call irq_faultHandler
    
    ; Restore the stack
    pop esp
    pop gs
    pop fs
    pop es
    pop ds
    
    popa
    
    add esp, 8
    iret

[Octocontrabass]

Code: Select all

    ; Restore the stack
    pop esp

Use some other register here, not ESP. There's no guarantee the value you passed earlier was preserved by the function you call.