Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: [WIP]cpu/x86/cache: CLFLUSH programs to memory before running
......................................................................
[WIP]cpu/x86/cache: CLFLUSH programs to memory before running
When cbmem is initialized in romstage and postcar placed in
the stage cache + cbmem where it is run, the assumption is made
that this are all in UC memory such that calling invd in
postcar is ok.
For performance reasons (e.g. postcar decompression) it is desirable
to cache cbmem and the stage cache during romstage. Another reason
is that AGESA sets up MTRR during romstage to cache all dram, which
is currently worked around by using additional MTRR to make that UC.
TODO be nice to reviewers and spit some parts off.
UNTESTED
Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/arch/x86/postcar_loader.c
M src/cpu/x86/cache/Makefile.inc
M src/cpu/x86/cache/cache.c
M src/include/cpu/x86/cache.h
4 files changed, 68 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/37196/1
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c
index b53cbf8..53ab4f9 100644
--- a/src/arch/x86/postcar_loader.c
+++ b/src/arch/x86/postcar_loader.c
@@ -208,6 +208,21 @@
MTRR_TYPE_WRBACK);
}
+/*
+ * POSTCAR will call invd so don't make assumptions on cbmem
+ * and external stage cache being UC.
+ */
+static void postcar_flush_cache(void)
+{
+ uintptr_t stage_cache_base;
+ size_t stage_cache_size;
+ prog_segment_loaded((uintptr_t)cbmem_top(), cbmem_overhead_size(), SEG_FINAL);
+ if (CONFIG(TSEG_STAGE_CACHE)) {
+ stage_cache_external_region((void **)&stage_cache_base, &stage_cache_size);
+ prog_segment_loaded(stage_cache_base, stage_cache_size, SEG_FINAL);
+ }
+}
+
void run_postcar_phase(struct postcar_frame *pcf)
{
struct prog prog =
@@ -222,8 +237,10 @@
parameters between S3 resume and normal boot. On the
platforms where the values are the same it's a nop. */
finalize_load(prog.arg, pcf->stack);
- } else
+ } else {
load_postcar_cbfs(&prog, pcf);
+ postcar_flush_cache();
+ }
/* As postcar exist, it's end of romstage here */
timestamp_add_now(TS_END_ROMSTAGE);
diff --git a/src/cpu/x86/cache/Makefile.inc b/src/cpu/x86/cache/Makefile.inc
index b33b9ee..759488e 100644
--- a/src/cpu/x86/cache/Makefile.inc
+++ b/src/cpu/x86/cache/Makefile.inc
@@ -1 +1,2 @@
+romstage-y += cache.c
ramstage-y += cache.c
diff --git a/src/cpu/x86/cache/cache.c b/src/cpu/x86/cache/cache.c
index 2313c4d..ac2c45b 100644
--- a/src/cpu/x86/cache/cache.c
+++ b/src/cpu/x86/cache/cache.c
@@ -11,8 +11,11 @@
* GNU General Public License for more details.
*/
+#include <cbmem.h>
+#include <program_loading.h>
#include <console/console.h>
#include <cpu/x86/cache.h>
+#include <arch/cpu.h>
void x86_enable_cache(void)
{
@@ -20,3 +23,47 @@
printk(BIOS_INFO, "Enabling cache\n");
enable_cache();
}
+
+int clflush_supported(void)
+{
+ return (cpuid_edx(1) >> 19) & 1;
+}
+
+static void clflush_region(uintptr_t start, size_t size)
+{
+ uintptr_t addr;
+ size_t cl_size = (cpuid_ebx(1) >> 8) & 0xff;
+ if (!clflush_supported())
+ return;
+ for (addr = (start / cl_size) * cl_size; addr < start + size; addr += cl_size)
+ clflush((void *)addr);
+}
+
+static int dram_ready;
+
+/*
+ * For each segment of a program loaded this function is called
+ * to invalidate caches for the addresses of the loaded segment
+ */
+void arch_segment_loaded(uintptr_t start, size_t size, int flags)
+{
+ /* INVD is only called in postcar stage so we only need
+ to make sure that our things hit dram during romstage. */
+ if (!ENV_ROMSTAGE)
+ return;
+ if (flags != SEG_FINAL)
+ return;
+ if (!dram_ready)
+ return;
+
+ if (!clflush_supported())
+ printk(BIOS_DEBUG, "Not flushing cache to ram, CLFLUSH not supported\n");
+ clflush_region(start, size);
+}
+
+static void set_dram_ready(int unused)
+{
+ dram_ready = 1;
+}
+
+ROMSTAGE_CBMEM_INIT_HOOK(set_dram_ready);
diff --git a/src/include/cpu/x86/cache.h b/src/include/cpu/x86/cache.h
index 713ca32..2d2727f 100644
--- a/src/include/cpu/x86/cache.h
+++ b/src/include/cpu/x86/cache.h
@@ -55,6 +55,8 @@
asm volatile ("clflush (%0)"::"r" (addr));
}
+int clflush_supported(void);
+
/* The following functions require the __always_inline due to AMD
* function STOP_CAR_AND_CPU that disables cache as
* RAM, the cache as RAM stack can no longer be used. Called
--
To view, visit https://review.coreboot.org/c/coreboot/+/37196
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b
Gerrit-Change-Number: 37196
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-MessageType: newchange
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage
......................................................................
arch/x86: Add option to compress postcar stage
The LZ4 decompressor was already linked in romstage.
Change-Id: I89fdc6066027447bf72968c66e6f5eb5fbb630c7
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/Kconfig
M src/arch/x86/Makefile.inc
2 files changed, 19 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/36674/1
diff --git a/src/Kconfig b/src/Kconfig
index 0d56291..17bf545 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -158,6 +158,16 @@
that decompression might slow down booting if the boot flash
is connected through a slow link (i.e. SPI).
+config COMPRESS_POSTCAR
+ bool "Compress postcar with LZ4"
+ depends on POSTCAR_STAGE
+ # Default value set at the end of the file
+ help
+ Compress postcar with LZ4 to save flash space and speed up boot,
+ since the time for reading the image from SPI (and in the vboot
+ case verifying it) is usually much greater than the time spent
+ decompressing.
+
config COMPRESS_PRERAM_STAGES
bool "Compress romstage and verstage with LZ4"
depends on !ARCH_X86 && (HAVE_ROMSTAGE || HAVE_VERSTAGE)
@@ -1203,6 +1213,9 @@
config COMPRESS_RAMSTAGE
default y if !UNCOMPRESSED_RAMSTAGE
+config COMPRESS_POSTCAR
+ default y
+
config COMPRESS_PRERAM_STAGES
depends on !ARCH_X86
default y
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc
index 447fd57..8c35176 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -278,11 +278,16 @@
$(objcbfs)/postcar.elf: $(objcbfs)/postcar.debug.rmod
cp $< $@
+CBFS_POSTCAR_COMPRESS_FLAG := none
+ifeq ($(CONFIG_COMPRESS_POSTCAR),y)
+CBFS_POSTCAR_COMPRESS_FLAG := lz4
+endif
+
# Add postcar to CBFS
cbfs-files-$(CONFIG_POSTCAR_STAGE) += $(CONFIG_CBFS_PREFIX)/postcar
$(CONFIG_CBFS_PREFIX)/postcar-file := $(objcbfs)/postcar.elf
$(CONFIG_CBFS_PREFIX)/postcar-type := stage
-$(CONFIG_CBFS_PREFIX)/postcar-compression := none
+$(CONFIG_CBFS_PREFIX)/postcar-compression := $(CBFS_POSTCAR_COMPRESS_FLAG)
###############################################################################
# ramstage
--
To view, visit https://review.coreboot.org/c/coreboot/+/36674
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I89fdc6066027447bf72968c66e6f5eb5fbb630c7
Gerrit-Change-Number: 36674
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange
Attention is currently required from: Nico Huber, Angel Pons, Arthur Heymans, Keith Hui.
Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61540 )
Change subject: mb/asus/p8x7x-series: Refactor mainboard_get_spd()
......................................................................
Patch Set 4:
(1 comment)
File src/mainboard/asus/p8x7x-series/variants/p8c_ws/early_init.c:
https://review.coreboot.org/c/coreboot/+/61540/comment/bf90646f_692de090
PS4, Line 5: #include <northbridge/intel/sandybridge/raminit_native.h>
> Correct, but as you can see in this thread, Angel wants to see them removed in a separate commit.
sounds odd to me.
Angel, why do you want a separate commit?
the header is used for removed function.
now it is not used anymore.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61540
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3b9c616a6c722e1f0fc124ced225efdcadb46b25
Gerrit-Change-Number: 61540
Gerrit-PatchSet: 4
Gerrit-Owner: Keith Hui <buurin(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Keith Hui <buurin(a)gmail.com>
Gerrit-Comment-Date: Tue, 28 Feb 2023 23:27:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Keith Hui <buurin(a)gmail.com>
Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Martin L Roth.
Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/66922 )
Change subject: util/crossgcc: Update GCC from 11.3 to 12.2
......................................................................
Patch Set 12:
(1 comment)
Patchset:
PS12:
> The problem is that CI should mark the build as failed.
yes I know.
same for 70771
--
To view, visit https://review.coreboot.org/c/coreboot/+/66922
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia15e48847c6aea4f3188588b5092250568f16a09
Gerrit-Change-Number: 66922
Gerrit-PatchSet: 12
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Comment-Date: Tue, 28 Feb 2023 23:21:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-MessageType: comment
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/73231 )
Change subject: soc/amd/*/acpi: add comment about p_lvl[2,3]_lat FADT field usage
......................................................................
soc/amd/*/acpi: add comment about p_lvl[2,3]_lat FADT field usage
The latency values in the _CST package override the values in the
p_lvl2_lat and p_lvl3_lat FADT fields. In Picasso, Cezanne, Mendocino,
Phoenix and Glinda generate_cpu_entries generates the _CST packages for
each CPU device. The coreboot code for Stoneyridge doesn't generate _CST
packages for the CPU objects, but those are provided via the PSTATE SSDT
binaryPI generates and agesa_write_acpi_tables gets and adds to the ACPI
tables. The AGESA reference code also sets those two FADT entries to the
equivalents of ACPI_FADT_C2_NOT_SUPPORTED and ACPI_FADT_C3_NOT_SUPPORTED
so this also matches the AGESA behavior.
From the ACPI 6.4 spec: "Values provided by the _CST object override
P_LVLx values in P_BLK and P_LVLx_LAT values in the FADT."
Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de>
Change-Id: I1116a3013576b18b6f521604d6b0a9d75b971e0b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/73231
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
---
M src/soc/amd/cezanne/acpi.c
M src/soc/amd/glinda/acpi.c
M src/soc/amd/mendocino/acpi.c
M src/soc/amd/phoenix/acpi.c
M src/soc/amd/picasso/acpi.c
M src/soc/amd/stoneyridge/acpi.c
6 files changed, 38 insertions(+), 0 deletions(-)
Approvals:
build bot (Jenkins): Verified
Martin Roth: Looks good to me, approved
diff --git a/src/soc/amd/cezanne/acpi.c b/src/soc/amd/cezanne/acpi.c
index edf20c9..631f388 100644
--- a/src/soc/amd/cezanne/acpi.c
+++ b/src/soc/amd/cezanne/acpi.c
@@ -76,6 +76,8 @@
fill_fadt_extended_pm_regs(fadt);
+ /* p_lvl2_lat and p_lvl3_lat match what the AGESA code does, but those values are
+ overridden by the _CST packages in the processor devices. */
fadt->p_lvl2_lat = ACPI_FADT_C2_NOT_SUPPORTED;
fadt->p_lvl3_lat = ACPI_FADT_C3_NOT_SUPPORTED;
fadt->duty_offset = 0; /* Not supported */
diff --git a/src/soc/amd/glinda/acpi.c b/src/soc/amd/glinda/acpi.c
index 8d6c008..31723b1 100644
--- a/src/soc/amd/glinda/acpi.c
+++ b/src/soc/amd/glinda/acpi.c
@@ -79,6 +79,8 @@
fill_fadt_extended_pm_regs(fadt);
+ /* p_lvl2_lat and p_lvl3_lat match what the AGESA code does, but those values are
+ overridden by the _CST packages in the processor devices. */
fadt->p_lvl2_lat = ACPI_FADT_C2_NOT_SUPPORTED;
fadt->p_lvl3_lat = ACPI_FADT_C3_NOT_SUPPORTED;
fadt->duty_offset = 0; /* Not supported */
diff --git a/src/soc/amd/mendocino/acpi.c b/src/soc/amd/mendocino/acpi.c
index 6b20b02..044b345 100644
--- a/src/soc/amd/mendocino/acpi.c
+++ b/src/soc/amd/mendocino/acpi.c
@@ -78,6 +78,8 @@
fill_fadt_extended_pm_regs(fadt);
+ /* p_lvl2_lat and p_lvl3_lat match what the AGESA code does, but those values are
+ overridden by the _CST packages in the processor devices. */
fadt->p_lvl2_lat = ACPI_FADT_C2_NOT_SUPPORTED;
fadt->p_lvl3_lat = ACPI_FADT_C3_NOT_SUPPORTED;
fadt->duty_offset = 0; /* Not supported */
diff --git a/src/soc/amd/phoenix/acpi.c b/src/soc/amd/phoenix/acpi.c
index 32dde83..09c4a15 100644
--- a/src/soc/amd/phoenix/acpi.c
+++ b/src/soc/amd/phoenix/acpi.c
@@ -79,6 +79,8 @@
fill_fadt_extended_pm_regs(fadt);
+ /* p_lvl2_lat and p_lvl3_lat match what the AGESA code does, but those values are
+ overridden by the _CST packages in the processor devices. */
fadt->p_lvl2_lat = ACPI_FADT_C2_NOT_SUPPORTED;
fadt->p_lvl3_lat = ACPI_FADT_C3_NOT_SUPPORTED;
fadt->duty_offset = 0; /* Not supported */
diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c
index 1d5a275..f6fb9f2 100644
--- a/src/soc/amd/picasso/acpi.c
+++ b/src/soc/amd/picasso/acpi.c
@@ -82,6 +82,8 @@
fill_fadt_extended_pm_regs(fadt);
+ /* p_lvl2_lat and p_lvl3_lat match what the AGESA code does, but those values are
+ overridden by the _CST packages in the processor devices. */
fadt->p_lvl2_lat = ACPI_FADT_C2_NOT_SUPPORTED;
fadt->p_lvl3_lat = ACPI_FADT_C3_NOT_SUPPORTED;
fadt->duty_offset = 1; /* CLK_VAL bits 3:1 */
diff --git a/src/soc/amd/stoneyridge/acpi.c b/src/soc/amd/stoneyridge/acpi.c
index e2fb2e3..3b45a5f 100644
--- a/src/soc/amd/stoneyridge/acpi.c
+++ b/src/soc/amd/stoneyridge/acpi.c
@@ -74,6 +74,8 @@
fill_fadt_extended_pm_regs(fadt);
+ /* p_lvl2_lat and p_lvl3_lat match what the AGESA code does, but those values are
+ overridden by the _CST packages in the PSTATE SSDT. */
fadt->p_lvl2_lat = ACPI_FADT_C2_NOT_SUPPORTED;
fadt->p_lvl3_lat = ACPI_FADT_C3_NOT_SUPPORTED;
fadt->duty_offset = 1; /* CLK_VAL bits 3:1 */
--
To view, visit https://review.coreboot.org/c/coreboot/+/73231
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1116a3013576b18b6f521604d6b0a9d75b971e0b
Gerrit-Change-Number: 73231
Gerrit-PatchSet: 5
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Felix Singer, Jason Glenesk, Martin L Roth, Angel Pons, Nicholas Chin.
Daniel Maslowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73158 )
Change subject: Docs: Replace Recommonmark with MyST Parser
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/73158
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0837c1722fa56d25c9441ea218e943d8f3d9b804
Gerrit-Change-Number: 73158
Gerrit-PatchSet: 5
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Maslowski <info(a)orangecms.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Tue, 28 Feb 2023 22:20:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment