coreboot-gerrit March 2024

coreboot-gerrit@coreboot.org

1 participants
3668 discussions

[L] Change in coreboot[main]: arch/riscv: remove misaligned load/store/fetch handling
by ron minnich (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Maximilian Brune, Philipp Hug. ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81416?usp=email ) Change subject: arch/riscv: remove misaligned load/store/fetch handling ...................................................................... Patch Set 6: (1 comment) File src/arch/riscv/trap_handler.c: https://review.coreboot.org/c/coreboot/+/81416/comment/76b71104_ad914999 : PS4, Line 140: case CAUSE_ILLEGAL_INSTRUCTION: > Why did you move CAUSE_ILLEGAL_INSTRUCTION? Could you mention it in the commit-msg? Done -- To view, visit https://review.coreboot.org/c/coreboot/+/81416?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: I84a8d433ed2f50745686a8c109d101e8718f2a46 Gerrit-Change-Number: 81416 Gerrit-PatchSet: 6 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:59:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: arch/riscv: remove misaligned load/store/fetch handling
by ron minnich (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Philipp Hug, ron minnich. Hello Maximilian Brune, Philipp Hug, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81416?usp=email to look at the new patch set (#6). Change subject: arch/riscv: remove misaligned load/store/fetch handling ...................................................................... arch/riscv: remove misaligned load/store/fetch handling Testing on the unmatched shows the code no longer works completely correctly; Linux has taken over the handling of misalignment anyway, because handling it in firmware, with the growing complexity of the ISA and the awkward way in which it has to be handled, is more trouble than its worth. Plus, we don't WANT misalignment handled, magically, in firmware: the cost of getting it wrong is high (as I've spent a month learning); the performance is terrible (350x slowdown; and most toolchains now know to avoid unaligned load/store on RISC-V anyway. But, mostly, if alignment problems exist, *we need to know*, and if they're handled invisibly in firmware, we don't. The problem with invisble handling was shown a while back in the Go toolchain: runtime had a small error, such that many misaligned load/store were happening, and it was not discovered for some time. Had a trap been directed to kernel or user on misalignment, the problem would have been known immediately, not after many months. (The error, btw, was masking the address with 3, not 7, to detect misalignment; an easy mistake!). But, the coreboot code does not work any more any way, and it's not worth fixing. Remove it. Tested by booting Linux to runlevel 1; before, it would hang on an alignment fault, as the alignment code was failing (somewhere). This takes the coreboot SBI code much closer to revival. To prepare for later support for older SoC, break out CAUSE_ILLEGAL_INSTRUCTION to its own separate part of the switch. Change-Id: I84a8d433ed2f50745686a8c109d101e8718f2a46 Signed-off-by: Ronald G Minnich <rminnich(a)gmail.com> --- M src/arch/riscv/Makefile.mk M src/arch/riscv/include/arch/exception.h D src/arch/riscv/misaligned.c M src/arch/riscv/trap_handler.c M src/arch/riscv/virtual_memory.c 5 files changed, 4 insertions(+), 265 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/81416/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/81416?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: I84a8d433ed2f50745686a8c109d101e8718f2a46 Gerrit-Change-Number: 81416 Gerrit-PatchSet: 6 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: ron minnich <rminnich(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[L] Change in coreboot[main]: mb/erying: Add Erying Polestar G613 Pro (TGL-H)
by Alicja Michalska (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Felix Singer, Michał Żygowski, Nicholas Chin, Paul Menzel. Alicja Michalska has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80853?usp=email ) Change subject: mb/erying: Add Erying Polestar G613 Pro (TGL-H) ...................................................................... Patch Set 2: (1 comment) File src/mainboard/erying/tgl/devicetree.cb: https://review.coreboot.org/c/coreboot/+/80853/comment/b4d7e37f_ed83d414 : PS2, Line 197: end > Looks like this corresponded to the `device domain 0` entry that was removed, so now it's superfluou […] It seems so, my bad for not building locally to test it. Working on fixing it right now 😊 -- To view, visit https://review.coreboot.org/c/coreboot/+/80853?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: Iffb9e357da2eb686bdcd9a9837df8a60fa94011e Gerrit-Change-Number: 80853 Gerrit-PatchSet: 2 Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:57:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: mb/erying: Add Erying Polestar G613 Pro (TGL-H)
by Nicholas Chin (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Alicja Michalska, Felix Singer, Michał Żygowski, Paul Menzel. Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80853?usp=email ) Change subject: mb/erying: Add Erying Polestar G613 Pro (TGL-H) ...................................................................... Patch Set 2: (1 comment) File src/mainboard/erying/tgl/devicetree.cb: https://review.coreboot.org/c/coreboot/+/80853/comment/cb6dc280_e6002b97 : PS2, Line 197: end Looks like this corresponded to the `device domain 0` entry that was removed, so now it's superfluous, causing the syntax error in the build. It does seem to build if this end line is removed, but I'm not sure if the domain device still needs to be present in the board dt for all the devices under it to work properly, despite it already being on in the chipset dt. The generated static.c files do seem to differ if domain 0 is present in the mb dt vs not, so it does at least make some difference. I'm not super familiar with how this should work, but I'd assume it does need to be present. -- To view, visit https://review.coreboot.org/c/coreboot/+/80853?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: Iffb9e357da2eb686bdcd9a9837df8a60fa94011e Gerrit-Change-Number: 80853 Gerrit-PatchSet: 2 Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:56:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access
by Bora Guvendik (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Andrey Petrov, Jérémy Compostella, Ronak Kanabar. Bora Guvendik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81256?usp=email ) Change subject: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access ...................................................................... Patch Set 3: Code-Review+2 (1 comment) File src/drivers/intel/fsp2_0/memory_init.c: https://review.coreboot.org/c/coreboot/+/81256/comment/cea69b77_b0019a08 : PS3, Line 267: hdr > I did not because this is a static function. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/81256?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: If355b5811a09a0b76acc8a297db719d54caedc54 Gerrit-Change-Number: 81256 Gerrit-PatchSet: 3 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:52:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Bora Guvendik <bora.guvendik(a)intel.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access
by Arthur Heymans (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Andrey Petrov, Bora Guvendik, Jérémy Compostella, Ronak Kanabar. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81256?usp=email ) Change subject: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/81256?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: If355b5811a09a0b76acc8a297db719d54caedc54 Gerrit-Change-Number: 81256 Gerrit-PatchSet: 3 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:51:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: superio/ite: Add special fan vectors and further options
by Matt DeVillier (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Joel Linn, Nico Huber. Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81426?usp=email ) Change subject: superio/ite: Add special fan vectors and further options ...................................................................... Patch Set 4: (1 comment) File src/superio/ite/common/env_ctrl.c: https://review.coreboot.org/c/coreboot/+/81426/comment/954bf4fa_ce2996c5 : PS1, Line 369: #endif > tl;dr I think you'd just have to free the API from the problematic index. […] I still think keeping ITE_EC_FAN_VECTOR_CNT fixed at 2 and letting the preprocessor exclude the loop via Kconfig is the least-worst option here -- To view, visit https://review.coreboot.org/c/coreboot/+/81426?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: I93df2b5652fc3fde775b6161fa5bebc4a34d5e94 Gerrit-Change-Number: 81426 Gerrit-PatchSet: 4 Gerrit-Owner: Joel Linn <jl_coreboot(a)conductive.de> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Joel Linn <jl_coreboot(a)conductive.de> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:51:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: Joel Linn <jl_coreboot(a)conductive.de> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access
by Jérémy Compostella (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Andrey Petrov, Bora Guvendik, Ronak Kanabar. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81256?usp=email ) Change subject: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access ...................................................................... Patch Set 3: (1 comment) File src/drivers/intel/fsp2_0/memory_init.c: https://review.coreboot.org/c/coreboot/+/81256/comment/74e26cca_d063d220 : PS3, Line 267: hdr > could hdr be null? worth adding a sanity check? I did not because this is a static function. -- To view, visit https://review.coreboot.org/c/coreboot/+/81256?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: If355b5811a09a0b76acc8a297db719d54caedc54 Gerrit-Change-Number: 81256 Gerrit-PatchSet: 3 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:47:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: superio/ite: Add special fan vectors and further options
by Nico Huber (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Joel Linn, Matt DeVillier. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81426?usp=email ) Change subject: superio/ite: Add special fan vectors and further options ...................................................................... Patch Set 4: (1 comment) File src/superio/ite/common/env_ctrl.c: https://review.coreboot.org/c/coreboot/+/81426/comment/22f97944_47779e46 : PS1, Line 369: #endif > A new C file would still require the code to be called from somewhere and thus yield similar "proble […] tl;dr I think you'd just have to free the API from the problematic index. AFAICT, you'd have to move all the code that references `->fan_vector[]` or the index. So that includes the `for` loop here that could be moved into an additional function. When configuring the Makefile.mk to only build it with the Kconfig set, e.g. ``` ramstage-$(CONFIG_SUPERIO_ITE_ENV_CTRL_FAN_VECTOR) += env_ctrl_fan_vector.c ``` the compiler would never see the code unless ITE_EC_FAN_VECTOR_COUNT is 2. -- To view, visit https://review.coreboot.org/c/coreboot/+/81426?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: I93df2b5652fc3fde775b6161fa5bebc4a34d5e94 Gerrit-Change-Number: 81426 Gerrit-PatchSet: 4 Gerrit-Owner: Joel Linn <jl_coreboot(a)conductive.de> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Joel Linn <jl_coreboot(a)conductive.de> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:46:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: Joel Linn <jl_coreboot(a)conductive.de> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access
by Bora Guvendik (Code Review) 25 Mar '24

25 Mar '24

Attention is currently required from: Andrey Petrov, Jérémy Compostella, Ronak Kanabar. Bora Guvendik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81256?usp=email ) Change subject: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access ...................................................................... Patch Set 3: (1 comment) File src/drivers/intel/fsp2_0/memory_init.c: https://review.coreboot.org/c/coreboot/+/81256/comment/53d76027_557e6527 : PS3, Line 267: hdr could hdr be null? worth adding a sanity check? -- To view, visit https://review.coreboot.org/c/coreboot/+/81256?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: If355b5811a09a0b76acc8a297db719d54caedc54 Gerrit-Change-Number: 81256 Gerrit-PatchSet: 3 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Mon, 25 Mar 2024 16:41:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2024