From 5b4144a3b3138fe044d968d14494cbb6ebe179e1 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 27 Jul 2023 16:42:13 -0400 Subject: [PATCH 1/7] Refactor flash check/verify --- cmd/flash/src/lib.rs | 150 +++++++++++++++++++++++++++---------------- 1 file changed, 96 insertions(+), 54 deletions(-) diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index a4fcbb7a1..6e7bf2546 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -7,12 +7,12 @@ //! Flashes the target with the image that is contained within the specified //! archive (or dump). As a precautionary measure, if the specified archive //! already appears to be on the target, `humility flash` will fail unless the -//! `-F` (`--force`) flag is set. Because this will only check the image -//! ID (and not the entire image), `humility flash` can be optionally told -//! to verify that all of the program text in the image is on the device -//! by specifying `-V` (`--verify`). Similarly, if one wishes to *only* -//! check the image against the archive (and not flash at all), specify -//! `-C` (`--check`). +//! `-F` (`--force`) flag is set. This will only check the image ID; it does +//! not check the entire image or auxiliary flash. `humility flash` can be +//! optionally told to verify all of the program text **and** the auxiliary +//! flash by specifying `-V` (`--verify`). Similarly, if one wishes to *only* +//! check the image against the archive (and not flash at all), specify `-C` +//! (`--check`). //! //! This attempts to natively flash the part within Humility using probe-rs, //! but for some parts or configurations, it may need to use OpenOCD as a @@ -32,7 +32,7 @@ //! will be programmed after the image is written. See RFD 311 for more //! information about auxiliary flash management. -use anyhow::{bail, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use clap::{CommandFactory, Parser}; use humility::{core::Core, hubris::*}; use humility_cli::{ @@ -108,7 +108,6 @@ fn force_openocd( let core = c.as_mut(); validate(hubris, core, subargs)?; - if subargs.check { return Ok(()); } @@ -216,69 +215,113 @@ fn force_openocd( Ok(()) } +/// Validates the image and auxiliary flash against our subcommand +/// +/// If `subargs.check` is true, returns `Ok(())` on a clean check and `Err(..)` +/// otherwise; the core is left running. +/// +/// If `subargs.check` is false, returns `Ok(())` if the check _fails_ (meaning +/// we should reflash), and `Err(..)` if all checks pass (meaning we should +/// _not_ reflash). The core is left halted if we should reflash and is left +/// running otherwise. fn validate( hubris: &mut HubrisArchive, core: &mut dyn humility::core::Core, subargs: &FlashArgs, ) -> Result<()> { - core.halt()?; + let r = get_image_state(hubris, core, subargs.verify || subargs.check); + if subargs.check { + core.run()?; + return r; + } - // - // We want to actually try validating to determine if this archive - // already matches; if it does, this command may well be in error, - // and we want to force the user to force their intent. - // - match hubris.validate(core, HubrisValidate::ArchiveMatch) { - Ok(_) => { + match r { + Ok(()) => { if subargs.force { humility::msg!( "archive appears to be already flashed; forcing re-flash" ); - } else if subargs.verify || subargs.check { - if let Err(err) = hubris.verify(core) { - if subargs.check { - core.run()?; - bail!( - "image IDs match, but flash contents do not match \ - archive contents: {err}", - ); - } - - humility::msg!( - "image IDs match, but flash contents do not match \ - archive contents: {err}; reflashing", - ); - } else { - core.run()?; - - if subargs.check { - humility::msg!("archive matches flash contents"); - return Ok(()); - } - - bail!( - "archive is already flashed on attached device; \ - use -F (\"--force\") to force re-flash" - ); - } + Ok(()) } else { core.run()?; - bail!( + Err(anyhow!(if subargs.verify { + "archive is already flashed on attached device; use -F \ + (\"--force\") to force re-flash" + } else { "archive appears to be already flashed on attached \ - device; use -F (\"--force\") to force re-flash or \ - -V (\"--verify\") to verify contents" - ); + device; use -F (\"--force\") to force re-flash or -V \ + (\"--verify\") to verify contents" + })) } } - Err(err) => { - if subargs.check { - core.run()?; - bail!("flash/archive mismatch: {}", err); - } + Err(e) => { + humility::msg!("{e}; reflashing"); + Ok(()) } } +} - Ok(()) +/// Checks the image and auxiliary flash +/// +/// Returns `Ok(())` if the image matches; returns an error otherwise. +/// +/// The core is halted when this function exits. +fn get_image_state( + hubris: &mut HubrisArchive, + core: &mut dyn humility::core::Core, + full_check: bool, +) -> Result<()> { + core.halt()?; + + // First pass: check only the image ID + hubris + .validate(core, HubrisValidate::ArchiveMatch) + .context("flash/archive mismatch")?; + + // More rigorous checks if requested + if full_check { + hubris.verify(core).with_context( + format!("image IDs match, but flash contents do not match \ + archive contents: {err}"), + )?; + + let r = if hubris.read_auxflash_data()?.is_some() { + // The core must be running for us to check the auxflash slot. + // + // However, we want it halted before we exit this function, which + // requires careful handling. + core.run()?; + let mut worker = match cmd_auxflash::AuxFlashHandler::new( + hubris, core, 15_000, + ) { + Ok(w) => w, + Err(e) => { + core.halt()?; + return Err(e); + } + }; + let r = match worker.active_slot() { + Ok(Some(..)) => Ok(()), + Ok(None) => Err(anyhow!("no active auxflash slot")), + Err(e) => Err(anyhow!("failed to check auxflash slot: {e}")), + }; + core.halt()?; + r + } else { + Ok(()) + }; + + // We don't actually read back the auxflash, because that + // would be slow; if Hubris has booted and has an active + // auxflash slot, then it must match (because we know that + // the SP code is correct, and the SP code contains a + // checksum of the auxflash data). + r.context( + "image ID and flash contents match, but auxflash is not loaded", + ) + } else { + Ok(()) + } } fn flashcmd(context: &mut ExecutionContext) -> Result<()> { @@ -322,7 +365,6 @@ fn flashcmd(context: &mut ExecutionContext) -> Result<()> { let core = c.as_mut(); validate(hubris, core, &subargs)?; - if subargs.check { return Ok(()); } From 7d23bcb1532cd050ed77537d531b42f7b8f30a86 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 27 Jul 2023 16:42:28 -0400 Subject: [PATCH 2/7] Update README --- README.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 5c3e378ed..dc580d905 100644 --- a/README.md +++ b/README.md @@ -598,12 +598,12 @@ to prevent accidental blasts of binary content to the console.) Flashes the target with the image that is contained within the specified archive (or dump). As a precautionary measure, if the specified archive already appears to be on the target, `humility flash` will fail unless the -`-F` (`--force`) flag is set. Because this will only check the image -ID (and not the entire image), `humility flash` can be optionally told -to verify that all of the program text in the image is on the device -by specifying `-V` (`--verify`). Similarly, if one wishes to *only* -check the image against the archive (and not flash at all), specify -`-C` (`--check`). +`-F` (`--force`) flag is set. This will only check the image ID; it does +not check the entire image or auxiliary flash. `humility flash` can be +optionally told to verify all of the program text **and** the auxiliary +flash by specifying `-V` (`--verify`). Similarly, if one wishes to *only* +check the image against the archive (and not flash at all), specify `-C` +(`--check`). This attempts to natively flash the part within Humility using probe-rs, but for some parts or configurations, it may need to use OpenOCD as a From 7143966996e9c03ffda3b3cd3f77535b2fe2a5a9 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 27 Jul 2023 16:44:36 -0400 Subject: [PATCH 3/7] Small fixes --- Cargo.lock | 2 +- Cargo.toml | 2 +- cmd/flash/src/lib.rs | 5 ++--- tests/cmd/chip.trycmd | 4 ++-- tests/cmd/version.trycmd | 4 ++-- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1f365aed5..0f701b116 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1080,7 +1080,7 @@ dependencies = [ [[package]] name = "humility" -version = "0.10.25" +version = "0.10.26" dependencies = [ "anyhow", "bitfield", diff --git a/Cargo.toml b/Cargo.toml index 51dddbcbb..5f0e8dd9a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,7 @@ name = "humility" # # Be sure to check in and push all of the files that change. Happy versioning! # -version = "0.10.25" +version = "0.10.26" authors = ["Bryan Cantrill "] edition = "2018" license = "MPL-2.0" diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index 6e7bf2546..fe11061e3 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -280,9 +280,8 @@ fn get_image_state( // More rigorous checks if requested if full_check { - hubris.verify(core).with_context( - format!("image IDs match, but flash contents do not match \ - archive contents: {err}"), + hubris.verify(core).context( + "image IDs match, but flash contents do not match archive contents" )?; let r = if hubris.read_auxflash_data()?.is_some() { diff --git a/tests/cmd/chip.trycmd b/tests/cmd/chip.trycmd index 034da31d9..bb5d085de 100644 --- a/tests/cmd/chip.trycmd +++ b/tests/cmd/chip.trycmd @@ -13,7 +13,7 @@ For more information try --help ``` $ humility --chip this-can-be-anything -V -humility 0.10.25 +humility 0.10.26 ``` @@ -28,7 +28,7 @@ For more information try --help ``` $ humility -c apx432 -V -humility 0.10.25 +humility 0.10.26 ``` diff --git a/tests/cmd/version.trycmd b/tests/cmd/version.trycmd index 0a6edd0f4..0c5779a14 100644 --- a/tests/cmd/version.trycmd +++ b/tests/cmd/version.trycmd @@ -2,7 +2,7 @@ Long version flag: ``` $ humility --version -humility 0.10.25 +humility 0.10.26 ``` @@ -10,6 +10,6 @@ Short version flag: ``` $ humility -V -humility 0.10.25 +humility 0.10.26 ``` From a12a764927813554e6b476116a770ab1c0f1817a Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 27 Jul 2023 16:49:41 -0400 Subject: [PATCH 4/7] Add logs --- cmd/flash/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index fe11061e3..2ea9aaf90 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -300,7 +300,10 @@ fn get_image_state( } }; let r = match worker.active_slot() { - Ok(Some(..)) => Ok(()), + Ok(Some(s)) => { + humility::msg!("verified auxflash in slot {s}"); + Ok(()) + }, Ok(None) => Err(anyhow!("no active auxflash slot")), Err(e) => Err(anyhow!("failed to check auxflash slot: {e}")), }; From 285e2fc9d1622a3a06c489c45635008514e2ca5e Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 27 Jul 2023 16:58:34 -0400 Subject: [PATCH 5/7] Always check auxflash --- cmd/flash/src/lib.rs | 59 ++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index 2ea9aaf90..120615c5b 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -281,46 +281,45 @@ fn get_image_state( // More rigorous checks if requested if full_check { hubris.verify(core).context( - "image IDs match, but flash contents do not match archive contents" + "image IDs match, but flash contents do not match archive contents", )?; + } - let r = if hubris.read_auxflash_data()?.is_some() { - // The core must be running for us to check the auxflash slot. - // - // However, we want it halted before we exit this function, which - // requires careful handling. - core.run()?; - let mut worker = match cmd_auxflash::AuxFlashHandler::new( - hubris, core, 15_000, - ) { + if hubris.read_auxflash_data()?.is_some() { + // The core must be running for us to check the auxflash slot. + // + // However, we want it halted before we exit this function, which + // requires careful handling of functions that could bail out. + core.run()?; + + // Note that we only run this check if we pass the image ID check; + // otherwise, the Idol / hiffy memory maps are unknown. + let mut worker = + match cmd_auxflash::AuxFlashHandler::new(hubris, core, 15_000) { Ok(w) => w, Err(e) => { + // Halt the core before returning! core.halt()?; return Err(e); } }; - let r = match worker.active_slot() { - Ok(Some(s)) => { - humility::msg!("verified auxflash in slot {s}"); - Ok(()) - }, - Ok(None) => Err(anyhow!("no active auxflash slot")), - Err(e) => Err(anyhow!("failed to check auxflash slot: {e}")), - }; - core.halt()?; - r - } else { - Ok(()) + let r = match worker.active_slot() { + Ok(Some(s)) => { + humility::msg!("verified auxflash in slot {s}"); + Ok(()) + } + Ok(None) => Err(anyhow!("no active auxflash slot")), + Err(e) => Err(anyhow!("failed to check auxflash slot: {e}")), }; - // We don't actually read back the auxflash, because that - // would be slow; if Hubris has booted and has an active - // auxflash slot, then it must match (because we know that - // the SP code is correct, and the SP code contains a - // checksum of the auxflash data). - r.context( - "image ID and flash contents match, but auxflash is not loaded", - ) + // Halt the core before returning results + core.halt()?; + + r.context(if full_check { + "image ID and flash contents match, but auxflash is not loaded" + } else { + "image IDs match, but auxflash is not loaded" + }) } else { Ok(()) } From 150c581a5f48d676aad4a9c6781545d3155bef65 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 27 Jul 2023 17:05:02 -0400 Subject: [PATCH 6/7] comments --- cmd/flash/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index 120615c5b..4c4c0b8c4 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -222,8 +222,8 @@ fn force_openocd( /// /// If `subargs.check` is false, returns `Ok(())` if the check _fails_ (meaning /// we should reflash), and `Err(..)` if all checks pass (meaning we should -/// _not_ reflash). The core is left halted if we should reflash and is left -/// running otherwise. +/// _not_ reflash). The core is left halted if we should reflash and is running +/// otherwise. fn validate( hubris: &mut HubrisArchive, core: &mut dyn humility::core::Core, From 92403e5a58dd6db642e1b36b5b455b2caabff133 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 27 Jul 2023 17:13:21 -0400 Subject: [PATCH 7/7] Revert README, because auxflash is now checked in both cases --- README.md | 12 ++++++------ cmd/flash/src/lib.rs | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index dc580d905..5c3e378ed 100644 --- a/README.md +++ b/README.md @@ -598,12 +598,12 @@ to prevent accidental blasts of binary content to the console.) Flashes the target with the image that is contained within the specified archive (or dump). As a precautionary measure, if the specified archive already appears to be on the target, `humility flash` will fail unless the -`-F` (`--force`) flag is set. This will only check the image ID; it does -not check the entire image or auxiliary flash. `humility flash` can be -optionally told to verify all of the program text **and** the auxiliary -flash by specifying `-V` (`--verify`). Similarly, if one wishes to *only* -check the image against the archive (and not flash at all), specify `-C` -(`--check`). +`-F` (`--force`) flag is set. Because this will only check the image +ID (and not the entire image), `humility flash` can be optionally told +to verify that all of the program text in the image is on the device +by specifying `-V` (`--verify`). Similarly, if one wishes to *only* +check the image against the archive (and not flash at all), specify +`-C` (`--check`). This attempts to natively flash the part within Humility using probe-rs, but for some parts or configurations, it may need to use OpenOCD as a diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index 4c4c0b8c4..1cb133d79 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -7,12 +7,12 @@ //! Flashes the target with the image that is contained within the specified //! archive (or dump). As a precautionary measure, if the specified archive //! already appears to be on the target, `humility flash` will fail unless the -//! `-F` (`--force`) flag is set. This will only check the image ID; it does -//! not check the entire image or auxiliary flash. `humility flash` can be -//! optionally told to verify all of the program text **and** the auxiliary -//! flash by specifying `-V` (`--verify`). Similarly, if one wishes to *only* -//! check the image against the archive (and not flash at all), specify `-C` -//! (`--check`). +//! `-F` (`--force`) flag is set. Because this will only check the image +//! ID (and not the entire image), `humility flash` can be optionally told +//! to verify that all of the program text in the image is on the device +//! by specifying `-V` (`--verify`). Similarly, if one wishes to *only* +//! check the image against the archive (and not flash at all), specify +//! `-C` (`--check`). //! //! This attempts to natively flash the part within Humility using probe-rs, //! but for some parts or configurations, it may need to use OpenOCD as a