Attention is currently required from: Angel Pons, Felix Held, Maxim, Paul Menzel.
Hello Angel Pons, Felix Held, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83196?usp=email
to look at the new patch set (#8).
The following approvals got outdated and were removed:
Code-Review+1 by Angel Pons, Verified+1 by build bot (Jenkins)
Change subject: util/superiotool: Add extra selectors support
......................................................................
util/superiotool: Add extra selectors support
Some chips (fintek [1,2]) have registers with specific selector-fields
that can affect the address space of the device (for example, switch the
register bank). At the same time, these registers contain fields that
should not change after they are configured in BIOS (for example, set
the port to 2E/2F or 4E/4F). In this case, the selector should take into
account the mask of the register fields and there is no convenient and
easy way to add this in the code in the utility. The selector-fields
should be set manually before the dump and this action is done several
times.
This patch adds an extra-selector mechanism that allows superiotool to
make a correct dump in automatic mode.
Just add a structure with an index, mask, and value for the selector
inside the superio_registers chip for the corresponding LDN to switch
the register bank:
{FINTEK_F81966_DID, "F81962/F81964/F81966/F81967", {
* * *
{NOLDN, "Global",
{0x28,0x2a,0x2b,0x2c,EOT},
{0x00,0x00,0x00,0x00,EOT},
{.idx = 0x27, .mask = 0xd, .val = 0x1} /* update extra selector */
},
{0x03, "LPT",
{0x30,0x60,0x61,0x70,0x74,0xf0,EOT},
{NANA,0x03,0x78,0x07,0x03,0xc2,EOT} /* without extra selector */
},
* * *
Tested with Fintek F81966 on Asrock IMB-1222:
- run superiotool on Ubuntu and dump the registers for the board with
the vendor's firmware;
- add the superio chip initialization code to the board configuration
in coreboot and build the project;
- boot Ubuntu on the board with coreboot and re-dump the registers;
- the register values from the board configuration code are the same
in both dumps.
Found Fintek F81962/F81964/F81966/F81967 (vid=0x3419, id=0x0215) at 0x2e
(Global) -- ESEL[27h] 0x00 (Port Select Register) --
idx 02 07 20 21 23 24 25 26 27 28 29 2a 2b 2c 2d
val 00 0b 15 02 19 34 5a 23 80 a0 f0 45 02 e3 2e
def NA 00 15 02 19 34 00 23 02 a0 00 00 02 0c 28
* * *
The changes do not affect the configuration of existing chips, which
was tested on the Asrock H110-STX motherboard with Nuvoton NCT5539D
(the dump before and after the changes are the same).
[1] CB:83004
[2] CB:83019
Change-Id: If56af9f977381e637245bdd26563f5ba7e6cbead
Signed-off-by: Maxim Polyakov <max.senia.poliak(a)gmail.com>
---
M util/superiotool/superiotool.c
M util/superiotool/superiotool.h
2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/83196/8
--
To view, visit https://review.coreboot.org/c/coreboot/+/83196?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: If56af9f977381e637245bdd26563f5ba7e6cbead
Gerrit-Change-Number: 83196
Gerrit-PatchSet: 8
Gerrit-Owner: Maxim <max.senia.poliak(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Maxim <max.senia.poliak(a)gmail.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Attention is currently required from: Christian Walter, Filip Lewiński, Julius Werner.
Michał Żygowski has posted comments on this change by Filip Lewiński. ( https://review.coreboot.org/c/coreboot/+/82695?usp=email )
Change subject: security/intel/txt: Handle TPM properly when vboot enabled
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> > Please justify what you're doing here and why? […]
&& err == INVALID_POSTINIT of course
--
To view, visit https://review.coreboot.org/c/coreboot/+/82695?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: I19dc3d910c23fcfd8732465c488f47dd86a96781
Gerrit-Change-Number: 82695
Gerrit-PatchSet: 4
Gerrit-Owner: Filip Lewiński <filip.lewinski(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Filip Lewiński <filip.lewinski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Fri, 26 Jul 2024 08:47:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Attention is currently required from: Christian Walter, Filip Lewiński, Julius Werner.
Michał Żygowski has posted comments on this change by Filip Lewiński. ( https://review.coreboot.org/c/coreboot/+/82695?usp=email )
Change subject: security/intel/txt: Handle TPM properly when vboot enabled
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> Please justify what you're doing here and why?
I simply wanted to have vboot working with Intel TXT. The commit message is just beyond the state of poor.
> I don't think making vboot play nice with INIT_BOOTBLOCK is as simple as turning on IGNORE_POSTINIT. IGNORE_POSTINIT is a hack and not really a secure thing to use by default — there are good reasons why you want to make sure the TPM was properly reset on boot.
> Also, you're still running tpm_setup() twice, that's still not good, even if you ignore the error that makes it fail.
Right, but wasn't it the same also for the case when tpm_setup is run in verstage (for antirollback) and then in ramstage again?
> You're calling tspi_measure_cache_to_pcr() again and end up adding all those entries twice.
I agree, this is something that needs some handling. How about a check like this:
```
if (CONFIG(TPM_MEASURED_BOOT_INIT_BOOTBLOCK) && err == IGNORE_POSTINIT)
<do not report error and return from tpm_setup with success skipping cache to PCR measurement>
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/82695?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: I19dc3d910c23fcfd8732465c488f47dd86a96781
Gerrit-Change-Number: 82695
Gerrit-PatchSet: 4
Gerrit-Owner: Filip Lewiński <filip.lewinski(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Filip Lewiński <filip.lewinski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Fri, 26 Jul 2024 08:44:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Attention is currently required from: Felix Held.
Sean Rhodes has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/83620?usp=email )
Change subject: mb/starlabs/starbook/rpl: Don't set tcss_aux_ori
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83620/comment/1b652ec4_b518fe03?us… :
PS1, Line 7: mb/starlabs/starbook/rpl: Don't set tcss_aux_ori
> yes, would be good if the commit message matches the code change :)
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/83620?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: Ia0e90179dd05b23f1f36935be51327250c5a8684
Gerrit-Change-Number: 83620
Gerrit-PatchSet: 2
Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 26 Jul 2024 08:28:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Rhodes <sean(a)starlabs.systems>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Attention is currently required from: Sean Rhodes.
Hello Matt DeVillier, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83620?usp=email
to look at the new patch set (#2).
Change subject: mb/starlabs/starbook/rpl: Don't set tcss_aux_ori
......................................................................
mb/starlabs/starbook/rpl: Don't set tcss_aux_ori
Not setting tcss_aux_ori in devicetree is the same as
setting it to zero so remove it.
Change-Id: Ia0e90179dd05b23f1f36935be51327250c5a8684
Signed-off-by: Sean Rhodes <sean(a)starlabs.systems>
---
M src/mainboard/starlabs/starbook/variants/rpl/devicetree.cb
1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/83620/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83620?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: Ia0e90179dd05b23f1f36935be51327250c5a8684
Gerrit-Change-Number: 83620
Gerrit-PatchSet: 2
Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems>
Attention is currently required from: Ashish Kumar Mishra, Dinesh Gehlot, Eran Mitrani, Felix Singer, Jakub Czapiga, Jérémy Compostella, Kapil Porwal, Saurabh Mishra, Tarun.
Subrata Banik has posted comments on this change by Saurabh Mishra. ( https://review.coreboot.org/c/coreboot/+/83354?usp=email )
Change subject: soc/intel/ptl: Do initial Panther Lake SoC commit till bootblock
......................................................................
Patch Set 17:
(5 comments)
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83354/comment/714cea1f_5ea4d70f?us… :
PS14, Line 25: select HAVE_X86_64_SUPPORT
> Acknowledged
I don;t see you have followed the order. may be lost during rebasing?
https://review.coreboot.org/c/coreboot/+/83354/comment/bca8148c_ada76011?us… :
PS14, Line 67: ~1KiB)
> Corrected to ~1KiB size.
why? i have asked to keep 512KB + 32KB for coreboot hence, the 0x88000 is the correct value. What i have asked is to update the help text to reflect the code comment as well.
File src/soc/intel/pantherlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/58c47a82_9ece6222?us… :
PS8, Line 37: VTD_BASE_ADDRESS
> VTD BAR and DMI3BAR are different BARs at different addresses. DMI3BAR NA for PTL mobile. VTD BAR(3 parts) required for PTL mobile.
This is a pretty simple question, but I want to make sure I understand the section 6.3 diagram correctly. In the diagram, it shows the DMI3BAR (formerly known as the VTD bar). What exactly does that mean?
File src/soc/intel/pantherlake/include/soc/p2sb.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/fa683ccb_4987550c?us… :
PS17, Line 7: #define HPTC_ADDR_ENABLE_BIT BIT(7)
give one space to start this macro
File src/soc/intel/pantherlake/include/soc/soc_info.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/eaa99164_e6b8984c?us… :
PS14, Line 5: enum {
: NOT_DETECTED = 0,
: PTLU,
: PTLH,
: };
> Removed and followed MTL refactoring for PTL.
> 81111: soc/intel/mtl: Improve functions in soc_info.c | https://review.coreboot.org/c/coreboot/+/81111
I don't think you have removed this code block. can you please check ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/83354?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: Ibcfe71eec27cebf04f10ec343a73dd92f1272aca
Gerrit-Change-Number: 83354
Gerrit-PatchSet: 17
Gerrit-Owner: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-CC: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-CC: Sanju Jose Thottan <sanjujose.thottan(a)intel.com>
Gerrit-CC: Saurabh Mishra <mishra.saurabh(a)intel.corp-partner.google.com>
Gerrit-CC: Vikrant L Jadeja <vikrant.l.jadeja(a)intel.com>
Gerrit-CC: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-Comment-Date: Fri, 26 Jul 2024 08:09:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Saurabh Mishra <mishra.saurabh(a)intel.com>
Comment-In-Reply-To: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Nico Huber, Patrick Rudolph, Tim Chu.
Shuo Liu has posted comments on this change by Shuo Liu. ( https://review.coreboot.org/c/coreboot/+/82133?usp=email )
Change subject: soc/intel/xeon_sp: Use pre-processor to define ASL handler names
......................................................................
Patch Set 8:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/82133/comment/b161e3e5_66338385?us… :
PS7, Line 16: As to the parameter count of the ASL handler, which is limited to
: 8 by ACPI specification. It is encouraged to use up the Arg0-7
: first. If the needed arguments exceed 8, the last Arg (Arg7)
: could be passed as a package of more parameters, which could be
: extracted into unused local variables in the ASL handler codes.
> Does this have anything to do with the current change? I am confused by it.
This is just a informal comment on how to deal with the case of parameter inadequacy as follow up of previous code reviews. I removed it in this patch.
File src/soc/intel/xeon_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/82133/comment/83e7de22_74efb921?us… :
PS7, Line 131: ASL_HANDLER_PATH(AH_PCIE_OSC)
> I am pretty sure this should work, as string literals can be concatenated at compile time: […]
Done
File src/soc/intel/xeon_sp/include/soc/asl_handler.h:
https://review.coreboot.org/c/coreboot/+/82133/comment/1f6bf1c1_97940e75?us… :
PS7, Line 6: #define TO_STR(name) #name
> I think there's a macro for this somewhere in commonlib
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/82133?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: I4134b275143d4f52622b864ed202252b2a150dae
Gerrit-Change-Number: 82133
Gerrit-PatchSet: 8
Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.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: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.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: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.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: Angel Pons <th3fanbus(a)gmail.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, 26 Jul 2024 08:00:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>