[M] Change in coreboot[main]: libpayload/x86: Extend exception handling to x86_64 architecture

[Julius Werner (Code Review)]

Attention is currently required from: Paul Menzel, Subrata Banik.

Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/83036?usp=email )

Change subject: libpayload/x86: Extend exception handling to x86_64 architecture
......................................................................

Patch Set 4:

(7 comments)

File payloads/libpayload/arch/x86/exception_asm_64.S:

https://review.coreboot.org/c/coreboot/+/83036/comment/484ace7d_d09e8e84?usp... :
PS4, Line 121: 	 * 16(%rsp) rflags
Is this actually true? I've read up a bit more on this and I don't think it's that simple for 64-bit mode. See section 6.14 (EXCEPTION AND INTERRUPT HANDLING IN 64-BIT MODE) in Intel's manual, e.g:
> The stack pointer (SS:RSP) is pushed unconditionally on interrupts. In legacy modes, this push is conditional and based on a change in current privilege level (CPL).

So I believe there's actually another
```
24(%rsp) old_rsp
32(%rsp) ss
```
on top of this, and we can't just assume that `rsp + 24` equals `old_rsp` here (because the processor aligns the stack pointer back to 16 bytes), we actually have to copy `old_rsp` (and `ss`) off that new stack. You'll also need to put the new `rsp` and `ss` (in case GDB changed them) back on this little stack for `iretq` to pick it up.

You could also try setting up an IST (see section 6.14.5 (Interrupt Stack Table)) to automatically start the new stack at `exception_stack_end`. I think this might be beneficial in rare cases where we get an exception because the code corrupted its stack pointer (e.g. set it to 0), because otherwise we'll get a double (and eventually triple) fault.

https://review.coreboot.org/c/coreboot/+/83036/comment/2acc3732_5f1ad128?usp... :
PS4, Line 142: 	push %rcx       /* gs */
Rather than the shifting let's just do
```
mov %gs, %rcx
movl %ecx, (%rsp)
mov %fs, %rcx
movl %ecx, -4(%rsp)
movl $0, -8(%rsp)
movl $0, -12(%rsp)
movl $0, -16(%rsp)
movq 8(rax), %rcx
movl %ecx, -20(%rsp)
sub $24, %rsp
```
?

Also, aren't we overwriting `%ecx` before we saved it here? I think you may need to introduce an `old_ecx` variable to temporarily stash it if you want to have it available as a scratch register here. (Or use `rax` here instead, and then later reload `rax` from `old_rsp`. You could delay the `add $24, old_rsp` until after that point to avoid having to subtract it back out again.)

Alternatively, if you use the IST then you don't need to indirectly access things via `old_rsp`/`%eax` at all, because then the automatically pushed values will be on the same stack that you're still using here. you can just access them directly via `offset(%rsp)`.

https://review.coreboot.org/c/coreboot/+/83036/comment/9a3e6b2f_b722d798?usp... :
PS4, Line 150: 	sub $12, %rsp   /* skip `es`, `ds` and `ss` */
It would be better to push zeroes to make sure the values are not undefined.

https://review.coreboot.org/c/coreboot/+/83036/comment/07e16ad8_1c73833b?usp... :
PS4, Line 183: 	call exception_dispatch
We need to be careful here that `%rsp` is actually 16-byte aligned at the function boundary, as demanded by the ABI. Right now, if I counted right, the `struct exception_state` is 23 * 8 bytes long, so if that's the only thing on the exception stack we'd be misaligned here. We could fix that by just pushing another `$0` before the structure.

Alternatively, if you install `exception_stack_end` in an IST as suggested above, you'll get the additional 5 values pushed automatically by the processor on top of that. So then you'd be aligned again automatically.

https://review.coreboot.org/c/coreboot/+/83036/comment/6cac4482_3302bc01?usp... :
PS4, Line 211: 	pop 8(%rax)
I'm not sure this is safe. Since pop always writes 8 bytes you're essentially writing `%fs` into the top 4 bytes of the space on the little processor-defined stack that only expects `%cs` there. The manual doesn't really define if that region is ignored or preserved or whatever, so it's probably better to not overwrite it with garbage.

You should be able to write this as
```
movl (%rsp), %eax
movl %eax, old_rsp + 8
movl 16(%rsp), %eax
mov %rax, %fs
movl 20(%rsp), %eax
mov %rax, %gs
```

https://review.coreboot.org/c/coreboot/+/83036/comment/c1f2997d_a485a200?usp... :
PS4, Line 264: 	.long \target
Actually, why don't we just make the last two here `.long 0`...

https://review.coreboot.org/c/coreboot/+/83036/comment/31974d3c_ae7097e8?usp... :
PS4, Line 334: 	mov $0x0, 8(%rax)        /* Target code segment offset [32:64] */
...and then we don't need this `mov` at all?