[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields
When configuring U-Boot to display a splash image, the user is free to designate whatever address they see fit as the in-memory location of BMP file. Unfortunately, this makes it easy to place the BMP header fields in unaligned addresses, which will cause a data abort on architectures which can't handle unaligned memory accesses. What's worse, addresses which are supposed to be issue free (32 bit aligned addresses) actually don't work because of the structure of the BMP header file (the header starts with 2 chars, followed by __u32 fields. The two chars offset the 32 bit fields away from proper alignment).
As a result, it is very easy to brick the board that U-Boot runs on with certain architectures. Once a bad address for splash image is selected and the board restarted, U-Boot never reaches command line, and yet the only way to prevent the data aborts from happening is to clear or change the environment variable splashimage.
While it is possible to fix the problem by compiling all relevant files with $(PLATFORM_NO_UNALIGNED), it is possible to handle it with finer granularity by creating an API for header accesses, and compiling only that with $(PLATFORM_NO_UNALIGNED).
This patchset creates such an API, and makes use of it wherever an in-memory BMP header is probed for data.
Further information can be found in this discussion: http://lists.denx.de/pipermail/u-boot/2013-January/144666.html
Nikita Kiryanov (5): Add bmp_layout module for accessing BMP header data lcd: use bmp_layout API cmd_bmp: use bmp_layout API video/cfb_console: use bmp_layout API video/bus_vcxk: use bmp_layout API
common/Makefile | 8 ++++ common/bmp_layout.c | 96 +++++++++++++++++++++++++++++++++++++++++++ common/cmd_bmp.c | 17 ++++---- common/lcd.c | 21 +++++----- drivers/video/bus_vcxk.c | 14 +++---- drivers/video/cfb_console.c | 22 +++++----- include/bmp_layout.h | 15 +++++++ 7 files changed, 152 insertions(+), 41 deletions(-) create mode 100644 common/bmp_layout.c
Currently code that displays BMP files does two things: * assume that any address is a valid load address for a BMP * access in-memory BMP header fields directly
Since some BMP header fields are 32 bit wide, this has a potential for causing data aborts when these fields are placed in unaligned addresses.
Create an API for safely accessing BMP header data, and compile it with $(PLATFORM_NO_UNALIGNED) to give it the ability to emulate unaligned memory accesses.
Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Signed-off-by: Igor Grinberg grinberg@compulab.co.il Cc: Anatolij Gustschin agust@denx.de Cc: Wolfgang Denk wd@denx.de Cc: Albert ARIBAUD albert.u.boot@aribaud.net Cc: Jeroen Hofstee jeroen@myspectrum.nl --- common/Makefile | 8 +++++ common/bmp_layout.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++ include/bmp_layout.h | 15 ++++++++ 3 files changed, 119 insertions(+) create mode 100644 common/bmp_layout.c
diff --git a/common/Makefile b/common/Makefile index 54fcc81..2a972c8 100644 --- a/common/Makefile +++ b/common/Makefile @@ -187,7 +187,14 @@ endif ifdef CONFIG_SPD_EEPROM SPD := y endif +ifdef CONFIG_CMD_BMP +BMP_LAYOUT := y +endif +ifdef CONFIG_SPLASH_SCREEN +BMP_LAYOUT := y +endif COBJS-$(SPD) += ddr_spd.o +COBJS-$(BMP_LAYOUT) += bmp_layout.o COBJS-$(CONFIG_HWCONFIG) += hwconfig.o COBJS-$(CONFIG_BOOTSTAGE) += bootstage.o COBJS-$(CONFIG_CONSOLE_MUX) += iomux.o @@ -250,6 +257,7 @@ $(obj)../tools/envcrc: # SEE README.arm-unaligned-accesses $(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) $(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) +$(obj)bmp_layout.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/common/bmp_layout.c b/common/bmp_layout.c new file mode 100644 index 0000000..c55b653 --- /dev/null +++ b/common/bmp_layout.c @@ -0,0 +1,96 @@ +/* (C) Copyright 2013 + * Nikita Kiryanov, Compulab, nikita@compulab.co.il. + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <asm/types.h> +#include <bmp_layout.h> +#include <compiler.h> + +int bmp_signature_valid(bmp_image_t *bmp) +{ + return bmp->header.signature[0] == 'B' && + bmp->header.signature[1] == 'M'; +} + +__u32 bmp_get_file_size(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.file_size); +} + +__u32 bmp_get_data_offset(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.data_offset); +} + +__u32 bmp_get_size(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.size); +} + +__u32 bmp_get_width(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.width); +} + +__u32 bmp_get_height(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.height); +} + +__u16 bmp_get_planes(bmp_image_t *bmp) +{ + return le16_to_cpu(bmp->header.planes); +} + +__u16 bmp_get_bit_count(bmp_image_t *bmp) +{ + return le16_to_cpu(bmp->header.bit_count); +} + +__u32 bmp_get_compression(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.compression); +} + +__u32 bmp_get_image_size(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.image_size); +} + +__u32 bmp_get_x_pixels_per_m(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.x_pixels_per_m); +} + +__u32 bmp_get_y_pixels_per_m(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.y_pixels_per_m); +} + +__u32 bmp_get_colors_used(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.colors_used); +} + +__u32 bmp_get_colors_important(bmp_image_t *bmp) +{ + return le32_to_cpu(bmp->header.colors_important); +} diff --git a/include/bmp_layout.h b/include/bmp_layout.h index d823de9..a983147 100644 --- a/include/bmp_layout.h +++ b/include/bmp_layout.h @@ -74,4 +74,19 @@ typedef struct bmp_image { #define BMP_BI_RLE8 1 #define BMP_BI_RLE4 2
+int bmp_signature_valid(bmp_image_t *bmp); +__u32 bmp_get_file_size(bmp_image_t *bmp); +__u32 bmp_get_data_offset(bmp_image_t *bmp); +__u32 bmp_get_size(bmp_image_t *bmp); +__u32 bmp_get_width(bmp_image_t *bmp); +__u32 bmp_get_height(bmp_image_t *bmp); +__u16 bmp_get_planes(bmp_image_t *bmp); +__u16 bmp_get_bit_count(bmp_image_t *bmp); +__u32 bmp_get_compression(bmp_image_t *bmp); +__u32 bmp_get_image_size(bmp_image_t *bmp); +__u32 bmp_get_x_pixels_per_m(bmp_image_t *bmp); +__u32 bmp_get_y_pixels_per_m(bmp_image_t *bmp); +__u32 bmp_get_colors_used(bmp_image_t *bmp); +__u32 bmp_get_colors_important(bmp_image_t *bmp); + #endif /* _BMP_H_ */
Dear Nikita Kiryanov,
In message 1359977979-28585-2-git-send-email-nikita@compulab.co.il you wrote:
Currently code that displays BMP files does two things:
- assume that any address is a valid load address for a BMP
- access in-memory BMP header fields directly
Since some BMP header fields are 32 bit wide, this has a potential for causing data aborts when these fields are placed in unaligned addresses.
Create an API for safely accessing BMP header data, and compile it with $(PLATFORM_NO_UNALIGNED) to give it the ability to emulate unaligned memory accesses.
Frankly, I think this is overkill. U-Boot is a bootloader, and it is supposed to be lean and eficient. We don't have all levels of safety systems and protective devices as in, for example, an aircraft. You are supposed to know what you are doing, and if you ignore the rules, you will quickly see the results yourself.
There is plenty of other areas where correct opration requires certain alignments, and none of these are enforced by U-Boot. And actually I think this is not only acceptable, but good as is.
"UNIX was not designed to stop you from doing stupid things, because that would also stop you from doing clever things." - Doug Gwyn
You talk about BMP header - but we also have alignment requirements for image headers, well, even for a plain "md" or "mw" command. And none of these provide any protection against accidsential (or intentional) access to unaligned addresses.
My recommendation is: just don;t do it, then.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Mon, 04 Feb 2013 20:26:18 +0100, Wolfgang Denk wd@denx.de wrote:
Dear Nikita Kiryanov,
In message 1359977979-28585-2-git-send-email-nikita@compulab.co.il you wrote:
Currently code that displays BMP files does two things:
- assume that any address is a valid load address for a BMP
- access in-memory BMP header fields directly
Since some BMP header fields are 32 bit wide, this has a potential for causing data aborts when these fields are placed in unaligned addresses.
Create an API for safely accessing BMP header data, and compile it with $(PLATFORM_NO_UNALIGNED) to give it the ability to emulate unaligned memory accesses.
Frankly, I think this is overkill. U-Boot is a bootloader, and it is supposed to be lean and eficient. We don't have all levels of safety systems and protective devices as in, for example, an aircraft. You are supposed to know what you are doing, and if you ignore the rules, you will quickly see the results yourself.
There is plenty of other areas where correct opration requires certain alignments, and none of these are enforced by U-Boot. And actually I think this is not only acceptable, but good as is.
"UNIX was not designed to stop you from doing stupid things, because that would also stop you from doing clever things." - Doug Gwyn
You talk about BMP header - but we also have alignment requirements for image headers, well, even for a plain "md" or "mw" command. And none of these provide any protection against accidsential (or intentional) access to unaligned addresses.
My recommendation is: just don;t do it, then.
The point about md not checking alignment is indeed valid: one should know that a md.l requires a 4-byte-aligned address or it will abort.
OTOH, a cautious user may think that to ensure proper alignment, a BMP should be loaded on a 4-byte boundary, but IIUC that it precisely what will cause the load to fail, due to the sole 4-byte field of the BMP header being misaligned by two bytes.
So if we leave BMP loading as it is now, the load address will need to be 16-bit-but-not-32-bit-aligned, which is complicated enough to require documentation.
Or, the BMP struct could be prepended with two bytes so that the load address alignment requirement becomes a simple 4-byte boundary, which most users are... bound... to choose naturally.
But ISTR the idea of prepending two bytes was already discussed and for some reason it could not work. Jeroen?
Best regards,
Wolfgang Denk
Amicalement,
Dear Albert ARIBAUD,
In message 20130204222628.545da91e@lilith you wrote:
The point about md not checking alignment is indeed valid: one should know that a md.l requires a 4-byte-aligned address or it will abort.
Or, one might do this _intentionally_ for some testing purposes. For me it is of utmost importance that U-Boot does not come in my way in such things. It is supposed to do _exactly_ what I ask it to do, even if this appears to be a stupid thing.
OTOH, a cautious user may think that to ensure proper alignment, a BMP should be loaded on a 4-byte boundary, but IIUC that it precisely what will cause the load to fail, due to the sole 4-byte field of the BMP header being misaligned by two bytes.
Sole 4 byte field? The bitmap file header has actually two 32-bit entries: file_size, and data_offset. [The "reserved" entry as we are using it should actually be two 16 bit entries, at least according to [1]).
Yes, struct bmp_header is a packed array with non-natural order of entries; see also section "Bitmap file header" at [1]; we may consider this a really stupid thing to do, but we have to live with this fact.
[1] http://en.wikipedia.org/wiki/BMP_file_format
As I understand the problem comes from the fact, that this issue has long been undetected, because the PTB that were/are responsible for GCC on ARM had decided that any access to apacked structure would be silently broken down into byte accesses, no matter what the actual data type was. While more recent versions of GCC started actually attempting 16 or 32 bit accesses, which failed on some systems.
So if we leave BMP loading as it is now, the load address will need to be 16-bit-but-not-32-bit-aligned, which is complicated enough to require documentation.
Indeed, this should be documented. And eventually the bmp command should print a warning message if it sees other alignment.
Or, the BMP struct could be prepended with two bytes so that the load address alignment requirement becomes a simple 4-byte boundary, which most users are... bound... to choose naturally.
That would violate the "standard" defining the BMP header structure.
Best regards,
Wolfgang Denk
On 02/05/13 01:11, Wolfgang Denk wrote:
Dear Albert ARIBAUD,
In message 20130204222628.545da91e@lilith you wrote:
The point about md not checking alignment is indeed valid: one should know that a md.l requires a 4-byte-aligned address or it will abort.
Or, one might do this _intentionally_ for some testing purposes. For me it is of utmost importance that U-Boot does not come in my way in such things. It is supposed to do _exactly_ what I ask it to do, even if this appears to be a stupid thing.
I agree on this one, except for the case where doing that stupid thing bricks the board.
OTOH, a cautious user may think that to ensure proper alignment, a BMP should be loaded on a 4-byte boundary, but IIUC that it precisely what will cause the load to fail, due to the sole 4-byte field of the BMP header being misaligned by two bytes.
Sole 4 byte field? The bitmap file header has actually two 32-bit entries: file_size, and data_offset. [The "reserved" entry as we are using it should actually be two 16 bit entries, at least according to [1]).
Yes, struct bmp_header is a packed array with non-natural order of entries; see also section "Bitmap file header" at [1]; we may consider this a really stupid thing to do, but we have to live with this fact.
It was not that stupid in times of DOS and Win 3.1 when int was 16 bits long (and DRAM was 64K or even less)...
[1] http://en.wikipedia.org/wiki/BMP_file_format
As I understand the problem comes from the fact, that this issue has long been undetected, because the PTB that were/are responsible for GCC on ARM had decided that any access to apacked structure would be silently broken down into byte accesses, no matter what the actual data type was. While more recent versions of GCC started actually attempting 16 or 32 bit accesses, which failed on some systems.
So if we leave BMP loading as it is now, the load address will need to be 16-bit-but-not-32-bit-aligned, which is complicated enough to require documentation.
Indeed, this should be documented. And eventually the bmp command should print a warning message if it sees other alignment.
Agreed on this also, but again what about the board brick case? Should we add the check for alignment and if it does not properly aligned deny further bmp header processing along with printing a warning?
Or, the BMP struct could be prepended with two bytes so that the load address alignment requirement becomes a simple 4-byte boundary, which most users are... bound... to choose naturally.
That would violate the "standard" defining the BMP header structure.
Yep, I would not want this to happen.
Dear Igor Grinberg,
In message 5110BDB2.8040800@compulab.co.il you wrote:
Yes, struct bmp_header is a packed array with non-natural order of entries; see also section "Bitmap file header" at [1]; we may consider this a really stupid thing to do, but we have to live with this fact.
It was not that stupid in times of DOS and Win 3.1 when int was 16 bits long (and DRAM was 64K or even less)...
It was as stupid then, too. At that time, a large number of systems with similar alignment requirements existed, and experience with these existed, too.
Do you remember the "The Ten Commandments for C Programmers"? If not, look them up and check how old these are. Note especially the ``All the world's a VAX'' part - this is exactly what we see here in operation.
The design of the BMP header is just BRAINDEAD.
Indeed, this should be documented. And eventually the bmp command should print a warning message if it sees other alignment.
Agreed on this also, but again what about the board brick case?
There is a ton of ways to brick a board. Why should we add protection for one special case while we agree to leave the 50 other ways onfixed?
Should we add the check for alignment and if it does not properly aligned deny further bmp header processing along with printing a warning?
Why should we? Who tells that this is not perfectly legal on the running system?
Let me repeat it: U-Boot is a boot loader. It is not intended for meddling by avarage Johnny Loser, but for system programmers who know what they are doing. And anyone doing such things is well adviced to _test_ his settings on the command line before storing these for automatic use. As I mentioned before, omitting such tests is a sin that carries with it its own punishment.
Best regards,
Wolfgang Denk
On 02/05/13 11:57, Wolfgang Denk wrote:
Dear Igor Grinberg,
In message 5110BDB2.8040800@compulab.co.il you wrote:
Yes, struct bmp_header is a packed array with non-natural order of entries; see also section "Bitmap file header" at [1]; we may consider this a really stupid thing to do, but we have to live with this fact.
It was not that stupid in times of DOS and Win 3.1 when int was 16 bits long (and DRAM was 64K or even less)...
It was as stupid then, too. At that time, a large number of systems with similar alignment requirements existed, and experience with these existed, too.
Do you remember the "The Ten Commandments for C Programmers"? If not, look them up and check how old these are. Note especially the ``All the world's a VAX'' part - this is exactly what we see here in operation.
Yep. Agreed on this. Although, I don't know, if anyone of us would tell the same 20+ years ago... It is now we can...
The design of the BMP header is just BRAINDEAD.
That is for sure.
Indeed, this should be documented. And eventually the bmp command should print a warning message if it sees other alignment.
Agreed on this also, but again what about the board brick case?
There is a ton of ways to brick a board. Why should we add protection for one special case while we agree to leave the 50 other ways onfixed?
Because, I think this one is a bit different because of the bmp header and also is of very high demand.
Should we add the check for alignment and if it does not properly aligned deny further bmp header processing along with printing a warning?
Why should we? Who tells that this is not perfectly legal on the running system?
It might be perfectly legal, but we also consider a tons of work explaining why and how this should be done (needless to mention the amount of bricked boards). Instead of simplifying the case, and I think this is a special case, at least because of the high demand and user (who is not a programmer) selectable address, you want us to teach the whole world about the bmp header alignment issues?
Let me repeat it: U-Boot is a boot loader. It is not intended for meddling by avarage Johnny Loser, but for system programmers who know what they are doing. And anyone doing such things is well adviced to _test_ his settings on the command line before storing these for automatic use. As I mentioned before, omitting such tests is a sin that carries with it its own punishment.
What are you trying to say? Is it that the environment variables change and in particular the splash screen installation _must_ be done by a programmer? Hmm.. that does not sound good...
Dear Igor,
In message 5110EF4F.3080308@compulab.co.il you wrote:
Why should we? Who tells that this is not perfectly legal on the running system?
It might be perfectly legal, but we also consider a tons of work explaining why and how this should be done (needless to mention the amount of bricked boards).
Please understand that I will not really buy this "bricked bord" argument. This is an issue where system builders and users are involved. Apparently the system builders agree that performance is so important that they compile with optimizer options that do not tolerate unaligned accesses, thus introducing the problem. This is OK for systems where only educated users have access. If you open the U-Boot console interface to uneducated users, you are always running some risk that a stupid command will brick the board or at least make it no longer usable to that user. And as a user you should well be aware that bad things can happen, and that it is an excellent idea to actually test any new settings or operations before installing these. If users ignore even such basic rules, then the situation is f*cked up and cannot be helped - if it's not the spalsh screen, then these users will find other ways to run into trouble.
Let me repeat it: U-Boot is a boot loader. It is not intended for meddling by avarage Johnny Loser, but for system programmers who know what they are doing. And anyone doing such things is well adviced to _test_ his settings on the command line before storing these for automatic use. As I mentioned before, omitting such tests is a sin that carries with it its own punishment.
What are you trying to say? Is it that the environment variables change and in particular the splash screen installation _must_ be done by a programmer?
I tried to be clear: people who work on such a level are supposed to know what they are doing.
I find it interesting that a lot of arguments get raised here how important this issue is (is it? who has actually bricked a system this way?), and how that is a special case (here I disagree), but so far you all appear to ignore my argument of testing settings before putting these to use.
I see little excuse for neglecting such really basic diligence.
Best regards,
Wolfgang Denk
On 02/05/13 15:20, Wolfgang Denk wrote:
Dear Igor,
In message 5110EF4F.3080308@compulab.co.il you wrote:
Why should we? Who tells that this is not perfectly legal on the running system?
It might be perfectly legal, but we also consider a tons of work explaining why and how this should be done (needless to mention the amount of bricked boards).
Please understand that I will not really buy this "bricked bord" argument. This is an issue where system builders and users are involved. Apparently the system builders agree that performance is so important that they compile with optimizer options that do not tolerate unaligned accesses, thus introducing the problem. This is OK for systems where only educated users have access. If you open the U-Boot console interface to uneducated users, you are always running some risk that a stupid command will brick the board or at least make it no longer usable to that user. And as a user you should well be aware that bad things can happen, and that it is an excellent idea to actually test any new settings or operations before installing these. If users ignore even such basic rules, then the situation is f*cked up and cannot be helped - if it's not the spalsh screen, then these users will find other ways to run into trouble.
Totally... I agree the bricked bored ;-) should not be an argument, but the tons of work might be...
Let me repeat it: U-Boot is a boot loader. It is not intended for meddling by avarage Johnny Loser, but for system programmers who know what they are doing. And anyone doing such things is well adviced to _test_ his settings on the command line before storing these for automatic use. As I mentioned before, omitting such tests is a sin that carries with it its own punishment.
What are you trying to say? Is it that the environment variables change and in particular the splash screen installation _must_ be done by a programmer?
I tried to be clear: people who work on such a level are supposed to know what they are doing.
I find it interesting that a lot of arguments get raised here how important this issue is (is it? who has actually bricked a system this way?),
This is a relatively new default setting for the compiler, and I think this is the reason why you (still) haven't heard about it. Also, do you really expect that whoever gets the board bricked will go complaining to the mailing list? I know many users, that don't even know about the mailing list existence at all and they don't care... What they do care is for the feature to work and have a simple yet usable user API.
and how that is a special case (here I disagree), but so far you all appear to ignore my argument of testing settings before putting these to use.
Loading of the splash screen has been a part of the U-Boot boot sequence for ages, so the test of the feature is roughly just place the bmp image in the right place on the storage device, set the splashimage variable and reset the board. They don't think about the new compiler right away and they don't think about the bmp header. All they think about is: "I always did it like this, so lets do it the same way...", and here comes the new compiler default...
Now, I agree with you, that the above might be not the best way. And I agree that U-Boot, as a boot loader, should be just a dumb piece of code that does whatever the user/programmer tells it to do.
Is there any argument, from what was said in this (and other) thread, you agree with? Can you propose a feasible (yet not too expensive and not out of mainline tree) solution?
Dear Igor,
In message 51111C0F.60404@compulab.co.il you wrote:
This is a relatively new default setting for the compiler, and I think this is the reason why you (still) haven't heard about it. Also, do you really expect that whoever gets the board bricked will go complaining to the mailing list?
Then how do we kno if this is just an anticipated or a real problem?
Loading of the splash screen has been a part of the U-Boot boot sequence for ages, so the test of the feature is roughly just place the bmp image in the right place on the storage device, set the splashimage variable and reset the board.
Maybe I'm so weird then - I think I have never blindly installed a splash screen without checking before that it actually works and looks good, using the "bmp" command...
Can you propose a feasible (yet not too expensive and not out of mainline tree) solution?
Well, if I understand the situation correctly, the problem we have is that the bitmap address for the splash screen should be aligned to an even 32 bit boundary + 1. The address is stored in an environment variable.
So why don't we implement a callback on that variable that checks itd's value when it gets set? We have all the features in place now; and we can even overwrite such a default setting on boards where it's not needed / wanted.
So my recommendation is: add a new alignment-checking extension as a callback to the variable handling code.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "Have you lived in this village all your life?" "No, not yet."
Dear Wolfgang Denk,
On 02/04/2013 09:26 PM, Wolfgang Denk wrote:
Dear Nikita Kiryanov,
In message 1359977979-28585-2-git-send-email-nikita@compulab.co.il you wrote:
Currently code that displays BMP files does two things:
- assume that any address is a valid load address for a BMP
- access in-memory BMP header fields directly
Since some BMP header fields are 32 bit wide, this has a potential for causing data aborts when these fields are placed in unaligned addresses.
Create an API for safely accessing BMP header data, and compile it with $(PLATFORM_NO_UNALIGNED) to give it the ability to emulate unaligned memory accesses.
Frankly, I think this is overkill. U-Boot is a bootloader, and it is supposed to be lean and eficient. We don't have all levels of safety systems and protective devices as in, for example, an aircraft. You are supposed to know what you are doing, and if you ignore the rules, you will quickly see the results yourself.
[...]
You talk about BMP header - but we also have alignment requirements for image headers, well, even for a plain "md" or "mw" command. And none of these provide any protection against accidsential (or intentional) access to unaligned addresses.
That's true, but when md traps you simply restart the board and everything's fine. If displaying a splash screen traps- you're stuck. I'm not saying we should start implementing protection against every possible mistake, but when the repercussions are this serious I feel that protection is in order.
There's a difference between a bicycle with no training wheels and one that falls apart when you turn it the wrong way.
My recommendation is: just don;t do it, then.
Best regards,
Wolfgang Denk
Dear Nikita,
In message 5110AC16.9000306@compulab.co.il you wrote:
That's true, but when md traps you simply restart the board and everything's fine. If displaying a splash screen traps- you're stuck.
In such a case you have forgotten to test your settings before activation these as default, i. e. you have committed a sin that carries with it its own punishment ;-)
There's a difference between a bicycle with no training wheels and one that falls apart when you turn it the wrong way.
It's not the bicyle falling apart, it's the rider falling down (with the risk of getting hurt) - and yes, exactly this happens in real life when you disobey basic caution.
The same will happen for other stupid settings, too - like setting a bootdelay of 0 with a non-working bootcmd; or incorrect update commands that blow away U-Boot, or ...
This is a boot loader, and there are no seatbelts or airbags; nothing prevents you to shoot yourself into the foot.
Best regards,
Wolfgang Denk
If the BMP headers are located in unaligned addresses, accessing them directly may lead to a data abort on some architectures. Use the safer bmp_layout API instead.
Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Signed-off-by: Igor Grinberg grinberg@compulab.co.il Cc: Anatolij Gustschin agust@denx.de Cc: Wolfgang Denk wd@denx.de Cc: Albert ARIBAUD albert.u.boot@aribaud.net Cc: Jeroen Hofstee jeroen@myspectrum.nl> --- common/lcd.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/common/lcd.c b/common/lcd.c index 66d4f94..4c0c87f 100644 --- a/common/lcd.c +++ b/common/lcd.c @@ -750,9 +750,9 @@ static void lcd_display_rle8_bitmap(bmp_image_t *bmp, ushort *cmap, uchar *fb, int x, y; int decode = 1;
- width = le32_to_cpu(bmp->header.width); - height = le32_to_cpu(bmp->header.height); - bmap = (uchar *)bmp + le32_to_cpu(bmp->header.data_offset); + width = bmp_get_width(bmp); + height = bmp_get_height(bmp); + bmap = (uchar *)bmp + bmp_get_data_offset(bmp);
x = 0; y = height - 1; @@ -866,16 +866,15 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y) unsigned long pwidth = panel_info.vl_col; unsigned colors, bpix, bmp_bpix;
- if (!bmp || !((bmp->header.signature[0] == 'B') && - (bmp->header.signature[1] == 'M'))) { + if (!bmp || !bmp_signature_valid(bmp)) { printf("Error: no valid bmp image at %lx\n", bmp_image);
return 1; }
- width = le32_to_cpu(bmp->header.width); - height = le32_to_cpu(bmp->header.height); - bmp_bpix = le16_to_cpu(bmp->header.bit_count); + width = bmp_get_width(bmp); + height = bmp_get_height(bmp); + bmp_bpix = bmp_get_bit_count(bmp); colors = 1 << bmp_bpix;
bpix = NBITS(panel_info.vl_bpix); @@ -891,7 +890,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y) if (bpix != bmp_bpix && !(bmp_bpix == 8 && bpix == 16)) { printf ("Error: %d bit/pixel mode, but BMP has %d bit/pixel\n", bpix, - le16_to_cpu(bmp->header.bit_count)); + bmp_get_bit_count(bmp));
return 1; } @@ -960,7 +959,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y) if ((y + height) > panel_info.vl_row) height = panel_info.vl_row - y;
- bmap = (uchar *)bmp + le32_to_cpu(bmp->header.data_offset); + bmap = (uchar *)bmp + bmp_get_data_offset(bmp); fb = (uchar *) (lcd_base + (y + height - 1) * lcd_line_length + x * bpix / 8);
@@ -968,7 +967,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y) case 1: /* pass through */ case 8: #ifdef CONFIG_LCD_BMP_RLE8 - if (le32_to_cpu(bmp->header.compression) == BMP_BI_RLE8) { + if (bmp_get_compression(bmp) == BMP_BI_RLE8) { if (bpix != 16) { /* TODO implement render code for bpix != 16 */ printf("Error: only support 16 bpix");
If the BMP headers are located in unaligned addresses, accessing them directly may lead to a data abort on some architectures. Use the safer bmp_layout API instead.
Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Signed-off-by: Igor Grinberg grinberg@compulab.co.il Cc: Anatolij Gustschin agust@denx.de Cc: Wolfgang Denk wd@denx.de Cc: Albert ARIBAUD albert.u.boot@aribaud.net Cc: Jeroen Hofstee jeroen@myspectrum.nl --- common/cmd_bmp.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/common/cmd_bmp.c b/common/cmd_bmp.c index 5a52edd..0713ba8 100644 --- a/common/cmd_bmp.c +++ b/common/cmd_bmp.c @@ -73,8 +73,7 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) /* * Check for bmp mark 'BM' */ - if (!((bmp->header.signature[0] == 'B') && - (bmp->header.signature[1] == 'M'))) { + if (!bmp_signature_valid(bmp)) { free(dst); return NULL; } @@ -191,8 +190,7 @@ static int bmp_info(ulong addr) bmp_image_t *bmp=(bmp_image_t *)addr; unsigned long len;
- if (!((bmp->header.signature[0]=='B') && - (bmp->header.signature[1]=='M'))) + if (!bmp_signature_valid(bmp)) bmp = gunzip_bmp(addr, &len);
if (bmp == NULL) { @@ -200,10 +198,10 @@ static int bmp_info(ulong addr) return 1; }
- printf("Image size : %d x %d\n", le32_to_cpu(bmp->header.width), - le32_to_cpu(bmp->header.height)); - printf("Bits per pixel: %d\n", le16_to_cpu(bmp->header.bit_count)); - printf("Compression : %d\n", le32_to_cpu(bmp->header.compression)); + printf("Image size : %d x %d\n", bmp_get_width(bmp), + bmp_get_height(bmp)); + printf("Bits per pixel: %d\n", bmp_get_bit_count(bmp)); + printf("Compression : %d\n", bmp_get_compression(bmp));
if ((unsigned long)bmp != addr) free(bmp); @@ -227,8 +225,7 @@ int bmp_display(ulong addr, int x, int y) bmp_image_t *bmp = (bmp_image_t *)addr; unsigned long len;
- if (!((bmp->header.signature[0]=='B') && - (bmp->header.signature[1]=='M'))) + if (!bmp_signature_valid(bmp)) bmp = gunzip_bmp(addr, &len);
if (!bmp) {
If the BMP headers are located in unaligned addresses, accessing them directly may lead to a data abort on some architectures. Use the safer bmp_layout API instead.
Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Signed-off-by: Igor Grinberg grinberg@compulab.co.il Cc: Anatolij Gustschin agust@denx.de Cc: Wolfgang Denk wd@denx.de Cc: Albert ARIBAUD albert.u.boot@aribaud.net Cc: Jeroen Hofstee jeroen@myspectrum.nl --- drivers/video/cfb_console.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c index 26f673a..6d4e2c5 100644 --- a/drivers/video/cfb_console.c +++ b/drivers/video/cfb_console.c @@ -1446,8 +1446,7 @@ int video_display_bitmap(ulong bmp_image, int x, int y)
WATCHDOG_RESET();
- if (!((bmp->header.signature[0] == 'B') && - (bmp->header.signature[1] == 'M'))) { + if (!bmp_signature_valid(bmp)) {
#ifdef CONFIG_VIDEO_BMP_GZIP /* @@ -1477,8 +1476,7 @@ int video_display_bitmap(ulong bmp_image, int x, int y) */ bmp = (bmp_image_t *) dst;
- if (!((bmp->header.signature[0] == 'B') && - (bmp->header.signature[1] == 'M'))) { + if (!bmp_signature_valid(bmp)) { printf("Error: no valid bmp.gz image at %lx\n", bmp_image); free(dst); @@ -1490,11 +1488,11 @@ int video_display_bitmap(ulong bmp_image, int x, int y) #endif /* CONFIG_VIDEO_BMP_GZIP */ }
- width = le32_to_cpu(bmp->header.width); - height = le32_to_cpu(bmp->header.height); - bpp = le16_to_cpu(bmp->header.bit_count); - colors = le32_to_cpu(bmp->header.colors_used); - compression = le32_to_cpu(bmp->header.compression); + width = bmp_get_width(bmp); + height = bmp_get_height(bmp); + bpp = bmp_get_bit_count(bmp); + colors = bmp_get_colors_used(bmp); + compression = bmp_get_compression(bmp);
debug("Display-bmp: %ld x %ld with %d colors\n", width, height, colors); @@ -1539,7 +1537,7 @@ int video_display_bitmap(ulong bmp_image, int x, int y) if ((y + height) > VIDEO_VISIBLE_ROWS) height = VIDEO_VISIBLE_ROWS - y;
- bmap = (uchar *) bmp + le32_to_cpu(bmp->header.data_offset); + bmap = (uchar *) bmp + bmp_get_data_offset(bmp); fb = (uchar *) (video_fb_address + ((y + height - 1) * VIDEO_COLS * VIDEO_PIXEL_SIZE) + x * VIDEO_PIXEL_SIZE); @@ -1551,7 +1549,7 @@ int video_display_bitmap(ulong bmp_image, int x, int y) #endif
/* We handle only 4, 8, or 24 bpp bitmaps */ - switch (le16_to_cpu(bmp->header.bit_count)) { + switch (bmp_get_bit_count(bmp)) { case 4: padded_line -= width / 2; ycount = height; @@ -1785,7 +1783,7 @@ int video_display_bitmap(ulong bmp_image, int x, int y) break; default: printf("Error: %d bit/pixel bitmaps not supported by U-Boot\n", - le16_to_cpu(bmp->header.bit_count)); + bmp_get_bit_count(bmp)); break; }
If the BMP headers are located in unaligned addresses, accessing them directly may lead to a data abort on some architectures. Use the safer bmp_layout API instead.
Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Signed-off-by: Igor Grinberg grinberg@compulab.co.il Cc: Anatolij Gustschin agust@denx.de Cc: Wolfgang Denk wd@denx.de Cc: Albert ARIBAUD albert.u.boot@aribaud.net Cc: Jeroen Hofstee jeroen@myspectrum.nl --- drivers/video/bus_vcxk.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/video/bus_vcxk.c b/drivers/video/bus_vcxk.c index a0607cf..d382ac1 100644 --- a/drivers/video/bus_vcxk.c +++ b/drivers/video/bus_vcxk.c @@ -401,14 +401,12 @@ int vcxk_display_bitmap(ulong addr, int x, int y) unsigned char *dataptr;
bmp = (bmp_image_t *) addr; - if ((bmp->header.signature[0] == 'B') && - (bmp->header.signature[1] == 'M')) { - width = le32_to_cpu(bmp->header.width); - height = le32_to_cpu(bmp->header.height); - bpp = le16_to_cpu(bmp->header.bit_count); - - dataptr = (unsigned char *) bmp + - le32_to_cpu(bmp->header.data_offset); + if (bmp_signature_valid(bmp)) { + width = bmp_get_width(bmp); + height = bmp_get_height(bmp); + bpp = bmp_get_bit_count(bmp); + + dataptr = (unsigned char *) bmp + bmp_get_data_offset(bmp);
if (display_width < (width + x)) c_width = display_width - x;
Hi Nikita,
On Mon, 4 Feb 2013 13:39:34 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
When configuring U-Boot to display a splash image, the user is free to designate whatever address they see fit as the in-memory location of BMP file. Unfortunately, this makes it easy to place the BMP header fields in unaligned addresses, which will cause a data abort on architectures which can't handle unaligned memory accesses. What's worse, addresses which are supposed to be issue free (32 bit aligned addresses) actually don't work because of the structure of the BMP header file (the header starts with 2 chars, followed by __u32 fields. The two chars offset the 32 bit fields away from proper alignment).
As a result, it is very easy to brick the board that U-Boot runs on with certain architectures. Once a bad address for splash image is selected and the board restarted, U-Boot never reaches command line, and yet the only way to prevent the data aborts from happening is to clear or change the environment variable splashimage.
While it is possible to fix the problem by compiling all relevant files with $(PLATFORM_NO_UNALIGNED), it is possible to handle it with finer granularity by creating an API for header accesses, and compiling only that with $(PLATFORM_NO_UNALIGNED).
This patchset creates such an API, and makes use of it wherever an in-memory BMP header is probed for data.
IIRC, you has submitted a fix so that BMP loads would result in correctly aligned fields and thus no need for accessors. Why this change of mind?
Amicalement,
Hi Albert,
On 02/04/2013 01:57 PM, Albert ARIBAUD wrote:
IIRC, you has submitted a fix so that BMP loads would result in correctly aligned fields and thus no need for accessors. Why this change of mind?
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
Amicalement,
Hi Nikita,
On Mon, 04 Feb 2013 14:37:06 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
Hi Albert,
On 02/04/2013 01:57 PM, Albert ARIBAUD wrote:
IIRC, you has submitted a fix so that BMP loads would result in correctly aligned fields and thus no need for accessors. Why this change of mind?
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
I actually like the new fix less, as it adds an API where there is no need of one -- it's not like the implementation of BMP is at risk of changing. What is the problem in fixing the load address at load time and then passing this fixed address around?
Amicalement,
On 02/04/2013 04:19 PM, Albert ARIBAUD wrote:
Hi Nikita,
On Mon, 04 Feb 2013 14:37:06 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
Hi Albert,
On 02/04/2013 01:57 PM, Albert ARIBAUD wrote:
IIRC, you has submitted a fix so that BMP loads would result in correctly aligned fields and thus no need for accessors. Why this change of mind?
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
I actually like the new fix less, as it adds an API where there is no need of one -- it's not like the implementation of BMP is at risk of changing. What is the problem in fixing the load address at load time and then passing this fixed address around?
The problem is that it makes the U-Boot display subsystems broken in terms of what kind of programming interface they provide. The load address fix has to be applied in every code path that leads to a BMP being loaded and read in memory. This means that anyone who wants to implement some BMP related functionality needs to care about this architecture/memory issue, and actively circumvent it. This isn't good design. If I'm manipulating BMPs I should only care about my BMPs, not hardware issues.
Amicalement,
Hi Nikita,
On Mon, 04 Feb 2013 17:07:05 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
On 02/04/2013 04:19 PM, Albert ARIBAUD wrote:
Hi Nikita,
On Mon, 04 Feb 2013 14:37:06 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
Hi Albert,
On 02/04/2013 01:57 PM, Albert ARIBAUD wrote:
IIRC, you has submitted a fix so that BMP loads would result in correctly aligned fields and thus no need for accessors. Why this change of mind?
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
I actually like the new fix less, as it adds an API where there is no need of one -- it's not like the implementation of BMP is at risk of changing. What is the problem in fixing the load address at load time and then passing this fixed address around?
The problem is that it makes the U-Boot display subsystems broken in terms of what kind of programming interface they provide.
I'm not sure I understand what this means. Once all places that construct BMPs in memory are fixed so that they don't misalign, and documentation is there to tell about the issue, what exactly remains broken?
The load address fix has to be applied in every code path that leads to a BMP being loaded and read in memory. This means that anyone who wants to implement some BMP related functionality needs to care about this architecture/memory issue, and actively circumvent it.
Yes; but it is only a matter of getting the properly aligned address from the non-aligned one, which a simple macro is enough to provide; the rest, namely accessing fields, does not have to be turned into an API, nor does it benefit from it. Besides, switching to an API requires searching every place where BMPs are used, just like fixing addressing does.
This isn't good design. If I'm manipulating BMPs I should only care about my BMPs, not hardware issues.
Except that you have to, because you're not simply manipulating BMPs, you're manipulating them in a context where alignment is required.
Amicalement,
Dear Albert ARIBAUD,
In message 20130204162528.2602782a@lilith you wrote:
Yes; but it is only a matter of getting the properly aligned address from the non-aligned one, which a simple macro is enough to provide; the
We should not even do this. If the user supplies address foo, he probably means it. I seriously dislike code that "tries to be clever" and does not allow me to do what I want it to do.
Best regards,
Wolfgang Denk
Hi Albert ARIBAUD,
On 02/04/2013 05:25 PM, Albert ARIBAUD wrote:
Hi Nikita,
On Mon, 04 Feb 2013 17:07:05 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
On 02/04/2013 04:19 PM, Albert ARIBAUD wrote:
Hi Nikita,
On Mon, 04 Feb 2013 14:37:06 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
Hi Albert,
On 02/04/2013 01:57 PM, Albert ARIBAUD wrote:
IIRC, you has submitted a fix so that BMP loads would result in correctly aligned fields and thus no need for accessors. Why this change of mind?
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
I actually like the new fix less, as it adds an API where there is no need of one -- it's not like the implementation of BMP is at risk of changing. What is the problem in fixing the load address at load time and then passing this fixed address around?
The problem is that it makes the U-Boot display subsystems broken in terms of what kind of programming interface they provide.
I'm not sure I understand what this means. Once all places that construct BMPs in memory are fixed so that they don't misalign, and documentation is there to tell about the issue, what exactly remains broken?
See below...
The load address fix has to be applied in every code path that leads to a BMP being loaded and read in memory. This means that anyone who wants to implement some BMP related functionality needs to care about this architecture/memory issue, and actively circumvent it.
Yes; but it is only a matter of getting the properly aligned address from the non-aligned one, which a simple macro is enough to provide;
I would've been in favour of the address fix solution if it was confined to lcd.c, because then it would've been an implementation detail of U-Boot's display subsystem, but it's not going to be necessarily confined there. We have display_draw_bitmap() in the API, so users can write standalone programs that display BMPs. Boards are required to load the BMP to memory on their own, so that's another place where there might be probing of the BMP header. So new code paths that lead to BMP header probing are to be expected, and this means that we would be adding a new requirement to anyone who wants to use BMPs.
the rest, namely accessing fields, does not have to be turned into an API, nor does it benefit from it.
I think an API has benefits, namely that it simplifies the code a bit, and it separates the alignment concern (handled with the compilation flag) from the reading-BMP-data concern (handled by using the API).
Besides, switching to an API requires
searching every place where BMPs are used, just like fixing addressing does.
This isn't good design. If I'm manipulating BMPs I should only care about my BMPs, not hardware issues.
Except that you have to, because you're not simply manipulating BMPs, you're manipulating them in a context where alignment is required.
That's true, but it's not an argument against simplifying the code.
Amicalement,
Dear Albert ARIBAUD,
In message 20130204151956.0c0e8486@lilith you wrote:
I actually like the new fix less, as it adds an API where there is no need of one -- it's not like the implementation of BMP is at risk of changing. What is the problem in fixing the load address at load time and then passing this fixed address around?
Actually - why do we need a fix in the firtst place?
Nothing prevents you to load an uImage, or a file system image, or an environment image, or ... or anything else to an address that does not meet the respectve alignment requirements. If you do, it doesn't work, but that is to be expected. Don't do it, then.
I don't think we should try to prevent all possible stupid actions.
[No _random_ signature today. :-) ]
Best regards,
Wolfgang Denk
On Mon, Feb 04, 2013 at 03:19:56PM +0100, Albert ARIBAUD wrote:
Hi Nikita,
On Mon, 04 Feb 2013 14:37:06 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
Hi Albert,
On 02/04/2013 01:57 PM, Albert ARIBAUD wrote:
IIRC, you has submitted a fix so that BMP loads would result in correctly aligned fields and thus no need for accessors. Why this change of mind?
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
I actually like the new fix less, as it adds an API where there is no need of one -- it's not like the implementation of BMP is at risk of changing. What is the problem in fixing the load address at load time and then passing this fixed address around?
OK, so I'm wondering. Isn't this an example of where our idea that unaligned accesses are a software problem to fix, rather than an area to let the hardware fixup for us, when able, is wrong?
In other words, a real life example of why we should go with -munaligned-access being set for v7, set the HW bit and let things go?
Hi Tom,
On Tue, 5 Feb 2013 09:58:52 -0500, Tom Rini trini@ti.com wrote:
On Mon, Feb 04, 2013 at 03:19:56PM +0100, Albert ARIBAUD wrote:
Hi Nikita,
On Mon, 04 Feb 2013 14:37:06 +0200, Nikita Kiryanov nikita@compulab.co.il wrote:
Hi Albert,
On 02/04/2013 01:57 PM, Albert ARIBAUD wrote:
IIRC, you has submitted a fix so that BMP loads would result in correctly aligned fields and thus no need for accessors. Why this change of mind?
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
I actually like the new fix less, as it adds an API where there is no need of one -- it's not like the implementation of BMP is at risk of changing. What is the problem in fixing the load address at load time and then passing this fixed address around?
OK, so I'm wondering. Isn't this an example of where our idea that unaligned accesses are a software problem to fix, rather than an area to let the hardware fixup for us, when able, is wrong?
In other words, a real life example of why we should go with -munaligned-access being set for v7, set the HW bit and let things go?
I feel this is addressed to me. :)
And no, I don't think the BMP issue should be solved by relaxing HW constraints. BMP is not a HW-enforced or protocol format.
Anyway, as I said, even if we really could not avoid misaligned fields (and we know at least two ways we could), then the solution would not be to flip a general HW switch controlling all accesses, since only the BMP accesses are a problem! We could just make sure we access the poorly aligned fields through dedicated unaligned accessors, e.g. readl_u(&field) / writel_u(&field).
Amicalement,
Dear Nikita Kiryanov,
In message 510FAB72.3050901@compulab.co.il you wrote:
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
Can you please explain (again) what exactly you consider the problem?
That I can load a BMP image to anodd address, and that this will result in odd results?
Best regards,
Wolfgang Denk
Hello Wolfgang,
On 02/04/2013 09:35 PM, Wolfgang Denk wrote:
Dear Nikita Kiryanov,
In message 510FAB72.3050901@compulab.co.il you wrote:
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
Can you please explain (again) what exactly you consider the problem?
That I can load a BMP image to anodd address, and that this will result in odd results?
If you use eldk 5.3, a board will be bricked if you set the load address to not (an aligned address _+ 2_). So anyone who didn't know the +2 requirement, has a bricked board (and can't recover). So this is really a different then md trapping, since resetting the board won't help... it is broken / kaput.
That said, I am not really in favor of this solution. But I do think it needs to be fixed.
Regards, Jeroen
On 02/04/13 23:02, Jeroen Hofstee wrote:
Hello Wolfgang,
On 02/04/2013 09:35 PM, Wolfgang Denk wrote:
Dear Nikita Kiryanov,
In message 510FAB72.3050901@compulab.co.il you wrote:
I did mention that I consider that fix as temporary. I believe this fix is better because it addresses the issue everywhere and does so in a more maintainable way (does not require the address fix to be duplicated everywhere there is a problem).
Can you please explain (again) what exactly you consider the problem?
That I can load a BMP image to anodd address, and that this will result in odd results?
If you use eldk 5.3, a board will be bricked if you set the load address to not (an aligned address _+ 2_). So anyone who didn't know the +2 requirement, has a bricked board (and can't recover). So this is really a different then md trapping, since resetting the board won't help... it is broken / kaput.
Agreed totally, by anyone we mean any _user_ that don't even need to care about the bmp header structure and uses the perfectly 32 bit aligned address.
That said, I am not really in favor of this solution. But I do think it needs to be fixed.
I actually like the approach as it gives an abstraction for: 1) different compilers and their default options, 2) architecture specific "problems" like weather it can deal with unaligned accesses
On 02/04/13 13:39, Nikita Kiryanov wrote:
When configuring U-Boot to display a splash image, the user is free to designate whatever address they see fit as the in-memory location of BMP file. Unfortunately, this makes it easy to place the BMP header fields in unaligned addresses, which will cause a data abort on architectures which can't handle unaligned memory accesses. What's worse, addresses which are supposed to be issue free (32 bit aligned addresses) actually don't work because of the structure of the BMP header file (the header starts with 2 chars, followed by __u32 fields. The two chars offset the 32 bit fields away from proper alignment).
As a result, it is very easy to brick the board that U-Boot runs on with certain architectures. Once a bad address for splash image is selected and the board restarted, U-Boot never reaches command line, and yet the only way to prevent the data aborts from happening is to clear or change the environment variable splashimage.
While it is possible to fix the problem by compiling all relevant files with $(PLATFORM_NO_UNALIGNED), it is possible to handle it with finer granularity by creating an API for header accesses, and compiling only that with $(PLATFORM_NO_UNALIGNED).
This patchset creates such an API, and makes use of it wherever an in-memory BMP header is probed for data.
Further information can be found in this discussion: http://lists.denx.de/pipermail/u-boot/2013-January/144666.html
Nikita Kiryanov (5): Add bmp_layout module for accessing BMP header data lcd: use bmp_layout API cmd_bmp: use bmp_layout API video/cfb_console: use bmp_layout API video/bus_vcxk: use bmp_layout API
In 5 above patches, Nikita has accidentally added my s-o-b, without me being participating in the patch development or delivery process. Nikita, please, don't do so in the future.
Thanks.
On 02/05/2013 08:53 AM, Igor Grinberg wrote:
On 02/04/13 13:39, Nikita Kiryanov wrote:
When configuring U-Boot to display a splash image, the user is free to designate whatever address they see fit as the in-memory location of BMP file. Unfortunately, this makes it easy to place the BMP header fields in unaligned addresses, which will cause a data abort on architectures which can't handle unaligned memory accesses. What's worse, addresses which are supposed to be issue free (32 bit aligned addresses) actually don't work because of the structure of the BMP header file (the header starts with 2 chars, followed by __u32 fields. The two chars offset the 32 bit fields away from proper alignment).
As a result, it is very easy to brick the board that U-Boot runs on with certain architectures. Once a bad address for splash image is selected and the board restarted, U-Boot never reaches command line, and yet the only way to prevent the data aborts from happening is to clear or change the environment variable splashimage.
While it is possible to fix the problem by compiling all relevant files with $(PLATFORM_NO_UNALIGNED), it is possible to handle it with finer granularity by creating an API for header accesses, and compiling only that with $(PLATFORM_NO_UNALIGNED).
This patchset creates such an API, and makes use of it wherever an in-memory BMP header is probed for data.
Further information can be found in this discussion: http://lists.denx.de/pipermail/u-boot/2013-January/144666.html
Nikita Kiryanov (5): Add bmp_layout module for accessing BMP header data lcd: use bmp_layout API cmd_bmp: use bmp_layout API video/cfb_console: use bmp_layout API video/bus_vcxk: use bmp_layout API
In 5 above patches, Nikita has accidentally added my s-o-b, without me being participating in the patch development or delivery process. Nikita, please, don't do so in the future.
Thanks.
I apologize. It won't happen again.
participants (6)
-
Albert ARIBAUD -
Igor Grinberg -
Jeroen Hofstee -
Nikita Kiryanov -
Tom Rini -
Wolfgang Denk