flash.sh: UX and logic fixes #1230
Conversation
|
Tested on Talos, works. @JonathonHall-Purism for obscure reason, 79a8146#diff-e33f7a1603ea94b6a0ff385404e9860ff48e1c5afe2ed5c8e68207d3b39186ccR36 loop was broken (break) before verification step to succeed. When adding debugging traces after the while true loop exiting too early, So to test for regression, my next step is to test on x230 on top of master (this PR) but as said the bad behavior seem to happen on top of newer flashrom, which is to be tested against https://app.circleci.com/pipelines/github/tlaurion/heads/1223/workflows/d6e6e2e8-9425-4ac8-bb16-489f08608bd6 produced roms Short:
|
If you have better implementation suggestion ( now the main loop doesn't break: the individual parts are responsible for breaking the loop if the state changes or fail. Previously, when no more log were processed, the main loop would break causing the issue). Otherwise, As said in previous post, regression has been done for this PR and this PR on top of #1222 I would suggest testing Librem boards produced from https://app.circleci.com/pipelines/github/tlaurion/heads/1223/workflows/d6e6e2e8-9425-4ac8-bb16-489f08608bd6/jobs/11148 so you can test flashrom version bump + flash.sh modifications in one shot, unless you have a better imppementation proposal for this PR. Note: this PR removes flashrom backup of 3 reads, and replaces it with one flashrom -r and one flashrom -v (to verify backuped content against SPI prior of injecting stuff and flashing it back. Newer version of flashrom verifies automatically at flash). Your call on keeping my code suggestion to verify rom content, or go more radical into removing flashrom -v completely and just using the backup content to persist content (cbfs injection) prior of flashing back at firmware upgrades, key injection etc. |
|
@JonathonHall-Purism further tests shows that a loooooooot of time is wasted into calculating and drawing a progress bar through bashisms, while latest flashrom actually draws a progress natively with Putting this PR as draft. the current implementation under flash.sh is not fit for larger flash chips as can be seen below in user time comparison, time spent doing irrelevant work (Talos II 64mb flash chip here). Standard without flash.sh progress bar, calling flashrom directly and dismissing any output on serial console: With actual flash.sh from this PR (still doing unneeded verification on flash chip content): |
|
Solution is to have native progress bar of latest flashrom, which means having ast1100 (kgpe-d16 bmc flashing support) that is still a patch under heads against used version of flashrom. This will should lan under https://github.com/Dasharo/flashrom, which should be used anyway for wp wp-status and other improvements from parallel work |
|
Hey @tlaurion, I think these are good directions. I agree we should move toward the native flashrom progress rather than the bashisms, I think this is also the source of very long flash times on Librem Server (several minutes for 16 MB flash IIRC). However, the progress indicator in the latest flashrom is relatively crude, it just dumps percentage updates to one long line. At least a line break (and maybe reducing frequency to 5% so it doesn't wipe out the whole terminal) would be nice, actually resetting the position in the terminal would be even better. I'd be happy to look into that in flashrom eventually, but right now my to-do list is growing faster than I can keep up with it. If you don't want to get into flashrom, maybe a middle ground would be to drop the 'spinner' from the bashism progress bar and use the 'progress' interface to feed it? That might be a bit more stable, and it only prints every 1%; it doesn't scale up with flash chip size. I'll test this on Librem boards to verify the flashrom update this week too. Re: additional improvements - the only suggestion I have would be to potentially just flash modified regions when changing settings - i.e. just flash the CBFS region when that's all that's needed, to avoid reading and verifying the rest of flash. Or even better, to move config, GPG keyring, etc., to their own region, so we could separate them from the payload too. There's no need to tie that to this work though. |
|
@JonathonHall-Purism well... I took dasharo/flashrom and started trying to tackle with the flash.sh script to deal with only refresh on the same line some kind of progress, to realise that there is an issue upstream on master, stating that on erase/write operations, the output goes backward (so writing progress jumps from 100% to 0%)....
That deserves separate issues and definitely are awesome ideas to debate prior of implementing. I see some issues there though, since some firmware upgrades might include new stuff in cbfs and bootblock might change as well. I do not see a problem, really, into the current process when upgrading while keeping persistence:
The same process above is used when deciding to wipe GPG public and keyring+trustdb+config.user injected back (with other type 50 cbfs files in current rom), where above operations only change between the source of the rom being a backup of current prior of copying it over for change and flashed back. I understand on my side (and for developers) that it is a burden when devlopping, but that is another subject of interest which should be linked with having pflash/mtd/qemu/kvm internal flashing support, which would definitely ease testing those sorta thing! |
tlaurion
force-pushed
the
flash_sh-fixes
branch
5 times, most recently
from
f79100b
to
9160149
Compare
|
@JonathonHall-Purism : modified OP. Flashing backup after having injected key: Note: the output of Flashing stays to 100%, which seems to be the result of https://ticket.coreboot.org/issues/390 Redoing manually: Then reflashing with clean just for good measure: Is that satisfying? As current state, when flashing so little content, there is a pause of 5 seconds between: It might be worrying to some, unfortunately. Otherwise, the script does what is expected and the progress of reading and verifying is actually an upgrade as currently under master. Let me know what you think, and if you think we should wait for https://ticket.coreboot.org/issues/390 to be fixed. |
tlaurion
force-pushed
the
flash_sh-fixes
branch
from
9160149
to
76d7ee5
Compare
|
With 76d7ee5: But the flashing operations (which encompasses ERASE and WRITE --progress output reports) is bogus. So as long as master doesn't change its Will propose bug fix, single read backup in this PR, and we can refer to next round of fixes by comparing this PR to the fixes proposed under 76d7ee5 at a later time. |
tlaurion
force-pushed
the
flash_sh-fixes
branch
from
76d7ee5
to
642338e
Compare
|
@JonathonHall-Purism : The working fix for this is under 642338e above: Will modify OP accordingly, leaving a trace pointing to 76d7ee5 for later work (after flashrom fixes its --progress reporting) As of today, on Talos II (after having called gpg-gui.sh to prepare firmware image with new gpg keyring + trustdb injected in rom to be flashed): We can see that processing the output of flashrom -V takes 43 seconds for 64mb spi chip in with mtd programmer, which relates to your librem server use case. I think this is best solution as of today. @JonathonHall-Purism Please review! |
@JonathonHall-Purism ready for review, should not cause any regression. As you said, we could tweak this further, but as noted under #1222 (comment), percentage jams on boards with more SPI, and the current progress bar changes is the only hint to the user that something is still happening while those ERASE+WRITE are happening. My call on this would be to keep those changes seperate, and version bump flashrom when flashrom fixes the behavior upstream prior of touching that code, revisiting 76d7ee5 later on. |
|
The slowdown is larger than user time (1m 41s vs. 26s): Either With: diff --git a/initrd/bin/flash.sh b/initrd/bin/flash.sh
index 66cce75..35812f6 100755
--- a/initrd/bin/flash.sh
+++ b/initrd/bin/flash.sh
@@ -27,6 +27,8 @@ flashrom_progress() {
local status='init'
local prev_word=''
local prev_prev_word=''
+ local spaces=' '
+ local hashes='##################################################'
progressbar2=$(for i in `seq 48` ; do echo -ne ' ' ; done)
echo -e "\nInitializing internal Flash Programmer"
@@ -35,13 +37,13 @@ flashrom_progress() {
prev_word=$IN
read -r -d' ' IN
if [ "$total_bytes" != "0" ]; then
- current=$(echo "$IN" | grep -E -o '0x[0-9a-f]+-0x[0-9a-f]+:.*' | grep -E -o "0x[0-9a-f]+" | tail -n 1)
+ current=$(echo "$IN" | sed -nE 's/.*(0x[0-9a-f]+).*/\1/p')
if [ "${current}" != "" ]; then
percent=$((100 * (current + 1) / total_bytes))
pct1=$((percent / 2))
- pct2=$((49 - percent / 2))
- progressbar=$(for i in `seq $pct1 2>/dev/null` ; do echo -ne '#' ; done)
- progressbar2=$(for i in `seq $pct2 2>/dev/null` ; do echo -ne ' ' ; done)
+ pct2=$((50 - percent / 2))
+ progressbar=${hashes:0:$pct1}
+ progressbar2=${spaces:0:$pct2}
fi
else
if [ "$prev_prev_word" == "Reading" ] && [ "$IN" == "bytes" ]; thenThings somewhat improve: By the way, the script prints "Initializing internal Flash Programmer", this might be worth changing. |
- Have Talos II supported by detecting correctly size of mtd chip (not internal: different flashrom output needs to be parsed for chip size) - Read SPI content only once: 66% speedup (TOCTOU? Don't think so, nothing should happen in parallel when flashing insingle user mode) - Have the main flash_progress loop not break, but break in flash_rom state subcases (otherwise, verifying step was breaking) - Change "Initializing internal Flash Programmer" -> "Initializing Flash Programmer" - Apply changes suggested by @SergiiDmytruk under linuxboot#1230 (comment) to reduce userland wasted time processing flashrom -V output
tlaurion
force-pushed
the
flash_sh-fixes
branch
from
642338e
to
5a7902c
Compare
- Have Talos II supported by detecting correctly size of mtd chip (not internal: different flashrom output needs to be parsed for chip size) - Read SPI content only once: 66% speedup (TOCTOU? Don't think so, nothing should happen in parallel when flashing insingle user mode) - Have the main flash_progress loop not break, but break in flash_rom state subcases (otherwise, verifying step was breaking) - Change "Initializing internal Flash Programmer" -> "Initializing Flash Programmer" - Apply changes suggested by @SergiiDmytruk under linuxboot#1230 (comment) to reduce userland wasted time processing flashrom -V output
|
Thanks a lot @SergiiDmytruk for the suggestions. That is sufficient performance enhancement until flashrom produces useful This PR updated with your suggestions, while building on top of #1222 as said under #1222 (comment). |
|
@JonathonHall-Purism Outside of getting weird time calculations that I do not comprehend as of now: This is massive improvement and fits the goal. Please review/approve when you have a minute in link with OP updated changes and commit message: #1230 (comment) Future work from your notes above should be opened in different issues:
|
|
Comparison before above proposed fix by @SergiiDmytruk : |
|
I think this looks great! Thanks @tlaurion, I think we have a lot of good future ideas above but this is a great improvement on its own. Let's ship it 💯 |
Next steps:
flashrom --progressreporting under https://ticket.coreboot.org/issues/390)EDIT:
flashrom --progressimplementation under comment flash.sh: UX and logic fixes #1230 (comment). We cannot useflashrom --progresstoday: that would leave the user in uncertainty under flashing operation (12 seconds with flashing status staying at 100% is really worrying UX).