[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Nikita Kiryanov]

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

-- 
1.7.10.4

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

[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 <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 at 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_ */
-- 
1.7.10.4

[U-Boot] [PATCH 2/5] lcd: use bmp_layout API

[Nikita Kiryanov]

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");
-- 
1.7.10.4

[U-Boot] [PATCH 3/5] cmd_bmp: use bmp_layout API

[Nikita Kiryanov]

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) {
-- 
1.7.10.4

[U-Boot] [PATCH 4/5] video/cfb_console: use bmp_layout API

[Nikita Kiryanov]

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;
 	}
 
-- 
1.7.10.4

[U-Boot] [PATCH 5/5] video/bus_vcxk: use bmp_layout API

[Nikita Kiryanov]

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;
-- 
1.7.10.4

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Nikita Kiryanov]

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,
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Nikita Kiryanov]

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,
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Albert ARIBAUD]

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,
-- 
Albert.

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

[Wolfgang Denk]

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

-- 
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 at denx.de
A wise person makes his  own  decisions,  a  weak  one  obeys  public
opinion.                                           -- Chinese proverb

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Wolfgang Denk]

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

-- 
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 at denx.de
"A fractal is by definition a set for which the Hausdorff Besicovitch
dimension strictly exceeds the topological dimension."
- Mandelbrot, _The Fractal Geometry of Nature_

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Wolfgang Denk]

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

-- 
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 at denx.de
Am I your nanny? The kernel is there to support  user  programs,  but
it's a _resource_ handler, not a baby feeder.     - Linus Torvalds in
      <Pine.LNX.3.91.960425074845.22041C-100000@linux.cs.Helsinki.FI>

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Wolfgang Denk]

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

-- 
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 at denx.de
Hegel was right when he said that we learn from history that man  can
never learn anything from history.              - George Bernard Shaw

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Jeroen Hofstee]

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

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

[Albert ARIBAUD]

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,
-- 
Albert.

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

[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: wd at denx.de
The use of anthropomorphic terminology when  dealing  with  computing
systems is a symptom of professional immaturity.   -- Edsger Dijkstra

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

[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-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
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Igor Grinberg]

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.

-- 
Regards,
Igor.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Nikita Kiryanov]

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,
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Nikita Kiryanov]

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.

-- 
Regards,
Nikita.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Igor Grinberg]

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


-- 
Regards,
Igor.

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

[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] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

[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

-- 
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 at 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] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

[Wolfgang Denk]

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

-- 
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 at 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] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

[Igor Grinberg]

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...


-- 
Regards,
Igor.

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

[Wolfgang Denk]

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

-- 
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 at 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] [PATCH 1/5] Add bmp_layout module for accessing BMP header data

[Igor Grinberg]

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?

-- 
Regards,
Igor.

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Tom Rini]

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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130205/8b2b04f4/attachment.pgp>

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

[Wolfgang Denk]

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 at denx.de
"Have you lived in this village all your life?"        "No, not yet."

[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Albert ARIBAUD]

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,
-- 
Albert.