Various bug fixes as well as a change to provide our own GDT. #4
Conversation
We can't include the host machines stdio.h header in our kernel code. It's not actually being used, so removing this doesn't break anything.
Create a simple Makefile to compile the kernel. 'kernel.asm' was changed to 'head.asm' to avoid naming conflict with 'kernel.c'. The Makefile generates .o files based on the filename, so 'kernel.asm' and 'kernel.c' would both compile to 'kernel.o'. A simple rename fixes the issue.
The bootloader will never call 'start' with interrupts enabled, so executing a cli here is redundent.
The 'hlt' instruction only halts the CPU until the next interrupt occurs. Beacuse of this, it's necessary to both use 'cli' before-hand to make-sure interrupts are off before we execute 'hlt', but also put the 'hlt' in a loop anyway to protect against nmi's, guarenteeing we don't continue.
C compilers are allowed to pad struct entries out, meaning that entry 2 may not come directly after entry 1, but instead empty space may be inbetween the two, for various reasons. gcc provides __attribute__((packed)) to guard against this, which when applied to a struct guarentees that the struct will not be compiled with any empty space. This is necessary for structs like IDT_entry, where they map to structure the hardware is going to expect in a very specefic setup with no padding.
GRUB documentation says that we can't rely on the GDT that GRUB supplies us, and that it may not be valid to use. Since our IDT requires specifying a selector, we need to have our own GDT so we can supply a valid selector for the interrupt handler.
Like the 'gdt_ptr' struct, the 'idt_ptr' struct is a structure defining the specefic layout x86 wants for the IDT description. Using the structure like this lets us avoid all the bitshifting that used to be required.
Right now, kmain does a busy loop while we wait for interrupts. It's much more effecient to put a hlt instruction inside of the loop, which pauses the CPU until the next interrupt is recieved. This still requires a loop because execution will continue after the hlt instruction once we return from an interrupt.
Currently, we don't back up any of the registers before calling the actual interrupt routine. This will become a problem because the interrupt routine is going to trash these values, and they won't be restored by 'iretd'. The simple solution is to push all the segments and then all the registers onto the stack, and then pop them right before we return from the interrupt.
Since building now uses 'make', reflect that in the README.md
|
Thanks for your contribution. The article and code is reduced to the bare minimum intentionally so that it's accessible for beginners, and is brief. I have resisted the temptation to add a Oh |
|
That's fair enough, a little later today I'll remove the Makefile, and change the names back. You will want to add -ffreestanding to the gcc line. See the multiboot spec here. Basically, while GRUB provides segments in a specefic configuration, it may also clear the GDTR register. When that's cleared, you can't reload any of the segment registers or else it will try to read the invalid GDTR - The solution is of course to propvide your own GDT as a replacement. You can also see the note in EFLAGS about IF always being cleared, so interrupts are always off. I think I may have a bug though - load_gdt should reload the segment registers as well I think. I'll look into that. |
|
Yes, it's anyway better to have a GDT to ourselves. I will try and incorporate the changes once I get the time to add the GDT part to the article. |
|
I have gone ahead and removed the |
| call keyboard_handler_main | ||
|
|
||
| popad | ||
| pop ds |
There was a problem hiding this comment.
I'd say you would be right. I'm guessing I never noticed this bug because all of those segments probably contain the same value everywhere in the program. but obviously it's still a bug and it would easily blow things up if/when features are added that make use of the segment registers (Such as using them for CPU-local data).
On that note it also appears the segments are also never assigned, so it's quite likely this code got lucky in other areas. A while back I had an issue stemming from something similar in my kernel, it's fixed here. The code in this PR doesn't reload the segments after the lgdt, but that's necessary to ensure they actually contain the correct values and also to ensure they start utilizing the newly defined segments.
|
FWIW, I think this is a really good PR @mkilgore as it adds several realistic features even the most barebones interrupt example requires. |
| idt_address = (unsigned long)IDT ; | ||
| idt_ptr[0] = (sizeof (struct IDT_entry) * IDT_SIZE) + ((idt_address & 0xffff) << 16); | ||
| idt_ptr[1] = idt_address >> 16 ; | ||
| idt_ptr.limit = sizeof(IDT); |
There was a problem hiding this comment.
Shouldn't this be sizeof(IDT) - 1 ?
There was a problem hiding this comment.
Yes I think you're right, but FWIW it shouldn't matter that much since nothing should read past the end of the IDT or GDT (and if they did, are likely to read more than one byte).
| struct gdt_ptr gdt_ptr; | ||
|
|
||
| gdt_ptr.base = (unsigned long)GDT; | ||
| gdt_ptr.limit = sizeof(GDT); |
There was a problem hiding this comment.
Shouldn't this be sizeof(GDT) - 1 ?
I would take a look at the commit list for better information on what all this pull request entails, each commit has a description describing it (Most of them are fairly small/simple fixes).
The bigger changes are the addition of a Makefile and the GDT. The Makefile just executes the same instructions as in the README.md - Though, I set CFLAGS to provide -ffreestanding, since this is kernel code.
For the GDT, it's extremely simple and just does a flat 4GB for the kernel code and data. The reason it's necessary to provide one is because GRUB doesn't guarentee that the GDT is actually usable when our kernel starts. Since interrupts cause the segment registers to be reloaded, the code would fault on an interrupt if we don't have a valid GDT. It's simple enough to provide one. It's worth noting, I modeled the GDT code off of my own kernel's GDT code, to make it easier. You can see it here, the file ./arch/x86/include/arch/gdt.h. I mostly just pulled the GDT_ENTRY macro, which makes creating GDT entries much easier.
The other small changes are removing the
stdio.hinclude (I suspect that was just left in accidentally), removing a redundentcliinstruction fromstart, fixing the end ofstartso that we guarentee execution will never continue beyond it, saving and restoring the registers in the keyboard interrupt handler, and make-sure the IDT and now GDT structures are packed by the compiler to guarentee we don't have any padding inside them.I'd be happy to separate out some of the commits if you'd prefer.