Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30500 )
Change subject: arch/x86/postcar: Add x86_64 support ......................................................................
Patch Set 25:
(20 comments)
https://review.coreboot.org/c/coreboot/+/30500/1/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/30500/1/src/arch/x86/exit_car.S@110 PS1, Line 110: pop %rbx
yes 64bit is fine, it holds both the maximum variable mtrrs and the count of mtrrs to pop from stack
Done
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/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.
Done
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/exit_car.S@38 PS3, Line 38: #endif
We are not consistent about this, perhaps . […]
Done
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/exit_car.S@89 PS3, Line 89: and $(~0xf), %sp
%rsp ?
Done
https://review.coreboot.org/c/coreboot/+/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. […]
Done
https://review.coreboot.org/c/coreboot/+/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.
Done
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/exit_car.S@120 PS3, Line 120: inc %rcx
%ecx ?
Done
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/exit_car.S@143 PS3, Line 143: dec %rbx
%ecx %ebx
Done
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/exit_car.S@151 PS3, Line 151: or $(MTRR_DEF_TYPE_EN), %rax
%eax and %ecx
Done
https://review.coreboot.org/c/coreboot/+/30500/4/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/30500/4/src/arch/x86/exit_car.S@32 PS4, Line 32: .macro pop_mtrr
Maybe pop_edx_eax ?
Done
https://review.coreboot.org/c/coreboot/+/30500/4/src/arch/x86/exit_car.S@50 PS4, Line 50: /* Migrate GDT to this text segment */
Keep empty line above.
Done
https://review.coreboot.org/c/coreboot/+/30500/4/src/arch/x86/exit_car.S@62 PS4, Line 62: mov %eax, %cr0
IA manual also says MOV r64, CR0-CR7 here. […]
Done
https://review.coreboot.org/c/coreboot/+/30500/4/src/arch/x86/exit_car.S@118 PS4, Line 118: pop %ebx /* Number to clear */
Something like this might do it, if we want to: […]
Done
https://review.coreboot.org/c/coreboot/+/30500/4/src/arch/x86/exit_car.S@165 PS4, Line 165: /* Align stack to 16 bytes at call instruction. */
Keep empty line?
Done
https://review.coreboot.org/c/coreboot/+/30500/24/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/30500/24/src/arch/x86/exit_car.S@75 PS24, Line 75: mov %cr0, %rax : and $(~(CR0_CD | CR0_NW)), %eax : mov %rax, %cr0 : #else : mov %cr0, %eax : and $(~(CR0_CD | CR0_NW)), %eax : mov %eax, %cr0
maybe 'call enable_cache' ? Not sure how that works with static inlines.
Doesn't work as it's static
https://review.coreboot.org/c/coreboot/+/30500/8/src/arch/x86/exit_car_x86_6... File src/arch/x86/exit_car_x86_64.S:
https://review.coreboot.org/c/coreboot/+/30500/8/src/arch/x86/exit_car_x86_6... PS8, Line 70: * 0x00: Number of variable MTRRs to clear
You changed last two items to size_t so they would use 8 bytes each.
Done
https://review.coreboot.org/c/coreboot/+/30500/8/src/arch/x86/exit_car_x86_6... PS8, Line 84: pop %rbx /* Number to clear, Number to set */
The comment is wrong?
Done
https://review.coreboot.org/c/coreboot/+/30500/8/src/arch/x86/exit_car_x86_6... PS8, Line 100: shr %rbx /* Number to set. */
Should this be 'pop %rbx'?
Done
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/gdt_init.S File src/arch/x86/gdt_init.S:
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/gdt_init.S@40 PS3, Line 40: .align 4
It's the same align as it was for x86_32.
Done
https://review.coreboot.org/c/coreboot/+/30500/3/src/arch/x86/gdt_init.S@42 PS3, Line 42: /* selgdt 0, unused */
Well.. removal of the hack happens regardless of x86_64 and can be done separately.
Done
-
Patrick Rudolph (Code Review)