Attention is currently required from: Michał Żygowski.
Anastasia Klimchuk has posted comments on this change by Michał Żygowski. ( https://review.coreboot.org/c/flashrom/+/84987?usp=email )
Change subject: chipset_enable.c: Add TGL chipset detection based on SPI PCI ID
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patchset:
PS1:
I didn't find a datasheet, but at least I found IDs 0xa0a4 and 0x43a4 on the internet as of being indeed Tiger Lake LP and Tiger Lake H :) And you tested (and you probably have a datasheet)
--
To view, visit https://review.coreboot.org/c/flashrom/+/84987?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Ie6859d81157760ca03fe34ba5ac311eba5a7c46b
Gerrit-Change-Number: 84987
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Comment-Date: Tue, 05 Nov 2024 01:39:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Anastasia Klimchuk, Peter Marheine.
Matti Finder has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84934?usp=email )
Change subject: rpmc: add rpmc commands feature
......................................................................
Patch Set 5:
(7 comments)
File include/rpmc.h:
https://review.coreboot.org/c/flashrom/+/84934/comment/08dcfa13_6490f47f?us… :
PS4, Line 71: dependant
> ```suggestion […]
Done
https://review.coreboot.org/c/flashrom/+/84934/comment/78b96543_dbc24fb3?us… :
PS4, Line 71: dependant
> ```suggestion […]
Done
https://review.coreboot.org/c/flashrom/+/84934/comment/dfb12815_7d20d2a9?us… :
PS4, Line 90: struct flashrom_flashctx * flash
> Sorry I just noticed! we usually have asterisk sticking to the variable name (struct flashrom_flashc […]
Done
File rpmc.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/5a8154a3_3ad5aaa0?us… :
PS4, Line 131: if (res != RPMC_SUCCESS) {
: return res;
: }
> Found another places here (rpmc_poll_until_finished) where { } not needed because conditional body i […]
Done
https://review.coreboot.org/c/flashrom/+/84934/comment/1fce12c7_fee71ca7?us… :
PS4, Line 157: if (keyfile == NULL || read_buf_from_file(key, RPMC_HMAC_KEY_LENGTH, keyfile) != 0) {
: return RPMC_ERROR_KEY_READ;
: }
> And here, { } not needed, 1-line body
Done
https://review.coreboot.org/c/flashrom/+/84934/comment/e629f8c4_be003353?us… :
PS4, Line 497: no error string defined for this value
> Maybe […]
Done
File sfdp.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/f33b9fd7_264fc3c3?us… :
PS4, Line 457: if (sfdp_fill_flash(flash->chip, tbuf, len) == 0) {
: ret = 1;
: }
> I have a question: if for some reasons sfdp_fill_flash fails (i.e. returns non-zero), would RPMC feature work? (or any other feature).
I don't know exactly but from looking at the code it only seems to set information about erase and write calls.
> Maybe this should return error straight away if sfdp_fill_flash fails? Previously it wasn't important because only page 0 of headers was parsed.
Yes for safety and also consistency I agree.
Failure to parse the rpmc page is no problem since the feature bit is only set when the function can't fail anymore. I have removed it's influence on the return value of the function.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84934?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38
Gerrit-Change-Number: 84934
Gerrit-PatchSet: 5
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 04 Nov 2024 14:51:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Peter Marheine.
Matti Finder has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email )
Change subject: cli_client: Add rpmc command support
......................................................................
Patch Set 9:
(4 comments)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/84840/comment/64bc5425_ed573d5f?us… :
PS8, Line 1272: any_rpmc_op
> Would it ever be needed to set multiple commands at the same time? And what would that mean? […]
Before your question the `cli_classic_validate_singleop(&operation_specified);` before any of the options prevented multiple rpmc operations.
But thanks to this question I realized that using multiple commands in one call is actually very usefull. Updating the hmac key is something that needs to be done at least after every chip reset. So calling --update-hmac-key and then immediatly after something like --get-counter is probably something a lot of users want to do.
The commands don't get into the way of eachother so running multiple commands at after each other is possible. I have adjusted the order slightly to have --get-rpmc-status run last.
https://review.coreboot.org/c/flashrom/+/84840/comment/cf31a42e_332e4a78?us… :
PS8, Line 1289: (options.enable_wp && options.disable_wp)
> Would it make sense to add few checks similar to this, what do you think? […]
See also #1272.
The else if is now gone so every commands gets run.
I have also added a rpmc_root_key_file to the cli_opts struct so the commands don't get into the way of -w, -r etc..
Doing something like writing something to flash and then incrementing the counter is probably how the counters are meant to be used. So beeing able to run other commands together with them is mostly usefull.
I can't imagine any other scenerio where running a rpmc command next to some other command is destructive, so in my opinion there are no other checks necessary.
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/84840/comment/47cc9cc7_9a3dc029?us… :
PS8, Line 334: If your chip is detected correctly but the feature is not enabled, try using
: ``-c "SFDP-capable chip"`` for automatic feature detection.
> I want to think how to maybe rephrase it to be more clear. […]
I have added a quote to the error message, this should make it better to understand.
Running multiple commands together would currently either fail at the rpmc command or force the read and write to use SFDP-capable chip (which works perfectly in my testing).
If the commands are run in seperate calls to flashrom (which read and write have to be anyways, since they mutually exclusive) everything works as normal, you just have to specify the chip manually for the rpmc command.
I also had another idea for detection we could always poll the SFDP-capable chip and then silently use it, instead of the user specified one, for the rpmc commands.
But that is something I would try in a different commit.
File doc/release_notes/devel.rst:
https://review.coreboot.org/c/flashrom/+/84840/comment/ee5453a0_0d1068e6?us… :
PS8, Line 35: Adding support for RPMC commands as specified by JESD260 to the cli_client. Main
> Let's give this paragraph its own header, it's a big deal: […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Gerrit-Change-Number: 84840
Gerrit-PatchSet: 9
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 04 Nov 2024 14:37:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Matti Finder, Peter Marheine.
Hello Anastasia Klimchuk, Peter Marheine, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84934?usp=email
to look at the new patch set (#5).
The following approvals got outdated and were removed:
Code-Review+2 by Peter Marheine, Verified+1 by build bot (Jenkins)
Change subject: rpmc: add rpmc commands feature
......................................................................
rpmc: add rpmc commands feature
Added optional support for all the commands specified in JESD260.
Added a new optional dependency to openssls libcrypto.
Added parsing for the rpmc parameter sfdp table.
Added necessary rpmc parameter information to flashchips struct and the
flash hardening feature to enable rpmc commands.
Enables future use of these commands in the cli_client and also
libflashrom.
Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38
Signed-off-by: Matti Finder <matti.finder(a)gmail.com>
---
M MAINTAINERS
M include/flash.h
A include/rpmc.h
M meson.build
M meson_options.txt
A rpmc.c
M sfdp.c
7 files changed, 822 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/84934/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/84934?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38
Gerrit-Change-Number: 84934
Gerrit-PatchSet: 5
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>
Attention is currently required from: Matti Finder, Peter Marheine.
Hello Anastasia Klimchuk, Peter Marheine, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84840?usp=email
to look at the new patch set (#9).
The following approvals got outdated and were removed:
Code-Review+2 by Peter Marheine, Verified+1 by build bot (Jenkins)
Change subject: cli_client: Add rpmc command support
......................................................................
cli_client: Add rpmc command support
This commit adds uses the new rpmc command implementation to
add them as a new feature to the cli_client.
Also adds the necessary documentation for this new feature.
Tested on the Winbond W25R128JV as a 'SFDP-capable chip'.
This patch was done to add rpmc command support to flashrom.
This enables users to write root keys to their flash chips while they
flash data on the chip. This might become useful in the future as rpmc
support is extended in coreboot.
Also adds debug tools to flashrom, which might be useful in
implementing coreboots rpmc support.
Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Signed-off-by: Matti Finder <matti.finder(a)gmail.com>
---
M cli_classic.c
M doc/classic_cli_manpage.rst
M doc/release_notes/devel.rst
3 files changed, 266 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/84840/9
--
To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Gerrit-Change-Number: 84840
Gerrit-PatchSet: 9
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>
Attention is currently required from: Anastasia Klimchuk.
Hello Peter Marheine, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84954?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: doc: Add few sections to recent development doc
......................................................................
doc: Add few sections to recent development doc
Change-Id: Iedaca4a704c57c1db399c7888f743ad2961cbf9d
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M doc/release_notes/devel.rst
1 file changed, 78 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/84954/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84954?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Iedaca4a704c57c1db399c7888f743ad2961cbf9d
Gerrit-Change-Number: 84954
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Matti Finder.
Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email )
Change subject: cli_client: Add rpmc command support
......................................................................
Patch Set 8:
(1 comment)
File doc/release_notes/devel.rst:
https://review.coreboot.org/c/flashrom/+/84840/comment/5c115fde_a533ae5c?us… :
PS8, Line 35: Adding support for RPMC commands as specified by JESD260 to the cli_client. Main
Let's give this paragraph its own header, it's a big deal:
> RPMC support added (works via SFDP detection)
Or something like that, you can compose another header title
--
To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Gerrit-Change-Number: 84840
Gerrit-PatchSet: 8
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Comment-Date: Mon, 04 Nov 2024 11:27:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Matti Finder.
Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: cli_client: Add rpmc command support
......................................................................
Patch Set 8:
(4 comments)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/84840/comment/ec662565_04199d24?us… :
PS8, Line 1272: any_rpmc_op
Would it ever be needed to set multiple commands at the same time? And what would that mean?
If not maybe you can check here (or few lines below where I added another comment on the same topic) that only one command is set.
https://review.coreboot.org/c/flashrom/+/84840/comment/1636a7d8_ea803a77?us… :
PS8, Line 1289: (options.enable_wp && options.disable_wp)
Would it make sense to add few checks similar to this, what do you think?
Although I see that code goes through `else if`, so only one command would actually be processed.
But it might prevent confusion if someone adds more than one command line option (maybe by accident) and then we silently run only one.
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/84840/comment/54768394_c7686e70?us… :
PS8, Line 334: If your chip is detected correctly but the feature is not enabled, try using
: ``-c "SFDP-capable chip"`` for automatic feature detection.
I want to think how to maybe rephrase it to be more clear.
We do have a situation, when flashchip has a definition, and it has a RPMC supported (but it won't work from definition), the error message, as I understand form previous patch, will be `Flash hardening is not supported on this chip, aborting.`
It might be not entirely clear for the user that this is the same as manpage (currently) says `If your chip is detected correctly but the feature is not enabled`, because "not enabled" and "not supported" is not exact same thing.
Also if RPMC can be detected and works reliably via SFDP - maybe that can be the official way to use it?
What happens if you need to run several commands over the same chip: for example
1) read
2) get-rpmc-status
3) write
Can you do 1 with classic probing, then 2 with -c "SFDP-capable chip" and then 3 again with classic probing, over the same chip? Would it work like this?
I wrote a long comment, but what I am trying to figure out is a perfect text for manpage, so that every user reads and immediately everything is clear.
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/84840/comment/0861c592_375be5d9?us… :
PS4, Line 581: rpmc_ctx
Oh this is such a good topic.
For this patch, I will maybe just think a bit more about the manpage description, so that it is crystal clear.
> I think for vendors that still have enough ids to give special ones to rpmc capable devices, we can just or the FEATURE_FLASH_HARDENING bit and set the struct rpmc_ctx values accordingly.
From the recent experience, the IDs repeat. Several models added recently and the pair of models with/without rpmc feature share the same ID.
> But I'm not sure how the future of flashrom should look like. If maintaining a complicated flashchips.c is worth it, when most of the information can also be parsed from sfdp pages (for newer flashchips).
I think parsing info from sfdp pages, for newer flashchips, is how future should look like. That's a lot of work for sure, but you know what's so amazing: we have been just recently discussing the topic of sfdp (it came up from repeated IDs) and now boom you sent your contribution, and this is going right in the direction of future! Thank you so much :)
--
To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Gerrit-Change-Number: 84840
Gerrit-PatchSet: 8
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Comment-Date: Mon, 04 Nov 2024 11:05:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Matti Finder <matti.finder(a)gmail.com>
Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84987?usp=email )
Change subject: chipset_enable.c: Add TGL chipset detection based on SPI PCI ID
......................................................................
chipset_enable.c: Add TGL chipset detection based on SPI PCI ID
Add detection of Tiger Point chipsets based on SPI controller PCI ID.
Current detection is based on ESPI PCI ID only which limits the
flashrom operability to 2 out of many chipset variants.
TEST=Read flash on a platform with Intel Corporation Tiger Lake-LP
SPI Controller [8086:a0a4] and ISA bridge [0601]: Intel Corporation
Device [8086:a088] ESPI device.
Change-Id: Ie6859d81157760ca03fe34ba5ac311eba5a7c46b
Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com>
---
M chipset_enable.c
1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/84987/1
diff --git a/chipset_enable.c b/chipset_enable.c
index 2adc425..ed53702 100644
--- a/chipset_enable.c
+++ b/chipset_enable.c
@@ -2106,6 +2106,8 @@
{0x8086, 0x9d84, B_S, DEP, "Intel", "Cannon Lake U Premium", enable_flash_pch300},
{0x8086, 0x0284, B_S, DEP, "Intel", "Comet Lake U Premium", enable_flash_pch400},
{0x8086, 0x0285, B_S, DEP, "Intel", "Comet Lake U Base", enable_flash_pch400},
+ {0x8086, 0xa0a4, B_S, DEP, "Intel", "Tiger Lake LP", enable_flash_pch500},
+ {0x8086, 0x43a4, B_S, DEP, "Intel", "Tiger Lake H", enable_flash_pch500},
{0x8086, 0xa082, B_S, DEP, "Intel", "Tiger Lake U Premium", enable_flash_pch500},
{0x8086, 0xa088, B_S, DEP, "Intel", "Tiger Lake UP3", enable_flash_pch500},
{0x8086, 0xa141, B_S, NT, "Intel", "Sunrise Point Desktop Sample", enable_flash_pch100},
--
To view, visit https://review.coreboot.org/c/flashrom/+/84987?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Ie6859d81157760ca03fe34ba5ac311eba5a7c46b
Gerrit-Change-Number: 84987
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>