Attention is currently required from: Simon Buhrow, Aarya, Anastasia Klimchuk.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65844 )
Change subject: flashrom.c: Add functions for new erase selction algorithm
......................................................................
Patch Set 87:
(1 comment)
Patchset:
PS87:
* The files are named `erasure_layout.h`. Is this intentional? You named the include guard `ERASE_LAYOUT_H`
* We have the `/include/` directory for header files
--
To view, visit https://review.coreboot.org/c/flashrom/+/65844
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256
Gerrit-Change-Number: 65844
Gerrit-PatchSet: 87
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 06 Feb 2023 19:14:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan, Aarya.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71119 )
Change subject: flashrom.c: Add switch for legacy impl of erasure path
......................................................................
Patch Set 13: Code-Review+2
(1 comment)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/71119/comment/7115e1a3_085eec62
PS12, Line 37: use_legacy_erase_path
> Cool. […]
That's a good plan! Thanks for elaborating it.
--
To view, visit https://review.coreboot.org/c/flashrom/+/71119
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib5660db0067c1c799dcb5c8e83b4a4826b236442
Gerrit-Change-Number: 71119
Gerrit-PatchSet: 13
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Mon, 06 Feb 2023 18:59:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Evan Benn.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72004 )
Change subject: flashrom_tester: Use Range and usize for all ranges
......................................................................
Patch Set 9: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/72004
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic09be66c3598acf45fbe8952093006a9b185810a
Gerrit-Change-Number: 72004
Gerrit-PatchSet: 9
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-Comment-Date: Mon, 06 Feb 2023 02:45:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen.
Hello Thomas Heijligen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/72828
to look at the new patch set (#2).
Change subject: wbsio_spi.c: Drop unmaintained driver path
......................................................................
wbsio_spi.c: Drop unmaintained driver path
The 'Intel D201GLY' board is extremely old and the specialised
superio path it uses is highly bespoke to it. This causes a undue
maintenance burden to core flashrom data structures in how the
spi driver entry-point is dispatched via a board_enable hook.
Therefore in the cost-benefit just deprecate it to allow the
overall flashrom project to progress.
Change-Id: I25c4d02126fcf183662dec73a800cac3988a1773
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M Makefile
M board_enable.c
M include/programmer.h
M meson.build
D wbsio_spi.c
5 files changed, 21 insertions(+), 231 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/28/72828/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/72828
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I25c4d02126fcf183662dec73a800cac3988a1773
Gerrit-Change-Number: 72828
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: newpatchset
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/71974 )
Change subject: flashrom_tester: Add positive check to verify_fail_test
......................................................................
flashrom_tester: Add positive check to verify_fail_test
In verify_fail_test test that verify works when expected, as well as
fails when expected. A verify_region_from_file function is added to
support this.
BUG=b:235916336
BRANCH=None
TEST=None
Change-Id: Ibbcc97086466b67cfab4f6c32140bb5f2c456beb
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/71974
Reviewed-by: Peter Marheine <pmarheine(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
---
M util/flashrom_tester/flashrom/src/cmd.rs
M util/flashrom_tester/flashrom/src/flashromlib.rs
M util/flashrom_tester/flashrom/src/lib.rs
M util/flashrom_tester/src/tests.rs
4 files changed, 72 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Edward O'Callaghan: Looks good to me, approved
Peter Marheine: Looks good to me, but someone else must approve
diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs
index 85cbb12..1f13a8e 100644
--- a/util/flashrom_tester/flashrom/src/cmd.rs
+++ b/util/flashrom_tester/flashrom/src/cmd.rs
@@ -269,6 +269,18 @@
Ok(())
}
+ fn verify_region_from_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> {
+ let opts = FlashromOpt {
+ io_opt: Some(IOOpt::Verify(OperationArgs::RegionFileRegion(
+ region, path, None,
+ ))),
+ ..Default::default()
+ };
+
+ self.dispatch(opts, "verify_region_from_file")?;
+ Ok(())
+ }
+
fn erase(&self) -> Result<(), FlashromError> {
let opts = FlashromOpt {
io_opt: Some(IOOpt::Erase),
diff --git a/util/flashrom_tester/flashrom/src/flashromlib.rs b/util/flashrom_tester/flashrom/src/flashromlib.rs
index bf09e6d..5e1747b 100644
--- a/util/flashrom_tester/flashrom/src/flashromlib.rs
+++ b/util/flashrom_tester/flashrom/src/flashromlib.rs
@@ -152,6 +152,27 @@
Ok(())
}
+ fn verify_region_from_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> {
+ let mut layout = self.flashrom.borrow_mut().layout_read_fmap_from_rom()?;
+ layout.include_region(region)?;
+ let range = layout.get_region_range(region)?;
+ let region_data = fs::read(path).map_err(|error| error.to_string())?;
+ if region_data.len() != range.len() {
+ return Err(format!(
+ "verify region range ({}) does not match provided file size ({})",
+ range.len(),
+ region_data.len()
+ )
+ .into());
+ }
+ let mut buf = vec![0; self.get_size()? as usize];
+ buf[range].copy_from_slice(®ion_data);
+ self.flashrom
+ .borrow_mut()
+ .image_verify(&buf, Some(layout))?;
+ Ok(())
+ }
+
fn erase(&self) -> Result<(), FlashromError> {
self.flashrom.borrow_mut().erase()?;
Ok(())
diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs
index 7c86649..41393e8 100644
--- a/util/flashrom_tester/flashrom/src/lib.rs
+++ b/util/flashrom_tester/flashrom/src/lib.rs
@@ -133,7 +133,8 @@
/// Read the whole flash to the file specified by `path`.
fn read_into_file(&self, path: &Path) -> Result<(), FlashromError>;
- /// Read only a region of the flash.
+ /// Read only a region of the flash into the file specified by `path`. Note
+ /// the first byte written to the file is the first byte from the region.
fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError>;
/// Write the whole flash to the file specified by `path`.
@@ -152,6 +153,10 @@
/// Verify the whole flash against the file specified by `path`.
fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError>;
+ /// Verify only the region against the file specified by `path`.
+ /// Note the first byte in the file is matched against the first byte of the region.
+ fn verify_region_from_file(&self, path: &Path, region: &str) -> Result<(), FlashromError>;
+
/// Erase the whole flash.
fn erase(&self) -> Result<(), FlashromError>;
diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs
index 847bfec..721a789 100644
--- a/util/flashrom_tester/src/tests.rs
+++ b/util/flashrom_tester/src/tests.rs
@@ -44,6 +44,7 @@
use std::sync::atomic::AtomicBool;
const ELOG_FILE: &str = "/tmp/elog.file";
+const FW_MAIN_B_PATH: &str = "/tmp/FW_MAIN_B.bin";
/// Iterate over tests, yielding only those tests with names matching filter_names.
///
@@ -312,7 +313,16 @@
/// Check that flashrom 'verify' will fail if the provided data does not match the chip data.
fn verify_fail_test(env: &mut TestEnv) -> TestResult {
- // Comparing the flash contents to random data says they're not the same.
+ env.ensure_golden()?;
+ // Verify that verify is Ok when the data matches. We verify only a region
+ // and not the whole chip because coprocessors or firmware may have written
+ // some data in other regions.
+ env.cmd
+ .read_region_into_file(FW_MAIN_B_PATH.as_ref(), "FW_MAIN_B")?;
+ env.cmd
+ .verify_region_from_file(FW_MAIN_B_PATH.as_ref(), "FW_MAIN_B")?;
+
+ // Verify that verify is false when the data does not match
match env.verify(env.random_data_file()) {
Ok(_) => Err("Verification says flash is full of random data".into()),
Err(_) => Ok(()),
--
To view, visit https://review.coreboot.org/c/flashrom/+/71974
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ibbcc97086466b67cfab4f6c32140bb5f2c456beb
Gerrit-Change-Number: 71974
Gerrit-PatchSet: 10
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged