Change in coreboot[master]: prog_loaders: Remove CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING
Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39442
to review the following change.
Change subject: prog_loaders: Remove CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING ......................................................................
prog_loaders: Remove CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING
This option is not used on any platform and is not user-visible. It seems that it has not been used by anyone for a long time (maybe ever). Let's get rid of it to make future CBFS / program loader development simpler.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I2fa4d6d6f7c1d7a5ba552177b45e890b70008f36 --- M src/cpu/x86/Kconfig M src/cpu/x86/Makefile.inc D src/cpu/x86/mirror_payload.c M src/include/program_loading.h M src/lib/prog_loaders.c 5 files changed, 0 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/39442/1
diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index 76446a0..dd7bb30 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -169,15 +169,6 @@ common AP setup. Intel documentation specifies an INIT SIPI SIPI sequence, however this doesn't work on some AMD platforms.
-config MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING - def_bool n - help - On certain platforms a boot speed gain can be realized if mirroring - the payload data stored in non-volatile storage. On x86 systems the - payload would typically live in a memory-mapped SPI part. Copying - the SPI contents to RAM before performing the load can speed up - the boot process. - config SOC_SETS_MSRS bool default n diff --git a/src/cpu/x86/Makefile.inc b/src/cpu/x86/Makefile.inc index 1191069..bbe5545 100644 --- a/src/cpu/x86/Makefile.inc +++ b/src/cpu/x86/Makefile.inc @@ -1,7 +1,6 @@ subdirs-y += pae subdirs-$(CONFIG_PARALLEL_MP) += name ramstage-$(CONFIG_PARALLEL_MP) += mp_init.c -ramstage-$(CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING) += mirror_payload.c ramstage-y += backup_default_smm.c
subdirs-$(CONFIG_CPU_INTEL_COMMON_SMM) += ../intel/smm diff --git a/src/cpu/x86/mirror_payload.c b/src/cpu/x86/mirror_payload.c deleted file mode 100644 index 9987347..0000000 --- a/src/cpu/x86/mirror_payload.c +++ /dev/null @@ -1,65 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#include <string.h> -#include <commonlib/helpers.h> -#include <console/console.h> -#include <bootmem.h> -#include <program_loading.h> -#include <types.h> - -void mirror_payload(struct prog *payload) -{ - char *buffer; - size_t size; - char *src; - uintptr_t alignment_diff; - const unsigned long cacheline_size = 64; - const uintptr_t intra_cacheline_mask = cacheline_size - 1; - const uintptr_t cacheline_mask = ~intra_cacheline_mask; - - src = prog_start(payload); - size = prog_size(payload); - - /* - * Adjust size so that the start and end points are aligned to a - * cacheline. The SPI hardware controllers on Intel machines should - * cache full length cachelines as well as prefetch data. Once the - * data is mirrored in memory all accesses should hit the CPU's cache. - */ - alignment_diff = (intra_cacheline_mask & (uintptr_t)src); - size += alignment_diff; - - size = ALIGN_UP(size, cacheline_size); - - printk(BIOS_DEBUG, "Payload aligned size: 0x%zx\n", size); - - buffer = bootmem_allocate_buffer(size); - - if (buffer == NULL) { - printk(BIOS_DEBUG, "No buffer for mirroring payload.\n"); - return; - } - - src = (void *)(cacheline_mask & (uintptr_t)src); - - /* - * Note that if mempcy is not using 32-bit moves the performance will - * degrade because the SPI hardware prefetchers look for - * cacheline-aligned 32-bit accesses to kick in. - */ - memcpy(buffer, src, size); - - /* Update the payload's backing store. */ - prog_set_area(payload, &buffer[alignment_diff], prog_size(payload)); -} diff --git a/src/include/program_loading.h b/src/include/program_loading.h index 320ff3c..d393008 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -205,9 +205,6 @@ /* Run the loaded payload. */ void payload_run(void);
-/* Mirror the payload to be loaded. */ -void mirror_payload(struct prog *payload); - /* * selfload() and selfload_check() load payloads into memory. * selfload() does not check the payload to see if it targets memory. diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 0319325..178209c 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -170,10 +170,6 @@ static struct prog global_payload = PROG_INIT(PROG_PAYLOAD, CONFIG_CBFS_PREFIX "/payload");
-void __weak mirror_payload(struct prog *payload) -{ -} - void payload_load(void) { struct prog *payload = &global_payload; @@ -183,8 +179,6 @@ if (prog_locate(payload)) goto out;
- mirror_payload(payload); - switch (prog_cbfs_type(payload)) { case CBFS_TYPE_SELF: /* Simple ELF */ selfload_check(payload, BM_MEM_RAM);
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39442 )
Change subject: prog_loaders: Remove CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING ......................................................................
Patch Set 1: Code-Review+2
I bet someone will say something again as this will be the 2nd time this has been removed. :)
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39442 )
Change subject: prog_loaders: Remove CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING ......................................................................
Patch Set 1: Code-Review+2
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39442 )
Change subject: prog_loaders: Remove CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING ......................................................................
prog_loaders: Remove CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING
This option is not used on any platform and is not user-visible. It seems that it has not been used by anyone for a long time (maybe ever). Let's get rid of it to make future CBFS / program loader development simpler.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I2fa4d6d6f7c1d7a5ba552177b45e890b70008f36 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39442 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/cpu/x86/Kconfig M src/cpu/x86/Makefile.inc D src/cpu/x86/mirror_payload.c M src/include/program_loading.h M src/lib/prog_loaders.c 5 files changed, 0 insertions(+), 84 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Patrick Rudolph: Looks good to me, approved
diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index 76446a0..dd7bb30 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -169,15 +169,6 @@ common AP setup. Intel documentation specifies an INIT SIPI SIPI sequence, however this doesn't work on some AMD platforms.
-config MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING - def_bool n - help - On certain platforms a boot speed gain can be realized if mirroring - the payload data stored in non-volatile storage. On x86 systems the - payload would typically live in a memory-mapped SPI part. Copying - the SPI contents to RAM before performing the load can speed up - the boot process. - config SOC_SETS_MSRS bool default n diff --git a/src/cpu/x86/Makefile.inc b/src/cpu/x86/Makefile.inc index 1191069..bbe5545 100644 --- a/src/cpu/x86/Makefile.inc +++ b/src/cpu/x86/Makefile.inc @@ -1,7 +1,6 @@ subdirs-y += pae subdirs-$(CONFIG_PARALLEL_MP) += name ramstage-$(CONFIG_PARALLEL_MP) += mp_init.c -ramstage-$(CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING) += mirror_payload.c ramstage-y += backup_default_smm.c
subdirs-$(CONFIG_CPU_INTEL_COMMON_SMM) += ../intel/smm diff --git a/src/cpu/x86/mirror_payload.c b/src/cpu/x86/mirror_payload.c deleted file mode 100644 index 9987347..0000000 --- a/src/cpu/x86/mirror_payload.c +++ /dev/null @@ -1,65 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#include <string.h> -#include <commonlib/helpers.h> -#include <console/console.h> -#include <bootmem.h> -#include <program_loading.h> -#include <types.h> - -void mirror_payload(struct prog *payload) -{ - char *buffer; - size_t size; - char *src; - uintptr_t alignment_diff; - const unsigned long cacheline_size = 64; - const uintptr_t intra_cacheline_mask = cacheline_size - 1; - const uintptr_t cacheline_mask = ~intra_cacheline_mask; - - src = prog_start(payload); - size = prog_size(payload); - - /* - * Adjust size so that the start and end points are aligned to a - * cacheline. The SPI hardware controllers on Intel machines should - * cache full length cachelines as well as prefetch data. Once the - * data is mirrored in memory all accesses should hit the CPU's cache. - */ - alignment_diff = (intra_cacheline_mask & (uintptr_t)src); - size += alignment_diff; - - size = ALIGN_UP(size, cacheline_size); - - printk(BIOS_DEBUG, "Payload aligned size: 0x%zx\n", size); - - buffer = bootmem_allocate_buffer(size); - - if (buffer == NULL) { - printk(BIOS_DEBUG, "No buffer for mirroring payload.\n"); - return; - } - - src = (void *)(cacheline_mask & (uintptr_t)src); - - /* - * Note that if mempcy is not using 32-bit moves the performance will - * degrade because the SPI hardware prefetchers look for - * cacheline-aligned 32-bit accesses to kick in. - */ - memcpy(buffer, src, size); - - /* Update the payload's backing store. */ - prog_set_area(payload, &buffer[alignment_diff], prog_size(payload)); -} diff --git a/src/include/program_loading.h b/src/include/program_loading.h index 320ff3c..d393008 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -205,9 +205,6 @@ /* Run the loaded payload. */ void payload_run(void);
-/* Mirror the payload to be loaded. */ -void mirror_payload(struct prog *payload); - /* * selfload() and selfload_check() load payloads into memory. * selfload() does not check the payload to see if it targets memory. diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 0319325..178209c 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -170,10 +170,6 @@ static struct prog global_payload = PROG_INIT(PROG_PAYLOAD, CONFIG_CBFS_PREFIX "/payload");
-void __weak mirror_payload(struct prog *payload) -{ -} - void payload_load(void) { struct prog *payload = &global_payload; @@ -183,8 +179,6 @@ if (prog_locate(payload)) goto out;
- mirror_payload(payload); - switch (prog_cbfs_type(payload)) { case CBFS_TYPE_SELF: /* Simple ELF */ selfload_check(payload, BM_MEM_RAM);
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39442 )
Change subject: prog_loaders: Remove CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING ......................................................................
Patch Set 2:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1243 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1242 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1241
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Julius Werner (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)