Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81269?usp=email )
Change subject: soc/intel/cmn/ramtop: Refactor MTRR handling for RAMTOP range
......................................................................
soc/intel/cmn/ramtop: Refactor MTRR handling for RAMTOP range
This patch refactors RAMTOP MTRR type selection to address a critical
NEM logic bug on SoCs with non-power-of-two cache sets. This bug can
cause runtime hangs when Write Back (WB) caching is enabled.
Workaround: Force MTRR type to WC (Write Combining) on affected SoCs
when the cache set count is not a power of two.
BUG=b:306677879
BRANCH=firmware-rex-15709.B
TEST=Verified boot on google/ovis and google/rex (including Ovis with
non-power-of-two cache configuration).
Change-Id: Ia9a8f0d37d581b05c19ea7f9b1a07933caa956d4
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
---
M src/soc/intel/common/basecode/ramtop/ramtop.c
1 file changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/81269/1
diff --git a/src/soc/intel/common/basecode/ramtop/ramtop.c b/src/soc/intel/common/basecode/ramtop/ramtop.c
index ec326bb..9cef9b1 100644
--- a/src/soc/intel/common/basecode/ramtop/ramtop.c
+++ b/src/soc/intel/common/basecode/ramtop/ramtop.c
@@ -2,6 +2,7 @@
#include <commonlib/bsd/ipchksum.h>
#include <console/console.h>
+#include <cpu/cpu.h>
#include <cpu/x86/mtrr.h>
#include <intelbasecode/ramtop.h>
#include <pc80/mc146818rtc.h>
@@ -125,9 +126,24 @@
printk(BIOS_WARNING, "ramtop_table update failure due to no free MTRR available!\n");
return;
}
+
+ /*
+ * Background: Some SoCs have a critical bug inside the NEM logic which is responsible
+ * for mapping cached memory to physical memory during tear down and
+ * eventually malfunctions if the number of cache sets is not a power of two.
+ * This can lead to runtime hangs.
+ *
+ * Workaround: To mitigate this issue on affected SoCs, we force the MTRR type to
+ * WC (Write Combining) unless the cache set count is a power of two.
+ * This change alters caching behavior but prevents the runtime failures.
+ */
+ unsigned int mtrr_type = MTRR_TYPE_WRCOMB;
/*
* We need to make sure late romstage (including FSP-M post mem) will be run
* cached. Caching 16MB below ramtop is a safe to cover late romstage.
*/
- set_var_mtrr(mtrr, ramtop - 16 * MiB, 16 * MiB, MTRR_TYPE_WRBACK);
+ if (is_cache_sets_power_of_two())
+ mtrr_type = MTRR_TYPE_WRBACK;
+
+ set_var_mtrr(mtrr, ramtop - 16 * MiB, 16 * MiB, mtrr_type);
}
--
To view, visit https://review.coreboot.org/c/coreboot/+/81269?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ia9a8f0d37d581b05c19ea7f9b1a07933caa956d4
Gerrit-Change-Number: 81269
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: newchange
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81268?usp=email )
Change subject: arch/x86: Add API to check if cache sets are power-of-two
......................................................................
arch/x86: Add API to check if cache sets are power-of-two
Introduce a function to determine whether the number of cache sets is
a power of two. This aligns with common cache design practices that
favor power-of-two counts for efficient indexing and addressing.
BUG=b:306677879
BRANCH=firmware-rex-15709.B
TEST=Verified functionality on google/ovis and google/rex (including
a non-power-of-two Ovis configuration).
Change-Id: I819e0d1aeb4c1dbe1cdf3115b2e172588a6e8da5
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
---
M src/arch/x86/cpu_common.c
M src/arch/x86/include/arch/cpu.h
2 files changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/81268/1
diff --git a/src/arch/x86/cpu_common.c b/src/arch/x86/cpu_common.c
index c4d30a2..ee07214 100644
--- a/src/arch/x86/cpu_common.c
+++ b/src/arch/x86/cpu_common.c
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
+#include <commonlib/helpers.h>
#include <cpu/cpu.h>
#include <types.h>
@@ -234,3 +235,18 @@
return true;
}
+
+bool is_cache_sets_power_of_two(void)
+{
+ struct cpu_cache_info info;
+
+ if (!fill_cpu_cache_info(CACHE_L3, &info))
+ return false;
+
+ size_t cache_sets = cpu_get_cache_sets(&info);
+
+ if (IS_POWER_OF_2(cache_sets))
+ return true;
+
+ return false;
+}
diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h
index 0c4decf..fa0d5f4 100644
--- a/src/arch/x86/include/arch/cpu.h
+++ b/src/arch/x86/include/arch/cpu.h
@@ -329,6 +329,15 @@
*/
bool fill_cpu_cache_info(uint8_t level, struct cpu_cache_info *info);
+/*
+ * Determines whether the number of cache sets is a power of two.
+ *
+ * Cache designs often favor power-of-two set counts for efficient indexing
+ * and addressing. This function checks if the provided cache configuration
+ * adheres to this practice.
+ */
+bool is_cache_sets_power_of_two(void);
+
#if CONFIG(RESERVED_PHYSICAL_ADDRESS_BITS_SUPPORT)
unsigned int get_reserved_phys_addr_bits(void);
#else
--
To view, visit https://review.coreboot.org/c/coreboot/+/81268?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I819e0d1aeb4c1dbe1cdf3115b2e172588a6e8da5
Gerrit-Change-Number: 81268
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: newchange
Attention is currently required from: Arthur Heymans.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81267?usp=email )
Change subject: src/Kconfig: Make it possible to override CCACHE in site-local
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81267/comment/329e8446_c490bc50 :
PS2, Line 10: default in src/Kconfig. Remove the default to make overrides possible.
It was already possible with a `select`. Don't mind the change, though.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81267?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I6b9dbbb31caa3ef09afd7ecb355c01bd53807b39
Gerrit-Change-Number: 81267
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Sat, 16 Mar 2024 12:35:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81280?usp=email )
Change subject: soc/intel/xeon_sp/spr: Enable x86_64 support
......................................................................
Patch Set 1:
(1 comment)
File src/soc/intel/xeon_sp/uncore_acpi.c:
https://review.coreboot.org/c/coreboot/+/81280/comment/f657ffc4_78be8e0a :
PS1, Line 473: }
> It does what UEFI native code does. However it looks like a workaround for some legacy feature. […]
That seems to be the most common use case. I was just surprised to
see it on a coreboot server. Unless you want to drop it rn., there's
nothing to do for now. I was merely curious.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81280?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I2c5ed0339a9c2e9b088b16dbb4c19df98e796d65
Gerrit-Change-Number: 81280
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Sat, 16 Mar 2024 12:33:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-MessageType: comment
Attention is currently required from: Martin L Roth, Maximilian Brune.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81290?usp=email )
Change subject: genbuild_h: Fix and harden major/minor version parsing
......................................................................
Patch Set 1:
(2 comments)
File util/genbuild_h/genbuild_h.sh:
https://review.coreboot.org/c/coreboot/+/81290/comment/40b3b410_9f8aa219 :
PS1, Line 39: MAJOR_VER="$(echo "${VERSION}" | sed -n 's/^0*\([0-9]*\)\.0*\([0-9]*\).*/\1/p')"
: MINOR_VER="$(echo "${VERSION}" | sed -n 's/^0*\([0-9]*\)\.0*\([0-9]*\).*/\2/p')"
> The `-E` is posix compliant extended regex. […]
I'm not concerned about the regex itself, it's always possible to make it
posix compatible (by not using special operations). But I looked a little
into sed now. If sed should support the `-E', seems in flux. The Open Group
doesn't mention it [1]. Yet the GNU sed history claims since 2013 it would
be standardized. I'm pretty much puzzled.
So far we avoided it. And I really wouldn't want to risk anything as part
of a bug fix. If you want, you could add that in a separate patch, WDYT?
[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
That's 2017, and 2016, 2013 don't show it either.
https://review.coreboot.org/c/coreboot/+/81290/comment/5a5dce51_31beb784 :
PS1, Line 40: *
> ```
> 's/^([0-9]{2})\.0*([0-9]+).*/\2/p'
> ```
Without `-E` we don't have `+` nor `{}`. Also, the `{2}` would feel
like documenting when we started to use the new version scheme (after
2009) and would make other (downstream) versioning harder without
gaining us anything. I could change it to
```
's/^\([0-9][0-9]*\)\.0*\([0-9][0-9]*\).*/\2/p'
```
To make it clear to the human reader that we always expect at least
one digit. But It wouldn't change the result of the script (an empty
string is an empty string).
> Regarding other version formats as described in the commit:
> The substitution that you have currently in place also does not work for the version tag described in the commit-msg ("v1.9308_26_0.0.22"),
I know. I just wanted to give an example that people actually have different
formats and care about support for that upstream. At least so far that the
build succeeds. And we accepted that upstream. Saying let downstream handle
their special cases, would be changing that direction.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81290?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie39381a8ef4b971556168b6996efeefe6adf2b14
Gerrit-Change-Number: 81290
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Comment-Date: Sat, 16 Mar 2024 12:29:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-MessageType: comment
Arthur Heymans has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/81267?usp=email )
Change subject: src/Kconfig: Make it possible to override CCACHE in site-local
......................................................................
src/Kconfig: Make it possible to override CCACHE in site-local
The value for CCACHE in site-local/Kconfig gets overridden by the
default in src/Kconfig. Remove the default to make overrides possible.
Change-Id: I6b9dbbb31caa3ef09afd7ecb355c01bd53807b39
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/Kconfig
1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/81267/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/81267?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I6b9dbbb31caa3ef09afd7ecb355c01bd53807b39
Gerrit-Change-Number: 81267
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81267?usp=email )
Change subject: src/Kconfig: Make it possible to override CCACHE in site-local
......................................................................
src/Kconfig: Make it possible to override CCACHE in site-local
The value for CCACHE in site-local/Kconfig gets overriden by the default
in src/Kconfig. Remove the default to make overrides possible.
Change-Id: I6b9dbbb31caa3ef09afd7ecb355c01bd53807b39
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/Kconfig
1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/81267/1
diff --git a/src/Kconfig b/src/Kconfig
index 2afb9db..59df165 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -113,7 +113,6 @@
config CCACHE
bool "Use ccache to speed up (re)compilation"
- default n
help
Enables the use of ccache for faster builds.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81267?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I6b9dbbb31caa3ef09afd7ecb355c01bd53807b39
Gerrit-Change-Number: 81267
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange
Attention is currently required from: Martin L Roth.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81285?usp=email )
Change subject: util/lint/lint: Fix shellcheck errors in getopt support for darwin
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/81285?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Icbdc4204f4c07d806e721fa39f96694c4df00e8d
Gerrit-Change-Number: 81285
Gerrit-PatchSet: 1
Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Comment-Date: Sat, 16 Mar 2024 10:49:09 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment