Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30500 )
Change subject: [WIP]arch/x86/postcar: Add x86_64 support ......................................................................
Patch Set 3:
(10 comments)
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S@27 PS3, Line 27: .quad 0 Is it u64 so just one quad? Or two long and no preprocessor here.
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S@38 PS3, Line 38: #endif We are not consistent about this, perhaps .code without indent?
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S@89 PS3, Line 89: and $(~0xf), %sp %rsp ?
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S@110 PS3, Line 110: pop %rbx I don't know what our preference is about using GAS macros. Popping 64bit from stack to edx.eax would be good candidate here. I believe this entire change would reduce to having that macro defined differently (using that shr $32, %rax) on __x86_64__.
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S@115 PS3, Line 115: mov $(MTRR_PHYS_BASE(0)), %rcx Why %rax instead of %eax here? Same for %ecx, %edx.
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S@120 PS3, Line 120: inc %rcx %ecx ?
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S@143 PS3, Line 143: dec %rbx %ecx %ebx
https://review.coreboot.org/#/c/30500/3/src/arch/x86/exit_car.S@151 PS3, Line 151: or $(MTRR_DEF_TYPE_EN), %rax %eax and %ecx
https://review.coreboot.org/#/c/30500/3/src/arch/x86/gdt_init.S File src/arch/x86/gdt_init.S:
https://review.coreboot.org/#/c/30500/3/src/arch/x86/gdt_init.S@40 PS3, Line 40: .align 4 Is this enough for x86_64?
https://review.coreboot.org/#/c/30500/3/src/arch/x86/gdt_init.S@42 PS3, Line 42: /* selgdt 0, unused */ Adding this is not x86_64 related change?
-
Kyösti Mälkki (Code Review)