- flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[master]: tests: add basic lifecycle test for ch341a_spi
by Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Felix Singer, Thomas Heijligen, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67664 ) Change subject: tests: add basic lifecycle test for ch341a_spi ...................................................................... Patch Set 10: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/67664 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If28fbe09ad685082152aa3a7e8d5a150169aee9e Gerrit-Change-Number: 67664 Gerrit-PatchSet: 10 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 24 Nov 2022 09:07:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: add mocks for libusb's asynchronous API
by Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69873 ) Change subject: tests: add mocks for libusb's asynchronous API ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/69873 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5a316687ab39a112d968eeaedb71f7b4b659d8d5 Gerrit-Change-Number: 69873 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 24 Nov 2022 08:58:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: add more wrappers for libusb funcs
by Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69872 ) Change subject: tests: add more wrappers for libusb funcs ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/69872 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic11efb9fd746cb91911dbe87e1c0028759f5bb0b Gerrit-Change-Number: 69872 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 24 Nov 2022 08:58:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashchips: Add support for GigaDevice GD251R512ME
by Kapil Porwal (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Harsha B R, Ravishankar Sarawadi. Kapil Porwal has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69928 ) Change subject: flashchips: Add support for GigaDevice GD251R512ME ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/69928/comment/cc2e75ab_9250ed43 PS1, Line 7: 1 `L`? -- To view, visit https://review.coreboot.org/c/flashrom/+/69928 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I15f76ce089cb2a2baf8bf8525510a7a7fe44b352 Gerrit-Change-Number: 69928 Gerrit-PatchSet: 1 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Harsha B R <harsha.b.r(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Harsha B R <harsha.b.r(a)intel.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Comment-Date: Thu, 24 Nov 2022 07:07:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: partial_lock: Use WriteProtectState cache
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69418 ) ( 3 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom_tester: partial_lock: Use WriteProtectState cache ...................................................................... flashrom_tester: partial_lock: Use WriteProtectState cache partial_lock test was bypassing the WriteProtectState cache of the software write protect by directly calling env.cmd.wp_range. It was also unnesesarily disabling software WP. Fix those issues and more clearly document what the test is doing and expecting. BUG=b:244663741 BRANCH=None TEST=flashrom_tester --libflashrom host Change-Id: Ic3f89ff5d22e74e4e6c94e755b936e58cb27182d Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69418 Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M util/flashrom_tester/src/tester.rs M util/flashrom_tester/src/tests.rs 2 files changed, 47 insertions(+), 4 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/src/tester.rs b/util/flashrom_tester/src/tester.rs index d19e45c..1fa44a8 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -246,6 +246,24 @@ } } + // Set software write protect with a custom range + pub fn set_range(&mut self, range: (i64, i64), enable: bool) -> Result<&mut Self, String> { + info!("set_range request={}, current={}", enable, self.current.sw); + self.cmd + .wp_range(range, enable) + .map_err(|e| e.to_string())?; + let actual_state = Self::get_sw(self.cmd).map_err(|e| e.to_string())?; + if actual_state != enable { + Err(format!( + "set_range request={}, real={}", + enable, actual_state + )) + } else { + self.current.sw = enable; + Ok(self) + } + } + /// Set the hardware write protect if supported and check that the state is as expected. pub fn set_hw(&mut self, enable: bool) -> Result<&mut Self, String> { info!("set_hw request={}, current={}", enable, self.current.hw); diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs index e6968c8..3e39e4d 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -272,10 +272,12 @@ env.ensure_golden()?; let (wp_section_name, start, len) = utils::layout_section(env.layout(), section); - // Disable software WP so we can do range protection, but hardware WP - // must remain enabled for (most) range protection to do anything. - env.wp.set_hw(false)?.set_sw(false)?; - env.cmd.wp_range((start, len), true)?; + // Disable hardware WP so we can modify the protected range. + env.wp.set_hw(false)?; + // Then enable software WP so the range is enforced and enable hardware + // WP so that flashrom does not disable software WP during the + // operation. + env.wp.set_range((start, len), true)?; env.wp.set_hw(true)?; // Check that we cannot write to the protected region. -- To view, visit https://review.coreboot.org/c/flashrom/+/69418 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic3f89ff5d22e74e4e6c94e755b936e58cb27182d Gerrit-Change-Number: 69418 Gerrit-PatchSet: 6 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

1 0

[M] Change in flashrom[master]: flashrom_tester: Change the wp_toggle semantics
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69417 ) Change subject: flashrom_tester: Change the wp_toggle semantics ...................................................................... flashrom_tester: Change the wp_toggle semantics wp_toggle and wp_range had some confusing behaviour where enabling wp would set a range, but disabling wp would not unset the range (explicitly). This was a way to workaround the MTD kernel driver semantics. Now make things very explicit, enabling software write protect will set the range to the whole chip. Disabling write protect will set the range to 0,0. This makes all drivers behave the same as MTD, and documents the exact behaviour explicitly. BUG=b:244663741 BRANCH=None TEST=flashrom_tester --libflashrom host # MTD and non-MTD TEST=flashrom_tester --flashrom_binary # MTD and non-MTD Change-Id: Ia01d612d988e6580a7c5f0fd448ccc319ce9b181 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69417 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 3 files changed, 40 insertions(+), 25 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs index 4078910..458f054 100644 --- a/util/flashrom_tester/flashrom/src/cmd.rs +++ b/util/flashrom_tester/flashrom/src/cmd.rs @@ -149,11 +149,12 @@ Ok(true) } - fn wp_range(&self, range: (i64, i64), wp_enable: bool) -> Result<bool, FlashromError> { + fn wp_range(&self, range: (i64, i64), en: bool) -> Result<bool, FlashromError> { let opts = FlashromOpt { wp_opt: WPOpt { + enable: en, + disable: !en, range: Some(range), - enable: wp_enable, ..Default::default() }, ..Default::default() @@ -200,28 +201,14 @@ } fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError> { - let status = if en { "en" } else { "dis" }; - - // For MTD, --wp-range and --wp-enable must be used simultaneously. let range = if en { let rom_sz: i64 = self.get_size()?; - Some((0, rom_sz)) // (start, len) + (0, rom_sz) // (start, len) } else { - None + (0, 0) }; - - let opts = FlashromOpt { - wp_opt: WPOpt { - range, - enable: en, - disable: !en, - ..Default::default() - }, - ..Default::default() - }; - - self.dispatch(opts, "wp_toggle")?; - + self.wp_range(range, en)?; + let status = if en { "en" } else { "dis" }; match self.wp_status(true) { Ok(_ret) => { info!("Successfully {}abled write-protect", status); diff --git a/util/flashrom_tester/flashrom/src/flashromlib.rs b/util/flashrom_tester/flashrom/src/flashromlib.rs index 3036394..6cdd55d 100644 --- a/util/flashrom_tester/flashrom/src/flashromlib.rs +++ b/util/flashrom_tester/flashrom/src/flashromlib.rs @@ -102,10 +102,8 @@ } fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError> { - // TODO why does the cmd impl not do this? - // for cmd, range is only set for enable - // and disable is not sent for the wp_range command - self.wp_range((0, self.get_size()?), en) + let range = if en { (0, self.get_size()?) } else { (0, 0) }; + self.wp_range(range, en) } fn read_into_file(&self, path: &Path) -> Result<(), FlashromError> { diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs index 11874f9..90e40e2 100644 --- a/util/flashrom_tester/flashrom/src/lib.rs +++ b/util/flashrom_tester/flashrom/src/lib.rs @@ -133,7 +133,7 @@ /// Write only a region of the flash. fn write_file_with_layout(&self, rws: &ROMWriteSpecifics) -> Result<bool, FlashromError>; - /// Set write protect status for a range. + /// Set write protect status and range. fn wp_range(&self, range: (i64, i64), wp_enable: bool) -> Result<bool, FlashromError>; /// Read the write protect regions for the flash. @@ -143,6 +143,10 @@ fn wp_status(&self, en: bool) -> Result<bool, FlashromError>; /// Set write protect status. + /// If en=true sets wp_range to the whole chip (0,getsize()). + /// If en=false sets wp_range to (0,0). + /// This is due to the MTD driver, which requires wp enable to use a range + /// length != 0 and wp disable to have the range 0,0. fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError>; /// Read the whole flash to the file specified by `path`. -- To view, visit https://review.coreboot.org/c/flashrom/+/69417 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia01d612d988e6580a7c5f0fd448ccc319ce9b181 Gerrit-Change-Number: 69417 Gerrit-PatchSet: 5 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: flashrom_tester: Log some cros lsb-release information
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69403 ) ( 5 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom_tester: Log some cros lsb-release information ...................................................................... flashrom_tester: Log some cros lsb-release information os-release does not quite have as much information so use lsb-release. BUG=b:258289727 BRANCH=None TEST=flashrom_tester --libflashrom host Lock Change-Id: If3452ead9e02e0ddeaa0fdf4852d7c17a8ab7650 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69403 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M util/flashrom_tester/src/cros_sysinfo.rs M util/flashrom_tester/src/tester.rs M util/flashrom_tester/src/tests.rs 3 files changed, 35 insertions(+), 0 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/src/cros_sysinfo.rs b/util/flashrom_tester/src/cros_sysinfo.rs index e047f8e..37e1ec6 100644 --- a/util/flashrom_tester/src/cros_sysinfo.rs +++ b/util/flashrom_tester/src/cros_sysinfo.rs @@ -34,6 +34,7 @@ // use std::ffi::OsStr; +use std::fs; use std::io::Result as IoResult; use std::process::{Command, Stdio}; @@ -58,3 +59,12 @@ pub fn bios_info() -> IoResult<String> { dmidecode_dispatch(&["-q", "-t0"]) } + +pub fn release_description() -> IoResult<String> { + for l in fs::read_to_string("/etc/lsb-release")?.lines() { + if l.starts_with("CHROMEOS_RELEASE_DESCRIPTION") { + return Ok(l.to_string()); + } + } + Err(std::io::ErrorKind::NotFound.into()) +} diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs index bedccaf..d19e45c 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -348,6 +348,7 @@ pub struct ReportMetaData { pub chip_name: String, pub os_release: String, + pub cros_release: String, pub system_info: String, pub bios_info: String, } @@ -451,6 +452,7 @@ println!(); println!(" %---------------------------%"); println!(" os release: {}", meta_data.os_release); + println!(" cros release: {}", meta_data.cros_release); println!(" chip name: {}", meta_data.chip_name); println!(" system info: \n{}", meta_data.system_info); println!(" bios info: \n{}", meta_data.bios_info); diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs index 5cbe9d2..e6968c8 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -134,12 +134,15 @@ } let os_release = sys_info::os_release().unwrap_or("<Unknown OS>".to_string()); + let cros_release = cros_sysinfo::release_description() + .unwrap_or("<Unknown or not a ChromeOS release>".to_string()); let system_info = cros_sysinfo::system_info().unwrap_or("<Unknown System>".to_string()); let bios_info = cros_sysinfo::bios_info().unwrap_or("<Unknown BIOS>".to_string()); let meta_data = tester::ReportMetaData { chip_name, os_release, + cros_release, system_info, bios_info, }; -- To view, visit https://review.coreboot.org/c/flashrom/+/69403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If3452ead9e02e0ddeaa0fdf4852d7c17a8ab7650 Gerrit-Change-Number: 69403 Gerrit-PatchSet: 8 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

1 0

[M] Change in flashrom[master]: flashrom_tester: Check the WP state when setting
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69402 ) ( 6 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom_tester: Check the WP state when setting ...................................................................... flashrom_tester: Check the WP state when setting Check that the hardware and software WP state are as expected in the setter methods. BUG=b:244663741 BRANCH=None TEST=flashrom_tester --libflashrom host Change-Id: Ie7f90ab478dca6f92eaa0908443e3cb156ea0319 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69402 Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M util/flashrom_tester/src/tester.rs 1 file changed, 46 insertions(+), 9 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/src/tester.rs b/util/flashrom_tester/src/tester.rs index c43c7d2..bedccaf 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -229,28 +229,44 @@ /// Set the software write protect and check that the state is as expected. pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, String> { - info!("request={}, current={}", enable, self.current.sw); + info!("set_sw request={}, current={}", enable, self.current.sw); if self.current.sw != enable { self.cmd .wp_toggle(/* en= */ enable) .map_err(|e| e.to_string())?; } - Ok(self) + if Self::get_sw(self.cmd).map_err(|e| e.to_string())? != enable { + Err(format!( + "Software write protect did not change state to {} when requested", + enable + )) + } else { + self.current.sw = enable; + Ok(self) + } } - /// Set the hardware write protect. + /// Set the hardware write protect if supported and check that the state is as expected. pub fn set_hw(&mut self, enable: bool) -> Result<&mut Self, String> { + info!("set_hw request={}, current={}", enable, self.current.hw); if self.can_control_hw_wp() { if self.current.hw != enable { super::utils::toggle_hw_wp(/* dis= */ !enable)?; - self.current.hw = enable; - } else if enable { - info!( - "Ignoring attempt to enable hardware WP with {:?} programmer", - self.fc - ); } + // toggle_hw_wp does check this, but we might not have called toggle_hw_wp so check again. + if Self::get_hw(self.cmd)? != enable { + return Err(format!( + "Hardware write protect did not change state to {} when requested", + enable + )); + } + } else { + info!( + "Ignoring attempt to set hardware WP with {:?} programmer", + self.fc + ); } + self.current.hw = enable; Ok(self) } -- To view, visit https://review.coreboot.org/c/flashrom/+/69402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie7f90ab478dca6f92eaa0908443e3cb156ea0319 Gerrit-Change-Number: 69402 Gerrit-PatchSet: 8 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-CC: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-MessageType: merged

1 0

[L] Change in flashrom[master]: flashrom_tester: Simplify WriteProtectState
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69401 ) ( 5 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom_tester: Simplify WriteProtectState ...................................................................... flashrom_tester: Simplify WriteProtectState Remove the WriteProtectState 'stack' implementation and the push function. This functionality allowed states to be stacked and then automatically unrolled via RAII lifetimes. This was useful. However this unrolling could make errors in a flashrom_tester run much harder to understand, as the actual failure would be followed by multiple write protect calls that could subsequently fail, potentially causing panicing inside the panic handler and the process to be hard aborted and the restore golden image function would not be run. The new approach is to prefer code simplicity. Ideally this makes errors easier to diagnose from logs. To that end the lifetime has been simplified. The stack has been removed. The mutex has been removed. This means tests may not be running in the same environment they were previously. However if they continue to specify their requirements with set_sw and set_hw there will be no difference and the errors will be clear. BUG=b:259494812 BRANCH=None TEST=flashrom_tester --libflashrom host Change-Id: I1c4251f69b42a327383b8a99fa933f411feb9568 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69401 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M util/flashrom_tester/src/tester.rs 1 file changed, 71 insertions(+), 187 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs index 323b536..c43c7d2 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -41,11 +41,9 @@ use serde_json::json; use std::fs::File; use std::io::Write; -use std::mem::MaybeUninit; use std::path::Path; use std::path::PathBuf; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::Mutex; // type-signature comes from the return type of lib.rs workers. type TestError = Box<dyn std::error::Error>; @@ -60,7 +58,7 @@ pub cmd: &'a dyn Flashrom, layout: LayoutSizes, - pub wp: WriteProtectState<'a, 'static>, + pub wp: WriteProtectState<'a>, /// The path to a file containing the flash contents at test start. original_flash_contents: PathBuf, /// The path to a file containing flash-sized random data @@ -174,64 +172,33 @@ } } +struct WriteProtect { + hw: bool, + sw: bool, +} + /// RAII handle for setting write protect in either hardware or software. /// /// Given an instance, the state of either write protect can be modified by calling -/// `set` or `push`. When it goes out of scope, the write protects will be returned +/// `set`. When it goes out of scope, the write protects will be returned /// to the state they had then it was created. -/// -/// The lifetime `'p` on this struct is the parent state it derives from; `'static` -/// implies it is derived from hardware, while anything else is part of a stack -/// created by `push`ing states. An initial state is always static, and the stack -/// forms a lifetime chain `'static -> 'p -> 'p1 -> ... -> 'pn`. -pub struct WriteProtectState<'a, 'p> { - /// The parent state this derives from. - /// - /// If it's a root (gotten via `from_hardware`), then this is Hardware and the - /// liveness flag will be reset on drop. - initial: InitialState<'p>, - // Tuples are (hardware, software) - current: (bool, bool), +pub struct WriteProtectState<'a> { + current: WriteProtect, + initial: WriteProtect, cmd: &'a dyn Flashrom, fc: FlashChip, } -enum InitialState<'p> { - Hardware(bool, bool), - Previous(&'p WriteProtectState<'p, 'p>), -} - -impl InitialState<'_> { - fn get_target(&self) -> (bool, bool) { - match self { - InitialState::Hardware(hw, sw) => (*hw, *sw), - InitialState::Previous(s) => s.current, - } - } -} - -impl<'a> WriteProtectState<'a, 'static> { +impl<'a> WriteProtectState<'a> { /// Initialize a state from the current state of the hardware. - /// - /// Panics if there is already a live state derived from hardware. In such a situation the - /// new state must be derived from the live one, or the live one must be dropped first. pub fn from_hardware(cmd: &'a dyn Flashrom, fc: FlashChip) -> Result<Self, FlashromError> { - let mut lock = Self::get_liveness_lock() - .lock() - .expect("Somebody panicked during WriteProtectState init from hardware"); - if *lock { - drop(lock); // Don't poison the lock - panic!("Attempted to create a new WriteProtectState when one is already live"); - } - let hw = Self::get_hw(cmd)?; let sw = Self::get_sw(cmd)?; - info!("Initial hardware write protect: HW={} SW={}", hw, sw); + info!("Initial write protect state: HW={} SW={}", hw, sw); - *lock = true; Ok(WriteProtectState { - initial: InitialState::Hardware(hw, sw), - current: (hw, sw), + current: WriteProtect { hw, sw }, + initial: WriteProtect { hw, sw }, cmd, fc, }) @@ -251,9 +218,7 @@ let b = cmd.wp_status(true)?; Ok(b) } -} -impl<'a, 'p> WriteProtectState<'a, 'p> { /// Return true if the current programmer supports setting the hardware /// write protect. /// @@ -262,22 +227,23 @@ self.cmd.can_control_hw_wp() } - /// Set the software write protect. - pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, FlashromError> { - info!("request={}, current={}", enable, self.current.1); - if self.current.1 != enable { - self.cmd.wp_toggle(/* en= */ enable)?; - self.current.1 = enable; + /// Set the software write protect and check that the state is as expected. + pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, String> { + info!("request={}, current={}", enable, self.current.sw); + if self.current.sw != enable { + self.cmd + .wp_toggle(/* en= */ enable) + .map_err(|e| e.to_string())?; } Ok(self) } /// Set the hardware write protect. pub fn set_hw(&mut self, enable: bool) -> Result<&mut Self, String> { - if self.current.0 != enable { - if self.can_control_hw_wp() { + if self.can_control_hw_wp() { + if self.current.hw != enable { super::utils::toggle_hw_wp(/* dis= */ !enable)?; - self.current.0 = enable; + self.current.hw = enable; } else if enable { info!( "Ignoring attempt to enable hardware WP with {:?} programmer", @@ -288,151 +254,35 @@ Ok(self) } - /// Stack a new write protect state on top of the current one. - /// - /// This is useful if you need to temporarily make a change to write protection: - /// - /// ```no_run - /// # fn main() -> Result<(), Box<dyn std::error::Error>> { - /// # let cmd: flashrom::FlashromCmd = unimplemented!(); - /// let wp = flashrom_tester::tester::WriteProtectState::from_hardware(&cmd, flashrom::FlashChip::SERVO)?; - /// { - /// let mut wp = wp.push(); - /// wp.set_sw(false)?; - /// // Do something with software write protect disabled - /// } - /// // Now software write protect returns to its original state, even if - /// // set_sw() failed. - /// # Ok(()) - /// # } - /// ``` - /// - /// This returns a new state which restores the original when it is dropped- the new state - /// refers to the old, so the compiler enforces that states are disposed of in the reverse - /// order of their creation and correctly restore the original state. - pub fn push<'p1>(&'p1 self) -> WriteProtectState<'a, 'p1> { - WriteProtectState { - initial: InitialState::Previous(self), - current: self.current, - cmd: self.cmd, - fc: self.fc, - } - } - - fn get_liveness_lock() -> &'static Mutex<bool> { - static INIT: std::sync::Once = std::sync::Once::new(); - /// Value becomes true when there is a live WriteProtectState derived `from_hardware`, - /// blocking duplicate initialization. - /// - /// This is required because hardware access is not synchronized; it's possible to leave the - /// hardware in an unintended state by creating a state handle from it, modifying the state, - /// creating another handle from the hardware then dropping the first handle- then on drop - /// of the second handle it will restore the state to the modified one rather than the initial. - /// - /// This flag ensures that a duplicate root state cannot be created. - /// - /// This is a Mutex<bool> rather than AtomicBool because acquiring the flag needs to perform - /// several operations that may themselves fail- acquisitions must be fully synchronized. - static mut LIVE_FROM_HARDWARE: MaybeUninit<Mutex<bool>> = MaybeUninit::uninit(); - - unsafe { - INIT.call_once(|| { - LIVE_FROM_HARDWARE.as_mut_ptr().write(Mutex::new(false)); - }); - &*LIVE_FROM_HARDWARE.as_ptr() - } - } - /// Reset the hardware to what it was when this state was created, reporting errors. /// /// This behaves exactly like allowing a state to go out of scope, but it can return /// errors from that process rather than panicking. pub fn close(mut self) -> Result<(), String> { - unsafe { - let out = self.drop_internal(); - // We just ran drop, don't do it again - std::mem::forget(self); - out - } + let out = self.drop_internal(); + // We just ran drop, don't do it again + std::mem::forget(self); + out } - /// Internal Drop impl. - /// - /// This is unsafe because it effectively consumes self when clearing the - /// liveness lock. Callers must be able to guarantee that self will be forgotten - /// if the state was constructed from hardware in order to uphold the liveness - /// invariant (that only a single state constructed from hardware exists at any - /// time). - unsafe fn drop_internal(&mut self) -> Result<(), String> { - let lock = match self.initial { - InitialState::Hardware(_, _) => Some( - Self::get_liveness_lock() - .lock() - .expect("Somebody panicked during WriteProtectState drop from hardware"), - ), - _ => None, - }; - let (hw, sw) = self.initial.get_target(); - - fn enable_str(enable: bool) -> &'static str { - if enable { - "en" - } else { - "dis" - } - } - + /// Sets both write protects to the state they had when this state was created. + fn drop_internal(&mut self) -> Result<(), String> { // Toggle both protects back to their initial states. // Software first because we can't change it once hardware is enabled. - if sw != self.current.1 { - // Is the hw wp currently enabled? - if self.current.0 { - super::utils::toggle_hw_wp(/* dis= */ true).map_err(|e| { - format!( - "Failed to {}able hardware write protect: {}", - enable_str(false), - e - ) - })?; - } - self.cmd.wp_toggle(/* en= */ sw).map_err(|e| { - format!( - "Failed to {}able software write protect: {}", - enable_str(sw), - e - ) - })?; + if self.set_sw(self.initial.sw).is_err() { + self.set_hw(false)?; + self.set_sw(self.initial.sw)?; } + self.set_hw(self.initial.hw)?; - assert!( - self.cmd.can_control_hw_wp() || (!self.current.0 && !hw), - "HW WP must be disabled if it cannot be controlled" - ); - if hw != self.current.0 { - super::utils::toggle_hw_wp(/* dis= */ !hw).map_err(|e| { - format!( - "Failed to {}able hardware write protect: {}", - enable_str(hw), - e - ) - })?; - } - - if let Some(mut lock) = lock { - // Initial state was constructed via from_hardware, now we can clear the liveness - // lock since reset is complete. - *lock = false; - } Ok(()) } } -impl<'a, 'p> Drop for WriteProtectState<'a, 'p> { - /// Sets both write protects to the state they had when this state was created. - /// - /// Panics on error because there is no mechanism to report errors in Drop. +impl<'a> Drop for WriteProtectState<'a> { fn drop(&mut self) { - unsafe { self.drop_internal() }.expect("Error while dropping WriteProtectState") + self.drop_internal() + .expect("Error while dropping WriteProtectState") } } -- To view, visit https://review.coreboot.org/c/flashrom/+/69401 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1c4251f69b42a327383b8a99fa933f411feb9568 Gerrit-Change-Number: 69401 Gerrit-PatchSet: 7 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

1 0

[S] Change in flashrom[master]: flashrom_tester: lock_test: Make the logic clear and explicit
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69400 ) ( 4 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom_tester: lock_test: Make the logic clear and explicit ...................................................................... flashrom_tester: lock_test: Make the logic clear and explicit Document clearly what the test is doing and expects. Do not use the push() function as it confuses the logs in the case of error. BUG=b:258357944 BRANCH=None TEST=flashrom_tester --libflashrom host Lock Change-Id: Ibad559a8ff9696fd91f45bca9d9ceb6e90c41393 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69400 Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M util/flashrom_tester/src/tests.rs 1 file changed, 25 insertions(+), 4 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/src/tests.rs b/util/flashrom_tester/src/tests.rs index af34208..5cbe9d2 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -199,12 +199,12 @@ } env.wp.set_hw(false)?.set_sw(true)?; - // Toggling software WP off should work when hardware is off. - // Then enable again for another go. - env.wp.push().set_sw(false)?; + // Toggling software WP off should work when hardware WP is off. + // Then enable software WP again for the next test. + env.wp.set_sw(false)?.set_sw(true)?; + // Toggling software WP off should not work when hardware WP is on. env.wp.set_hw(true)?; - // Clearing should fail when hardware is enabled if env.wp.set_sw(false).is_ok() { return Err("Software WP was reset despite hardware WP being enabled".into()); } -- To view, visit https://review.coreboot.org/c/flashrom/+/69400 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibad559a8ff9696fd91f45bca9d9ceb6e90c41393 Gerrit-Change-Number: 69400 Gerrit-PatchSet: 6 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

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit