Nico Huber 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:
(13 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
where should it be documented instead?
It's an API. I'd suggest the header file.
https://review.coreboot.org/#/c/31550/7/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/#/c/31550/7/src/arch/x86/Makefile.inc@281 PS7, Line 281: postcar-$(CONFIG_PLATFORM_HAS_DRAM_CLEAR) += memory_clear.c
postcar has global variables in RAM, doesn't it? […]
Done
https://review.coreboot.org/#/c/31550/7/src/arch/x86/include/arch/memory_cle... File src/arch/x86/include/arch/memory_clear.h:
https://review.coreboot.org/#/c/31550/7/src/arch/x86/include/arch/memory_cle... PS7, Line 2: * This file is part of the coreboot project. : * : * Copyright (C) 2019 9elements Agency GmbH : * Copyright (C) 2019 Facebook Inc. : * : * Redistribution and use in source and binary forms, with or without : * modification, are permitted provided that the following conditions : * are met: : * 1. Redistributions of source code must retain the above copyright : * notice, this list of conditions and the following disclaimer. : * 2. Redistributions in binary form must reproduce the above copyright : * notice, this list of conditions and the following disclaimer in the : * documentation and/or other materials provided with the distribution. : * 3. The name of the author may not be used to endorse or promote products : * derived from this software without specific prior written permission. : * : * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND : * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE : * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE : * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE : * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL : * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS : * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) : * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT : * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY : * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF : * SUCH DAMAGE.
Unlikely to apply to an interface anyway...
Done
https://review.coreboot.org/#/c/31550/7/src/arch/x86/include/arch/memory_cle... PS7, Line 29: *
Remove this line?
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@38 PS7, Line 38: const struct memranges *mem_soc)
Why not make it simply a pre-filled `struct memranges *`? […]
Done
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)); : } :
memranges_clone works only in ramstage
Ack
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.
Correct, removed postcar
Done
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);
reworked the logic using an additional reserved range for page tables.
Done
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@85 PS7, Line 85: void *scratch = (void *)ALIGN_UP(start, 4 * KiB);
NB. Weaker alignment makes this a no-op.
Done
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?
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?
Hmmm, in case, for the alignments too.
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?
https://review.coreboot.org/#/c/31550/8/src/arch/x86/memory_clear.c@100 PS8, Line 100: /* fastpath */ Is it really faster?