The patch adds command line option to handle the following situation:
There is a region which is requested to be erased (or written, because
the write operation uses erase too). Some of the areas inside this
region don't need to be erased, because the bytes already have expected
value. Such areas can be skipped.
The logic selects eraseblocks that can cover the areas which need to be
erased. Suppose there is a region which is partially covered by
eraseblocks of size S (partially because remaining areas don't need to
be erased). Now suppose we can cover the whole region with eraseblock
of larger size, S+1, and erase it all at once. This will run faster:
erase opcode will only be sent once instead of many smaller opcodes.
However, this will run erase over some areas of the chip memory that
didn't need to be erased. Which means, the chip, as a hardware, will
wear faster.
New command line option sets the maximum % memory that is allowed for
redundant erase. Default is 0, S+1 size block only selected if all the
area needs to be erased in full. 50 means that if more than a half of
the area needs to be erased, a S+1 size block can be selected to cover
all area with one block.
The tradeoff is the speed of programming operation VS the longevity of
the chip. Default is longevity.
Change-Id: I154e8a713f626c37dbbe118db700055b96d24803
Co-developed-by: persmule <persmule@hardenedlinux.org
Co-developed-by: Anastasia Klimchuk <aklm@flashrom.org>
Signed-off-by: persmule <persmule@hardenedlinux.org>
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84721
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
Original progress reporting implemented in CB:49643 and it has some
issues, for example:
size_t start_address = start;
size_t end_address = len - start;
End address is anything but length minus start address.
update_progress(flash,
FLASHROM_PROGRESS_READ,
/*current*/ start - start_address + to_read,
/*total*/ end_address);
Total should just be length if that's how current value is computed.
---
libflashrom needs to know total size ahead of time.
That's init_progress() and changed update_progress().
It also needs to store the last current value to be able to update it.
That's stage_progress in flashrom_flashctx.
Measuring accurately amount of data which will be read/erased/written
isn't easy because things can be skipped as optimizations. The next
patch in the chain aims to address this, there are TODO/FIXME
comments there.
---
CLI shares terminal with the rest of the code and has to maintain more
state to handle that reasonably well.
Similar to CB:64668, an effort is made to keep the progress on a
single line. Non-progress output is kept track of to know when
moving to a new line cannot be avoided.
---
A script to test the CLI:
\#!/bin/bash
t=${1:-rewW}
shift
if [[ $t =~ r ]]; then
echo ">>> READ"
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -r dump.rom --progress "$@"
echo
fi
if [[ $t =~ e ]]; then
echo ">>> ERASE"
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -E --progress "$@"
echo
fi
if [[ $t =~ w ]]; then
echo ">>> WRITE (without erase)"
dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -w zero.rom --progress "$@"
echo
fi
if [[ $t =~ W ]]; then
echo ">>> WRITE (with erase)"
dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null
dd if=/dev/random of=random.rom bs=1M count=16 2> /dev/null
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz,image=random.rom -w zero.rom --progress "$@"
echo
fi
Co-developed-by: Anastasia Klimchuk <aklm@flashrom.org>
Co-developed-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84102
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
Previously the logic which selected large block did deselect of
smaller blocks, but only one level below. So some even smaller blocks
could still remain selected, and this would result in duplicate erase.
This patch deselects all smaller blocks of all levels below, down to
the smallest size. If the area is covered by one large block, no
other smaller blocks inside are needed.
Change-Id: Icfc18d5c090b1dcb92ab157e2c139be71af59300
Spotted-by: persmule <persmule@hardenedlinux.org>
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Co-authored-by: persmule <persmule@hardenedlinux.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84686
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
A larger (not the smallest) erase block used to get erased when half
of sub-blocks it contains need erase, which has at least 2 issues:
1. The rest half of sub-blocks that do not need erase are also erased,
introducing some erase overheads.
2. More severely, since this logic only selects a block and delects
its sub-blocks when half of sub-blocks need erase, but this logic
does not deselect "nested sub-blocks (sub-blocks of sub-block)" not
reach the limit under this block, the logic may cause duplicated
erase. For example, if a erase block (often the largest one
corresponding to the whole chip) has half of its sub-blocks and
some incontiguous nested sub-blocks needing erase, these double
sub-blocks will end up being erased twice, introducing even more
erase overheads than whole-chip erase.
The older behavior of flashrom before adding erasure_layout.c, when no
communicational error occurs, will neither erase blocks that do not
need erase, nor cause duplicated erase. Higher efficiency should be
achieved without introducing extra erase overheads, by allowing
combining contiguous small erase blocks only when they can
coincidently form a larger erase block.
Change-Id: I9e10749186e395da67ec80e296119f33c3f83122
Signed-off-by: persmule <persmule@hardenedlinux.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84614
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
Fix a segmentation fault that is caused by accessing an invalid
"subedata" pointer on the last iteration of the init_eraseblock loop.
Instead, short circuit the loop condition to check the sub block index
first, and do not access the invalid pointer if it is the last sub
block.
Issue was encountered in:
- OS: OpenBSD 7.5 amd64
- Compiler: clang 16.0.6
- Chip: Macronix MX25U6435E/F
BUG=https://ticket.coreboot.org/issues/555
Change-Id: I61bf0d93aa9f0b2b420b146be16fcd5124f0dc5d
Signed-off-by: Grant Pannell <grant@digitaldj.net>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84234
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: DigitalDJ
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
The patch adds new functionality to the test: tracking the areas of
chip memory that were modified (i.e. by erase or write operation),
and then checking those areas were completely covered by verify
operation.
The test operates over the mock chip memory of 16 bytes, so it is
possible to track each byte which was modified, and assert that is
has been verified afterwards.
Adding the test found a bug which is fixed in this commit:
Post-cleanup after processing unaligned region for the case when end
region needs to be extended to align with erase block. Writing was
done correctly, but post-processing of newcontents could cause
one-off offset at the end of the region, which would make
verification appear false-negative (see test cases #16-19).
Change-Id: I3c5d55a0deb20f23f4072caac8c0dce04cc98fd4
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/84078
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
Current code path for erase and write has been enabled in the tree
since May 2023, which is more than 1 year ago (15 months ago),
and legacy path has been disabled since the same time.
Current logic has been officially released in v1.4.0 in July 2024.
Change-Id: I08fd686fecf6a5313eea2d66b368661c664f4800
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/83846
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Aarya <aarya.chaumal@gmail.com>
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
When flash regions are protected, erase could incorrectly erase regions
which were meant to be protected by requesting the correct size but
using an erase opcode with coarser granularity than desired (for
instance using a 16-byte erase command while attempting to erase only 8
bytes).
This fixes that by exchanging the nesting of the loops over erase blocks
and flash regions.
Old:
- Select erasefns
- Loop over blocks to erase for each selected erasefn
- Loop over programmer flash regions within erase block
- Erase regions (may fail since selected erasefn will be
too big if flash region is smaller than erase block)
New:
- Loop over programmer flash regions within erase block
- Select erasefns within programer flash region
- Loop over blocks to erase for each selected erasefn
- Erase regions
Eraser selection and erasing has also been factored out into a helper
function to manage nesting depth.
TEST=New test cases pass, whereas some of them fail without the changes
to erasure_layout.c
BUG=https://ticket.coreboot.org/issues/525
Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9
Co-authored-by: Nikolai Artemiev <nartemiev@google.com>
Co-authored-by: Anastasia Klimchuk <aklm@flashrom.org>
Signed-off-by: Nikolai Artemiev <nartemiev@google.com>
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/82393
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
get_flash_region() emits a struct flash_region, which uses chipoff_t for
the start and end addresses of a region. chipoff_t is defined as a valid
flash address, so it was wrong to be setting the end address to start +
len; this is clearly wrong in the case where there is a single region
because setting end to the flash size generates an address that is
beyond the end of the chip (due to zero-indexing).
This changes the one actual implementation of .get_region in ichspi.c to
use inclusive upper bounds, and corrects all callers of
get_flash_region() to treat the upper bounds as inclusive. Overall this
reduces complexity slightly by removing more downward adjustments by 1
than it needs to add upward adjustments.
TEST=on yaviks, `flashrom -V -x` prints equivalent messages about
"x region (0xZZZZ..0xZZZZ) is readable" before and after this
patch.
Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec
Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/82496
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Hsuan-ting Chen <roccochen@google.com>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
memcpy() function expects 2nd parameter to be non-null. Make sure that
the pointer is non-null before passing it to the function.
Also move allocations under if conditions to avoid allocating memory for
a potentially 0 size.
Found-by: scan-build, clang v17.0.6
Signed-off-by: Alexander Goncharov <chat@joursoir.net>
Change-Id: I99aad4119f9c11bea75e3419926cc833bc1f68c5
Reviewed-on: https://review.coreboot.org/c/flashrom/+/81548
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
In the case when layout region is not aligned with eraseblock,
end region boundary is extended to match the eraseblock. There is
a special handling of this extended area (between original end of
region and extended one). Fix the offset of this extended area by +1
so that it covers the extended area and not the original region.
Before the patch, the last byte of the original region was failed to
write since it was treated as if an extended area, while it was the
last byte of the normal layout region.
Ticket: https://ticket.coreboot.org/issues/494
Change-Id: I7f78a0090065cd2a952cba1a5d28179483ba4c55
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/78984
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
Previously, in the worst-case scenario of erasing region content then
writing new data, three rounds of verification were performed inside of
the `erase_write` function through calls to:
1. `check_erased_range` when erasing with respect to region boundaries
2. `check_erased_range` for the entire erase block after the loop
containing verification 1 completed
3. `verify_range` when all erases/writes were complete
Verification 2 duplicated verification 1 and was orphaned by commit
fa8808595a, which dropped its paired erasefn call but missed this
related step.
Verification 3 duplicated verification which occurs in
`flashrom_image_write` based on `flashctx flags`.
Now, these 2 redundant verifications are removed to improve the
performance of `erase_write`.
This change was tested using the linux_spi programmer to erase and write
to an MT25QL512 chip.
Change-Id: I638835facd9311979c4991cc4ca41a4b9e174bd5
Signed-off-by: Carly Zlabek <carlyzlabek@gmail.com>
Signed-off-by: Vincent Fazio <vfazio@gmail.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/79354
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
Erasefn was invoked over the same block of memory twice.
This patch removes the second redundant invokation. It was
accidentally introduced during earlier refactoring of the code.
Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/77747
Reviewed-by: Alexander Goncharov <chat@joursoir.net>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
Header files can be found without explicitly specifying include
directory on the file path.
Change-Id: I6ef34273d088d8fb4559fe3d801d1247ad9b3fa2
Signed-off-by: Anastasia Klimchuk <aklm@chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/75322
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Aarya <aarya.chaumal@gmail.com>
On failure of erasefn in erase_write it didn't set the error value in
ret which caused send success status as return value.
Change-Id: Ia3bd5fd250dcd0a03f0281c478b9bacb71872f31
Signed-off-by: Aarya Chaumal <aarya.chaumal@gmail.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/74882
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
DJGPP for compiling DOS has other sizes for the normal int types and
therefore throwing errors when using %i %d or %x with uint32_t.
Fix these warnings by using the macros created for it and provided in
inttypes.h.
Change-Id: Ia75b6df981ce60c891161fe553c7ceab8570178d
Signed-off-by: Thomas Heijligen <thomas.heijligen@secunet.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/73040
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
In erasure_layout.c:create_erase_layout() the layout will be allocated
based on erasefn_count, But calling calloc with 0 is unspecified
behavior. Also it is not freed when erasefn_count is 0.
So test first if erasefn_count is 0, and only when not allocate the
memory for *layout.
Reported by Coverty Scan:
*** CID 1505171: Resource leaks (RESOURCE_LEAK)
/erasure_layout.c: 105 in create_erase_layout()
98 if(!layout) {
99 msg_gerr("Out of memory!\n");
100 return -1;
101 }
102
103 if (!erasefn_count) {
104 msg_gerr("No erase functions supported\n");
>>> CID 1505171: Resource leaks (RESOURCE_LEAK)
>>> Variable "layout" going out of scope leaks the storage it points to.
105 return 0;
106 }
Change-Id: If13b050ac8525fee44d3f3bf74a9c9b6a8d38399
Signed-off-by: Thomas Heijligen <thomas.heijligen@secunet.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/73041
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
1) Add function to flatten out the addresses of the flash chip as per
the different erase functions. This function will return a list of
layouts which is dynamically allocated. So after use all the layouts as
well as the list itself should be freed. The free_erase_layout function
does that.
2) Add function to align start and end address of the region (in struct
walk_info) to some erase sector boundaries and modify the region start
and end addresses to match nearest erase sector boundaries. This
function will be used in the new algorithm for erase function selection.
3) Add function that returns a list of sectors (as seen by the first
erase function) that need erasing.
4) Add a function to call the erase algorithm.
Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256
Signed-off-by: Aarya Chaumal <aarya.chaumal@gmail.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/65844
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
Reviewed-by: Thomas Heijligen <src@posteo.de>
Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>
