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 7:
(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 if it's arch-agnostic, it shouldn't be documented here
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? why would we want this in postcar?
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.
Document in the commit message, why BSD(?) is used?
Unlikely to apply to an interface anyway...
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 *`? Having special cases for low/high mem seems arch specific and makes it harder to guess the holes and patch things together.
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? or in regular protected mode?
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 not used in coreboot currently, but some platforms have some SMM-only accessible crap here).
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?
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@56 PS7, Line 56: BM_MEM_RESERVED); and BSS? and stack?
I would prefer that we only use this in romstage and don't care.
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.
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?
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@78 PS7, Line 78: if ((uintptr_t)cbmem_top() >= 4ULL * GiB) { The cast takes precedence, so in 32-bit it flow over already before the comparison. Use `uint64_t`?
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?
Or a rather alternative approach: Use a constant for `scratch2`, e.g. 0x200000. And search for the first RAM range above 0x400000, reserve that for `scratch` and clear later. That would avoid to reason about CBMEM and negative offsets and would already check that the space for `scratch` is actually useable.
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.
-
Nico Huber (Code Review)