Attention is currently required from: Aarya, Bill XIE.
Anastasia Klimchuk 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 5:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/84721/comment/f57f2ed5_a9fec48d?us… :
PS5, Line 9: Someone may prefer to sacrifice sub-blocks needless to change within a
: larger erase block for programming speed. These people who are okay to
: fry their chip sooner can set this option. If the percentage of
: sub-blocks needless to change within a larger erase block is lower
: than the given value, the whole larger erase block will be erased.
I wrote another version of commit message, what do you think? I can update myself, but first I wanted to ask what do you think:
(commit title is fine)
===
The patch adds command line option to handle the following situation:
There is a region which is requested to be erased (or written, because the write operation uses erase too). Some of the areas inside this region don't need to be erased, because the bytes already have expected value. Such areas can be skipped.
The logic selects eraseblocks that can cover the areas which need to be erased. Suppose there is a region which is partially covered by eraseblocks of size S (partially because remaining areas don't need to be erased). Now suppose we can cover the whole region with eraseblock of larger size, S+1, and erase it all at once. This will run faster: erase opcode will only be sent once instead of many smaller opcodes. However, this will run erase over some areas of the chip memory that 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.
The tradeoff is the speed of programming operation VS the longevity of the chip. Default is longevity.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/84721/comment/2d1f60bb_1cd94751?us… :
PS3, Line 124: " --sacrifice-ratio <ratio> The percentage of overall sub-blocks needless to\n"
: " change within a larger erase block to be sacrificed\n"
: " for programming speed, default 0, max 100\n"
I have an idea, what do you think:
> Maximum % memory that is allowed for redundant erase, valid range 0 to 50. 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.
The tradeoff is the speed of programming operation VS the longevity of the chip. Default is longevity.
DANGEROUS! It wears your chip faster!
--
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: 5
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: 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-Comment-Date: Sat, 26 Oct 2024 13:04:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
Change subject: Complete and fix progress feature implementation for all operations
......................................................................
Patch Set 13: Code-Review+2
(1 comment)
Patchset:
PS13:
I just remembered, from our guidelines if there are multiple authors, each should approve each other work. I approve!
--
To view, visit https://review.coreboot.org/c/flashrom/+/84102?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: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Gerrit-Change-Number: 84102
Gerrit-PatchSet: 13
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Sat, 26 Oct 2024 10:19:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84439?usp=email )
Change subject: Display progress for what is actually erased/written
......................................................................
Patch Set 16:
(4 comments)
Patchset:
PS16:
Just out of curiosity I ran the test case from here https://review.coreboot.org/c/flashrom/+/84614/1..3//COMMIT_MSG#b19 but with added progress and freq param. That other patch is not about progress indicator, but it is a deep dive into erase/write logic, and in the test case there are various areas which need/need not to be written or erased.
The progress is shown! Read and write eventually get to 100, erase did not (only got up to 50), I guess because only selective regions were needed to erase.
Not blocking this patch, but it was interesting to check. It seems there is a room for improvement, but as I said I think not blocking this patch
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/84439/comment/6a40e284_2f8ceb46?us… :
PS13, Line 1270: // FIXME: round up to granularity?
> I think `get_next_write` limits length to not try to suggest writing more data then there is. […]
Okay so I will close this (and the other one). I removed FIXMEs, is this fine?
https://review.coreboot.org/c/flashrom/+/84439/comment/b1635465_9cfb818c?us… :
PS13, Line 1281: // FIXME: should be rounded up to page size?
> It does take granularity into account so maybe it's fine as is, assuming that regions never cross bo […]
Acknowledged
https://review.coreboot.org/c/flashrom/+/84439/comment/c50461a4_b0e66f74?us… :
PS13, Line 1463: // TODO: use erase_layout?
> I want to try to do this, leaving this as comment
Actually not, at this stage erase_layout is not useful: because it is only created but not initialised and more importantly, not processed for which erase blocks are selected for being run. There is no info for progress in erase_layout at this stage.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84439?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: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08
Gerrit-Change-Number: 84439
Gerrit-PatchSet: 16
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Sat, 26 Oct 2024 10:18:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Attention is currently required from: Sergii Dmytruk.
Hello Sergii Dmytruk, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84439?usp=email
to look at the new patch set (#16).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: Display progress for what is actually erased/written
......................................................................
Display progress for what is actually erased/written
The patch updates calculation of total length for the operation
which is displayed with progress.
The reason is: even if, for example the whole chip erase or write
was requested, the actual length of bytes modified can be less than
whole chip size (areas which already have expected content,
are skipped).
Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08
Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M doc/classic_cli_manpage.rst
M flashrom.c
M tests/chip.c
M tests/tests.c
M tests/tests.h
5 files changed, 113 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/84439/16
--
To view, visit https://review.coreboot.org/c/flashrom/+/84439?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: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08
Gerrit-Change-Number: 84439
Gerrit-PatchSet: 16
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Attention is currently required from: Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84439?usp=email )
Change subject: Display progress for what is actually erased/written
......................................................................
Patch Set 15:
(1 comment)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/84439/comment/cdf62aec_7458f8f7?us… :
PS7, Line 1265: // TODO: take FEATURE_NO_ERASE into account?
> I like the second approach better too. […]
Inspired by your suggestion, I made a solution which maybe not perfect, but at least it's simple (lines 1285-1290) :)
Total gets doubled for writing for no_erase chips, and then the rest running as normal.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84439?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: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08
Gerrit-Change-Number: 84439
Gerrit-PatchSet: 15
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Sat, 26 Oct 2024 08:51:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Attention is currently required from: Anastasia Klimchuk.
Hello Sergii Dmytruk, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84439?usp=email
to look at the new patch set (#15).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: Display progress for what is actually erased/written
......................................................................
Display progress for what is actually erased/written
The patch updates calculation of total length for the operation
which is displayed with progress.
The reason is: even if, for example the whole chip erase or write
was requested, the actual length of bytes modified can be less than
whole chip size (areas which already have expected content,
are skipped).
Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08
Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M doc/classic_cli_manpage.rst
M flashrom.c
M tests/chip.c
M tests/tests.c
M tests/tests.h
5 files changed, 117 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/84439/15
--
To view, visit https://review.coreboot.org/c/flashrom/+/84439?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: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08
Gerrit-Change-Number: 84439
Gerrit-PatchSet: 15
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84826?usp=email )
Change subject: writeprotect: Fix inaccurate return code
......................................................................
writeprotect: Fix inaccurate return code
If hardware protection is requested but not supported by the flash
chip, return an error code indicating that the protection mode is
unsupported, rather than indicating that all WP features are unsupported.
TEST=ninja test
Change-Id: I29e9069c7781fbb238f30aa9a9285b692b0c7200
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84826
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M writeprotect.c
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
Anastasia Klimchuk: Looks good to me, approved
build bot (Jenkins): Verified
diff --git a/writeprotect.c b/writeprotect.c
index 411089d..964c311 100644
--- a/writeprotect.c
+++ b/writeprotect.c
@@ -482,7 +482,7 @@
case FLASHROM_WP_MODE_HARDWARE:
if (!bits->srp_bit_present)
- return FLASHROM_WP_ERR_CHIP_UNSUPPORTED;
+ return FLASHROM_WP_ERR_MODE_UNSUPPORTED;
bits->srl = 0;
bits->srp = 1;
--
To view, visit https://review.coreboot.org/c/flashrom/+/84826?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I29e9069c7781fbb238f30aa9a9285b692b0c7200
Gerrit-Change-Number: 84826
Gerrit-PatchSet: 3
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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_classic: Add rpmc command support
......................................................................
Patch Set 3:
(1 comment)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/84840/comment/8709319f_bb0f0e83?us… :
PS2, Line 159: printf(".\n\nYou can specify one of -h, -R, -L, "
> Should this list be extended, since any RPMC command is also allowed?
Do you think the way I extended it is ok. Otherwise I think the message would get too long. The wp-range options are also not specified here.
--
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: 3
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: Fri, 25 Oct 2024 14:41:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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_classic: Add rpmc command support
......................................................................
Patch Set 3:
(3 comments)
Patchset:
PS3:
I didn't have my editor set up correctly for this coding style. Now with 8-wide tabs, I have restored the things i retabbed back to their original state.
Also I adjusted my patch to better match the coding style.
Please let me know if there are more things that need adjustment to match the coding style.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/84840/comment/d479cedd_bfd3983e?us… :
PS2, Line 113:
> I don't think we should re-tab all this text, since at default 8-char tabs some of them now exceed 8 […]
Done
https://review.coreboot.org/c/flashrom/+/84840/comment/00d2d6d8_8a91fa90?us… :
PS2, Line 147: #if CONFIG_RPMC_ENABLED == 1
> These options are all related to RPMC, so adding a heading might be useful. […]
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: 3
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: Fri, 25 Oct 2024 14:34:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
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 (#3).
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/3
--
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: 3
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>