Change in ...coreboot[master]: cpu/x86/mtrr: Fix _FROM_4G_TOP() macro

[Nico Huber (Code Review)]

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31322

Change subject: cpu/x86/mtrr: Fix _FROM_4G_TOP() macro
......................................................................

cpu/x86/mtrr: Fix _FROM_4G_TOP() macro

This macro was unnecessarily complex. Trying to avoid an overflow for
unknown reasons, and instead shifted the result into the sign bit in
C. Using a plain number seems to be safe in all languages.

This assumes that any processing entity (preprocessor, assembler,
romcc, GCC) would complain if it can't handle the 4G number literal.

Change-Id: Ibb0c5b88a6e42d3ef2990196a5b99ace90ea8ee8
Signed-off-by: Nico Huber 
---
M src/include/cpu/x86/mtrr.h
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/31322/1

diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h
index eb7d78d..a15e186 100644
--- a/src/include/cpu/x86/mtrr.h
+++ b/src/include/cpu/x86/mtrr.h
@@ -150,7 +150,7 @@
 #define _ALIGN_DOWN_POW2(x)	((x) & ~_POW2_MASK(x))

/* Calculate `4GiB - x` (e.g. absolute address for offset from 4GiB) */
-#define _FROM_4G_TOP(x) (((1 << 20) - ((x) >> 12)) << 12)
+#define _FROM_4G_TOP(x) (0x100000000 - (x))

/* At the end of romstage, low RAM 0..CACHE_TM_RAMTOP may be set
  * as write-back cacheable to speed up ramstage decompression.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31322 )

Change subject: cpu/x86/mtrr: Fix _FROM_4G_TOP() macro
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31322 )

Change subject: cpu/x86/mtrr: Fix _FROM_4G_TOP() macro
......................................................................

Patch Set 1: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31322 )

Change subject: cpu/x86/mtrr: Fix _FROM_4G_TOP() macro
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31322/1/src/include/cpu/x86/mtrr.h 
File src/include/cpu/x86/mtrr.h:

https://review.coreboot.org/#/c/31322/1/src/include/cpu/x86/mtrr.h@153 
PS1, Line 153: #define _FROM_4G_TOP(x) (0x100000000 - (x))
Also possible and doesn't require more than 32 bits even at compile time:

((0xffffffff - (x)) + 1)

[Nico Huber (Code Review)]

Hello Kyösti Mälkki, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31322

to look at the new patch set (#2).

Change subject: cpu/x86/mtrr: Fix _FROM_4G_TOP() macro
......................................................................

cpu/x86/mtrr: Fix _FROM_4G_TOP() macro

This macro was unnecessarily complex. Trying to avoid an overflow
for unknown reasons, and instead shifted the result into the sign
bit in C. Using a plain number literal that forces C to use an
adequate integer type seems to be safe. We start with 0xffffffff,
subtract `x` and add 1 again. Turned out to be a common pattern
and can't overflow for any positive 32-bit `x`.

Change-Id: Ibb0c5b88a6e42d3ef2990196a5b99ace90ea8ee8
Signed-off-by: Nico Huber 
---
M src/include/cpu/x86/mtrr.h
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/31322/2

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31322 )

Change subject: cpu/x86/mtrr: Fix _FROM_4G_TOP() macro
......................................................................

Patch Set 2: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31322 )

Change subject: cpu/x86/mtrr: Fix _FROM_4G_TOP() macro
......................................................................

cpu/x86/mtrr: Fix _FROM_4G_TOP() macro

This macro was unnecessarily complex. Trying to avoid an overflow
for unknown reasons, and instead shifted the result into the sign
bit in C. Using a plain number literal that forces C to use an
adequate integer type seems to be safe. We start with 0xffffffff,
subtract `x` and add 1 again. Turned out to be a common pattern
and can't overflow for any positive 32-bit `x`.

Change-Id: Ibb0c5b88a6e42d3ef2990196a5b99ace90ea8ee8
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/31322
Tested-by: build bot (Jenkins) 
Reviewed-by: Kyösti Mälkki 
---
M src/include/cpu/x86/mtrr.h
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Kyösti Mälkki: Looks good to me, approved

diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h
index eb7d78d..49ed462 100644
--- a/src/include/cpu/x86/mtrr.h
+++ b/src/include/cpu/x86/mtrr.h
@@ -150,7 +150,7 @@
 #define _ALIGN_DOWN_POW2(x)	((x) & ~_POW2_MASK(x))

/* Calculate `4GiB - x` (e.g. absolute address for offset from 4GiB) */
-#define _FROM_4G_TOP(x) (((1 << 20) - ((x) >> 12)) << 12)
+#define _FROM_4G_TOP(x) ((0xffffffff - (x)) + 1)

/* At the end of romstage, low RAM 0..CACHE_TM_RAMTOP may be set
  * as write-back cacheable to speed up ramstage decompression.