coreboot-gerrit May 2024

coreboot-gerrit@coreboot.org

1 participants
3266 discussions

[L] Change in coreboot[main]: soc/intel/skylake: Drop redundant PcieRpEnable
by Angel Pons (Code Review) 29 May '24

29 May '24

Attention is currently required from: Erik van den Bogaert, Frans Hendriks, Jan Samek, Jeremy Soller, Jonathon Hall, Michael Niewöhner, Michał Żygowski, Nico Huber, Piotr Król, Reagan, Sean Rhodes, Tim Crawford. Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/79917?usp=email ) Change subject: soc/intel/skylake: Drop redundant PcieRpEnable ...................................................................... Patch Set 2: (1 comment) File src/mainboard/razer/blade_stealth_kbl/devicetree.cb: https://review.coreboot.org/c/coreboot/+/79917/comment/754b4df6_279602f1?us… : PS2, Line 190: device ref pcie_rp1 on end > IIRC, it's because a PCI device has to have a function 0 so it can be enumerated. Yes. > If the root port corresponding to function 0 isn't enabled, the PCH promotes the first enabled function to function 0. It's actually software-configurable, and I think you can swap any set of functions. However, FSP does this configuration on newer Intel platforms. On older Intel platforms, coreboot configures this and (depending on the platform) can even set up function numbers so that they're contiguous ("coalesce" them); see `RPFN` register in southbridge code as well as `pcie_port_coalesce` devicetree setting. -- To view, visit https://review.coreboot.org/c/coreboot/+/79917?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: I2f7e3e1dc6b3d8d6159bd4701e6fd90f4b0f67f4 Gerrit-Change-Number: 79917 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Jan Samek <samekh(a)email.cz> Gerrit-Reviewer: Jeremy Soller <jeremy(a)system76.com> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Reagan <xbjfk.github(a)gmail.com> Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Attention: Jeremy Soller <jeremy(a)system76.com> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Reagan <xbjfk.github(a)gmail.com> Gerrit-Attention: Jan Samek <samekh(a)email.cz> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Wed, 29 May 2024 13:51:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jonathon Hall <jonathon.hall(a)puri.sm> Comment-In-Reply-To: Reagan <xbjfk.github(a)gmail.com>

1 0

[XS] Change in coreboot[main]: mb/siemens/mc_ehl5: Remove DDI settings from devicetree
by Mario Scheithauer (Code Review) 29 May '24

29 May '24

Attention is currently required from: Paul Menzel. Mario Scheithauer has posted comments on this change by Mario Scheithauer. ( https://review.coreboot.org/c/coreboot/+/82663?usp=email ) Change subject: mb/siemens/mc_ehl5: Remove DDI settings from devicetree ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/82663/comment/9783d512_6f66a1e8?us… : PS1, Line 9: Since this mainboard no longer uses the FSP GOP driver > Maybe reference the commit? Do you use libgfxinit now? For posterity for readers without background […] The GOP driver was used in the initial phase of development where we used tianocore as payload for some test cases. The graphic is now initialized in our self-made Linux payload. -- To view, visit https://review.coreboot.org/c/coreboot/+/82663?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: Ie9e135fbc2627546d6ef95d7d5ff3e9a9222b5d2 Gerrit-Change-Number: 82663 Gerrit-PatchSet: 1 Gerrit-Owner: Mario Scheithauer <mario.scheithauer(a)siemens.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Uwe Poeche <uwe.poeche(a)siemens.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 29 May 2024 13:44:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>

1 0

[XS] Change in coreboot[main]: mb/siemens/mc_ehl5: Remove DDI settings from devicetree
by Paul Menzel (Code Review) 29 May '24

29 May '24

Attention is currently required from: Mario Scheithauer. Paul Menzel has posted comments on this change by Mario Scheithauer. ( https://review.coreboot.org/c/coreboot/+/82663?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: mb/siemens/mc_ehl5: Remove DDI settings from devicetree ...................................................................... Patch Set 1: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/82663/comment/77bd57f6_810f0090?us… : PS1, Line 9: Since this mainboard no longer uses the FSP GOP driver Maybe reference the commit? Do you use libgfxinit now? For posterity for readers without background knowledge: …, and Siemens is the only entity using this board and not selling it, so no other payloads need to be supported, … -- To view, visit https://review.coreboot.org/c/coreboot/+/82663?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: Ie9e135fbc2627546d6ef95d7d5ff3e9a9222b5d2 Gerrit-Change-Number: 82663 Gerrit-PatchSet: 1 Gerrit-Owner: Mario Scheithauer <mario.scheithauer(a)siemens.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Uwe Poeche <uwe.poeche(a)siemens.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Mario Scheithauer <mario.scheithauer(a)siemens.com> Gerrit-Comment-Date: Wed, 29 May 2024 13:30:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[XS] Change in coreboot[main]: util/smmstoretool: explain what happens when no store is found
by Maciej Pijanowski (Code Review) 29 May '24

29 May '24

Attention is currently required from: Maciej Pijanowski. Hello Sergii Dmytruk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/82692?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: util/smmstoretool: explain what happens when no store is found ...................................................................... util/smmstoretool: explain what happens when no store is found We are going to expose ths tool to end users, and want to take care that the presented information can be consumed by them. The current code simply prints below warnings if we use release binary available for end-user to download: No firmware volume header present No valid firmware volume was found It will be concerning and not clear to end users, they might not understant why it happens, what are the implications, and whether it is something that they should worry about. This commit tries to explain what actually happens here. Change-Id: Iaa2678f5ae7c243811484c0567ced97ae0b3fc0a Signed-off-by: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> --- M util/smmstoretool/storage.c 1 file changed, 6 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/82692/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/82692?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: Iaa2678f5ae7c243811484c0567ced97ae0b3fc0a Gerrit-Change-Number: 82692 Gerrit-PatchSet: 3 Gerrit-Owner: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>

1 0

[S] Change in coreboot[main]: payloads/iPXE: Add an option for reproducible building.
by Filip Lewiński (Code Review) 29 May '24

29 May '24

Attention is currently required from: Martin L Roth, Michał Kopeć, Michał Żygowski, Paul Menzel. Hello Krystian Hebel, Martin L Roth, Michał Kopeć, Michał Żygowski, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/82039?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: payloads/iPXE: Add an option for reproducible building. ...................................................................... payloads/iPXE: Add an option for reproducible building. Allows to set a custom 32 bit hex build_id option for an iPXE build. Useful for reproducibility. Change-Id: I926b096e77a9d45ba2beecd1fc2b7d0a9b9a7b9c Signed-off-by: Krystian Hebel <krystian.hebel(a)3mdeb.com> --- M payloads/external/Makefile.mk M payloads/external/iPXE/Kconfig M payloads/external/iPXE/Makefile 3 files changed, 21 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/82039/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/82039?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: I926b096e77a9d45ba2beecd1fc2b7d0a9b9a7b9c Gerrit-Change-Number: 82039 Gerrit-PatchSet: 4 Gerrit-Owner: Filip Lewiński <filip.lewinski(a)3mdeb.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com>

1 0

[S] Change in coreboot[main]: payloads/iPXE: Hook up TRUST_CMD switch
by Filip Lewiński (Code Review) 29 May '24

29 May '24

Attention is currently required from: Felix Singer, Filip Lewiński, Krystian Hebel, Maciej Pijanowski, Martin L Roth, Michał Kopeć, Michał Żygowski, Paul Menzel, Stefan Reinauer. Filip Lewiński has uploaded a new patch set (#10) to the change originally created by Maciej Pijanowski. ( https://review.coreboot.org/c/coreboot/+/79684?usp=email ) The following approvals got outdated and were removed: Code-Review+1 by Paul Menzel, Verified+1 by build bot (Jenkins) Change subject: payloads/iPXE: Hook up TRUST_CMD switch ...................................................................... payloads/iPXE: Hook up TRUST_CMD switch Change-Id: Ia4f5d4140eeb8625c5ee41e38f048658db28a199 Signed-off-by: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> --- M payloads/external/Makefile.mk M payloads/external/iPXE/Kconfig M payloads/external/iPXE/Makefile 3 files changed, 13 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/79684/10 -- To view, visit https://review.coreboot.org/c/coreboot/+/79684?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: Ia4f5d4140eeb8625c5ee41e38f048658db28a199 Gerrit-Change-Number: 79684 Gerrit-PatchSet: 10 Gerrit-Owner: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Filip Lewiński <filip.lewinski(a)3mdeb.com> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Filip Lewiński <filip.lewinski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>

1 0

[L] Change in coreboot[main]: soc/intel/skylake: Drop redundant PcieRpEnable
by Jonathon Hall (Code Review) 29 May '24

29 May '24

Attention is currently required from: Erik van den Bogaert, Frans Hendriks, Jan Samek, Jeremy Soller, Michael Niewöhner, Michał Żygowski, Nico Huber, Piotr Król, Reagan, Sean Rhodes, Tim Crawford. Jonathon Hall has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/79917?usp=email ) Change subject: soc/intel/skylake: Drop redundant PcieRpEnable ...................................................................... Patch Set 2: (2 comments) File src/mainboard/purism/librem_skl/devicetree.cb: https://review.coreboot.org/c/coreboot/+/79917/comment/6cb388e0_9e4214eb?us… : PS2, Line 141: register "PcieRpEnable[4]" = "1" > Tested librem_13v2. The device tree should have pcie_rp5, not pcie_rp1. […] I fixed librem_skl in CB:80065. File src/mainboard/razer/blade_stealth_kbl/devicetree.cb: https://review.coreboot.org/c/coreboot/+/79917/comment/d4055f97_8a488bf5?us… : PS2, Line 190: device ref pcie_rp1 on end > I happen to have a very similar board (see CB:82506). You are indeed correct. […] IIRC, it's because a PCI device has to have a function 0 so it can be enumerated. If the root port corresponding to function 0 isn't enabled, the PCH promotes the first enabled function to function 0. -- To view, visit https://review.coreboot.org/c/coreboot/+/79917?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: I2f7e3e1dc6b3d8d6159bd4701e6fd90f4b0f67f4 Gerrit-Change-Number: 79917 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Jan Samek <samekh(a)email.cz> Gerrit-Reviewer: Jeremy Soller <jeremy(a)system76.com> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Reagan <xbjfk.github(a)gmail.com> Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Attention: Jeremy Soller <jeremy(a)system76.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Reagan <xbjfk.github(a)gmail.com> Gerrit-Attention: Jan Samek <samekh(a)email.cz> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Wed, 29 May 2024 13:12:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jonathon Hall <jonathon.hall(a)puri.sm> Comment-In-Reply-To: Reagan <xbjfk.github(a)gmail.com>

1 0

[L] Change in coreboot[main]: soc/intel/skylake: Drop redundant PcieRpEnable
by Angel Pons (Code Review) 29 May '24

29 May '24

Attention is currently required from: Erik van den Bogaert, Frans Hendriks, Jan Samek, Jeremy Soller, Jonathon Hall, Michael Niewöhner, Michał Żygowski, Nico Huber, Piotr Król, Reagan, Sean Rhodes, Tim Crawford. Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/79917?usp=email ) Change subject: soc/intel/skylake: Drop redundant PcieRpEnable ...................................................................... Patch Set 2: (1 comment) File src/mainboard/razer/blade_stealth_kbl/devicetree.cb: https://review.coreboot.org/c/coreboot/+/79917/comment/48c550a0_a2ed256e?us… : PS2, Line 190: device ref pcie_rp1 on end > I happen to have a very similar board (see CB:82506). You are indeed correct. […] The PCI specification says that multi-function devices must always implement function 0. To provide flexibility, Intel allows remapping PCH PCIe root port functions so that root port 1 doesn't have to be enabled when it's not in use. In `lspci`, the device names will list the root port numbers regardless of function swapping. This can be useful to know which root ports should be enabled. As for this board, I can +2 a change that adds `device ref pcie_rp3 on end` without removing any other lines (even without testing, I'm fairly sure it wouldn't cause any regressions). -- To view, visit https://review.coreboot.org/c/coreboot/+/79917?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: I2f7e3e1dc6b3d8d6159bd4701e6fd90f4b0f67f4 Gerrit-Change-Number: 79917 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Jan Samek <samekh(a)email.cz> Gerrit-Reviewer: Jeremy Soller <jeremy(a)system76.com> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Reagan <xbjfk.github(a)gmail.com> Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Attention: Jeremy Soller <jeremy(a)system76.com> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Reagan <xbjfk.github(a)gmail.com> Gerrit-Attention: Jan Samek <samekh(a)email.cz> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Wed, 29 May 2024 13:09:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jonathon Hall <jonathon.hall(a)puri.sm> Comment-In-Reply-To: Reagan <xbjfk.github(a)gmail.com>

1 0

[M] Change in coreboot[main]: cpu/x86/pae/pgtbl.c: extract reusable code from memset_pae()
by Felix Held (Code Review) 29 May '24

29 May '24

Felix Held has posted comments on this change by Krystian Hebel. ( https://review.coreboot.org/c/coreboot/+/82249?usp=email ) Change subject: cpu/x86/pae/pgtbl.c: extract reusable code from memset_pae() ...................................................................... Patch Set 4: (1 comment) File src/cpu/x86/pae/pgtbl.c: https://review.coreboot.org/c/coreboot/+/82249/comment/7b074c09_00357358?us… : PS3, Line 113: * @return 0 on success, 1 on error not directly related to this patch, but something for a follow-up: it would make the code much easier to read if enum cb_err was used here -- To view, visit https://review.coreboot.org/c/coreboot/+/82249?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: I8aa80eb246ff0e77e1f51d71933d3d00ab75aaeb Gerrit-Change-Number: 82249 Gerrit-PatchSet: 4 Gerrit-Owner: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 29 May 2024 13:04:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in coreboot[main]: cpu/x86/pae/pgtbl.c: extract reusable code from memset_pae()
by Felix Held (Code Review) 29 May '24

29 May '24

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/82249?usp=email ) Change subject: cpu/x86/pae/pgtbl.c: extract reusable code from memset_pae() ...................................................................... cpu/x86/pae/pgtbl.c: extract reusable code from memset_pae() Code dealing with PAE can be used outside of memset_pae(). This change extracts creation of identity mapped pagetables to init_pae_pagetables() and mapping of single 2 MiB map to pae_map_2M_page(). Both functions are exported in include/cpu/x86/pae.h to allow use outside of pgtbl.c. MEMSET_PAE_* macros were renamed to PAE_* since they no longer apply only to memset_pae(). Change-Id: I8aa80eb246ff0e77e1f51d71933d3d00ab75aaeb Signed-off-by: Krystian Hebel <krystian.hebel(a)3mdeb.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/82249 Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/cpu/x86/pae/pgtbl.c M src/include/cpu/x86/pae.h M src/security/memory/memory_clear.c 3 files changed, 115 insertions(+), 50 deletions(-) Approvals: build bot (Jenkins): Verified Sergii Dmytruk: Looks good to me, approved diff --git a/src/cpu/x86/pae/pgtbl.c b/src/cpu/x86/pae/pgtbl.c index e1803aa..3df6122 100644 --- a/src/cpu/x86/pae/pgtbl.c +++ b/src/cpu/x86/pae/pgtbl.c @@ -101,6 +101,97 @@ } /* + * Prepare PAE pagetables that identity map the whole 32-bit address space using + * 2 MiB pages. The PAT are set to all cacheable, but MTRRs still apply. CR3 is + * loaded and PAE is enabled by this function. + * + * Requires a scratch memory for pagetables. + * + * @param pgtbl Where pagetables reside, must be 4 KiB aligned and 20 KiB in + * size. + * Content at physical address isn't preserved. + * @return 0 on success, 1 on error + */ +int init_pae_pagetables(void *pgtbl) +{ + struct pg_table *pgtbl_buf = (struct pg_table *)pgtbl; + struct pde *pd = pgtbl_buf->pd, *pdp = pgtbl_buf->pdp; + + printk(BIOS_DEBUG, "%s: Using address %p for page tables\n", + __func__, pgtbl_buf); + + /* Cover some basic error conditions */ + if (!IS_ALIGNED((uintptr_t)pgtbl_buf, s4KiB)) { + printk(BIOS_ERR, "%s: Invalid alignment\n", __func__); + return 1; + } + + paging_disable_pae(); + + /* Point the page directory pointers at the page directories. */ + memset(pgtbl_buf->pdp, 0, sizeof(pgtbl_buf->pdp)); + + pdp[0].addr_lo = ((uintptr_t)&pd[512*0]) | PDPTE_PRES; + pdp[1].addr_lo = ((uintptr_t)&pd[512*1]) | PDPTE_PRES; + pdp[2].addr_lo = ((uintptr_t)&pd[512*2]) | PDPTE_PRES; + pdp[3].addr_lo = ((uintptr_t)&pd[512*3]) | PDPTE_PRES; + + /* Identity map the whole 32-bit address space */ + for (size_t i = 0; i < 2048; i++) { + pd[i].addr_lo = (i << PDE_IDX_SHIFT) | PDE_PS | PDE_PRES | PDE_RW; + pd[i].addr_hi = 0; + } + + paging_enable_pae_cr3((uintptr_t)pdp); + + return 0; +} + +/* + * Map single 2 MiB page in pagetables created by init_pae_pagetables(). + * + * The function does not check if the page was already non identity mapped, + * this allows callers to reuse one page without having to explicitly unmap it + * between calls. + * + * @param pgtbl Where pagetables created by init_pae_pagetables() reside. + * Content at physical address is preserved except for single + * entry corresponding to vmem_addr. + * @param paddr Physical memory address to map. Function prints a warning if + * it isn't aligned to 2 MiB. + * @param vmem_addr Where the virtual non identity mapped page resides, must + * be at least 2 MiB in size. Function prints a warning if it + * isn't aligned to 2 MiB. + * Content at physical address is preserved. + * @return 0 on success, 1 on error + */ +void pae_map_2M_page(void *pgtbl, uint64_t paddr, void *vmem_addr) +{ + struct pg_table *pgtbl_buf = (struct pg_table *)pgtbl; + struct pde *pd; + + if (!IS_ALIGNED(paddr, s2MiB)) { + printk(BIOS_WARNING, "%s: Aligning physical address to 2MiB\n", + __func__); + paddr = ALIGN_DOWN(paddr, s2MiB); + } + + if (!IS_ALIGNED((uintptr_t)vmem_addr, s2MiB)) { + printk(BIOS_WARNING, "%s: Aligning virtual address to 2MiB\n", + __func__); + vmem_addr = (void *)ALIGN_DOWN((uintptr_t)vmem_addr, s2MiB); + } + + /* Map a page using PAE at virtual address vmem_addr. */ + pd = &pgtbl_buf->pd[((uintptr_t)vmem_addr) >> PDE_IDX_SHIFT]; + pd->addr_lo = paddr | PDE_PS | PDE_PRES | PDE_RW; + pd->addr_hi = paddr >> 32; + + /* Update page tables */ + asm volatile ("invlpg (%0)" :: "b"(vmem_addr) : "memory"); +} + +/* * Use PAE to map a page and then memset it with the pattern specified. * In order to use PAE pagetables for virtual addressing are set up and reloaded * on a 2MiB boundary. After the function is done, virtual addressing mode is @@ -130,22 +221,18 @@ int memset_pae(uint64_t dest, unsigned char pat, uint64_t length, void *pgtbl, void *vmem_addr) { - struct pg_table *pgtbl_buf = (struct pg_table *)pgtbl; ssize_t offset; + const uintptr_t pgtbl_s = (uintptr_t)pgtbl; + const uintptr_t pgtbl_e = pgtbl_s + sizeof(struct pg_table); printk(BIOS_DEBUG, "%s: Using virtual address %p as scratchpad\n", __func__, vmem_addr); - printk(BIOS_DEBUG, "%s: Using address %p for page tables\n", - __func__, pgtbl_buf); /* Cover some basic error conditions */ - if (!IS_ALIGNED((uintptr_t)pgtbl_buf, s4KiB) || - !IS_ALIGNED((uintptr_t)vmem_addr, s2MiB)) { + if (!IS_ALIGNED((uintptr_t)vmem_addr, s2MiB)) { printk(BIOS_ERR, "%s: Invalid alignment\n", __func__); return 1; } - const uintptr_t pgtbl_s = (uintptr_t)pgtbl_buf; - const uintptr_t pgtbl_e = pgtbl_s + sizeof(struct pg_table); if (OVERLAP(dest, dest + length, pgtbl_s, pgtbl_e)) { printk(BIOS_ERR, "%s: destination overlaps page tables\n", @@ -160,31 +247,12 @@ return 1; } - paging_disable_pae(); - - struct pde *pd = pgtbl_buf->pd, *pdp = pgtbl_buf->pdp; - /* Point the page directory pointers at the page directories. */ - memset(pgtbl_buf->pdp, 0, sizeof(pgtbl_buf->pdp)); - - pdp[0].addr_lo = ((uintptr_t)&pd[512*0]) | PDPTE_PRES; - pdp[1].addr_lo = ((uintptr_t)&pd[512*1]) | PDPTE_PRES; - pdp[2].addr_lo = ((uintptr_t)&pd[512*2]) | PDPTE_PRES; - pdp[3].addr_lo = ((uintptr_t)&pd[512*3]) | PDPTE_PRES; + if (init_pae_pagetables(pgtbl)) + return 1; offset = dest - ALIGN_DOWN(dest, s2MiB); dest = ALIGN_DOWN(dest, s2MiB); - /* Identity map the whole 32-bit address space */ - for (size_t i = 0; i < 2048; i++) { - pd[i].addr_lo = (i << PDE_IDX_SHIFT) | PDE_PS | PDE_PRES | PDE_RW; - pd[i].addr_hi = 0; - } - - /* Get pointer to PD that's not identity mapped */ - pd = &pgtbl_buf->pd[((uintptr_t)vmem_addr) >> PDE_IDX_SHIFT]; - - paging_enable_pae_cr3((uintptr_t)pdp); - do { const size_t len = MIN(length, s2MiB - offset); @@ -192,11 +260,7 @@ * Map a page using PAE at virtual address vmem_addr. * dest is already 2 MiB aligned. */ - pd->addr_lo = dest | PDE_PS | PDE_PRES | PDE_RW; - pd->addr_hi = dest >> 32; - - /* Update page tables */ - asm volatile ("invlpg (%0)" :: "b"(vmem_addr) : "memory"); + pae_map_2M_page(pgtbl, dest, vmem_addr); printk(BIOS_SPEW, "%s: Clearing %llx[%lx] - %zx\n", __func__, dest + offset, (uintptr_t)vmem_addr + offset, len); diff --git a/src/include/cpu/x86/pae.h b/src/include/cpu/x86/pae.h index d1cf734..e4b0a2c 100644 --- a/src/include/cpu/x86/pae.h +++ b/src/include/cpu/x86/pae.h @@ -35,11 +35,15 @@ * failure. */ int paging_enable_for_car(const char *pdpt_name, const char *pt_name); -/* To be used with memset_pae */ -#define MEMSET_PAE_VMEM_ALIGN (2 * MiB) -#define MEMSET_PAE_VMEM_SIZE (2 * MiB) -#define MEMSET_PAE_PGTL_ALIGN (4 * KiB) -#define MEMSET_PAE_PGTL_SIZE (20 * KiB) +/* To be used with memset_pae and pae_map_2M_page */ +#define PAE_VMEM_ALIGN (2 * MiB) +#define PAE_VMEM_SIZE (2 * MiB) +#define PAE_PGTL_ALIGN (4 * KiB) +#define PAE_PGTL_SIZE (20 * KiB) + +int init_pae_pagetables(void *pgtbl); + +void pae_map_2M_page(void *pgtbl, uint64_t paddr, void *vmem_addr); int memset_pae(uint64_t dest, unsigned char pat, uint64_t length, void *pgtbl, void *vmem_addr); diff --git a/src/security/memory/memory_clear.c b/src/security/memory/memory_clear.c index 996b1d3..9af3205 100644 --- a/src/security/memory/memory_clear.c +++ b/src/security/memory/memory_clear.c @@ -4,10 +4,10 @@ #include <cpu/x86/pae.h> #else #define memset_pae(a, b, c, d, e) 0 -#define MEMSET_PAE_PGTL_ALIGN 0 -#define MEMSET_PAE_PGTL_SIZE 0 -#define MEMSET_PAE_VMEM_ALIGN 0 -#define MEMSET_PAE_VMEM_SIZE 0 +#define PAE_PGTL_ALIGN 0 +#define PAE_PGTL_SIZE 0 +#define PAE_VMEM_ALIGN 0 +#define PAE_VMEM_SIZE 0 #endif #include <memrange.h> @@ -84,15 +84,12 @@ if (ENV_X86) { /* Find space for PAE enabled memset */ - pgtbl = get_free_memory_range(&mem, MEMSET_PAE_PGTL_ALIGN, - MEMSET_PAE_PGTL_SIZE); + pgtbl = get_free_memory_range(&mem, PAE_PGTL_ALIGN, PAE_PGTL_SIZE); /* Don't touch page tables while clearing */ - memranges_insert(&mem, pgtbl, MEMSET_PAE_PGTL_SIZE, - BM_MEM_TABLE); + memranges_insert(&mem, pgtbl, PAE_PGTL_SIZE, BM_MEM_TABLE); - vmem_addr = get_free_memory_range(&mem, MEMSET_PAE_VMEM_ALIGN, - MEMSET_PAE_VMEM_SIZE); + vmem_addr = get_free_memory_range(&mem, PAE_VMEM_ALIGN, PAE_VMEM_SIZE); printk(BIOS_SPEW, "%s: pgtbl at %p, virt memory at %p\n", __func__, (void *)pgtbl, (void *)vmem_addr); @@ -128,9 +125,9 @@ if (ENV_X86) { /* Clear previously skipped memory reserved for pagetables */ printk(BIOS_DEBUG, "%s: Clearing DRAM %016lx-%016lx\n", - __func__, pgtbl, pgtbl + MEMSET_PAE_PGTL_SIZE); + __func__, pgtbl, pgtbl + PAE_PGTL_SIZE); - memset((void *)pgtbl, 0, MEMSET_PAE_PGTL_SIZE); + memset((void *)pgtbl, 0, PAE_PGTL_SIZE); } memranges_teardown(&mem); -- To view, visit https://review.coreboot.org/c/coreboot/+/82249?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I8aa80eb246ff0e77e1f51d71933d3d00ab75aaeb Gerrit-Change-Number: 82249 Gerrit-PatchSet: 4 Gerrit-Owner: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2024