Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-05 Thread Igor Grinberg 
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.


-- 
Regards,
Igor.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-05 Thread Wolfgang Denk 
Dear Nikita,

In message 5110ac16.9000...@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

-- 
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: w...@denx.de
Alan Turing thought about criteria to settle the question of  whether
machines  can think, a question of which we now know that it is about
as relevant as the question of whether submarines can swim.
   -- Edsger Dijkstra
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-05 Thread Wolfgang Denk 
Dear Igor Grinberg,

In message 5110bdb2.8040...@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

-- 
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: w...@denx.de
Pig: An animal (Porcus omnivorous) closely allied to the  human  race
by  the splendor and vivacity of its appetite, which, however, is in-
ferior in scope, for it balks at pig.- Ambrose Bierce
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-05 Thread Igor Grinberg 
On 02/05/13 11:57, Wolfgang Denk wrote:
 Dear Igor Grinberg,
 
 In message 5110bdb2.8040...@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...


-- 
Regards,
Igor.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-05 Thread Wolfgang Denk 
Dear Igor,

In message 5110ef4f.3080...@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

-- 
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: w...@denx.de
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is a nanocentury.
- Tom Duff, Bell Labs
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-05 Thread Igor Grinberg 
On 02/05/13 15:20, Wolfgang Denk wrote:
 Dear Igor,
 
 In message 5110ef4f.3080...@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?

-- 
Regards,
Igor.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-05 Thread Wolfgang Denk 
Dear Igor,

In message 5c0f.60...@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: w...@denx.de
Have you lived in this village all your life?No, not yet.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-04 Thread Nikita Kiryanov 
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 nik...@compulab.co.il
Signed-off-by: Igor Grinberg grinb...@compulab.co.il
Cc: Anatolij Gustschin ag...@denx.de
Cc: Wolfgang Denk w...@denx.de
Cc: Albert ARIBAUD albert.u.b...@aribaud.net
Cc: Jeroen Hofstee jer...@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 000..c55b653
--- /dev/null
+++ b/common/bmp_layout.c
@@ -0,0 +1,96 @@
+/* (C) Copyright 2013
+ * Nikita Kiryanov, Compulab, nik...@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_RLE81
 #define BMP_BI_RLE42
 
+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

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-04 Thread Wolfgang Denk 
Dear Nikita Kiryanov,

In message 1359977979-28585-2-git-send-email-nik...@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

-- 
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: w...@denx.de
A wise person makes his  own  decisions,  a  weak  one  obeys  public
opinion.   -- Chinese proverb
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-04 Thread Albert ARIBAUD 
Hi Wolfgang,

On Mon, 04 Feb 2013 20:26:18 +0100, Wolfgang Denk w...@denx.de wrote:

 Dear Nikita Kiryanov,
 
 In message 1359977979-28585-2-git-send-email-nik...@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,
-- 
Albert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-04 Thread Wolfgang Denk 
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

-- 
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: w...@denx.de
The use of anthropomorphic terminology when  dealing  with  computing
systems is a symptom of professional immaturity.   -- Edsger Dijkstra
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

2013-02-04 Thread Nikita Kiryanov 

Dear Wolfgang Denk,

On 02/04/2013 09:26 PM, Wolfgang Denk wrote:

Dear Nikita Kiryanov,

In message 1359977979-28585-2-git-send-email-nik...@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




--
Regards,
Nikita.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot