[L] Change in coreboot[main]: libpayload: Add x86_64 (64-bit) support

[Subrata Banik (Code Review)]

Attention is currently required from: Arthur Heymans, Julius Werner, Kapil Porwal.

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

Change subject: libpayload: Add x86_64 (64-bit) support
......................................................................

Patch Set 74:

(9 comments)

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

https://review.coreboot.org/c/coreboot/+/81968/comment/22ba63cc_14e901f9?usp... :
PS74, Line 38:  * Payload Entry Point Behavior with below code.
> This table is okay but I think there should also be a more explicit warning that this code needs to  […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/81968/comment/91db6ef5_3d576609?usp... :
PS74, Line 41: LP_ARCH_X86_64
> This column seems pointless.

Acknowledged

https://review.coreboot.org/c/coreboot/+/81968/comment/6abd2a75_c3ac8662?usp... :
PS74, Line 121:
> What about `fninit`?

initially I thought we don't need this code because modern 64-bit processors automatically initialize the FPU and configure the necessary control bits upon entering 64-bit mode. However, this code now may executed while transitioning from either a 32-bit coreboot and 64-bit coreboot hence, make sense to keep those logic here as well

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

https://review.coreboot.org/c/coreboot/+/81968/comment/773a4bd2_796c691d?usp... :
PS74, Line 51: .global pde_table
> I think these names are problematic now because you're using `pde_table` for the PDE table in the 2M […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/81968/comment/ab401341_931ad161?usp... :
PS74, Line 65: .type init_page_table, @function
> Should be a similar warning here, I'd suggest: […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/81968/comment/43f48856_c1e77602?usp... :
PS74, Line 75: 	mov $(_PRES + _RW + _US + _A + _D), %eax
> I still don't understand why you're not setting `_PS` here tbh. Am I misunderstanding how this works? Table 4-17 "Format of an IA-32e Page-Directory Entry that Maps a 2-MByte Page" says "7 (PS) |  Page size; must be 1 (otherwise, this entry references a page table; see Table 4-18)".

you are right, _PS is needed. I thought I added that but looking at upstream, it might have lost in patch trail
> Did you test this code (commenting out the `jnz` above to force it to run) and confirm that it works this way?

yes, i have tested this flow as well where 1GB paging is not supported

https://review.coreboot.org/c/coreboot/+/81968/comment/5c8c1773_f9f2d200?usp... :
PS74, Line 82: 	mov %ebx, 4(%rsi, %rcx, 8)
> Actually, the same argument as below also applies here: we're only writing 2MB tables for the low 4G […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/81968/comment/4b483ecc_6966366a?usp... :
PS74, Line 104: 	cmp %edx, %eax
> Sorry, I'm very confused about what you're doing here and what you're using EDX for (instead of EAX like with the other versions of this loop). What's the point of adding EDX to EAX and then comparing with EAX? You're not initializing EAX anywhere so it would still point to the end of the PDE table from the previous loop (and then you're adding more to it)?
>
> In general, the point of this overflow check is just to see if we need to increment EBX because EAX overflowed over the 4GB limit. In this case, since we know that EAX just walks the PDE table and libpayload (including the entire PDE table in BSS) is linked below 4GB, we can actually skip that entire check because we know that it will never cross that boundary (and EBX will never need to be anything other than 0). So in fact I think we can simplify this entire thing to:
> mov $4, %edi
> mov $(_PRES + _RW + _US + _A + pde_table), %eax
> mov $0, %ebx
> mov $0, %ecx
> mov $pdpe_table, %esi
>
> fill_pdpe:
> mov %eax, (%rsi, %rcx, 8)
> mov $0, 4(%rsi, rcx, 8)
> add $4096, %eax
> inc %ecx
> cmp %edi, %ecx
> jb fill_pdpe
>
> (I don't know if the linker supports a relocation for a construct like `mov $(_PRES + _RW + _US + _A + pde_table), %eax`. If not, then just do `mov $pde_table, %eax` and `add $(_PRES + _RW + _US + _A), %eax`.)

you are right and if u follow the previous patch set, I had added that initially then may be while adding 64-bit API, I got confused that I had to handle the overflow but true this is <4GB table creation.

https://review.coreboot.org/c/coreboot/+/81968/comment/55d91030_02bf4b90?usp... :
PS74, Line 116: 	jmp leave
> nit: Easier to just do `ret` here?

Acknowledged