On 16.03.2017 00:08, David Hendricks wrote:
On Fri, Mar 10, 2017 at 8:02 AM, Nico Huber nico.huber@secunet.com wrote:
Hi,
I've just updated my solution to _the_ layout problem that I wrote last year [1]. I'm not asking for a review at this moment. There are at least two competing approaches that I want to discuss first (I couldn't start a discussion before I wrote it, due to time constraints).
We can
walk over the given layout regions in an outer loop and walk over the erase blocks in each region in an inner loop
This is how my patch tackles it.
or
- walk over all erase blocks in an outer loop and walk over the touched layout regions in an inner loop
Thoughts?
Both sounds easy to do but it gets really complicated when you account for a) regions that are not erase block aligned, b) our fallback to dif- ferent erase functions (and blocks thereby) and c) read-locks that might render a) impossible.
I think the second approach results in an extra layer of logic that really isn't needed. With the first, we:
- We find what needs to change
- Select the appropriate block eraser
Until now, I kept flashrom's fallback mechanism: it just tries all erase functions until one succeeds. I think that's very questionable though. So far I've only seen it triggered in two scenarios only:
1. Wrong flash chip selected (i.e. wrong opcode or wrong block size). => Might be a bug in flashrom that gets never reported. 2. Unreliable reads that made it look like the erase didn't work. => Fallback made things even worse, resulting in a full chip erase.
So I'd agree to remove that fallback mechanism.
- Do the work.
The second approach requires selecting a block erase size beforehand for the outer loop, and that blockerase size might not be the one we want to use to actually do the work (especially with the unaligned and optimized cases).
Agreed. In the long run, we'll want to choose the erase function more dynamically.
One pretty simple solution would be to exclude unaligned layout regions. How about this? anybody ever needed it?
Historically some ChromeOS devices have had unaligned layout regions. Generally speaking, excluding them would impose an artificial limitation on the images that flashrom can handle that will just bite users. Layout regions might be defined automatically by a firmware's build system without awareness of a particular flash chip's blockerase sizes, or some users may want to use unaligned regions to avoid wasting space (like putting multiple small regions into a 64KB block).
Ok, ok, I'm convinced. Never asked for it ;)
Another related thing is an optimization that I have in mind since ever I used flashrom: Selection of the biggest possible erase block size. Currently, if we erase/write say 16 blocks of 4KiB, we waste a lot of time because most chips would erase a 64KiB block much faster. If we ever add something like this, I guess it would be easier with approach 1). But maybe it'd be so invasive that it doesn't matter much...
I'd like this as well, and agree that it would be easier with the first approach. As far as invasiveness goes, the tricky part IMO is updating portions of read logic to deal with unaligned layout regions to ensure data outside the targeted region is restored, which requires knowing the eraseblock size in advance. Otherwise you end up with hacks like this: https://chromium-review.googlesource.com/c/240176/
I handle this at erase-block level (first code-block in read_erase_ write_block(), https://review.coreboot.org/#/c/17945/2/flashrom.c). It's not pretty but reduces the assumptions you have to take care of in higher levels.
Nico