Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31550 )
Change subject: security/memory: Clear memory in ramstage ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/#/c/31550/8/src/arch/x86/memory_clear.c File src/arch/x86/memory_clear.c:
https://review.coreboot.org/#/c/31550/8/src/arch/x86/memory_clear.c@56 PS8, Line 56: 20 * KiB
Maybe we should make this a constant as part of the memset_pae() interface?
Done
https://review.coreboot.org/#/c/31550/8/src/arch/x86/memory_clear.c@79 PS8, Line 79: 2 * MiB
This too, constant for the interface? […]
Done
https://review.coreboot.org/#/c/31550/8/src/arch/x86/memory_clear.c@73 PS8, Line 73: found = 0; : /* Find a spot for virtual memory address */ : memranges_each_entry(r, &mem) { : if (range_entry_tag(r) != BM_MEM_RAM) : continue; : : if (ALIGN_UP(range_entry_base(r) + 2 * MiB, 2 * MiB) + 2 * MiB > : range_entry_end(r)) : continue; : : vmem_addr = ALIGN_UP(range_entry_base(r) + 2 * MiB, 2 * MiB); : found = 1; : break; : } : : if (!found) { : printk(BIOS_ERR, "%s: Couldn't place vmem address\n", __func__); : return 1; : }
Move into a helper function?
Done
https://review.coreboot.org/#/c/31550/8/src/arch/x86/memory_clear.c@100 PS8, Line 100: /* fastpath */
Is it really faster?
Yes, as it doesn't need to generate page tables and set up paging.