OSDev.org

Hello,

I'm currently trying to implement a TSS for multitasking on my kernel. Here is my code:
Basically, I'm getting a General Protection Fault when the kernel reaches the ltr instruction in tss_flush.
Is my GDT entry wrong or could the problem come from somewhere else?

Note: I already loaded my GDT when calling this code

Thanks in advance

The type field of your TSS descriptor should be 0x9, not 0x6.

Change to

Crumble14 wrote:

Code: Select all

static tss_entry_t tss_entry;

__attribute__((cold))
static void tss_init(void)
{
	const uint32_t base = (uint32_t) &tss_entry;
	const uint64_t limit = sizeof(tss_entry_t);

Limit should be size minus one.

And there's no need for uint64_t.

Crumble14 wrote:

Code: Select all

	const uint8_t flags = 0x40;

Can't be 0x40. Most likely should be 0x10 or 0.

Crumble14 wrote:

Code: Select all

	const uint8_t access = 0x86;

As already noted, it must be 0x89.

Crumble14 wrote:

Code: Select all

	void *tss_gdt = tss_gdt_entry();
	bzero(tss_gdt, sizeof(uint64_t));
	*((uint16_t *) (tss_gdt)) = limit & 0xffff;

Why don't you use a proper C structure here?

Crumble14 wrote:

Code: Select all

	*((uint16_t *) (tss_gdt + 2)) = base & 0xffff;

Adding anything to a pointer to void should not compile in proper C. I bet you're using the compiler in a rather permissive mode and this may be hiding bugs.

Crumble14 wrote:

Code: Select all

	*((uint8_t *) (tss_gdt + 4)) = (base >> 16) & 0xff;
	*((uint8_t *) (tss_gdt + 5)) = access;
	*((uint8_t *) (tss_gdt + 6)) = ((limit >> 16) & 0xf) | flags;
	*((uint8_t *) (tss_gdt + 7)) = (base >> 24) & 0xff;

	bzero(&tss_entry, sizeof(tss_entry_t));
	tss_entry.ss0 = 0x10;
	asm volatile("mov %%esp, %0" : "=a"(tss_entry.esp0));

The SS#:ESP# pairs get loaded into SS:ESP when there's a privilege transition. In most/simplest cases it's when switching from ring/level 3 to ring/level 0 upon a hardware or software interrupt/exception, in which case SS:ESP gets loaded with SS0:ESP0 before the interrupt/exception handler starts. SS0:ESP0 should point to the memory used as stack for interrupt/exception handlers.
When already running in ring/level 0, there's no ring/level "-1" to switch to (and no SS"-1":ESP"-1") and the current SS:ESP continues to be used (and thus you may overflow your kernel stack).

IOW, those two lines are unneeded (if you intend to stay in ring/level 0) or wrong (if you intend to use e.g. ring/level 3).

Also, if you find yourself manipulating the stack in inline assembly, you must be doing something wrong. Reason: you don't know how the compiler generates code for stack management, what ESP and EBP point to. You don't know not because you didn't look at the disassembly. You don't know because there's no guarantee for the stack to have any particular layout in the middle of C code (ditto beginning/end). If you need to mess with the current stack, write a full assembly routine in a separate file (you'll normally do it for your entry points to interrupt/exception handlers and things similar to setjmp()/longjmp()).

Crumble14 wrote:

Code: Select all

	tss_flush();
}

Code: Select all

__attribute__((packed))
struct tss_entry
{
	uint32_t prev_tss;
	uint32_t esp0;
	uint32_t ss0;
	uint32_t esp1;
	uint32_t ss1;
	uint32_t esp2;
	uint32_t ss2;
	uint32_t cr3;
	uint32_t eip;
	uint32_t eflags;
	uint32_t eax;
	uint32_t ecx;
	uint32_t edx;
	uint32_t ebx;
	uint32_t esp;
	uint32_t ebp;
	uint32_t esi;
	uint32_t edi;
	uint32_t es;
	uint32_t cs;
	uint32_t ss;
	uint32_t ds;
	uint32_t fs;
	uint32_t gs;
	uint32_t ldt;
	uint16_t trap;
	uint16_t iomap_base;
};

typedef struct tss_entry tss_entry_t;

I'd rather not make the reserved fields part of the non-reserved ones. Set those reserved fields to 0 and don't access them afterwards. You're packing the structure anyway, so, you should be able to use more uint16_t's safely. Your structure should still retain at least a 4 byte alignment since it has 32-bit values in it.

Crumble14 wrote:

Code: Select all

.global tss_gdt_entry
.global tss_flush

tss_gdt_entry:
	mov gdt_tss, %eax

	ret

If gdt_tss is the label at the beginning of a structure, you probably want this:

Crumble14 wrote:

Code: Select all

tss_flush:
	mov TSS_OFFSET, %ax
	ltr %ax

	ret

Likewise, if TSS_OFFSET is the TSS selector (or the offset of the TSS descriptor from the beginning of the TSS), as it should be, you want this instead: If there's no $, the operand is considered to be in memory and so the mov instruction will read or write memory. If there is, the operand is just that number/address, no memory read/write will be involved.

EDIT: corrected the 0x40 part.

alexfru wrote: 0x40 will overwrite bits 19...16 of the segment limit below... What did you want to get with this 0x40?

The value of 0x40 in binary is 0100 0000, so doing a binary OR like I do just writes on the high nibble.

alexfru wrote: Why don't you use a proper C structure here?

Didn't thought about it at the moment, it's a good idea, thanks ^^

alexfru wrote: Adding anything to a pointer to void should not compile in proper C. I bet you're using the compiler in a rather permissive mode and this may be hiding bugs.

Well, I'm compiling with -Wall -Wextra -Werror and I'm not getting any errors. Should I have more flags?


Anyway, I fixed everything you pointed out and it seems to work now. Thank you very much :)

Crumble14 wrote: Well, I'm compiling with -Wall -Wextra -Werror and I'm not getting any errors. Should I have more flags?

-std=c99 to restrict to standard C w/o most (or all?) GNU extensions
-O2 (or -O3) will enable code analysis and make a number of warnings possible
-pedantic
Play with those.

Crumble14 wrote: Anyway, I fixed everything you pointed out and it seems to work now. Thank you very much

Cool!