coreboot-gerrit March 2024

coreboot-gerrit@coreboot.org

1 participants
3667 discussions

[XL] Change in coreboot[main]: util: add smmstoretool for editing SMMSTORE
by Michał Żygowski (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Filip Lewiński, Michał Kopeć, Sergii Dmytruk. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79080?usp=email ) Change subject: util: add smmstoretool for editing SMMSTORE ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/79080?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: I6c1c06f1d0c39c13b5be76a3070f09b715aca6e0 Gerrit-Change-Number: 79080 Gerrit-PatchSet: 8 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Filip Lewiński 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: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Attention: Filip Lewiński Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 01 Mar 2024 09:56:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: soc/intel/xeon_sp: Drop SPI_BASE_ADDRESS from _CRS
by Patrick Rudolph (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Christian Walter, Johnny Lin, Lean Sheng Tan, Shuo Liu, Tim Chu. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80795?usp=email ) Change subject: soc/intel/xeon_sp: Drop SPI_BASE_ADDRESS from _CRS ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/80795/comment/63d0944a_2a3d2e55 : PS1, Line 10: hidden the fast_spi driver will generate the _CRS marking the BAR > If the device is hidden, configuration space read will return invalid values, hence we cannot get th […] Usually only the VENDOR/DEVID changes to 0xffffffff when a device is hidden. The code is here: src/soc/intel/common/block/fast_spi/fast_spi.c -- To view, visit https://review.coreboot.org/c/coreboot/+/80795?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: I150397a7ac5d60719f327f6ac6480a38fe295c32 Gerrit-Change-Number: 80795 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(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: 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: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 01 Mar 2024 09:38:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: mb/google/brox: Update Verbtable for beep functionality
by Poornima Tom (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Shelley Chen, Vamshi Krishna Gopal, Varshit Pandya. Hello Shelley Chen, Vamshi Krishna Gopal, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80416?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: mb/google/brox: Update Verbtable for beep functionality ...................................................................... mb/google/brox: Update Verbtable for beep functionality For boot beep functionality, relevant register values are required to be updated. BUG=b:324528901 BRANCH=None TEST=Build Brox image Change-Id: If236c8ac173a279db676af412377fa4e4122c1cd Signed-off-by: poornima tom <poornima.tom(a)intel.com> --- M src/mainboard/google/brox/hda_verb.c 1 file changed, 11 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/80416/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/80416?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: If236c8ac173a279db676af412377fa4e4122c1cd Gerrit-Change-Number: 80416 Gerrit-PatchSet: 3 Gerrit-Owner: Poornima Tom <poornima.tom(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.corp-partner.google.com> Gerrit-CC: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Attention: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[main]: soc/intel/xeon_sp: Add ACPI names
by Shuo Liu (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Lean Sheng Tan, Patrick Rudolph, Tim Chu. Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80794?usp=email ) Change subject: soc/intel/xeon_sp: Add ACPI names ...................................................................... Patch Set 2: (1 comment) File src/soc/intel/xeon_sp/acpi.c: https://review.coreboot.org/c/coreboot/+/80794/comment/f750f375_b636f5d1 : PS2, Line 144: char *name = xmalloc(ACPI_NAME_BUFFER_SIZE); > I could squash this with Ic4cc81d198fb88300394055682a3954bf22db570 and generate the proper ACPI name […] From a reader point of view, handling among multiple names, a.k.a. device->name, and acpi_name ..., are somewhat confusing. Maybe we can add a new field 'tag' to struct device. This field could be defined for device specific usage (just like the resource->index). For IIO domains devices, we can use the device->tag to hold the domain type. While for device->name, we can use it for acpi names. We can xmalloc at the 1st reference but read the values for subsequent references after the name is created. What about this? -- To view, visit https://review.coreboot.org/c/coreboot/+/80794?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: I4353db33e7ac98db41728bcf61ee01e21433dded Gerrit-Change-Number: 80794 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(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: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.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, 01 Mar 2024 09:28:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: soc/intel/xeon_sp: Drop SPI_BASE_ADDRESS from _CRS
by Shuo Liu (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Christian Walter, Johnny Lin, Lean Sheng Tan, Patrick Rudolph, Tim Chu. Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80795?usp=email ) Change subject: soc/intel/xeon_sp: Drop SPI_BASE_ADDRESS from _CRS ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/80795/comment/4a8ff6a6_f379fa27 : PS1, Line 10: hidden the fast_spi driver will generate the _CRS marking the BAR > Updated the commit message to clarify. If the device is hidden, configuration space read will return invalid values, hence we cannot get the BAR address through the configuration read. Could you please help to point me the code in fast-spi driver that can handle this? -- To view, visit https://review.coreboot.org/c/coreboot/+/80795?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: I150397a7ac5d60719f327f6ac6480a38fe295c32 Gerrit-Change-Number: 80795 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(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: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 01 Mar 2024 09:16:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: soc/intel/xeon_sp: Add ACPI names
by Patrick Rudolph (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Lean Sheng Tan, Tim Chu. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80794?usp=email ) Change subject: soc/intel/xeon_sp: Add ACPI names ...................................................................... Patch Set 2: (1 comment) File src/soc/intel/xeon_sp/acpi.c: https://review.coreboot.org/c/coreboot/+/80794/comment/56a4dfe6_1a56edc5 : PS2, Line 144: char *name = xmalloc(ACPI_NAME_BUFFER_SIZE); > > What's soc_info? […] I could squash this with Ic4cc81d198fb88300394055682a3954bf22db570 and generate the proper ACPI name directly there. iio_domain_acpi_name() could then simply return dev->name for all PCI domain and never leak memory. -- To view, visit https://review.coreboot.org/c/coreboot/+/80794?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: I4353db33e7ac98db41728bcf61ee01e21433dded Gerrit-Change-Number: 80794 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(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: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.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, 01 Mar 2024 09:00:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: [WIP]soc/intel/xeon_sp: Use common _CRS code generation
by Patrick Rudolph (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Lean Sheng Tan, Shuo Liu, Tim Chu. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80796?usp=email ) Change subject: [WIP]soc/intel/xeon_sp: Use common _CRS code generation ...................................................................... Patch Set 3: (1 comment) File src/soc/intel/xeon_sp/spr/soc_acpi.c: https://review.coreboot.org/c/coreboot/+/80796/comment/bcf0db4a_318126cd : PS3, Line 78: /* For stack with CXL device, the PCIe bus resource is BusBase only. */ : if (is_iio_cxl_stack_res(ri)) : acpigen_resource_word(2, 0xc, 0, 0, ri->BusBase, ri->BusBase, 0x0, 1); > I had a very similar patchtrain to this one. CXL stacks are a problem... […] Yes, that's what the reference code does. The CXL domain starts at ri->BusBase+1, but the PCI domain covers all PCI buses. -- To view, visit https://review.coreboot.org/c/coreboot/+/80796?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: I8b0bc2eb02569b5d74f8521d79e0af8fee880c80 Gerrit-Change-Number: 80796 Gerrit-PatchSet: 3 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(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: 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, 01 Mar 2024 08:55:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: soc/intel/xeon_sp: Add ACPI names
by Arthur Heymans (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Christian Walter, Johnny Lin, Lean Sheng Tan, Patrick Rudolph, Tim Chu. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80794?usp=email ) Change subject: soc/intel/xeon_sp: Add ACPI names ...................................................................... Patch Set 2: (1 comment) File src/soc/intel/xeon_sp/acpi.c: https://review.coreboot.org/c/coreboot/+/80794/comment/ba67d915_41f5d82c : PS2, Line 144: char *name = xmalloc(ACPI_NAME_BUFFER_SIZE); > What's soc_info? Sorry I meant chip_info in struct device, my bad... Normally it points chip.h stuff. I think it should be unused on most stack domains. Maybe an exception is domain 0 as it's present in the static devicetree, but there I suppose you can have a hardcoded in a separate .acpi_name? -- To view, visit https://review.coreboot.org/c/coreboot/+/80794?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: I4353db33e7ac98db41728bcf61ee01e21433dded Gerrit-Change-Number: 80794 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(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: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 01 Mar 2024 08:55:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: soc/intel/xeon_sp: Add ACPI names
by Patrick Rudolph (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Lean Sheng Tan, Tim Chu. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80794?usp=email ) Change subject: soc/intel/xeon_sp: Add ACPI names ...................................................................... Patch Set 2: (1 comment) File src/soc/intel/xeon_sp/acpi.c: https://review.coreboot.org/c/coreboot/+/80794/comment/74b52aa9_e0580c04 : PS2, Line 144: char *name = xmalloc(ACPI_NAME_BUFFER_SIZE); > This means you leak memory each time acpi_name is called. […] What's soc_info? -- To view, visit https://review.coreboot.org/c/coreboot/+/80794?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: I4353db33e7ac98db41728bcf61ee01e21433dded Gerrit-Change-Number: 80794 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(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: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.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, 01 Mar 2024 08:49:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: drivers/smm_payload_interface: Add initial support for SMM payload
by Arthur Heymans (Code Review) 01 Mar '24

01 Mar '24

Attention is currently required from: Benjamin Doron, Martin L Roth, Michał Żygowski, Patrick Rudolph, Sean Rhodes. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70378?usp=email ) Change subject: drivers/smm_payload_interface: Add initial support for SMM payload ...................................................................... Patch Set 18: (9 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/70378/comment/c1ecba72_b2b4d03a : PS18, Line 37: - coreboot provides register definitions in CBMEM to avoid : silicon-dependence in payload. I don't think coreboot informing the payload should be done in this case. The payload can figure out just fine how to do all those things based on runtime detection of SOC. Having code directly in the payload instead of some ACPI like instructions on how to do things is much less error prone. After all there aren't that many variations to support: AMD (same since AMD k8 in 2003), Intel starting with skylake, older Intel with PCH, even older Intel with special SMMR. I don't think it's desirable to inform the payload like this. There is no usecase where you want to couple a new coreboot with and payload. You pretty much always develop those things in parallel. Patchset: PS18: Overall quite reasonable to me except the part about informing CPU/SOC specific details to the payload. I think the payload should be able to figure that out on its own, be it with compilation option or runtime detection (which should be possible). After all you also don't pass on details on how the SPI controller works in the payload too, why would SMM related registers be any different? File src/commonlib/include/commonlib/coreboot_tables.h: https://review.coreboot.org/c/coreboot/+/70378/comment/e9cafdbd_99755ebe : PS18, Line 620: lb_pld_smram_descriptor_block what is this? Are there systems that can have multiple SMRAM regions? File src/drivers/smm_payload_interface/cbtable.c: https://review.coreboot.org/c/coreboot/+/70378/comment/049a2b4a_bfb95503 : PS18, Line 14: lb_save_restore can a better name be used? lb_payload_smm_info ? https://review.coreboot.org/c/coreboot/+/70378/comment/099c8320_118a6305 : PS18, Line 35: ACPI_BASE_ADDRESS + SMI_EN; Not safe. ACPI_BASE_ADDRESS can be reconfigured. The SMI handler needs to fetch that at runtime from the LPC device on Intel. https://review.coreboot.org/c/coreboot/+/70378/comment/ad822861_36fe012d : PS18, Line 30: smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_GBL_EN; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_IO; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = log2(GBL_SMI_EN); : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = ACPI_BASE_ADDRESS + SMI_EN; : smm_reg->count++; : : smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_GBL_EN_LOCK; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_MEMORY; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = log2(SMI_LOCK); : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = PCH_PWRM_BASE_ADDRESS + GEN_PMCON_B; : smm_reg->count++; : : smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_EOS; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_IO; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = log2(EOS); : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = ACPI_BASE_ADDRESS + SMI_EN; : smm_reg->count++; : : smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_APM_EN; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_IO; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = log2(APMC_EN); : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = ACPI_BASE_ADDRESS + SMI_EN; : smm_reg->count++; : : smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_APM_STS; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_IO; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = APM_STS_BIT; : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = ACPI_BASE_ADDRESS + SMI_STS; : smm_reg->count++; All very specific to Intel. File src/drivers/smm_payload_interface/smm_core_support.c: https://review.coreboot.org/c/coreboot/+/70378/comment/23ed944d_6afb576b : PS18, Line 119: d perform_save(void *unused) : { : uint32_t reg32; : : // Payload does not enable sleep SMI : reg32 = inl(ACPI_BASE_ADDRESS + SMI_EN); : reg32 &= ~SLP_SMI_EN; : outl(reg32, ACPI_BASE_ADDRESS + SMI_EN); : : // Enable SCI mode when there's no 'ACPI enable' SMI handler : reg32 = inl(ACPI_BASE_ADDRESS + PM1_CNT); : reg32 |= SCI_EN; : outl(reg32, ACPI_BASE_ADDRESS + PM1_CNT); : : clear_s3_save_region(); : } : : static void perform_s3_restore(void *unused) : { : uint32_t reg32; : : // Payload does not enable sleep SMI : reg32 = inl(ACPI_BASE_ADDRESS + SMI_EN); : reg32 &= ~SLP_SMI_EN; : outl(reg32, ACPI_BASE_ADDRESS + SMI_EN); : : // Enable SCI mode when there's no 'ACPI enable' SMI handler : reg32 = inl(ACPI_BASE_ADDRESS + PM1_CNT); : reg32 |= SCI_EN; : outl(reg32, ACPI_BASE_ADDRESS + PM1_CNT); : intel specific. File src/include/smm_payload_interface.h: https://review.coreboot.org/c/coreboot/+/70378/comment/529694e4_c3afb0ff : PS18, Line 44: ApicId Call it lapicid or initial_lapic to make it clear. With apicid this could be the ID the apic (IOAPIC or LAPIC) has which is typically changeable at runtime. The initial_lapic from cpuid cannot be changed. https://review.coreboot.org/c/coreboot/+/70378/comment/97860077_bd784152 : PS18, Line 43: truct CPU_SMMBASE { : uint32_t ApicId; : uint32_t SmmBase; : }; : : struct SMM_S3_INFO { : uint8_t SwSmiData; : uint8_t SwSmiTriggerValue; : uint16_t Reserved; : uint32_t CpuCount; : struct CPU_SMMBASE SmmBase[0]; Any reason to deviate from coreboot style? -- To view, visit https://review.coreboot.org/c/coreboot/+/70378?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: I3dd505a154175b6da8929b1157723de1645ca8fa Gerrit-Change-Number: 70378 Gerrit-PatchSet: 18 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sean Rhodes <sean(a)starlabs.systems> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Comment-Date: Fri, 01 Mar 2024 08:44:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2024