Change in ...coreboot[master]: Revert "cpu/x86/mtrr: Fix sign overflow"

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Revert "cpu/x86/mtrr: Fix sign overflow"

This reverts commit 6bbc8d8050b1d51ec4bf15003a2da54e20d476c7.

The macro is used in assembly where integer suffixes are not portable.

Also, it is unclear how this can overflow as it's already the macros
purpose to avoid the overflow.

Change-Id: I12c9bfe40891ae3afbfda05f60a20b59e2954aed
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/90/31290/1

diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h
index 0398a2e..eb7d78d 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) (((1UL << 20) - ((x) >> 12)) << 12)
+#define _FROM_4G_TOP(x) (((1 << 20) - ((x) >> 12)) << 12)

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

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:

Sorry, I guess the original problem lies elsewhere.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31290 )

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:

Please introduce a better fix on x86_64.

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:
> Please introduce a better fix on x86_64.

Fix for what? you've never mentioned what you are trying to fix.
And it's not every obvious how a 21-bit number that doesn't
overflow on x86_32 overflows on x86_64.

If your change is really fixing something, it very likely hides
a problem somewhere else.

Also, please read the comment above the macros, it's supposed
to work not only in C.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31290 )

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:

It was mentioned in the commit message: "Fixes wrong MTRRs settings on x86_64 romstage."

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:
> Patch Set 1:
>
> It was mentioned in the commit message: "Fixes wrong MTRRs settings on x86_64 romstage."

Oh that explains everything. Except that it explains nothing ;)
There is no x86_64 romstage in coreboot (yet). So nothing to
fix there. There also doesn't seem to be anything in queue that
would get fixed (I run the numbers of the qemu ports through
the surrounding macros and couldn't find any problem, actually
didn't even find a case where the C compiler would see the
result of _FROM_4G_TOP()).

It would be easy to convince me to fix something, if you'd
point out the *operation* that overflows.

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

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1: Code-Review+2

I cannot figure out the original issue either. Some casting from 'int' to 'uintptr' would fail because bit31 is set? Something not merged?

I feel the revert is in order just because the original commit raises these questions. And for the portability of asm maybe fix should be somewhere else.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31290 )

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1: Code-Review+1

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31290 )

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > It was mentioned in the commit message: "Fixes wrong MTRRs settings on x86_64 romstage."
>
> Oh that explains everything. Except that it explains nothing ;)
> There is no x86_64 romstage in coreboot (yet). So nothing to
> fix there. There also doesn't seem to be anything in queue that
> would get fixed (I run the numbers of the qemu ports through
> the surrounding macros and couldn't find any problem, actually
> didn't even find a case where the C compiler would see the
> result of _FROM_4G_TOP()).
>
> It would be easy to convince me to fix something, if you'd
> point out the *operation* that overflows.

It's used in romstage:
postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, ...
where it casts from int to uintptr_t.

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:
> It's used in romstage:
> postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, ...
> where it casts from int to uintptr_t.

Is that with your qemu patch on Gerrit? Can you
please check what the macros evaluate to, e.g.
run the gcc command line with `-E` instead of
`-o ...o`.

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:
> > It's used in romstage:
> > postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, ...
> > where it casts from int to uintptr_t.
>
> Is that with your qemu patch on Gerrit? Can you
> please check what the macros evaluate to, e.g.
> run the gcc command line with `-E` instead of
> `-o ...o`.

With and without the `UL`, please.

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:

(1 comment)

Did you ever place CAR above 4G?

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

https://review.coreboot.org/#/c/31290/1/src/include/cpu/x86/mtrr.h@a179 
PS1, Line 179: 
... that would overflow here

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31290 )

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/#/c/31290/1/src/include/cpu/x86/mtrr.h@a179 
PS1, Line 179:
> ... […]

On qemu it's below 4GiB and I only tested x86_64 on qemu.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31290 )

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:
> Patch Set 1:
> > > It's used in romstage:
> > > postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, ...
> > > where it casts from int to uintptr_t.
> >
> > Is that with your qemu patch on Gerrit? Can you
> > please check what the macros evaluate to, e.g.
> > run the gcc command line with `-E` instead of
> > `-o ...o`.
>
> With and without the `UL`, please.

without UL:

void postcar_frame_add_romcache(struct postcar_frame *pcf, int type)
{
 if (!1)
  return;
 postcar_frame_add_mtrr(pcf, (((1 << 20) - ((((0x200000 + ((0x200000>>1)|(0x200000>>2)|(0x200000>>3)|(0x200000>>4)|(0x200000>>5)| (0x200000>>6)|(0x200000>>7)|(0x200000>>8)|((1<<18)-1))) & ~((0x200000>>1)|(0x200000>>2)|(0x200000>>3)|(0x200000>>4)|(0x200000>>5)| (0x200000>>6)|(0x200000>>7)|(0x200000>>8)|((1<<18)-1)))) >> 12)) << 12), ((0x200000 + ((0x200000>>1)|(0x200000>>2)|(0x200000>>3)|(0x200000>>4)|(0x200000>>5)| (0x200000>>6)|(0x200000>>7)|(0x200000>>8)|((1<<18)-1))) & ~((0x200000>>1)|(0x200000>>2)|(0x200000>>3)|(0x200000>>4)|(0x200000>>5)| (0x200000>>6)|(0x200000>>7)|(0x200000>>8)|((1<<18)-1))), type);
}

with UL:

void postcar_frame_add_romcache(struct postcar_frame *pcf, int type)
{
 if (!1)
  return;
 postcar_frame_add_mtrr(pcf, (((1UL << 20) - ((((0x200000 + ((0x200000>>1)|(0x200000>>2)|(0x200000>>3)|(0x200000>>4)|(0x200000>>5)| (0x200000>>6)|(0x200000>>7)|(0x200000>>8)|((1<<18)-1))) & ~((0x200000>>1)|(0x200000>>2)|(0x200000>>3)|(0x200000>>4)|(0x200000>>5)| (0x200000>>6)|(0x200000>>7)|(0x200000>>8)|((1<<18)-1)))) >> 12)) << 12), ((0x200000 + ((0x200000>>1)|(0x200000>>2)|(0x200000>>3)|(0x200000>>4)|(0x200000>>5)| (0x200000>>6)|(0x200000>>7)|(0x200000>>8)|((1<<18)-1))) & ~((0x200000>>1)|(0x200000>>2)|(0x200000>>3)|(0x200000>>4)|(0x200000>>5)| (0x200000>>6)|(0x200000>>7)|(0x200000>>8)|((1<<18)-1))), type);
}

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 1:

Ok, I see it now. It's not an overflow. We shift into
the sign bit and it gets sign-extended when converting
to the `uintptr_t`.

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 2:

Follow up is the best I could come up with yet,
will try something preprocessor based next.

[Nico Huber (Code Review)]

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

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Patch Set 2:
> Follow up is the best I could come up with yet,
> will try something preprocessor based next.

Did something unexpected instead: CB:31322. Should
be build tested if it changes anything.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31290 )

Change subject: Revert "cpu/x86/mtrr: Fix sign overflow"
......................................................................

Revert "cpu/x86/mtrr: Fix sign overflow"

This reverts commit 6bbc8d8050b1d51ec4bf15003a2da54e20d476c7.

The macro is used in assembly where integer suffixes are not portable.

Also, it is unclear how this can overflow as it's already the macros
purpose to avoid the overflow.

Change-Id: I12c9bfe40891ae3afbfda05f60a20b59e2954aed
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/31290
Tested-by: build bot (Jenkins) 
Reviewed-by: Kyösti Mälkki 
Reviewed-by: Angel Pons 
---
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
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h
index 0398a2e..eb7d78d 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) (((1UL << 20) - ((x) >> 12)) << 12)
+#define _FROM_4G_TOP(x) (((1 << 20) - ((x) >> 12)) << 12)

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