Cache bootloader firmware writes - #2002
Conversation
Erase the full firmware area on OP_ERASE, then cache sequential 4 KiB firmware chunks into 8 KiB flash blocks. Flush cached blocks once, using flash_append() when the target block is already erased and flash_write() otherwise. Also reject signature data before all declared firmware chunks have been received and flushed. Verification: - ./scripts/dev_exec.sh ./scripts/format - ./scripts/dev_exec.sh make bootloader - ./scripts/dev_exec.sh make unit-test - ./scripts/dev_exec.sh make run-unit-tests
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
Seems overly complicated, I think the cache/flush is not needed?
Why not just erase in OP_ERASE, and just use flash_append with 4kB chunks during flashing?
Also reject signature data before all declared firmware chunks have been received and flushed.
Why? One can't successfully do it anyway beforehand, as that API call rejects on invalid signatures. Can't hurt but seems not needed.
adam2k
left a comment
adam2k
left a comment
There was a problem hiding this comment.
I'm new here and disclaimer up front I do not have a dev device, so I am doing static analysis only.
@benma asked whether the cache is needed. I believe it is not, and the HAL explains the speedup: flash_write -> _flash_write (hpl_nvmctrl.c:158) is read-modify-write per 8 kB block, so two 4 kB chunks cost two erase-and-reprogram cycles. flash_append -> _flash_append (:204) programs page by page with no erase. Chunks sit at FLASH_APP_START + n*4096 with FLASH_APP_START == 0x10000, so they're always page-aligned. After the upfront erase, plain flash_append per chunk should be enough.
Two branches in the current shape already look dead as a result. I've noted those inline.
| return OP_STATUS_OK; | ||
| } | ||
|
|
||
| if (_firmware_app_erased || _is_erased((const void*)addr, FIRMWARE_BLOCK_LEN)) { |
There was a problem hiding this comment.
This branch looks unreachable. _firmware_app_erased is cleared only at 588, immediately after _loading_ready = false (586), and set true at 604 before _loading_ready = true (608). _api_write_chunk requires _loading_ready on entry and only re-arms it on success, so _loading_ready implies _firmware_app_erased. Both the _is_erased() call and the flash_write branch below are dead.
| } | ||
|
|
||
| const uint32_t addr = FLASH_APP_START + _firmware_cached_block * FIRMWARE_BLOCK_LEN; | ||
| if (MEMEQ((const void*)addr, _firmware_block_cache, FIRMWARE_BLOCK_LEN)) { |
There was a problem hiding this comment.
Near-dead: post-erase the block is 0xFF and each block is written once, so this only hits on an all-0xFF chunk pair, at the cost of an 8 kB compare per block. The old per-chunk MEMEQ paid for itself because flash wasn't pre-erased.
| if (chunknum > _firmware_num_chunks - 1 || chunknum > FIRMWARE_MAX_NUM_CHUNKS - 1) { | ||
| // The second is redundant, as _firmware_num_chunks <= FIRMWARE_MAX_NUM_CHUNKS. | ||
| if (chunknum > _firmware_num_chunks - 1 || chunknum > FIRMWARE_MAX_NUM_CHUNKS - 1 || | ||
| chunknum != _firmware_next_chunk) { |
There was a problem hiding this comment.
This makes out-of-order writes a hard error, tightening the host protocol permanently in the bootloader. bootloader.py:191 is sequential so the bundled client is fine, but worth confirming BitBoxApp is too, and that it's intended as a contract change.
| if (!MEMEQ((const void*)addr, empty_page, sizeof(empty_page))) { | ||
| return _report_status(OP_STATUS_ERR_CHECK, output); | ||
|
|
||
| for (uint32_t addr = FLASH_APP_START; addr < FLASH_APP_START + FLASH_APP_LEN; |
There was a problem hiding this comment.
This erases 864 kB (108 blocks) in one blocking call, where previously a full-size firmware erased almost nothing here. Any host-side timeout on OP_ERASE? _render_progress(0) is also drawn before the loop, so the screen sits at 0% throughout. Disclaimer: I do not have a dev device, so I can't measure it.
| bootloader_render_default_screen(); | ||
| } else { | ||
| _render_progress((float)chunk_num / (float)(_firmware_num_chunks - 1)); | ||
| float progress = 1; |
There was a problem hiding this comment.
Real fix rather than cleanup: at _firmware_num_chunks == 1 the old expression is 0/0 = NaN. Unrelated to the caching change, so maybe its own commit.
+1 for the above comment. |
3 participants
Approximately halfs the current flashing time to 55s.
Erase the full firmware area on OP_ERASE, then cache sequential 4 KiB firmware chunks into 8 KiB flash blocks. Flush cached blocks once, using flash_append() when the target block is already erased and flash_write() otherwise.
Also reject signature data before all declared firmware chunks have been received and flushed.
Verification: