October 2024 - flashrom-gerrit

[L] Change in flashrom[main]: cli_classic: Add rpmc command support
by Anastasia Klimchuk (Code Review) 29 Oct '24

29 Oct '24

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_classic: Add rpmc command support ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > Matti, wow, that's so much work that you did! Thank you so much for your contribution! […] Oh one more thing I forgot to mention sorry... it would be great at some point to make a post on the mailing list about new feature. -- 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: 4 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: Tue, 29 Oct 2024 05:27:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[L] Change in flashrom[main]: cli_classic: Add rpmc command support
by Anastasia Klimchuk (Code Review) 29 Oct '24

29 Oct '24

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_classic: Add rpmc command support ...................................................................... Patch Set 4: (8 comments) Patchset: PS4: Matti, wow, that's so much work that you did! Thank you so much for your contribution! Sorry I am joining the review a bit later. I added comments around the code, but also I have few comments to the patch in general: 1) What do you think splitting this into two commits: first can add the feature implementation (basically everything except of cli_classic). Second would be cli client which is using new feature, also manpage update (mentioned in the other comment). Also, see next point -> 2) This cool new feature needs to be added to "Recent development", so that whoever is interested knows what to expect in the next release, the doc is here: `doc/release_notes/devel.rst` 3) You are now the author of lots of code in flashrom, what do you think about becoming a maintainer for your feature? The commitment would be only for your feature, not for everything that is happening on flashrom We have a doc about the team https://flashrom.org/about_flashrom/team.html (but you are welcome to get involved into anything interested for you! :) Commit Message: https://review.coreboot.org/c/flashrom/+/84840/comment/a1878c01_09f775c9?us… : PS4, Line 21: usefull usefull -> useful (typo) (3 times in commit message :) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84840/comment/3cedd297_ca38f78d?us… : PS4, Line 147: #if CONFIG_RPMC_ENABLED == 1 : "RPMC COMMANDS\n" : " --get-rpmc-status read the extended status\n" : " --write-root-key write the root key register\n" : " --update-hmac-key update the hmac key register\n" : " --increment-counter <previous>\n" : " increment rpmc counter\n" : " --get-counter get rpmc counter\n" : "RPMC OPTIONS\n" : " --counter-address <number> counter address (default: 0)\n" : " --rpmc-root-key <keyfile> rpmc root key file\n" : " --key-data <keydata> hex number to use as key data (default: 0)\n" : #endif // CONFIG_RPMC_ENABLED These new commands need to also be documented on the manpage. Manpage source is here: `doc/classic_cli_manpage.rst` and from this source we generated the manpage and also a webpage (https://flashrom.org/classic_cli_manpage.html) https://review.coreboot.org/c/flashrom/+/84840/comment/f238b960_f90f467b?us… : PS4, Line 1323: if (options.rpmc_read_data) : ret = rpmc_read_data(fill_flash); I am thinking, and maybe this is more the question to Peter, what do you think, if we were too add this to libflashrom, would that be easy? Currently it's only cli feature, but it shouldn't be cli only in the long term: cli should be using libflashrom API as with everything else. In ideal world :) we would add libflashrom API first, and then cli client which is using it. Not all in one commit of course. File include/flash.h: https://review.coreboot.org/c/flashrom/+/84840/comment/abd265b2_4fc3aab6?us… : PS4, Line 553: implement I know it's a small thing, but if you start with uppercase it would look better. https://review.coreboot.org/c/flashrom/+/84840/comment/195ab568_f5db14d5?us… : PS4, Line 575: /* : * all times in microsecond (us) : */ Also small comment, but this can be 1-line comment, and starting with uppercase `/* All times in microsecond (us) */` https://review.coreboot.org/c/flashrom/+/84840/comment/100f756f_277db26f?us… : PS4, Line 581: rpmc_ctx I understand this goes into a flashchip definition? and you mentioned you tested on one chip? It would be great if you could update the definition for the chip you tested, probably better in a separate commit. It would be a very useful as an real example of how to do it. File meson.build: https://review.coreboot.org/c/flashrom/+/84840/comment/daa23c24_d1227966?us… : PS4, Line 177: if libcrypto.found() : add_project_arguments('-DCONFIG_RPMC_ENABLED=1', language : 'c') This ignores the meson option you added. It is auto by default, however it is possible to disable it. Even if libcrypto found, you also need to check that `rpmc` option is enabled or auto. For example docs are doing this, in doc/meson.build , lines 9-10 are checking the library, and then the meson option too. -- 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: 4 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: Tue, 29 Oct 2024 05:25:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: erasure_layout: Add an option to sacrifice unchanged blocks for speed
by Peter Marheine (Code Review) 29 Oct '24

29 Oct '24

Attention is currently required from: Aarya, Anastasia Klimchuk, Bill XIE. Peter Marheine has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email ) Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed ...................................................................... Patch Set 10: (2 comments) File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/84721/comment/a3d79336_431c45f2?us… : PS10, Line 23: | [-V[V[V]]] [-o <logfile>] [--progress][--sacrifice-ratio <ratio>] Add a space between flags. ```suggestion | [-V[V[V]]] [-o <logfile>] [--progress] [--sacrifice-ratio <ratio>] ``` File doc/release_notes/devel.rst: https://review.coreboot.org/c/flashrom/+/84721/comment/905d7b53_a095bf8a?us… : PS10, Line 57: didn't need to be erased. Which means, the chip, as a hardware, will : wear faster. : : New command line option sets the maximum % memory that is allowed for : redundant erase. Default is 0, S+1 size block only selected if all the : area needs to be erased in full. 50 means that if more than a half of : the area needs to be erased, a S+1 size block can be selected to cover : all area with one block. Small rewording suggestions. ```suggestion didn't need to be erased. Which means the physical chip will wear out faster. This new option sets the maximum % of memory that is allowed for redundant erase. Default is 0, S+1 size block only selected if all the area needs to be erased in full. 50 means that if more than a half of the area needs to be erased, a S+1 size block can be selected to cover the entire area with one block. ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803 Gerrit-Change-Number: 84721 Gerrit-PatchSet: 10 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Aarya <aarya.chaumal(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: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 29 Oct 2024 02:54:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[L] Change in flashrom[main]: cli_classic: Add rpmc command support
by Peter Marheine (Code Review) 29 Oct '24

29 Oct '24

Attention is currently required from: Anastasia Klimchuk, Matti Finder. Peter Marheine has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email ) Change subject: cli_classic: Add rpmc command support ...................................................................... Patch Set 4: Code-Review+2 -- 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: 4 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: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Tue, 29 Oct 2024 02:49:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XS] Change in flashrom[main]: linux_mtd: fix build with clang >= 19
by Anastasia Klimchuk (Code Review) 29 Oct '24

29 Oct '24

Attention is currently required from: Arnaud Ferraris, Maximilian Brune. Anastasia Klimchuk has posted comments on this change by Arnaud Ferraris. ( https://review.coreboot.org/c/flashrom/+/84856?usp=email ) Change subject: linux_mtd: fix build with clang >= 19 ...................................................................... Patch Set 2: Code-Review+2 (1 comment) File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/84856/comment/bfe82da6_8d0c4b02?us… : PS2, Line 52: char path[sizeof(LINUX_MTD_SYSFS_ROOT) + 32]; Oh I got worried that I missed something. Size has changed because, it's +1 longer because now it counts `\0` char, is that correct? and it's fine because the same data still fits in. Yes, maybe 1-line in commit message: > The result is the array init with +1 longer size, because sizeof in the case counts \0 char. (which is fine) or something like that -- To view, visit https://review.coreboot.org/c/flashrom/+/84856?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: If470a65702e9ae08e4303123a0014e53a1fee56e Gerrit-Change-Number: 84856 Gerrit-PatchSet: 2 Gerrit-Owner: Arnaud Ferraris <arnaud.ferraris(a)collabora.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Arnaud Ferraris <arnaud.ferraris(a)collabora.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Comment-Date: Tue, 29 Oct 2024 01:06:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>

1 0

[S] Change in flashrom[main]: flashchips: add GD25F128F
by Victor Lim (Code Review) 28 Oct '24

28 Oct '24

Attention is currently required from: Nikolai Artemiev, Stefan Reinauer. Victor Lim has posted comments on this change by Victor Lim. ( https://review.coreboot.org/c/flashrom/+/84827?usp=email ) Change subject: flashchips: add GD25F128F ...................................................................... Patch Set 2: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/84827/comment/4e074d5a_b88cf16e?us… : PS1, Line 8199: OTP: 1024B total, 256B reserved > I see that you updated, so I mark it as resolved, right? […] thanks. resolved -- To view, visit https://review.coreboot.org/c/flashrom/+/84827?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: I14c0905f50e92492287d50f8377790b997c4acfe Gerrit-Change-Number: 84827 Gerrit-PatchSet: 2 Gerrit-Owner: Victor Lim <vlim(a)gigadevice.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 28 Oct 2024 18:49:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Victor Lim <vlim(a)gigadevice.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: linux_mtd: fix build with clang >= 19
by Maximilian Brune (Code Review) 28 Oct '24

28 Oct '24

Attention is currently required from: Arnaud Ferraris. Maximilian Brune has posted comments on this change by Arnaud Ferraris. ( https://review.coreboot.org/c/flashrom/+/84856?usp=email ) Change subject: linux_mtd: fix build with clang >= 19 ...................................................................... Patch Set 2: (1 comment) File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/84856/comment/00e51fff_ab27b6e0?us… : PS2, Line 52: char path[sizeof(LINUX_MTD_SYSFS_ROOT) + 32]; You have changed the size of the path variable. It doesn't look like to be a problem here, but a notable difference nonethless. nit: either keep the previous size or mention it in the commit msg. -- To view, visit https://review.coreboot.org/c/flashrom/+/84856?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: If470a65702e9ae08e4303123a0014e53a1fee56e Gerrit-Change-Number: 84856 Gerrit-PatchSet: 2 Gerrit-Owner: Arnaud Ferraris <arnaud.ferraris(a)collabora.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Arnaud Ferraris <arnaud.ferraris(a)collabora.com> Gerrit-Comment-Date: Mon, 28 Oct 2024 18:41:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: linux_mtd: fix build with clang >= 19
by Arnaud Ferraris (Code Review) 28 Oct '24

28 Oct '24

Arnaud Ferraris has posted comments on this change by Arnaud Ferraris. ( https://review.coreboot.org/c/flashrom/+/84856?usp=email ) Change subject: linux_mtd: fix build with clang >= 19 ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > Arnaud, thank you for the patch! […] Hi Anastasia, Yes, I did run the tests, which built successfully and passed using both clang 19 and HEAD from Oct 8th. -- To view, visit https://review.coreboot.org/c/flashrom/+/84856?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: If470a65702e9ae08e4303123a0014e53a1fee56e Gerrit-Change-Number: 84856 Gerrit-PatchSet: 2 Gerrit-Owner: Arnaud Ferraris <arnaud.ferraris(a)collabora.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 28 Oct 2024 12:10:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[L] Change in flashrom[main]: cli_classic: Add rpmc command support
by Matti Finder (Code Review) 28 Oct '24

28 Oct '24

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_classic: Add rpmc command support ...................................................................... Patch Set 4: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84840/comment/f2831dbf_d12a4008?us… : PS3, Line 165: ", the RPMC commands" > "a RPMC command" since specifying more than one in a single invocation is an error. 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: 4 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, 28 Oct 2024 10:32:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

[L] Change in flashrom[main]: cli_classic: Add rpmc command support
by Matti Finder (Code Review) 28 Oct '24

28 Oct '24

Attention is currently required from: Anastasia Klimchuk, Matti Finder. 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 (#4). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: cli_classic: Add rpmc command support ...................................................................... cli_classic: Add rpmc command support 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. 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 usefull in the future as rpmc support is extended in coreboot. Also adds usefull debug tools to flashrom, which might be usefull in implementing coreboots rpmc support. Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1 Signed-off-by: Matti Finder <matti.finder(a)gmail.com> --- M cli_classic.c M include/flash.h A include/rpmc.h M meson.build M meson_options.txt A rpmc.c M sfdp.c 7 files changed, 799 insertions(+), 18 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/84840/4 -- 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: 4 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: Matti Finder <matti.finder(a)gmail.com>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2024