Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31550 )
Change subject: arch/x86: Introduce helper to clear memory using PAE ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/#/c/31550/7/Documentation/arch/x86/pae.md File Documentation/arch/x86/pae.md:
https://review.coreboot.org/#/c/31550/7/Documentation/arch/x86/pae.md@13 PS7, Line 13: arch
if it's arch-agnostic, it shouldn't be documented here
where should it be documented instead?
https://review.coreboot.org/#/c/31550/7/Documentation/arch/x86/pae.md@17 PS7, Line 17: 4GiB
Space after 4?
Done
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c File src/arch/x86/memory_clear.c:
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@48 PS7, Line 48: causes exception
This should be fixed. What did you try? clearing in PAE mode? […]
found the problem: CBMEM_FSP_HOB_PTR residing in the first page
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@50 PS7, Line 50: /* Hole at 0xa0000 */
This hole may contain RAM. Even security critical RAM (probably […]
Needs to be fixed by implemented an API that returns memory ranges based on PAMx registers.
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@51 PS7, Line 51: memranges_insert(&mem, 0xf0000, 0x10000, BM_MEM_RAM);
Isn't this usually (flash) ROM?
not in coreboot
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@56 PS7, Line 56: BM_MEM_RESERVED);
and BSS? and stack? […]
looking at it _programm and _stack are useless on FSP1.0 platforms, as FSP relocates into DRAM after FSP-M.
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@61 PS7, Line 61: memranges_each_entry(r, mem_soc) { : memranges_insert(&mem, range_entry_base(r), range_entry_size(r), : range_entry_tag(r)); : } :
Could also use memranges_clone() instead from the beginning.
memranges_clone works only in ramstage
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@67 PS7, Line 67: * Reserve CBMEM - Used as scratch memory for memcpy_pae.
If we were in postcar, CBMEM would already be initialized?
Correct, removed postcar
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@82 PS7, Line 82: intptr_t start = (uintptr_t)cbmem_top() - cbmem_size; : void *scratch2 = (void *)ALIGN_UP(start, 2 * MiB); : start = (uintptr_t)scratch2 + 2 * MiB; : void *scratch = (void *)ALIGN_UP(start, 4 * KiB);
This is not easy to follow. Maybe draw some ascii art? […]
reworked the logic using an additional reserved range for page tables.