Attention is currently required from: Arthur Heymans, Kapil Porwal, Subrata Banik.
Julius Werner 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 68:
(5 comments)
File payloads/libpayload/arch/x86/head_64.S:
https://review.coreboot.org/c/coreboot/+/81968/comment/921c370b_cd0a8907?usp... : PS64, Line 100: movq %rax, %cr3 Which section are you quoting? I'm looking at 4.10.4.1 "Operations that Invalidate TLBs and Paging-Structure Caches":
• MOV to CR3. The behavior of the instruction depends on the value of CR4.PCIDE:
— If CR4.PCIDE = 0, the instruction invalidates all TLB entries associated with PCID 000H except those for global pages. It also invalidates all entries in all paging-structure caches associated with PCID 000H.
Doesn't say anything about it making a difference what value was in there before?
https://review.coreboot.org/c/coreboot/+/81968/comment/029ec02b_f1251b94?usp... : PS64, Line 118:
Do we not need the `fninit` and `OSFXSR, OSXMMEXCPT` stuff in 64-bit mode? […]
Remember that this payload can be booted from either 32-bit or 64-bit coreboot, so if 64-bit coreboot does something we cannot rely on that. In general, this handoff API seems to be written such that we should try to not rely on stuff as much as possible (e.g. a non-coreboot firmware could try to boot this through the multiboot API). So if 32-bit libpayload used to turn these on explicitly, we should probably continue to do that. (I don't think we need the CPUID checks if we're sure that there are no 64-bit CPUs that don't support those features, though, we can just turn them on unconditionally.)
File payloads/libpayload/arch/x86/pt.S:
https://review.coreboot.org/c/coreboot/+/81968/comment/2247c1c4_cd37c1c9?usp... : PS64, Line 71: +
Doesn't this still need a _PS bit? Otherwise, it points to a 4KB page table. […]
_PS is (according to my understanding from quickly looking through the manual, so let me know if I got it wrong) the bit that tells it at each level whether this is the final (leaf) level and pointing directly to the physical page frame address, or this is a directory that points to another lower level of page tables. So for 1GB paging, 1GB is the lowest level of page tables we have and we have to set the _PS bit at the 1GB level (PDPTE). For the 2MB case, we must *not* set _PS at the PDPTE level (because there's still the 2MB level below it), but we *must* set it at the PDE level (because that's the final (leaf) level, there's no 4KB level below it).
https://review.coreboot.org/c/coreboot/+/81968/comment/3a106a32_cd4e4343?usp... : PS64, Line 82: incl %ebx
These may be the other reason... […]
Can you tell which instruction gives you an exception when you try that?
File payloads/libpayload/include/x86/arch/exception.h:
https://review.coreboot.org/c/coreboot/+/81968/comment/b046ea52_eaaa765b?usp... : PS64, Line 74: #else
I think it would be good to write this as a single struct definition where you only have an `i […]
Sorry, no, you were absolutely right earlier, I didn't pay close enough attention to the order. The order is important and needs to be the same that GDB uses, so you can't change the order for the 32-bit structure. Please revert to the state where you had two separate struct definitions.
-
Julius Werner (Code Review)