Issues with interrupts

[Octocontrabass]

It always points to the last-pushed item.

Since the stack grows downwards, the last item you push is the first item of your struct. A pointer to a struct is equal to a pointer to the first item in the struct...

[Octocontrabass]

It seems like it happens at the "iretq" instruction.

What is the purpose of this instruction? Is 8 bytes the correct amount?

[nullplan]

Oh dear lord, so many comments...

On x86_64 (as well as all other architectures I have ever earnestly looked at) stack grows downward. Meaning that subtracting from the stack pointer allocates memory on stack, and adding to it frees it. On x86_64 in particular, the instruction "push <number>" pushes eight bytes. Your code pushes two numbers on stack before everything else, so the counter instruction must add sixteen to RSP. Also, since stack grows downward, the address of the start of the structure is the value of RSP after pushing the structure. Also, you can push segment registers directly, you don't need to use a temporary register first. And technically you need to observe stack alignment when calling C code.

So put it all together and your stub ought to be something like

Code: Select all

ISRCommonStub:
  cld
  PUSHALL
  pushq %ds
  movq %rsp, %rdi
  movq %rsp, %r12  /* save unaligned stack pointer in non-volatile register. */
  andq $-16, %rsp
  movl $0x10, %eax
  movl %eax, %ds
  movl %eax, %es
  movl %eax, %fs
  movl %eax, %gs

  call ISRHandler

  movq %r12, %rsp
  popq %rax
  movl %eax, %ds
  movl %eax, %es
  movl %eax, %fs
  movl %eax, %gs
  POPALL
  addq $16, %rsp
  iretq

However, then your problems go on in your C code. What argument type does your ISRHandler function take, and what argument type are you giving it here? That really ought to have been a pointer to the structure. The difference is, if you give the structure itself to the function, not only does the stack alignment not work that way, the registers also become local variable space for the function, and the function is under no obligation to keep these values the same.

I have seen that code before. Have you been learning from a bad tutorial?

[Techflash]

Oh dear lord, so many comments...

I like to keep my code commented for both myself and anyone else looking at it, especially ASM. Even for me it's pretty tricky to just read it and understand what's going on. For any beginners I can't even image where they would start. That's why I put so many comments.

On x86_64 (as well as all other architectures I have ever earnestly looked at) stack grows downward. Meaning that subtracting from the stack pointer allocates memory on stack, and adding to it frees it.

I've known that for a long time, it's just hard to wrap my head around the logic of it being in a kind of reverse order like that.

On x86_64 in particular, the instruction "push <number>" pushes eight bytes. Your code pushes two numbers on stack before everything else, so the counter instruction must add sixteen to RSP.

Ah. I really should have caught that. 64 bits / 8 bits per byte is 8 bytes. There's really no excuse for not seeing that .

Also, since stack grows downward, the address of the start of the structure is the value of RSP after pushing the structure.

Oh. I thought it needed to be at the topmost part of the strut in the stack.

Also, you can push segment registers directly, you don't need to use a temporary register first.

I never actually tried to push ds (and idk why I move ds into ax there) No. You can't:

Code: Select all

hardware/CPU/interrupts/ISRASM.S: Assembler messages:
hardware/CPU/interrupts/ISRASM.S:98: Error: you can't `push %ds'

And technically you need to observe stack alignment when calling C code.

True, didn't think of that.

However, then your problems go on in your C code. What argument type does your ISRHandler function take, and what argument type are you giving it here? That really ought to have been a pointer to the structure. The difference is, if you give the structure itself to the function, not only does the stack alignment not work that way, the registers also become local variable space for the function, and the function is under no obligation to keep these values the same.

I have modified the C code to use pointers for the struct. See this commit.

I have seen that code before. Have you been learning from a bad tutorial?

Possible. I think it's kind of a mishmash of different tutorials.

[nullplan]

I like to keep my code commented for both myself and anyone else looking at it,[...]

Not what I meant, and that is generally good practice. I meant there are so many things here I have to comment on.

No. You can't:

Oh bloody hell, why did they do that? I just looked it up: https://www.felixcloutier.com/x86/push
You can directly push all segments in compat and legacy mode, but not in 64-bit mode. In 64-bit mode you can directly push FS and GS, but that is of limited utility, seeing as their base address comes from an MSR.

However, the other segments are mostly ignored, anyway. So much so I didn't even think of them when writing my interrupt code. CS is important because it sets the CPL, but CS is changed with every instruction that transitions to kernel space. There is SS, but the only important thing about it is that it not be 0. Indeed all segments are ignored except for the "Present" bit. So personally I don't even care about the data segments.

[Techflash]

I like to keep my code commented for both myself and anyone else looking at it,[...]

Not what I meant, and that is generally good practice. I meant there are so many things here I have to comment on.

No. You can't:

Oh bloody hell, why did they do that? I just looked it up: https://www.felixcloutier.com/x86/push
You can directly push all segments in compat and legacy mode, but not in 64-bit mode. In 64-bit mode you can directly push FS and GS, but that is of limited utility, seeing as their base address comes from an MSR.

However, the other segments are mostly ignored, anyway. So much so I didn't even think of them when writing my interrupt code. CS is important because it sets the CPL, but CS is changed with every instruction that transitions to kernel space. There is SS, but the only important thing about it is that it not be 0. Indeed all segments are ignored except for the "Present" bit. So personally I don't even care about the data segments.

Alright then, if it's not possible, for now I'll just push a 0 on to the stack in that spot and I can worry about making it work later.

UPDATE: After trying that, I get a bunch of "access_read_linear(): canonical failure" and a triple fault in bochs. Is it possible that maybe it's some of my IRQ code that's borked now? Since that has a similar but different "IRQCommonStub" that hasn't been messed with in a while.

[Demindiro]

Why are you even bothering trying to save the segment registers (other than SS and CS, which are saved automatically anyways) in long mode?. They have no use whatsoever and can't even be set from userspace.

Feel free to try for yourself (compile with `gcc -nostartfiles` on Linux):

Code: Select all

.intel_syntax noprefix
.globl _start

_start:

	mov ax, 0x10
	#mov ds, ax
	mov fs, ax

	# exit
	mov eax, 60
	xor edi, edi
	syscall
	ud2 # just in case

[Octocontrabass]

Is it possible that maybe it's some of my IRQ code that's borked now? Since that has a similar but different "IRQCommonStub" that hasn't been messed with in a while.

You should delete that whole file and use the same stubs for all 256 ISRs. It doesn't make sense to have different code to dispatch legacy PIC IRQs when you already have generic ISR dispatching.

But if the ISR stub you've installed for the first 32 ISRs works correctly now, that means you can write your exception handlers and finally do something other than triple fault when an exception occurs. Just having your handlers hang the CPU is a good start - you can use your VM's debugger to examine the CPU state for now.

[Techflash]

Is it possible that maybe it's some of my IRQ code that's borked now? Since that has a similar but different "IRQCommonStub" that hasn't been messed with in a while.

You should delete that whole file and use the same stubs for all 256 ISRs. It doesn't make sense to have different code to dispatch legacy PIC IRQs when you already have generic ISR dispatching.

But if the ISR stub you've installed for the first 32 ISRs works correctly now, that means you can write your exception handlers and finally do something other than triple fault when an exception occurs. Just having your handlers hang the CPU is a good start - you can use your VM's debugger to examine the CPU state for now.

So you mean just fill every ISR slots with "ISRStub" and remove everything related to IRQ?

[Techflash]

Why are you even bothering trying to save the segment registers (other than SS and CS, which are saved automatically anyways) in long mode?. They have no use whatsoever and can't even be set from userspace.

It's a part of the struct. What should I do there? A. Remove it from the struct and the code or B. Push the expected value

[Octocontrabass]

So you mean just fill every ISR slots with "ISRStub" and remove everything related to IRQ?

Create 256 ISR stubs, one for each IDT entry, and remove the IRQ stuff. Your ISRHandler() function will call the handler for each ISR, and the handler will know whether the ISR is for an IRQ.

...But you don't need to do that until after you have working exception handlers.

It's a part of the struct. What should I do there? A. Remove it from the struct and the code or B. Push the expected value

You don't need that value and you're not going to use it, so you should remove it from the struct.

[Techflash]

So you mean just fill every ISR slots with "ISRStub" and remove everything related to IRQ?

Create 256 ISR stubs, one for each IDT entry, and remove the IRQ stuff. Your ISRHandler() function will call the handler for each ISR, and the handler will know whether the ISR is for an IRQ.

...But you don't need to do that until after you have working exception handlers.

It's a part of the struct. What should I do there? A. Remove it from the struct and the code or B. Push the expected value

You don't need that value and you're not going to use it, so you should remove it from the struct.

Ok. Why can't I just use the same 2 stubs for each ISR? One for interrupts that push an error code, and one for interrupts that don't? Wouldn't 256 different versions of the same code (for now at least) be kinda wasteful?
Also for exception handlers, for now I'll probably just have them call "panic()" with a predefined string.

[Octocontrabass]

Why can't I just use the same 2 stubs for each ISR?

You have 32 stubs. Are you talking about the two macros? Yes, you can use the same two macros for all 256 stubs. (All stubs above 31 will use the ISR_NOERR macro, so you can wrap that in another macro to generate the rest.)

Also for exception handlers, for now I'll probably just have them call "panic()" with a predefined string.

That's a good start. Once you have that working, you can print more information, such as the contents of your registers struct.

[Techflash]

Why can't I just use the same 2 stubs for each ISR?

You have 32 stubs. Are you talking about the two macros? Yes, you can use the same two macros for all 256 stubs. (All stubs above 31 will use the ISR_NOERR macro, so you can wrap that in another macro to generate the rest.)

Also for exception handlers, for now I'll probably just have them call "panic()" with a predefined string.

That's a good start. Once you have that working, you can print more information, such as the contents of your registers struct.

No, I was talking about using the same 2 functions for the slots of the IDT. Every slot would point to either a function "ISRStubNoErr" or "ISRStubErr" depending on whether or not there was an error code pushed. Would that not work? That's how I have the non macro-populated parts IDT set up now.

[Octocontrabass]

That won't work for exceptions. You need to know which IDT entry the CPU used in order to tell which exception occurred.

It might work for IRQs, but it'll be a lot slower than looking up the interrupt number in your registers struct.