Hi Albert,
On 01/22/2013 09:37 AM, Albert ARIBAUD wrote:
Hi Nikita,
[...]
Barring that, we should at least protect lcd.c from this issue by making some sort of check for affected targets, or maybe limit the possible values for splashimage... This issue makes it way too easy to accidentally break the boot process in a way that's hard to recover from.
I suggest a few solutions:
- enforce given load address alignment so that BMP header fields are
natively aligned, with a clear error message. Simple to implement, difficut for users to understand and accept.
Yes I agree that from a user point of view this looks terrible, which is why I prefer not to do something like this.
- once the address provided by the user is known, if it is not
properly aligned, then the next properly aligned address should be used, and the byte at given address should contain the offset from the given address to the used address. This is a general solution that works for any given load address, odd ones included:
Given address: First bytes: Used address: 10000000 2 x 'B' 'M' 10000002 10000001 1 'B' 'M' 10000002 10000002 'B' 'M' 10000002 10000003 3 x x 'B' 'M' 10000006 10000004 2 x 'B' 'M' 10000006 ...
Note that if the user address is constrained to be 4-byte-aligned, then only the "2 x 'B' 'M'" case would apply.
I think a simpler way to implement something like this is to just use modulo 4 to check alignment and fix the address dynamically; perhaps even fixing it in the environment.
This is a localized approach though. We will have to do this from all the code paths that lead to a bmp header being probed in memory. I would prefer a more localized solution.
- define an internal 'BMP holder' structure which contains a
two-byte prefix before the 'BMP file' header and data. This way, if the overall structure is aligned, then the fields in the BMP header are aligned too.
- Build a time machine and tell the designers of the BMP header format,
in great inventive and colorful detail, what horrible things will happen to them if they don't order and size their fields so that they naturally land on aligned offsets from the header start. This solution gives the best results IMO.
The problem with 3 (and 4) is that it still doesn't protect the user from bricking their board by choosing a non-aligned address for their BMP. This might happen because the user: - is unaware of the dangers of choosing a non-aligned address - made a typo - relies on a script or program that runs under U-Boot to setup stuff - I"m sure there are other possibilities
In terms of usability it's a *big* regression. If we do not actually prevent the user from setting splashimage to an unaligned address we should make sure that all usable addresses are safe.
- if none the above (including 4) is feasible for some reason, then
use unaligned accessors for this BMP fields, with a Big Fat Comment about why this is so.
I think, once this feature is merged, I'll try to see if something nice can be done with an approach like this. For now I'll add a suggestion-#2-style check in lcd.c
Note that all solutions except 2 (and 4) depend on the given address being constrained in some way -- such a constraint does not seem excessive to me.
Amicalement,