Attention is currently required from: Arthur Heymans, Jérémy Compostella.
Angel Pons has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84048?usp=email )
Change subject: arch/x86/car.ld: Fix overlapping regions
......................................................................
Patch Set 10: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/84048?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I058bd6790afc313e06f1888e5b783d97b7e93b1e
Gerrit-Change-Number: 84048
Gerrit-PatchSet: 10
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-CC: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 14:02:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Arthur Heymans, Christian Walter, Jakub Czapiga, Johnny Lin, Jonathan Zhang, Jérémy Compostella, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.
Angel Pons has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84039?usp=email )
Change subject: cbmem.h: Change return type of cbmem_get_region
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/84039?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ib3e09a75380faf9f533601368993261f042422ef
Gerrit-Change-Number: 84039
Gerrit-PatchSet: 7
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.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: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Fri, 23 Aug 2024 14:02:20 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84044?usp=email )
Change subject: Makefile.mk: Suppress stack-usage LTO link warning
......................................................................
Patch Set 7:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/84044/comment/efe41caa_23cba852?us… :
PS3, Line 7: Makefile.mk: Suppress stack-usage link warning
> > ... for LTO builds […]
But when the compiler runs with `-flto` would it still warn during "compilation"?
The function in question already has local pragmas to avoid the warning, I guess
that just gets lost... this piece of code has kept us busy for years, maybe it's
time we look into a heap solution.
```
/* TODO: This code is quite possibly broken and overflowing stacks. Fix ASAP! */
```
From 2014, lol :D
--
To view, visit https://review.coreboot.org/c/coreboot/+/84044?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: If08d6d543a4fcff07003af8d1f8dd59ab79f42f8
Gerrit-Change-Number: 84044
Gerrit-PatchSet: 7
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 13:51:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84043?usp=email )
Change subject: util/cbfstool: Deal with how lld organizes loadable segments
......................................................................
Patch Set 7:
(1 comment)
File util/cbfstool/cbfs-mkstage.c:
https://review.coreboot.org/c/coreboot/+/84043/comment/d2f2e300_bd931faa?us… :
PS7, Line 301: prev = cur
> Hmmm, forgot about the return value that should include the XIP segment, right?
Maybe I'm confusing myself too much. What do we actually try to assert?
That things for `_data .. _edata` are consecutive, I guess?
Looking at a BFD linked romstage, I would check `vaddr` there as well:
```
$ readelf --segments build/cbfs/fallback/romstage.elf
Elf file type is EXEC (Executable file)
Entry point 0x2000031
There are 2 program headers, starting at offset 52
Program Headers:
Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
LOAD 0x000080 0x02000000 0x02000000 0x0c1d0 0x0c1d0 R E 0x20
LOAD 0x00c250 0xff7cb0a8 0x0200c1d0 0x00018 0x00018 RW 0x4
Section to Segment mapping:
Segment Sections...
00 .text
01 .data
```
Just not compare the executable segment.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84043?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1a0541c8ea3dfbebfba83d505d84b6db12000723
Gerrit-Change-Number: 84043
Gerrit-PatchSet: 7
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 13:29:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84043?usp=email )
Change subject: util/cbfstool: Deal with how lld organizes loadable segments
......................................................................
Patch Set 7:
(1 comment)
File util/cbfstool/cbfs-mkstage.c:
https://review.coreboot.org/c/coreboot/+/84043/comment/a20c1179_d76f772c?us… :
PS7, Line 301: prev = cur
> The whole loop looks suspicious. Unconditionally setting `prev` means […]
Hmmm, forgot about the return value that should include the XIP segment, right?
--
To view, visit https://review.coreboot.org/c/coreboot/+/84043?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1a0541c8ea3dfbebfba83d505d84b6db12000723
Gerrit-Change-Number: 84043
Gerrit-PatchSet: 7
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 13:01:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84043?usp=email )
Change subject: util/cbfstool: Deal with how lld organizes loadable segments
......................................................................
Patch Set 7:
(1 comment)
File util/cbfstool/cbfs-mkstage.c:
https://review.coreboot.org/c/coreboot/+/84043/comment/c8903520_d6bd3466?us… :
PS7, Line 301: prev = cur
The whole loop looks suspicious. Unconditionally setting `prev` means
we potentially run the comparison against segments `!= PT_LOAD`, right?
Could we do something like checking `!(.p_flags & PF_X)` and only update
`prev` if `cur` is a loadable, not executable segment? That should avoid
the special lld case, I guess. (all assuming that `PF_X` is set as one
would expect)
Also, given that we are about to make an XIP assumption either way, we
should rename the function.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84043?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1a0541c8ea3dfbebfba83d505d84b6db12000723
Gerrit-Change-Number: 84043
Gerrit-PatchSet: 7
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 12:58:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Angel Pons, Nico Huber.
Arthur Heymans has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84042?usp=email )
Change subject: soc/intel/pmclib.c: Work around compiler bug -Werror=stringop-overread
......................................................................
Patch Set 7:
(1 comment)
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/84042/comment/ed5e56a5_3602bb3f?us… :
PS7, Line 104: for (i = num_bits - 1; i >= 0; i--) {
> I don't get it, it should never enter the loop body with `num_bits == 0`, right?
> On first sight, this looks like a compiler bug? What did it complain about specifically?
src/soc/intel/common/block/pmc/pmclib.c:355:17: error: 'print_num_status_bits' reading 4 bytes from a region of size 0 [-Werror=stringop-overread]
355 | print_num_status_bits(array_size, gpe_sts, sts_arr);
| ^
src/soc/intel/common/block/pmc/pmclib.c:355:17: note: referencing argument 3 of type 'const char * const[0:]'
src/soc/intel/common/block/pmc/pmclib.c: In function 'smm_southbridge_clear_state':
src/soc/intel/common/block/pmc/pmclib.c:96:13: note: in a call to function 'print_num_status_bits'
96 | static void print_num_status_bits(int num_bits, uint32_t status,
Changing the signature of the function from an array to a pointer is a cleaner solution it seems.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84042?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ieee6e9bddc4e738eb560dd0e69dc3087ac9f5da6
Gerrit-Change-Number: 84042
Gerrit-PatchSet: 7
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 23 Aug 2024 12:53:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Attention is currently required from: Angel Pons, Arthur Heymans.
Hello Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/84042?usp=email
to look at the new patch set (#8).
Change subject: soc/intel/pmclib.c: Work around compiler bug -Werror=stringop-overread
......................................................................
soc/intel/pmclib.c: Work around compiler bug -Werror=stringop-overread
On xeon-sp this is a zero length array. With GCC LTO this triggers the
stringop-overread warning. To work around this change the signature of
the function from an array to a pointer.
Change-Id: Ieee6e9bddc4e738eb560dd0e69dc3087ac9f5da6
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/soc/intel/common/block/pmc/pmclib.c
1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/84042/8
--
To view, visit https://review.coreboot.org/c/coreboot/+/84042?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ieee6e9bddc4e738eb560dd0e69dc3087ac9f5da6
Gerrit-Change-Number: 84042
Gerrit-PatchSet: 8
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84042?usp=email )
Change subject: soc/intel/pmclib.c: Make sure array_size is not 0
......................................................................
Patch Set 7:
(1 comment)
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/84042/comment/248233b9_02b88c21?us… :
PS7, Line 104: for (i = num_bits - 1; i >= 0; i--) {
I don't get it, it should never enter the loop body with `num_bits == 0`, right?
On first sight, this looks like a compiler bug? What did it complain about specifically?
--
To view, visit https://review.coreboot.org/c/coreboot/+/84042?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ieee6e9bddc4e738eb560dd0e69dc3087ac9f5da6
Gerrit-Change-Number: 84042
Gerrit-PatchSet: 7
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 12:31:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84041?usp=email )
Change subject: drivers/intel/opregion.c: Also set vbt_size if size is 0
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
File src/drivers/intel/gma/opregion.c:
https://review.coreboot.org/c/coreboot/+/84041/comment/73f3aa8d_d662c24a?us… :
PS7, Line 28: goto out;
Guessing this is a path that is hard to analyze... :D
--
To view, visit https://review.coreboot.org/c/coreboot/+/84041?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I4fcc6c02f898640e9b40d769e1165a4a0fb0fdf2
Gerrit-Change-Number: 84041
Gerrit-PatchSet: 7
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 12:13:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes