Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43014 )
Change subject: arch/x86: Remove RELOCATABLE_RAMSTAGE ......................................................................
arch/x86: Remove RELOCATABLE_RAMSTAGE
We always have it, no need to support opting-out.
Change-Id: I5cbf4063c69571db92de2d321c14d30c272e8098 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/Kconfig M src/arch/x86/Makefile.inc D src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/lib/prog_loaders.c M src/security/memory/Kconfig M src/security/memory/memory_clear.c 7 files changed, 5 insertions(+), 78 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/43014/1
diff --git a/src/Kconfig b/src/Kconfig index eb85cd9..a4c2fa6 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -270,20 +270,9 @@ say N because it adds a small performance penalty and may abort on code that happens to work in spite of the UB.
-config RELOCATABLE_RAMSTAGE - bool - default y if ARCH_X86 - select RELOCATABLE_MODULES - help - The reloctable ramstage support allows for the ramstage to be built - as a relocatable module. The stage loader can identify a place - out of the OS way so that copying memory is unnecessary during an S3 - wake. When selecting this option the romstage is responsible for - determing a stack location to use for loading the ramstage. - choice prompt "Stage Cache for ACPI S3 resume" - default NO_STAGE_CACHE if !HAVE_ACPI_RESUME || !RELOCATABLE_RAMSTAGE + default NO_STAGE_CACHE if !HAVE_ACPI_RESUME default TSEG_STAGE_CACHE if SMM_TSEG
config NO_STAGE_CACHE @@ -576,7 +565,6 @@ config HAVE_ACPI_RESUME bool default n - depends on RELOCATABLE_RAMSTAGE
config DISABLE_ACPI_HIBERNATE bool diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 1a1aa40..8f71c95 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -245,7 +245,6 @@ ramstage-y += ebda.c ramstage-y += exception.c ramstage-y += idt.S -ramstage-y += gdt.c ramstage-$(CONFIG_IOAPIC) += ioapic.c ramstage-y += memcpy.c ramstage-y += memmove.c @@ -294,14 +293,10 @@
ramstage-libs ?=
-ifeq ($(CONFIG_RELOCATABLE_RAMSTAGE),y) - # The rmodule_link definition creates an elf file with .rmod extension. $(objcbfs)/ramstage.elf: $(objcbfs)/ramstage.debug.rmod cp $< $@
-endif - $(objcbfs)/ramstage.debug: $(objgenerated)/ramstage.o $(call src-to-obj,ramstage,$(CONFIG_MEMLAYOUT_LD_FILE)) @printf " CC $(subst $(obj)/,,$(@))\n" $(LD_ramstage) $(CPPFLAGS) $(LDFLAGS_ramstage) -o $@ -L$(obj) $< -T $(call src-to-obj,ramstage,$(CONFIG_MEMLAYOUT_LD_FILE)) diff --git a/src/arch/x86/gdt.c b/src/arch/x86/gdt.c deleted file mode 100644 index 9c85566..0000000 --- a/src/arch/x86/gdt.c +++ /dev/null @@ -1,52 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <types.h> -#include <string.h> -#include <cbmem.h> -#include <commonlib/helpers.h> -#include <console/console.h> -#include <cpu/x86/gdt.h> - -/* i386 lgdt argument */ -struct gdtarg { - u16 limit; -#ifdef __x86_64__ - u64 base; -#else - u32 base; -#endif -} __packed; - -/* - * Copy GDT to new location and reload it. - * FIXME: We only do this for BSP CPU. - */ -static void move_gdt(int is_recovery) -{ - void *newgdt; - u16 num_gdt_bytes; - struct gdtarg gdtarg; - - /* ramstage is already in high memory. No need to use a new gdt. */ - if (CONFIG(RELOCATABLE_RAMSTAGE)) - return; - - newgdt = cbmem_find(CBMEM_ID_GDT); - num_gdt_bytes = (uintptr_t)&gdt_end - (uintptr_t)&gdt; - if (!newgdt) { - newgdt = cbmem_add(CBMEM_ID_GDT, ALIGN_UP(num_gdt_bytes, 512)); - if (!newgdt) { - printk(BIOS_ERR, "Error: Could not relocate GDT.\n"); - return; - } - memcpy((void *)newgdt, &gdt, num_gdt_bytes); - } - printk(BIOS_DEBUG, "Moving GDT to %p...", newgdt); - - gdtarg.base = (uintptr_t)newgdt; - gdtarg.limit = num_gdt_bytes - 1; - - __asm__ __volatile__ ("lgdt %0\n\t" : : "m" (gdtarg)); - printk(BIOS_DEBUG, "ok\n"); -} -RAMSTAGE_CBMEM_INIT_HOOK(move_gdt) diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index 12974ca..3659cc9 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -13,8 +13,7 @@ * conditionalize with macros. */ #if ENV_RAMSTAGE - RAMSTAGE(CONFIG_RAMBASE, (CONFIG(RELOCATABLE_RAMSTAGE) ? 8M : - CONFIG_RAMTOP - CONFIG_RAMBASE)) + RAMSTAGE(CONFIG_RAMBASE, 8M)
#elif ENV_ROMSTAGE /* The 1M size is not allocated. It's just for basic size checking. diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 69e81cd..39412ba 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -129,9 +129,8 @@
timestamp_add_now(TS_START_COPYRAM);
- if (CONFIG(RELOCATABLE_RAMSTAGE)) { - if (load_relocatable_ramstage(&ramstage)) - goto fail; + if (ENV_X86 && load_relocatable_ramstage(&ramstage)) + goto fail; } else if (cbfs_prog_stage_load(&ramstage)) goto fail;
diff --git a/src/security/memory/Kconfig b/src/security/memory/Kconfig index 5104f34..1f5aae1 100644 --- a/src/security/memory/Kconfig +++ b/src/security/memory/Kconfig @@ -6,7 +6,6 @@ bool default y if ARCH_X86 default n - depends on RELOCATABLE_RAMSTAGE help Selected by platforms that support clearing all DRAM after DRAM initialization. diff --git a/src/security/memory/memory_clear.c b/src/security/memory/memory_clear.c index 031ca84..557125d 100644 --- a/src/security/memory/memory_clear.c +++ b/src/security/memory/memory_clear.c @@ -77,8 +77,7 @@ void *baseptr = NULL; size_t size = 0;
- /* Only skip CBMEM, as RELOCATABLE_RAMSTAGE is a requirement, no need - * to separately protect stack or heap */ + /* Only skip CBMEM, stage program, stack and heap are included there. */
cbmem_get_region(&baseptr, &size); memranges_insert(&mem, (uintptr_t)baseptr, size, BM_MEM_TABLE);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43014 )
Change subject: arch/x86: Remove RELOCATABLE_RAMSTAGE ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43014/1/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/43014/1/src/lib/prog_loaders.c@134 PS1, Line 134: } else if (cbfs_prog_stage_load(&ramstage)) spurious brace
https://review.coreboot.org/c/coreboot/+/43014/1/src/security/memory/Kconfig File src/security/memory/Kconfig:
https://review.coreboot.org/c/coreboot/+/43014/1/src/security/memory/Kconfig... PS1, Line 8: default n Given the CBMEM assumption in the implementation, should this be
default y depends on ARCH_X86
?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43014
to look at the new patch set (#2).
Change subject: arch/x86: Remove RELOCATABLE_RAMSTAGE ......................................................................
arch/x86: Remove RELOCATABLE_RAMSTAGE
We always have it, no need to support opting-out.
For PLATFORM_HAS_DRAM_CLEAR there is a dependency of ramstage located inside CBMEM, which is only true with ARCH_X86.
Change-Id: I5cbf4063c69571db92de2d321c14d30c272e8098 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/Kconfig M src/arch/x86/Makefile.inc D src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/lib/prog_loaders.c M src/security/memory/Kconfig M src/security/memory/memory_clear.c 7 files changed, 8 insertions(+), 81 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/43014/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43014 )
Change subject: arch/x86: Remove RELOCATABLE_RAMSTAGE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43014/1/src/security/memory/Kconfig File src/security/memory/Kconfig:
https://review.coreboot.org/c/coreboot/+/43014/1/src/security/memory/Kconfig... PS1, Line 8: default n
Given the CBMEM assumption in the implementation, should this be […]
I agree. I should exclude AGESA and binaryPI with followups. Not sure about soc/amd yet.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43014 )
Change subject: arch/x86: Remove RELOCATABLE_RAMSTAGE ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43014 )
Change subject: arch/x86: Remove RELOCATABLE_RAMSTAGE ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43014/1/src/security/memory/Kconfig File src/security/memory/Kconfig:
https://review.coreboot.org/c/coreboot/+/43014/1/src/security/memory/Kconfig... PS1, Line 8: default n
I agree. I should exclude AGESA and binaryPI with followups. Not sure about soc/amd yet.
Ack
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43014 )
Change subject: arch/x86: Remove RELOCATABLE_RAMSTAGE ......................................................................
arch/x86: Remove RELOCATABLE_RAMSTAGE
We always have it, no need to support opting-out.
For PLATFORM_HAS_DRAM_CLEAR there is a dependency of ramstage located inside CBMEM, which is only true with ARCH_X86.
Change-Id: I5cbf4063c69571db92de2d321c14d30c272e8098 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43014 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/Kconfig M src/arch/x86/Makefile.inc D src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/lib/prog_loaders.c M src/security/memory/Kconfig M src/security/memory/memory_clear.c 7 files changed, 8 insertions(+), 81 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Aaron Durbin: Looks good to me, approved
diff --git a/src/Kconfig b/src/Kconfig index eb85cd9..a4c2fa6 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -270,20 +270,9 @@ say N because it adds a small performance penalty and may abort on code that happens to work in spite of the UB.
-config RELOCATABLE_RAMSTAGE - bool - default y if ARCH_X86 - select RELOCATABLE_MODULES - help - The reloctable ramstage support allows for the ramstage to be built - as a relocatable module. The stage loader can identify a place - out of the OS way so that copying memory is unnecessary during an S3 - wake. When selecting this option the romstage is responsible for - determing a stack location to use for loading the ramstage. - choice prompt "Stage Cache for ACPI S3 resume" - default NO_STAGE_CACHE if !HAVE_ACPI_RESUME || !RELOCATABLE_RAMSTAGE + default NO_STAGE_CACHE if !HAVE_ACPI_RESUME default TSEG_STAGE_CACHE if SMM_TSEG
config NO_STAGE_CACHE @@ -576,7 +565,6 @@ config HAVE_ACPI_RESUME bool default n - depends on RELOCATABLE_RAMSTAGE
config DISABLE_ACPI_HIBERNATE bool diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index b5352e7..2adb294 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -245,7 +245,6 @@ ramstage-y += ebda.c ramstage-y += exception.c ramstage-y += idt.S -ramstage-y += gdt.c ramstage-$(CONFIG_IOAPIC) += ioapic.c ramstage-y += memcpy.c ramstage-y += memmove.c @@ -291,14 +290,10 @@
ramstage-libs ?=
-ifeq ($(CONFIG_RELOCATABLE_RAMSTAGE),y) - # The rmodule_link definition creates an elf file with .rmod extension. $(objcbfs)/ramstage.elf: $(objcbfs)/ramstage.debug.rmod cp $< $@
-endif - $(objcbfs)/ramstage.debug: $(objgenerated)/ramstage.o $(call src-to-obj,ramstage,$(CONFIG_MEMLAYOUT_LD_FILE)) @printf " CC $(subst $(obj)/,,$(@))\n" $(LD_ramstage) $(CPPFLAGS) $(LDFLAGS_ramstage) -o $@ -L$(obj) $< -T $(call src-to-obj,ramstage,$(CONFIG_MEMLAYOUT_LD_FILE)) diff --git a/src/arch/x86/gdt.c b/src/arch/x86/gdt.c deleted file mode 100644 index 9c85566..0000000 --- a/src/arch/x86/gdt.c +++ /dev/null @@ -1,52 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <types.h> -#include <string.h> -#include <cbmem.h> -#include <commonlib/helpers.h> -#include <console/console.h> -#include <cpu/x86/gdt.h> - -/* i386 lgdt argument */ -struct gdtarg { - u16 limit; -#ifdef __x86_64__ - u64 base; -#else - u32 base; -#endif -} __packed; - -/* - * Copy GDT to new location and reload it. - * FIXME: We only do this for BSP CPU. - */ -static void move_gdt(int is_recovery) -{ - void *newgdt; - u16 num_gdt_bytes; - struct gdtarg gdtarg; - - /* ramstage is already in high memory. No need to use a new gdt. */ - if (CONFIG(RELOCATABLE_RAMSTAGE)) - return; - - newgdt = cbmem_find(CBMEM_ID_GDT); - num_gdt_bytes = (uintptr_t)&gdt_end - (uintptr_t)&gdt; - if (!newgdt) { - newgdt = cbmem_add(CBMEM_ID_GDT, ALIGN_UP(num_gdt_bytes, 512)); - if (!newgdt) { - printk(BIOS_ERR, "Error: Could not relocate GDT.\n"); - return; - } - memcpy((void *)newgdt, &gdt, num_gdt_bytes); - } - printk(BIOS_DEBUG, "Moving GDT to %p...", newgdt); - - gdtarg.base = (uintptr_t)newgdt; - gdtarg.limit = num_gdt_bytes - 1; - - __asm__ __volatile__ ("lgdt %0\n\t" : : "m" (gdtarg)); - printk(BIOS_DEBUG, "ok\n"); -} -RAMSTAGE_CBMEM_INIT_HOOK(move_gdt) diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index 12974ca..3659cc9 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -13,8 +13,7 @@ * conditionalize with macros. */ #if ENV_RAMSTAGE - RAMSTAGE(CONFIG_RAMBASE, (CONFIG(RELOCATABLE_RAMSTAGE) ? 8M : - CONFIG_RAMTOP - CONFIG_RAMBASE)) + RAMSTAGE(CONFIG_RAMBASE, 8M)
#elif ENV_ROMSTAGE /* The 1M size is not allocated. It's just for basic size checking. diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 69e81cd..419f4cd 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -129,10 +129,9 @@
timestamp_add_now(TS_START_COPYRAM);
- if (CONFIG(RELOCATABLE_RAMSTAGE)) { - if (load_relocatable_ramstage(&ramstage)) - goto fail; - } else if (cbfs_prog_stage_load(&ramstage)) + if (ENV_X86 && load_relocatable_ramstage(&ramstage)) + goto fail; + else if (cbfs_prog_stage_load(&ramstage)) goto fail;
stage_cache_add(STAGE_RAMSTAGE, &ramstage); diff --git a/src/security/memory/Kconfig b/src/security/memory/Kconfig index 5104f34..d3dec04 100644 --- a/src/security/memory/Kconfig +++ b/src/security/memory/Kconfig @@ -4,9 +4,8 @@
config PLATFORM_HAS_DRAM_CLEAR bool - default y if ARCH_X86 - default n - depends on RELOCATABLE_RAMSTAGE + default y + depends on ARCH_X86 help Selected by platforms that support clearing all DRAM after DRAM initialization. diff --git a/src/security/memory/memory_clear.c b/src/security/memory/memory_clear.c index 031ca84..557125d 100644 --- a/src/security/memory/memory_clear.c +++ b/src/security/memory/memory_clear.c @@ -77,8 +77,7 @@ void *baseptr = NULL; size_t size = 0;
- /* Only skip CBMEM, as RELOCATABLE_RAMSTAGE is a requirement, no need - * to separately protect stack or heap */ + /* Only skip CBMEM, stage program, stack and heap are included there. */
cbmem_get_region(&baseptr, &size); memranges_insert(&mem, (uintptr_t)baseptr, size, BM_MEM_TABLE);
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)